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 1842cca2..1e5961d4 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 @@ -3149,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) { 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 6047197f..4624155d 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -2069,8 +2069,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); + }); +});