From a899a8b67aec053132194da81ac1362958953362 Mon Sep 17 00:00:00 2001 From: arkon Date: Thu, 26 Feb 2026 15:58:26 +0100 Subject: [PATCH] fix: address code review findings for OpenCode integration - Add _isStopped guard to OpenCode 3s readiness timeout (session.ts) - Block respawn for opencode sessions on interactive-respawn and respawn/enable routes (server.ts) - Fail fast in direct PTY fallback for OpenCode mode (session.ts) - Validate configContent as JSON at schema level (schemas.ts) - Update JSDoc example for createSession options API (tmux-manager.ts) - Un-hide Context tab for OpenCode sessions (index.html) - Add OpenCode UI tests (opencode-resize.test.ts) Co-Authored-By: Claude Opus 4.6 --- src/session.ts | 5 + src/tmux-manager.ts | 2 +- src/web/public/index.html | 2 +- src/web/schemas.ts | 5 +- src/web/server.ts | 13 +- test/opencode-resize.test.ts | 345 +++++++++++++++++++++++++++++++++++ 6 files changed, 367 insertions(+), 5 deletions(-) create mode 100644 test/opencode-resize.test.ts diff --git a/src/session.ts b/src/session.ts index 38328dd8..97f54059 100644 --- a/src/session.ts +++ b/src/session.ts @@ -989,6 +989,7 @@ export class Session extends EventEmitter { // Emit needsRefresh so the client fetches the full buffer once the TUI has rendered. this._promptCheckTimeout = setTimeout(() => { this._promptCheckTimeout = null; + if (this._isStopped) return; this._status = 'idle'; this.emit('needsRefresh'); }, 3000); @@ -1035,6 +1036,10 @@ export class Session extends EventEmitter { // Fallback to direct PTY if mux is not used if (!this.ptyProcess) { + // OpenCode sessions require tmux for env var injection (API keys via setenv) + if (this.mode === 'opencode') { + throw new Error('OpenCode sessions require tmux. Direct PTY fallback is not supported.'); + } try { // Pass --session-id to use the SAME ID as the Claudeman session // This ensures subagents can be directly matched to the correct tab diff --git a/src/tmux-manager.ts b/src/tmux-manager.ts index 7b2627d9..94731a77 100644 --- a/src/tmux-manager.ts +++ b/src/tmux-manager.ts @@ -258,7 +258,7 @@ function setOpenCodeConfigContent(muxName: string, config?: OpenCodeConfig): voi * const manager = new TmuxManager(); * * // Create a tmux session for Claude - * const session = await manager.createSession(sessionId, '/project', 'claude'); + * const session = await manager.createSession({ sessionId, workingDir: '/project', mode: 'claude' }); * * // Send input (single command, no delay!) * manager.sendInput(sessionId, '/clear\r'); diff --git a/src/web/public/index.html b/src/web/public/index.html index d9818b26..9d1cf6af 100644 --- a/src/web/public/index.html +++ b/src/web/public/index.html @@ -510,7 +510,7 @@
- +
diff --git a/src/web/schemas.ts b/src/web/schemas.ts index 8ef87ba6..78082c12 100644 --- a/src/web/schemas.ts +++ b/src/web/schemas.ts @@ -70,7 +70,10 @@ const OpenCodeConfigSchema = z.object({ autoAllowTools: z.boolean().optional(), continueSession: z.string().max(100).regex(/^[a-zA-Z0-9_-]+$/).optional(), forkSession: z.boolean().optional(), - configContent: z.string().max(10000).optional(), + configContent: z.string().max(10000).refine( + (val) => { try { JSON.parse(val); return true; } catch { return false; } }, + { message: 'configContent must be valid JSON' }, + ).optional(), }).optional(); export const CreateSessionSchema = z.object({ diff --git a/src/web/server.ts b/src/web/server.ts index 16c78cbd..08077521 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -2026,10 +2026,14 @@ export class WebServer extends EventEmitter { return createErrorResponse(ApiErrorCode.SESSION_BUSY, 'Session is busy'); } + // Respawn is not supported for opencode sessions + if (session.mode === 'opencode') { + return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Respawn is not supported for opencode sessions'); + } + try { // Auto-detect completion phrase from CLAUDE.md BEFORE starting (only if globally enabled and not explicitly disabled by user) - // Ralph tracker is not supported for opencode sessions - if (session.mode !== 'opencode' && this.store.getConfig().ralphEnabled && !session.ralphTracker.autoEnableDisabled) { + if (this.store.getConfig().ralphEnabled && !session.ralphTracker.autoEnableDisabled) { autoConfigureRalph(session, session.workingDir, () => {}); if (!session.ralphTracker.enabled) { session.ralphTracker.enable(); @@ -2084,6 +2088,11 @@ export class WebServer extends EventEmitter { return createErrorResponse(ApiErrorCode.NOT_FOUND, 'Session not found'); } + // Respawn is not supported for opencode sessions + if (session.mode === 'opencode') { + return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Respawn is not supported for opencode sessions'); + } + // Check if session is running (has a PID) if (!session.pid) { return createErrorResponse(ApiErrorCode.OPERATION_FAILED, 'Session is not running. Start it first.'); diff --git a/test/opencode-resize.test.ts b/test/opencode-resize.test.ts new file mode 100644 index 00000000..af330777 --- /dev/null +++ b/test/opencode-resize.test.ts @@ -0,0 +1,345 @@ +/** + * OpenCode session UI tests + * + * Tests OpenCode-specific UI behavior: + * - Initial terminal resize (not stuck at 120x40) + * - Close modal shows "Kill Tmux & OpenCode" (not "Claude Code") + * - needsRefresh handler sends resize + * + * Port: 3211 (opencode UI tests) + * + * Run: npx vitest run test/opencode-resize.test.ts + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { chromium, type Browser, type BrowserContext, type Page } from 'playwright'; +import { WebServer } from '../src/web/server.js'; + +const PORT = 3211; +const BASE_URL = `http://localhost:${PORT}`; + +let server: WebServer; +let browser: Browser; + +async function freshPage(): Promise<{ context: BrowserContext; page: Page }> { + const context = await browser.newContext({ + viewport: { width: 1280, height: 800 }, + }); + const page = await context.newPage(); + return { context, page }; +} + +async function navigateAndWait(page: Page): Promise { + await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' }); + await page.waitForFunction(() => document.body.classList.contains('app-loaded'), { + timeout: 5000, + }); +} + +beforeAll(async () => { + server = new WebServer(PORT, false, true); // testMode + await server.start(); + browser = await chromium.launch({ headless: true }); +}, 30_000); + +afterAll(async () => { + await browser?.close(); + await server?.stop(); +}, 30_000); + +describe('OpenCode session initial resize', () => { + let context: BrowserContext; + let page: Page; + + afterAll(async () => { + await context?.close(); + }); + + it('selectSession is not bypassed when runOpenCode sets activeSessionId', async () => { + // This test verifies at the code level that runOpenCode does NOT + // pre-set activeSessionId before calling selectSession. + // If it did, selectSession would early-return and skip sendResize. + ({ context, page } = await freshPage()); + await navigateAndWait(page); + + // Read the runOpenCode source from the live app and verify + // it doesn't assign activeSessionId before selectSession + const hasPreAssignment = await page.evaluate(() => { + const app = (window as unknown as { app: { runOpenCode: { toString: () => string } } }).app; + const source = app.runOpenCode.toString(); + + // Check: the source should NOT have activeSessionId = ... before selectSession + // Find positions of both patterns + const assignIdx = source.indexOf('this.activeSessionId = data.sessionId'); + const selectIdx = source.indexOf('this.selectSession(data.sessionId)'); + + // If assign doesn't exist at all, that's the correct fix + if (assignIdx === -1) return false; + + // If assign comes before select, that's the bug + return assignIdx < selectIdx; + }); + + expect(hasPreAssignment).toBe(false); + }); + + it('sends resize to server after creating a session via quick-start', async () => { + ({ context, page } = await freshPage()); + await navigateAndWait(page); + + // Intercept resize API calls to track when they happen + const resizeCalls: Array<{ url: string; cols: number; rows: number }> = []; + await page.route('**/api/sessions/*/resize', async (route) => { + const request = route.request(); + const body = request.postDataJSON(); + resizeCalls.push({ + url: request.url(), + cols: body.cols, + rows: body.rows, + }); + // Let the request through to the server + await route.continue(); + }); + + // Create a session via API (simulating what quick-start does) + const sessionId = await page.evaluate(async () => { + const res = await fetch('/api/sessions', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ workingDir: '/tmp', name: 'oc-resize-test' }), + }); + const data = await res.json(); + return data.id ?? data.session?.id; + }); + + expect(sessionId).toBeTruthy(); + + // Call selectSession (which is what runOpenCode does after fix) + await page.evaluate(async (sid: string) => { + const app = (window as unknown as { app: { selectSession: (id: string) => Promise } }).app; + await app.selectSession(sid); + }, sessionId); + + // Wait for the resize to be sent (it's fire-and-forget in selectSession) + await page.waitForTimeout(500); + + // Verify resize was called with reasonable dimensions (not 120x40 default) + expect(resizeCalls.length).toBeGreaterThanOrEqual(1); + const lastResize = resizeCalls[resizeCalls.length - 1]; + expect(lastResize.url).toContain(sessionId); + // Browser viewport is 1280x800 — terminal cols/rows should be substantially + // different from the hardcoded 120x40 default. xterm.js calculates these + // from container dimensions and cell size, but in headless mode with a + // 1280x800 viewport, we should get something reasonable (>= 40 cols). + expect(lastResize.cols).toBeGreaterThanOrEqual(40); + expect(lastResize.rows).toBeGreaterThanOrEqual(10); + + console.log(`[opencode-resize] resize sent: ${lastResize.cols}x${lastResize.rows}`); + + // Cleanup + await page.evaluate(async (sid: string) => { + await fetch(`/api/sessions/${sid}`, { method: 'DELETE' }); + }, sessionId); + }); + + it('selectSession does NOT early-return for a new session', async () => { + ({ context, page } = await freshPage()); + await navigateAndWait(page); + + // Create a session + const sessionId = await page.evaluate(async () => { + const res = await fetch('/api/sessions', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ workingDir: '/tmp', name: 'oc-earlyret-test' }), + }); + const data = await res.json(); + return data.id ?? data.session?.id; + }); + + expect(sessionId).toBeTruthy(); + + // Verify activeSessionId is NOT the new session before selectSession + const activeBeforeSelect = await page.evaluate(() => { + const app = (window as unknown as { app: { activeSessionId: string | null } }).app; + return app.activeSessionId; + }); + + // activeSessionId should be null or empty (welcome screen) — not our session + expect(activeBeforeSelect).not.toBe(sessionId); + + // Now call selectSession and verify it actually runs (sets activeSessionId) + await page.evaluate(async (sid: string) => { + const app = (window as unknown as { app: { selectSession: (id: string) => Promise } }).app; + await app.selectSession(sid); + }, sessionId); + + const activeAfterSelect = await page.evaluate(() => { + const app = (window as unknown as { app: { activeSessionId: string | null } }).app; + return app.activeSessionId; + }); + + expect(activeAfterSelect).toBe(sessionId); + + // Cleanup + await page.evaluate(async (sid: string) => { + await fetch(`/api/sessions/${sid}`, { method: 'DELETE' }); + }, sessionId); + }); + + it('needsRefresh handler includes sendResize call', async () => { + // The needsRefresh handler is registered inside a closure (connectSSE), + // so we can't directly invoke it from tests. Instead, verify that the + // handler source code dispatched to the EventSource includes sendResize. + // This is a structural test — if the handler code changes, this test + // ensures the resize call is preserved. + ({ context, page } = await freshPage()); + await navigateAndWait(page); + + // Dispatch a needsRefresh event on the EventSource and intercept + // the resulting resize API call + const sessionId = await page.evaluate(async () => { + const res = await fetch('/api/sessions', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ workingDir: '/tmp', name: 'oc-refresh-test' }), + }); + const data = await res.json(); + return data.id ?? data.session?.id; + }); + + expect(sessionId).toBeTruthy(); + + // Select the session first so activeSessionId is set + await page.evaluate(async (sid: string) => { + const app = (window as unknown as { app: { selectSession: (id: string) => Promise } }).app; + await app.selectSession(sid); + }, sessionId); + + await page.waitForTimeout(300); + + // Intercept resize calls + const resizeCalls: Array<{ url: string }> = []; + await page.route('**/api/sessions/*/resize', async (route) => { + resizeCalls.push({ url: route.request().url() }); + await route.continue(); + }); + + // Dispatch the needsRefresh event directly on the EventSource + // (this is how the server sends SSE events — as named events) + await page.evaluate((sid: string) => { + const app = (window as unknown as { app: { eventSource: EventSource } }).app; + if (app.eventSource) { + const event = new MessageEvent('session:needsRefresh', { + data: JSON.stringify({ id: sid }), + }); + app.eventSource.dispatchEvent(event); + } + }, sessionId); + + // Wait for the async handler (fetches /terminal buffer + sends resize) + await page.waitForTimeout(1500); + + // Verify resize was called + expect(resizeCalls.length).toBeGreaterThanOrEqual(1); + console.log(`[opencode-resize] needsRefresh triggered ${resizeCalls.length} resize call(s)`); + + // Cleanup + await page.route('**/api/sessions/*/resize', (route) => route.continue()); + await page.evaluate(async (sid: string) => { + await fetch(`/api/sessions/${sid}`, { method: 'DELETE' }); + }, sessionId); + }); +}); + +describe('OpenCode close modal text', () => { + let context: BrowserContext; + let page: Page; + + afterAll(async () => { + await context?.close(); + }); + + it('shows "Kill Tmux & OpenCode" for opencode sessions', async () => { + ({ context, page } = await freshPage()); + await navigateAndWait(page); + + // Create a session and mark it as opencode mode + const sessionId = await page.evaluate(async () => { + const res = await fetch('/api/sessions', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ workingDir: '/tmp', name: 'oc-close-test', mode: 'opencode' }), + }); + const data = await res.json(); + return data.id ?? data.session?.id; + }); + + expect(sessionId).toBeTruthy(); + + // Wait for SSE to propagate the session + await page.waitForTimeout(500); + + // Open the close confirmation modal + await page.evaluate((sid: string) => { + const app = (window as unknown as { app: { requestCloseSession: (id: string) => void } }).app; + app.requestCloseSession(sid); + }, sessionId); + + // Check the kill button text + const killTitle = await page.locator('#closeConfirmKillTitle').textContent(); + expect(killTitle).toBe('Kill Tmux & OpenCode'); + + // Close the modal + await page.evaluate(() => { + const app = (window as unknown as { app: { cancelCloseSession: () => void } }).app; + app.cancelCloseSession(); + }); + + // Cleanup + await page.evaluate(async (sid: string) => { + await fetch(`/api/sessions/${sid}`, { method: 'DELETE' }); + }, sessionId); + }); + + it('shows "Kill Tmux & Claude Code" for claude sessions', async () => { + ({ context, page } = await freshPage()); + await navigateAndWait(page); + + // Create a standard claude session + const sessionId = await page.evaluate(async () => { + const res = await fetch('/api/sessions', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ workingDir: '/tmp', name: 'cc-close-test' }), + }); + const data = await res.json(); + return data.id ?? data.session?.id; + }); + + expect(sessionId).toBeTruthy(); + + await page.waitForTimeout(500); + + // Open the close confirmation modal + await page.evaluate((sid: string) => { + const app = (window as unknown as { app: { requestCloseSession: (id: string) => void } }).app; + app.requestCloseSession(sid); + }, sessionId); + + // Check the kill button text + const killTitle = await page.locator('#closeConfirmKillTitle').textContent(); + expect(killTitle).toBe('Kill Tmux & Claude Code'); + + // Close the modal + await page.evaluate(() => { + const app = (window as unknown as { app: { cancelCloseSession: () => void } }).app; + app.cancelCloseSession(); + }); + + // Cleanup + await page.evaluate(async (sid: string) => { + await fetch(`/api/sessions/${sid}`, { method: 'DELETE' }); + }, sessionId); + }); +});