From 2c2f69adba9138a2b076d214cd7f276beac48749 Mon Sep 17 00:00:00 2001 From: timkjr Date: Tue, 27 Oct 2026 09:22:17 -0500 Subject: [PATCH 1/2] fix: don't point the image watcher at a remote session's directory A remote-SSH session's workingDir is a path on the remote host, and the image watcher watched it on the local disk: a folder that is not there, or a same-named local folder whose images then popped up as the remote session's. Same root cause as the Ralph @fix_plan.md watcher fix. Three places start a watch, and all three now skip a remote session: setupSessionListeners(), the app-wide toggle in PUT /api/settings (which re-watches every enabled session), and POST /api/sessions/:id/image-watcher, which refuses to turn it on (INVALID_INPUT, like the Git fetch settings route) and still lets it be turned off. The Session Options row is hidden for a remote session, the way the Git tab is, since the client's _apiPost() does not throw on a refused request. --- src/web/public/session-ui.js | 5 +- src/web/routes/session-routes.ts | 5 + src/web/routes/system-routes.ts | 5 +- src/web/server.ts | 6 +- test/image-watcher-remote.test.ts | 190 ++++++++++++++++++++++++++++++ 5 files changed, 206 insertions(+), 5 deletions(-) create mode 100644 test/image-watcher-remote.test.ts diff --git a/src/web/public/session-ui.js b/src/web/public/session-ui.js index 1842cca2..45c3af07 100644 --- a/src/web/public/session-ui.js +++ b/src/web/public/session-ui.js @@ -2709,7 +2709,10 @@ Object.assign(CodemanApp.prototype, { // Git: the session's own choice if it has one, otherwise the app setting. updateGitFetchStatus is also // what SSE calls when a scheduled run lands, so the rows stay current while the modal is open. this.updateGitFetchStatus(sessionId); - document.getElementById('modalImageWatcherEnabled').checked = session.imageWatcherEnabled ?? true; + const imageWatcherToggle = document.getElementById('modalImageWatcherEnabled'); + imageWatcherToggle.checked = session.imageWatcherEnabled ?? true; + // The server never watches a remote session's directory (it is on another host), so the row is hidden. + imageWatcherToggle.closest('.set-row').style.display = session.remote ? 'none' : ''; document.getElementById('modalFlickerFilterEnabled').checked = session.flickerFilterEnabled ?? false; // Populate session name input with prefix/suffix split diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index a2bf2d57..0eb80b82 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -3780,6 +3780,11 @@ export function registerSessionRoutes( const { id } = req.params as { id: string }; const body = parseBody(ImageWatcherSchema, req.body, 'Invalid request body'); const session = findSessionOrFail(ctx, id, req); + // A remote session's workingDir is a path on the remote host; the watcher would watch it on the + // LOCAL disk (a missing folder, or a same-named local one whose images are not the session's). + if (body.enabled && session.remote) { + return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Image watching does not run for remote sessions'); + } if (body.enabled) { imageWatcher.watchSession(session.id, session.workingDir); diff --git a/src/web/routes/system-routes.ts b/src/web/routes/system-routes.ts index c15f0ecf..aa3515ad 100644 --- a/src/web/routes/system-routes.ts +++ b/src/web/routes/system-routes.ts @@ -1183,9 +1183,10 @@ export function registerSystemRoutes( // Handle image watcher toggle dynamically toggleService((merged.imageWatcherEnabled as boolean) ?? false, imageWatcher, 'Image watcher', () => { - // Re-watch all active sessions that have image watcher enabled + // Re-watch all active sessions that have image watcher enabled (never a remote session: its + // workingDir is a path on the remote host, not on this disk) for (const session of ctx.sessions.values()) { - if (session.imageWatcherEnabled) { + if (session.imageWatcherEnabled && !session.remote) { imageWatcher.watchSession(session.id, session.workingDir); } } diff --git a/src/web/server.ts b/src/web/server.ts index 8942c0d7..7e56a98f 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -2067,8 +2067,10 @@ export class WebServer extends EventEmitter { session.ralphTracker.setWorkingDir(session.workingDir); } - // Start watching for new images in this session's working directory (if enabled globally and per-session) - if ((await this.isImageWatcherEnabled()) && session.imageWatcherEnabled) { + // Start watching for new images in this session's working directory (if enabled globally and per-session). + // A remote session's workingDir is a path on the remote host, and the watcher watches it on the LOCAL + // disk, so it is skipped there (the per-session and app-wide toggles skip it too). + if ((await this.isImageWatcherEnabled()) && session.imageWatcherEnabled && !session.remote) { imageWatcher.watchSession(session.id, session.workingDir); } diff --git a/test/image-watcher-remote.test.ts b/test/image-watcher-remote.test.ts new file mode 100644 index 00000000..5b3871b2 --- /dev/null +++ b/test/image-watcher-remote.test.ts @@ -0,0 +1,190 @@ +/** + * The image watcher must never watch a REMOTE session's working directory. + * + * A remote-SSH session's `workingDir` is a path on the remote host. The watcher + * (chokidar) watches it on the LOCAL disk: where nothing exists there it watches + * a missing folder, and where a same-named local folder does exist it reports that + * folder's images as the remote session's. Three places start a watch, and all + * three skip a remote session: + * - `WebServer.setupSessionListeners()` (create, respawn, restore, recovery); + * - `POST /api/sessions/:id/image-watcher` (the per-session toggle), which refuses + * to turn it on, the way the Git fetch settings route refuses a remote session; + * - `PUT /api/settings` turning the app-wide watcher on, which re-watches every + * session that has it enabled. + * + * Spies on the real `imageWatcher` singleton, so all three modules see the spy. + * Port: none (app.inject(), and a WebServer that is never started). + */ +import { mkdirSync } from 'node:fs'; +import { homedir } from 'node:os'; +import { join } from 'node:path'; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import { safeRmHomeTree } from './mocks/test-helpers.js'; +import { createRouteTestHarness, type RouteTestHarness } from './routes/_route-test-utils.js'; + +import { imageWatcher } from '../src/image-watcher.js'; +import { WebServer } from '../src/web/server.js'; +import { Session } from '../src/session.js'; +import { TmuxManager } from '../src/tmux-manager.js'; +import { registerSessionRoutes } from '../src/web/routes/session-routes.js'; +import { registerSystemRoutes } from '../src/web/routes/system-routes.js'; + +const WORKSPACE = join(homedir(), '.codeman-test-image-watcher-remote'); +// A path on the remote host: nothing exists there locally. +const REMOTE_PATH = '/nonexistent-remote-host-path/homelab-infrastructure'; +const REMOTE = { + hostId: 'proxx', + label: 'proxx', + host: 'proxx.example.lan', + username: 'user', + remotePath: REMOTE_PATH, + owned: true, +}; + +let watchSession: ReturnType; + +beforeEach(() => { + watchSession = vi.spyOn(imageWatcher, 'watchSession').mockImplementation(() => {}); +}); + +afterEach(() => { + vi.restoreAllMocks(); + imageWatcher.stop(); +}); + +describe('setupSessionListeners()', () => { + interface ServerInternals { + registerSessionWithLayout(session: Session): Promise; + setupSessionListeners(session: Session): Promise; + discardPartiallyBuiltSession(sessionId: string): Promise; + isImageWatcherEnabled(): Promise; + } + const SESSION_ID = 'a1b2c3d4e5f60718'; + let server: WebServer; + let internals: ServerInternals; + let mux: TmuxManager; + + beforeAll(() => { + mkdirSync(WORKSPACE, { recursive: true }); + server = new WebServer(0, false, true); + internals = server as unknown as ServerInternals; + mux = new TmuxManager(); + }); + + afterEach(async () => { + await internals.discardPartiallyBuiltSession(SESSION_ID).catch(() => {}); + }); + + afterAll(async () => { + await server.stop().catch(() => {}); + safeRmHomeTree(WORKSPACE); + }); + + async function wire(remote: boolean) { + const session = new Session({ + id: SESSION_ID, + workingDir: remote ? REMOTE_PATH : WORKSPACE, + mode: 'claude', + name: remote ? 'remote session' : 'local session', + mux, + useMux: true, + ...(remote ? { remote: REMOTE } : {}), + }); + session.imageWatcherEnabled = true; + vi.spyOn(internals, 'isImageWatcherEnabled').mockResolvedValue(true); + await internals.registerSessionWithLayout(session); + await internals.setupSessionListeners(session); + } + + it('does not watch a remote session working directory', async () => { + await wire(true); + expect(watchSession).not.toHaveBeenCalled(); + }); + + it('still watches a local session working directory', async () => { + await wire(false); + expect(watchSession).toHaveBeenCalledWith(SESSION_ID, WORKSPACE); + }); +}); + +describe('POST /api/sessions/:id/image-watcher', () => { + let harness: RouteTestHarness; + + beforeEach(async () => { + harness = await createRouteTestHarness(registerSessionRoutes); + }); + + afterEach(async () => { + await harness.app.close(); + }); + + const post = (enabled: boolean) => + harness.app.inject({ + method: 'POST', + url: `/api/sessions/${harness.ctx._sessionId}/image-watcher`, + payload: { enabled }, + }); + + it('refuses to turn it on for a remote session, storing nothing', async () => { + const session = harness.ctx._session as unknown as Record; + session.remote = REMOTE; + session.workingDir = REMOTE_PATH; + const res = await post(true); + expect(JSON.parse(res.body)).toMatchObject({ success: false, errorCode: 'INVALID_INPUT' }); + expect(watchSession).not.toHaveBeenCalled(); + expect(session.imageWatcherEnabled).not.toBe(true); + expect(harness.ctx.persistSessionState).not.toHaveBeenCalled(); + }); + + it('still turns it off for a remote session', async () => { + (harness.ctx._session as unknown as Record).remote = REMOTE; + const unwatchSession = vi.spyOn(imageWatcher, 'unwatchSession'); + const res = await post(false); + expect(res.statusCode).toBe(200); + expect(JSON.parse(res.body)).toMatchObject({ success: true, data: { imageWatcherEnabled: false } }); + expect(unwatchSession).toHaveBeenCalledWith(harness.ctx._sessionId); + expect(harness.ctx.persistSessionState).toHaveBeenCalled(); + }); + + it('still turns it on for a local session', async () => { + const res = await post(true); + expect(res.statusCode).toBe(200); + expect(watchSession).toHaveBeenCalledWith(harness.ctx._sessionId, harness.ctx._session.workingDir); + }); +}); + +describe('PUT /api/settings turning the app-wide watcher on', () => { + let harness: RouteTestHarness; + + beforeEach(async () => { + harness = await createRouteTestHarness(registerSystemRoutes); + }); + + afterEach(async () => { + await harness.app.close(); + }); + + it('re-watches the local sessions that have it enabled, never a remote one', async () => { + const local = harness.ctx._session as unknown as Record; + local.imageWatcherEnabled = true; + harness.ctx.sessions.set('remote-session', { + ...harness.ctx._session, + id: 'remote-session', + workingDir: REMOTE_PATH, + imageWatcherEnabled: true, + remote: REMOTE, + } as unknown as typeof harness.ctx._session); + expect(imageWatcher.isRunning()).toBe(false); + + const res = await harness.app.inject({ + method: 'PUT', + url: '/api/settings', + payload: { imageWatcherEnabled: true }, + }); + + expect(res.statusCode).toBe(200); + expect(imageWatcher.isRunning()).toBe(true); + expect(watchSession).toHaveBeenCalledTimes(1); + expect(watchSession).toHaveBeenCalledWith(harness.ctx._sessionId, harness.ctx._session.workingDir); + }); +}); From dd56674a1d5c4dc12129668cdeba9d918645f02e Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Wed, 28 Oct 2026 09:08:03 +0100 Subject: [PATCH 2/2] fix: apply the review items for #727 at merge toggleSessionImageWatcher checks the response, so any refusal puts the switch back and toasts an error instead of a success; architecture- invariants says the image watcher never watches a remote workingDir. Co-Authored-By: Claude Opus 5.5 --- docs/architecture-invariants.md | 2 +- src/web/public/session-ui.js | 12 ++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index d4484540..c6282ade 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -81,7 +81,7 @@ OMP's registry entry declares `privilegedParams: []` and a launch spec that only Further detail: ⚠️ **Auto-reconnect revives ONLY when the durable remote tmux session is verifiably still alive** (`remoteTmuxSessionAlive()`, a `has-session` probe over ssh, #355): a clean agent exit (Ctrl-C, Ctrl-D, `exit`) tears that session down, and `isPaneDead()` cannot tell it from a transport drop, so the watcher used to relaunch a FRESH agent after every clean exit (claude only looked fine because its `|| --resume` fallback masked it). An unreachable host answers `undefined`, which also means do not revive. ⚠️ `has-session` prints NOTHING on success, so the probe is classified by EXIT STATUS (`classifyRemoteAliveExit`: 0 alive, ssh's 255 or a timeout unknown, anything else gone); reading stdout classified every live session as gone and silently disabled transport-drop reconnects. The answer is cached per session and forgotten whenever the pane is seen alive again, or a stale `true` from one transport drop would revive the next clean exit. -File reads in a remote case are the second ssh surface (see Remote SSH cases below). The reason writes, office previews and thumbnails are deliberately unsupported over ssh is that no remote file is ever copied onto the server's disk. ssh children are bounded because terminal output in a remote session is written on the remote host, so a prompt-injected agent there can print hundreds of `codeman://attach` links. The third surface is a remote session's full-history load, which reads the scrollback from the remote tmux (`src/remote-pane-history.ts`) through the same limiter, with a bounded slot wait (`maxWaitMs`) since the local capture is its fallback. +⚠️ The image watcher never watches a remote session's `workingDir` (a path on the remote host, which it would watch on the LOCAL disk): every call site that starts a watch skips `session.remote` (`setupSessionListeners()`, the app-wide toggle in `PUT /api/settings`, and `POST /api/sessions/:id/image-watcher`, which refuses `enabled: true` with `INVALID_INPUT`), so a new call site must too. File reads in a remote case are the second ssh surface (see Remote SSH cases below). The reason writes, office previews and thumbnails are deliberately unsupported over ssh is that no remote file is ever copied onto the server's disk. ssh children are bounded because terminal output in a remote session is written on the remote host, so a prompt-injected agent there can print hundreds of `codeman://attach` links. The third surface is a remote session's full-history load, which reads the scrollback from the remote tmux (`src/remote-pane-history.ts`) through the same limiter, with a bounded slot wait (`maxWaitMs`) since the local capture is its fallback. ### Remote SSH cases diff --git a/src/web/public/session-ui.js b/src/web/public/session-ui.js index 45c3af07..1e5961d4 100644 --- a/src/web/public/session-ui.js +++ b/src/web/public/session-ui.js @@ -3152,9 +3152,17 @@ Object.assign(CodemanApp.prototype, { async toggleSessionImageWatcher() { if (!this.editingSessionId) return; - const enabled = document.getElementById('modalImageWatcherEnabled').checked; + const toggle = document.getElementById('modalImageWatcherEnabled'); + const enabled = toggle.checked; try { - await this._apiPost(`/api/sessions/${this.editingSessionId}/image-watcher`, { enabled }); + const res = await this._apiPost(`/api/sessions/${this.editingSessionId}/image-watcher`, { enabled }); + // `_apiPost` hands back the Response without checking it, so a refused request (a 400) must not + // read as a save: put the switch back and say so. + if (!res?.ok) { + toggle.checked = !enabled; + this.showToast('Failed to toggle image watcher', 'error'); + return; + } // Update local session state const session = this.sessions.get(this.editingSessionId); if (session) {