From 8e5e207386f18c820679659466b58664a61c2a5f Mon Sep 17 00:00:00 2001 From: d fei Date: Sat, 29 Aug 2026 23:31:28 -0700 Subject: [PATCH] fix(docker): send the probe body as an object, and explain an unreachable container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The run menu still offered every mode for an attached container. The browser's actual request showed why: POST /api/docker-cases/adopt-preflight -> 400 {"error":"Invalid input: expected object, received string"} _api serializes `body` and sets Content-Type itself, and three call sites each passed an already-stringified body, so it was encoded twice and the server saw a JSON string where it expects an object. curl was fine throughout, so nothing in the server logs pointed at it. Also fixes the design defect underneath: a failed probe fell through to "do not gate", which silently offered every mode. When the container has been recreated, is stopped, or the engine is unreachable, the user sees claude, clicks it, and it can only fail — with the reason visible nowhere. A failed probe now hides every agent mode (Shell needs no CLI and stays) and shows the server's own reason at the top of the menu. Two static guards switched from a character window to brace matching. They sliced between two call sites, and _loadRunModeHistory's call appears above its definition, so the slice came out empty and the assertion verified nothing — the same trap twice in one file. --- src/types/session.ts | 10 ++++- src/web/public/session-ui.js | 52 +++++++++++++++++++++----- src/web/public/styles.css | 12 ++++++ src/web/routes/case-routes.ts | 4 +- src/web/routes/session-routes.ts | 15 +++++--- src/web/routes/system-routes.ts | 5 ++- test/docker-adopted-container.test.ts | 54 ++++++++++++++++++++++++++- 7 files changed, 131 insertions(+), 21 deletions(-) diff --git a/src/types/session.ts b/src/types/session.ts index d3fe4565..39910bfc 100644 --- a/src/types/session.ts +++ b/src/types/session.ts @@ -47,7 +47,15 @@ export type ClaudeMode = 'dangerously-skip-permissions' | 'auto' | 'normal' | 'a /** Session mode: which CLI backend a session runs */ export type SessionMode = - 'claude' | 'shell' | 'opencode' | 'codex' | 'gemini' | 'antigravity' | 'pi' | 'grok' | 'deepseek'; + | 'claude' + | 'shell' + | 'opencode' + | 'codex' + | 'gemini' + | 'antigravity' + | 'pi' + | 'grok' + | 'deepseek'; export type RemoteCommandMode = Extract< SessionMode, diff --git a/src/web/public/session-ui.js b/src/web/public/session-ui.js index 15109b16..11fd15a0 100644 --- a/src/web/public/session-ui.js +++ b/src/web/public/session-ui.js @@ -498,14 +498,18 @@ Object.assign(CodemanApp.prototype, { ? this._dockerCaseModes?.[caseName] || activeCase.docker?.availableModes || null : null; if (isDocker && !this._dockerCaseModes?.[caseName]) void this._probeDockerCaseModes(activeCase, menu); + // An unreachable container hides every agent mode and explains why, instead + // of silently offering modes that cannot start. + const probeError = isDocker ? this._dockerCaseProbeError?.[caseName] : null; for (const mode of ['claude', 'opencode', 'codex', 'gemini', 'antigravity', 'pi', 'grok', 'deepseek']) { const btn = menu.querySelector(`.run-mode-option[data-mode="${mode}"]`); if (!btn) continue; let available; - if (activeCase?.location === 'docker') available = containerModes ? containerModes.includes(mode) : true; + if (isDocker) available = probeError ? false : containerModes ? containerModes.includes(mode) : true; else available = this.isCliAvailable(mode); btn.style.display = available ? 'flex' : 'none'; } + this._renderRunModeNotice(menu, probeError); // DeepSeek is the one mode whose availability has two halves: `dsh` can be // perfectly installed while no pane-capable profile exists, because DeepSeek // ships no terminal front door. In that state the honest offer is "add one", @@ -647,6 +651,27 @@ Object.assign(CodemanApp.prototype, { } }, + /** + * One-line explanation at the top of the run menu. Only a container that could + * not be read produces one; everything else removes it, so a stale reason can + * never outlive the condition that caused it. + */ + _renderRunModeNotice(menu, message) { + if (!menu) return; + let el = menu.querySelector('.run-mode-notice'); + if (!message) { + el?.remove(); + return; + } + if (!el) { + el = document.createElement('div'); + el.className = 'run-mode-notice'; + menu.prepend(el); + } + // Server-supplied text: set it, never parse it as markup. + el.textContent = message; + }, + /** * Ask the container which CLIs it actually has, and re-gate the menu once the * answer lands. Cached per case for the page's lifetime: the menu re-opens @@ -668,16 +693,27 @@ Object.assign(CodemanApp.prototype, { this._dockerModeProbeInFlight = this._dockerModeProbeInFlight || {}; this._dockerModeProbeInFlight[name] = true; try { + // ⚠️ _api serializes `body` and sets Content-Type itself. Passing an + // already-stringified body double-encodes it and the server rejects a + // JSON string where it expects an object (400 INVALID_INPUT). const probe = await this._apiJson('/api/docker-cases/adopt-preflight', { method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ hostId, container }), + body: { hostId, container }, }); if (probe?.ok && Array.isArray(probe.availableModes)) { this._dockerCaseModes[name] = probe.availableModes; - // Only repaint while the menu the user opened is still on screen. - if (menu?.classList.contains('active')) this._refreshRunModeAvailability(menu); + delete this._dockerCaseProbeError?.[name]; + } else { + // A container that cannot be probed — recreated, stopped, engine down — + // must NOT fall through to "show everything". Offering claude on a + // container that is not running is a click that can only fail, with the + // reason visible nowhere. Record the reason and say it in the menu. + this._dockerCaseProbeError = this._dockerCaseProbeError || {}; + this._dockerCaseProbeError[name] = probe?.error || `Could not read container "${container}".`; + delete this._dockerCaseModes[name]; } + // Only repaint while the menu the user opened is still on screen. + if (menu?.classList.contains('active')) this._refreshRunModeAvailability(menu); } finally { delete this._dockerModeProbeInFlight[name]; } @@ -2934,8 +2970,7 @@ Object.assign(CodemanApp.prototype, { fetchListing: async (path) => { const data = await this._apiJson('/api/docker-cases/browse', { method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ hostId, container, path: path || '/' }), + body: { hostId, container, path: path || '/' }, }); if (!data) return { success: false, error: `Could not read ${container}. Is it running?` }; if (data.error) return { success: false, error: data.error }; @@ -3109,8 +3144,7 @@ Object.assign(CodemanApp.prototype, { // reason it failed, so the envelope is unwrapped by hand here. const probe = await this._apiJson('/api/docker-cases/adopt-preflight', { method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ hostId, container, ...(containerWorkdir ? { containerWorkdir } : {}) }), + body: { hostId, container, ...(containerWorkdir ? { containerWorkdir } : {}) }, }); if (!statusEl) return; if (!probe) { diff --git a/src/web/public/styles.css b/src/web/public/styles.css index 7ca4edc3..50912082 100644 --- a/src/web/public/styles.css +++ b/src/web/public/styles.css @@ -16608,6 +16608,18 @@ html[data-tab-orientation='vertical'] .home-sessions { panel stays exactly as it was until the checkbox is ticked. Rules carry `!important` because the adapter block above paints `.form-row` as a row card and `details.advanced-options` has its own display. */ +/* Run-menu notice: why a container case is offering no agent modes. Lives at the + top of the menu so the reason is where the missing entries would have been. */ +.run-mode-notice { + padding: 8px 12px; + margin: 0 0 4px; + font-size: 12px; + line-height: 1.45; + color: var(--text-muted, #9aa0a6); + border-bottom: 1px solid var(--border, #333); + white-space: normal; +} + #createCaseModal .docker-adopt-only { display: none !important; } diff --git a/src/web/routes/case-routes.ts b/src/web/routes/case-routes.ts index 5aa4a2f0..2024d994 100644 --- a/src/web/routes/case-routes.ts +++ b/src/web/routes/case-routes.ts @@ -1175,7 +1175,9 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config engine: result.manifest.engine, image: result.importedImage ?? result.manifest.image, network: (['bridge', 'none', 'custom'].includes(result.manifest.network) ? result.manifest.network : 'bridge') as - 'bridge' | 'none' | 'custom', + | 'bridge' + | 'none' + | 'custom', }; await writeDockerHosts( CODEMAN_CONFIG_DIR, diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 3fc80c94..c50ddcb3 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -944,8 +944,9 @@ export function registerSessionRoutes( } } if (body.mode === 'antigravity') { - const { isAntigravityAvailable, getAntigravityNotFoundMessage } = - await import('../../utils/antigravity-cli-resolver.js'); + const { isAntigravityAvailable, getAntigravityNotFoundMessage } = await import( + '../../utils/antigravity-cli-resolver.js' + ); if (!isAntigravityAvailable()) { return createErrorResponse(ApiErrorCode.OPERATION_FAILED, getAntigravityNotFoundMessage()); } @@ -3025,8 +3026,9 @@ export function registerSessionRoutes( // Check OpenCode availability if requested. Error text comes from the // resolver so it carries the resolution diagnostics; same for the modes below. if (mode === 'opencode') { - const { isOpenCodeAvailable, getOpenCodeNotFoundMessage } = - await import('../../utils/opencode-cli-resolver.js'); + const { isOpenCodeAvailable, getOpenCodeNotFoundMessage } = await import( + '../../utils/opencode-cli-resolver.js' + ); if (!isOpenCodeAvailable()) { return createErrorResponse(ApiErrorCode.OPERATION_FAILED, getOpenCodeNotFoundMessage()); } @@ -3050,8 +3052,9 @@ export function registerSessionRoutes( // Check Antigravity availability if requested if (mode === 'antigravity') { - const { isAntigravityAvailable, getAntigravityNotFoundMessage } = - await import('../../utils/antigravity-cli-resolver.js'); + const { isAntigravityAvailable, getAntigravityNotFoundMessage } = await import( + '../../utils/antigravity-cli-resolver.js' + ); if (!isAntigravityAvailable()) { return createErrorResponse(ApiErrorCode.OPERATION_FAILED, getAntigravityNotFoundMessage()); } diff --git a/src/web/routes/system-routes.ts b/src/web/routes/system-routes.ts index 4929e977..a23fb286 100644 --- a/src/web/routes/system-routes.ts +++ b/src/web/routes/system-routes.ts @@ -683,8 +683,9 @@ export function registerSystemRoutes( `Installing ${pkg} into profile "${profile}" failed: ${detail}` ); } - const { listDeepSeekProfiles, resolveDefaultDeepSeekProfile, isDeepSeekRunnable } = - await import('../../utils/deepseek-cli-resolver.js'); + const { listDeepSeekProfiles, resolveDefaultDeepSeekProfile, isDeepSeekRunnable } = await import( + '../../utils/deepseek-cli-resolver.js' + ); return { profile, package: pkg, diff --git a/test/docker-adopted-container.test.ts b/test/docker-adopted-container.test.ts index 36f86780..62d26cfd 100644 --- a/test/docker-adopted-container.test.ts +++ b/test/docker-adopted-container.test.ts @@ -123,7 +123,7 @@ describe('adopted container: the launch chain never mutates lifecycle', () => { expect(adopted).not.toContain('"'); expect(adopted).not.toContain('$('); // Every other line already quotes with the single-quote helper. - expect(adopted).toContain("grep -qx true"); + expect(adopted).toContain('grep -qx true'); }); it('skips the base-image gate, which describes an image adoption never uses', () => { @@ -142,6 +142,44 @@ describe('adopted container: the launch chain never mutates lifecycle', () => { }); }); +describe('adopted container: the probe request must reach the server', () => { + const ui = readFileSync(new URL('../src/web/public/session-ui.js', import.meta.url), 'utf8'); + const api = readFileSync(new URL('../src/web/public/api-client.js', import.meta.url), 'utf8'); + + it('never hands _apiJson an already-stringified body', () => { + // _api serializes `body` and sets Content-Type itself. Passing a string + // double-encodes it, the server sees a JSON string where it expects an + // object, and answers 400 INVALID_INPUT — which the caller reads as "the + // container could not be probed", so the menu silently showed every mode. + expect(api).toContain('fetchOpts.body = JSON.stringify(body)'); + const calls = [...ui.matchAll(/_apiJson\([^)]*\{[\s\S]{0,400}?\}\s*\)/g)].map((m) => m[0]); + expect(calls.length).toBeGreaterThan(0); + for (const call of calls) expect(call).not.toContain('body: JSON.stringify'); + }); + + it('hides every agent mode and says why when the container cannot be read', () => { + // Offering claude on a container that is not running is a click that can + // only fail, with the reason visible nowhere. + // Brace-matched, not a character window: slicing between two call sites + // silently yields '' when the second one appears ABOVE the first, and the + // assertion then passes over nothing. That has bitten this file twice. + const start = ui.indexOf('async _probeDockerCaseModes(activeCase, menu) {'); + expect(start).toBeGreaterThan(-1); + const open = ui.indexOf('{', start); + let depth = 0; + let fn = ''; + for (let i = open; i < ui.length; i++) { + if (ui[i] === '{') depth++; + else if (ui[i] === '}' && --depth === 0) { + fn = ui.slice(start, i + 1); + break; + } + } + expect(fn).toContain('_dockerCaseProbeError'); + expect(ui).toContain('_renderRunModeNotice'); + }); +}); + describe('adopted container: claude as root', () => { it('drops --dangerously-skip-permissions when the container runs as root', () => { // Claude Code refuses the flag as root ("cannot be used with root/sudo @@ -249,10 +287,22 @@ describe('adopted container: run modes come from the CONTAINER, not the host', ( /** Slice the method BODY. Anchored on the definition, not a call site: the * menu opener calls _loadRunModeHistory() ABOVE this definition, so slicing * between call sites silently yields an empty string and passes nothing. */ + /** + * The method BODY, delimited by brace depth rather than a character budget. + * A fixed window silently truncates the moment the method grows — which is + * exactly what happened twice: a comment added above the assertion pushed the + * asserted line past the cutoff and CI failed on a test that was still true. + */ const refreshFn = (src) => { const start = src.indexOf('_refreshRunModeAvailability(menu) {'); expect(start).toBeGreaterThan(-1); - return src.slice(start, start + 2000); + const open = src.indexOf('{', start); + let depth = 0; + for (let i = open; i < src.length; i++) { + if (src[i] === '{') depth++; + else if (src[i] === '}' && --depth === 0) return src.slice(start, i + 1); + } + throw new Error('unbalanced braces in _refreshRunModeAvailability'); }; it('gates a docker case on availableModes instead of host CLI probes', () => {