From dff4884d16c5a490d49c3d58c45942878aecc861 Mon Sep 17 00:00:00 2001 From: Proletariat Agent Date: Wed, 6 May 2026 14:55:00 -0600 Subject: [PATCH 1/2] feat(PRLT-1365): consolidate Docker runners into docker-management.ts; delete docker.ts --- apps/cli/src/lib/execution/runners.ts | 2 +- .../execution/runners/docker-management.ts | 133 +++++++++++++++++- apps/cli/src/lib/execution/runners/docker.ts | 125 ---------------- apps/cli/src/lib/execution/runners/index.ts | 6 +- apps/cli/src/lib/execution/runners/shared.ts | 67 ++------- apps/cli/src/lib/execution/types.ts | 19 +++ .../unit/docker-single-mount-path.test.ts | 60 ++++---- 7 files changed, 200 insertions(+), 212 deletions(-) delete mode 100644 apps/cli/src/lib/execution/runners/docker.ts diff --git a/apps/cli/src/lib/execution/runners.ts b/apps/cli/src/lib/execution/runners.ts index 9624d47e3..5223c14b1 100644 --- a/apps/cli/src/lib/execution/runners.ts +++ b/apps/cli/src/lib/execution/runners.ts @@ -8,7 +8,7 @@ * @see ./runners/shared.ts — Shared utilities * @see ./runners/host.ts — Host runner * @see ./runners/devcontainer.ts — Devcontainer runner - * @see ./runners/docker.ts — Docker runner + * @see ./runners/docker-management.ts — Container lifecycle + simple detached runDocker (PRLT-1365) * @see ./runners/orchestrator.ts — Orchestrator-in-Docker runner * @see ./runners/sandbox.ts — Sandbox runner * @see ./runners/cloud.ts — Cloud/VM runner diff --git a/apps/cli/src/lib/execution/runners/docker-management.ts b/apps/cli/src/lib/execution/runners/docker-management.ts index f4e679605..605736d17 100644 --- a/apps/cli/src/lib/execution/runners/docker-management.ts +++ b/apps/cli/src/lib/execution/runners/docker-management.ts @@ -14,18 +14,62 @@ import { ExecutorType, ExecutionContext, ExecutionConfig, + RunnerResult, } from '../types.js' import { readDevcontainerJson } from '../devcontainer.js' -import { isClaudeExecutor } from './executor.js' +import { isClaudeExecutor, getExecutorCommand } from './executor.js' import { CLAUDE_CREDENTIALS_VOLUME } from './docker-credentials.js' import { getMachineId } from '../../telemetry/analytics.js' import { getCCAppPermissionSettings, } from '../cc-version.js' +import { buildPrompt } from './prompt-builder.js' +import { validateCodexMode } from '../codex-adapter.js' /** Docker volume name for the shared pnpm store cache (PRLT-1130) */ export const PNPM_STORE_CACHE_VOLUME = 'pnpm-store-cache' +// ============================================================================= +// Docker Daemon Status (TKT-081) +// ============================================================================= + +export type DockerDaemonStatus = { + available: boolean + reason: 'ready' | 'not-installed' | 'daemon-not-ready' + message: string +} + +export function checkDockerDaemon(): DockerDaemonStatus { + try { + execSync('which docker', { stdio: 'pipe', timeout: 3000 }) + } catch { + return { available: false, reason: 'not-installed', message: 'Docker is not installed.' } + } + const timeout = 5000 + try { + execSync('docker ps -q --no-trunc', { stdio: 'pipe', timeout }) + return { available: true, reason: 'ready', message: 'Docker daemon is ready.' } + } catch (error: unknown) { + const stderr = (error as { stderr?: Buffer })?.stderr?.toString() || '' + const isTimeout = (error as { killed?: boolean })?.killed === true + let message: string + if (isTimeout) { + message = 'Docker daemon is not responding (timed out after 5s). Docker Desktop may be initializing or stuck — check for license/login prompts.' + } else if (stderr.includes('500') || stderr.includes('Internal Server Error')) { + message = 'Docker daemon is returning errors (500). Docker Desktop needs attention — check for license/login prompts.' + } else if (stderr.includes('connect') || stderr.includes('Cannot connect') || stderr.includes('Is the docker daemon running')) { + message = 'Docker daemon is not running. Start Docker Desktop and try again.' + } else { + message = `Docker daemon is not ready: ${stderr.trim() || 'unknown error'}. Check Docker Desktop status.` + } + return { available: false, reason: 'daemon-not-ready', message } + } +} + +export function isDockerRunning(): boolean { + return checkDockerDaemon().available +} + /** * Parse a Docker memory string (e.g., '4g', '512m', '2048m') into bytes. */ @@ -945,3 +989,90 @@ export function ensureDockerContainer( } return result.containerId } + +// ============================================================================= +// Simple detached Docker runner (PRLT-1365) +// +// Runs a one-shot prompt in a detached `docker run -d` container. Unlike the +// devcontainer pipeline (image build + entrypoint + tmux), this path is for +// the 'docker' execution environment — a fire-and-forget container that +// executes the prompt and exits. Both paths share buildContainerMounts() as +// the single source of truth for volume mounts. +// ============================================================================= + +export async function runDocker( + context: ExecutionContext, + executor: ExecutorType, + config: ExecutionConfig +): Promise { + const prompt = buildPrompt(context) + const containerName = `work-${context.ticketId}-${Date.now()}` + + try { + const dockerStatus = checkDockerDaemon() + if (!dockerStatus.available) { + return { + success: false, + error: `Docker daemon is not available. ${dockerStatus.message}`, + } + } + + const mounts = buildContainerMounts(context, executor) + + let dockerCmd = `docker run -d --name ${containerName}` + dockerCmd += ` ${mounts.join(' ')}` + dockerCmd += ` -w /workspace` + dockerCmd += ` -e TICKET_ID="${context.ticketId}"` + + if (config.docker.network) { + dockerCmd += ` --network ${config.docker.network}` + } + if (config.docker.memory) { + dockerCmd += ` --memory ${config.docker.memory}` + } + if (config.docker.cpus) { + dockerCmd += ` --cpus ${config.docker.cpus}` + } + + // HEALTHCHECK for heartbeat-based stale detection. Session watcher reads + // this status via `docker inspect`. + dockerCmd += ` --health-cmd "kill -0 1 || exit 1"` + dockerCmd += ` --health-interval 5m` + dockerCmd += ` --health-timeout 10s` + dockerCmd += ` --health-retries 3` + dockerCmd += ` --health-start-period 30s` + + if (executor === 'codex') { + const codexPermission: PermissionMode = config.permissionMode + const modeError = validateCodexMode(codexPermission, 'non-tty') + if (modeError) { + return { success: false, error: modeError.message } + } + } + + const escapedPrompt = prompt.replace(/'/g, "'\\''") + const { cmd, args } = getExecutorCommand(executor, escapedPrompt, config.permissionMode === 'danger') + + dockerCmd += ` ${config.docker.image}` + if (isClaudeExecutor(executor)) { + // TKT-053: Disable plan mode — detached, no user to approve. + // PRLT-950: Use -- to separate flags from positional prompt argument. + dockerCmd += ` ${cmd} --print --disallowedTools EnterPlanMode -- '${escapedPrompt}'` + } else { + const argsStr = args.map(a => a === escapedPrompt ? `'${escapedPrompt}'` : a).join(' ') + dockerCmd += ` ${cmd} ${argsStr}` + } + + const containerId = execSync(dockerCmd, { encoding: 'utf-8' }).trim() + + return { + success: true, + containerId: containerId.substring(0, 12), + } + } catch (error) { + return { + success: false, + error: error instanceof Error ? error.message : 'Failed to start docker container', + } + } +} diff --git a/apps/cli/src/lib/execution/runners/docker.ts b/apps/cli/src/lib/execution/runners/docker.ts deleted file mode 100644 index e8d99f569..000000000 --- a/apps/cli/src/lib/execution/runners/docker.ts +++ /dev/null @@ -1,125 +0,0 @@ -/** - * Docker Runner - * - * @deprecated Prefer the devcontainer runner for new code paths. - * This module is kept for backward compatibility with the 'docker' execution - * environment type but delegates mount construction to buildContainerMounts() - * in docker-management.ts so that all Docker code paths share a single source - * of truth for volume mounts (PRLT-1364). - * - * Runs commands in detached Docker containers. - * Uses simple docker run for standalone container execution. - */ - -import { - execSync, - PermissionMode, - ExecutorType, - ExecutionContext, - ExecutionConfig, - validateCodexMode, -} from './shared.js' - -import { - RunnerResult, - buildPrompt, - getExecutorCommand, - isClaudeExecutor, - checkDockerDaemon, - buildContainerMounts, -} from './shared.js' - -/** - * Run command in a detached Docker container. - * Uses simple docker run with -d flag for background execution. - * - * @deprecated Prefer the devcontainer runner (runDevcontainer) for new code. - * PRLT-1364: Now delegates to buildContainerMounts() for volume mounts - * instead of building mounts inline. - */ -export async function runDocker( - context: ExecutionContext, - executor: ExecutorType, - config: ExecutionConfig -): Promise { - const prompt = buildPrompt(context) - const containerName = `work-${context.ticketId}-${Date.now()}` - - try { - // Check if docker is available and daemon is responsive (TKT-081) - const dockerStatus = checkDockerDaemon() - if (!dockerStatus.available) { - return { - success: false, - error: `Docker daemon is not available. ${dockerStatus.message}`, - } - } - - // PRLT-1364: Use buildContainerMounts() — single source of truth for - // all Docker volume mounts including credential bind-mounts, HQ, PMO, - // repo worktrees, and pnpm store cache. - const mounts = buildContainerMounts(context, executor) - - // Build docker run command - let dockerCmd = `docker run -d --name ${containerName}` - dockerCmd += ` ${mounts.join(' ')}` - dockerCmd += ` -w /workspace` - dockerCmd += ` -e TICKET_ID="${context.ticketId}"` - - if (config.docker.network) { - dockerCmd += ` --network ${config.docker.network}` - } - if (config.docker.memory) { - dockerCmd += ` --memory ${config.docker.memory}` - } - if (config.docker.cpus) { - dockerCmd += ` --cpus ${config.docker.cpus}` - } - - // Add Docker HEALTHCHECK for heartbeat-based stale detection. - // Checks if the main process (PID 1) is still running every 5 minutes. - // The session watcher uses `docker inspect` to read this health status. - dockerCmd += ` --health-cmd "kill -0 1 || exit 1"` - dockerCmd += ` --health-interval 5m` - dockerCmd += ` --health-timeout 10s` - dockerCmd += ` --health-retries 3` - dockerCmd += ` --health-start-period 30s` - - // Validate Codex mode: Docker runner is always non-tty (detached with -d) - if (executor === 'codex') { - const codexPermission: PermissionMode = config.permissionMode - const modeError = validateCodexMode(codexPermission, 'non-tty') - if (modeError) { - return { success: false, error: modeError.message } - } - } - - // Build executor command using getExecutorCommand() for correct invocation - const escapedPrompt = prompt.replace(/'/g, "'\\''") - const { cmd, args } = getExecutorCommand(executor, escapedPrompt, config.permissionMode === 'danger') - - // For Claude Code in Docker, use --print for non-interactive output - // Non-Claude executors use their native command format from getExecutorCommand() - dockerCmd += ` ${config.docker.image}` - if (isClaudeExecutor(executor)) { - // TKT-053: Disable plan mode — Docker runner is always detached (no user to approve) - // PRLT-950: Use -- to separate flags from positional prompt argument. - dockerCmd += ` ${cmd} --print --disallowedTools EnterPlanMode -- '${escapedPrompt}'` - } else { - const argsStr = args.map(a => a === escapedPrompt ? `'${escapedPrompt}'` : a).join(' ') - dockerCmd += ` ${cmd} ${argsStr}` - } - - const containerId = execSync(dockerCmd, { encoding: 'utf-8' }).trim() - - return { - success: true, - containerId: containerId.substring(0, 12), - } - } catch (error) { - return { - success: false, - error: error instanceof Error ? error.message : 'Failed to start docker container', - } - } -} diff --git a/apps/cli/src/lib/execution/runners/index.ts b/apps/cli/src/lib/execution/runners/index.ts index 2606f9afc..35e2864fa 100644 --- a/apps/cli/src/lib/execution/runners/index.ts +++ b/apps/cli/src/lib/execution/runners/index.ts @@ -8,7 +8,7 @@ * - shared.ts — Shared utilities (session names, credentials, docker, prompts, etc.) * - host.ts — Host runner with tmux session persistence * - devcontainer.ts — Docker container runner with raw Docker commands - * - docker.ts — Simple detached Docker container runner + * - docker-management.ts — Container lifecycle + simple detached runDocker (PRLT-1365) * - orchestrator.ts — Orchestrator-in-Docker runner (sibling container pattern) * - sandbox.ts — srt sandbox runner (wraps host runner) * - cloud.ts — Remote execution via SSH @@ -28,7 +28,7 @@ import { import { RunnerResult, ensureTmuxServerHasKeychainAccess } from './shared.js' import { runHost } from './host.js' import { runDevcontainer } from './devcontainer.js' -import { runDocker } from './docker.js' +import { runDocker } from './docker-management.js' import { runSandbox } from './sandbox.js' import { runCloud } from './cloud.js' @@ -131,7 +131,7 @@ export { // Individual runners export { runHost } from './host.js' export { runDevcontainer, buildDevcontainerCommand } from './devcontainer.js' -export { runDocker } from './docker.js' +export { runDocker } from './docker-management.js' export { runOrchestratorInDocker } from './orchestrator.js' export { runSandbox, isSrtInstalled, buildSrtCommand } from './sandbox.js' export { runCloud, runVm } from './cloud.js' diff --git a/apps/cli/src/lib/execution/runners/shared.ts b/apps/cli/src/lib/execution/runners/shared.ts index 44d2565e2..09ab905f4 100644 --- a/apps/cli/src/lib/execution/runners/shared.ts +++ b/apps/cli/src/lib/execution/runners/shared.ts @@ -27,6 +27,8 @@ import { ExecutionConfig, DEFAULT_EXECUTION_CONFIG, normalizeEnvironment, + RunnerResult, + Runner, } from '../types.js' import type { TerminalApp } from '../types.js' import { getSetTitleCommands } from '../../terminal.js' @@ -35,25 +37,6 @@ import type { OrchestratorDockerOptions } from '../devcontainer.js' import { getCodexCommand, resolveCodexExecutionContext, validateCodexMode, CodexModeError } from '../codex-adapter.js' import { resolveToolsForSpawn } from '../../tool-registry/index.js' -// ============================================================================= -// Runner Interface -// ============================================================================= - -export interface RunnerResult { - success: boolean - pid?: string - containerId?: string - sessionId?: string - logPath?: string - error?: string -} - -export type Runner = ( - context: ExecutionContext, - executor: ExecutorType, - config: ExecutionConfig -) => Promise - // ============================================================================= // Terminal Title Helpers // ============================================================================= @@ -285,46 +268,9 @@ export function ensureWorkflowScope(): boolean { } // ============================================================================= -// Docker Status Check +// Docker Status Check (re-exported from docker-management.js) // ============================================================================= -export type DockerDaemonStatus = { - available: boolean - reason: 'ready' | 'not-installed' | 'daemon-not-ready' - message: string -} - -export function checkDockerDaemon(): DockerDaemonStatus { - try { - execSync('which docker', { stdio: 'pipe', timeout: 3000 }) - } catch { - return { available: false, reason: 'not-installed', message: 'Docker is not installed.' } - } - const timeout = 5000 - try { - execSync('docker ps -q --no-trunc', { stdio: 'pipe', timeout }) - return { available: true, reason: 'ready', message: 'Docker daemon is ready.' } - } catch (error: unknown) { - const stderr = (error as { stderr?: Buffer })?.stderr?.toString() || '' - const isTimeout = (error as { killed?: boolean })?.killed === true - let message: string - if (isTimeout) { - message = 'Docker daemon is not responding (timed out after 5s). Docker Desktop may be initializing or stuck — check for license/login prompts.' - } else if (stderr.includes('500') || stderr.includes('Internal Server Error')) { - message = 'Docker daemon is returning errors (500). Docker Desktop needs attention — check for license/login prompts.' - } else if (stderr.includes('connect') || stderr.includes('Cannot connect') || stderr.includes('Is the docker daemon running')) { - message = 'Docker daemon is not running. Start Docker Desktop and try again.' - } else { - message = `Docker daemon is not ready: ${stderr.trim() || 'unknown error'}. Check Docker Desktop status.` - } - return { available: false, reason: 'daemon-not-ready', message } - } -} - -export function isDockerRunning(): boolean { - return checkDockerDaemon().available -} - /** @deprecated No longer required - we use raw Docker commands now */ export function isDevcontainerCliInstalled(): boolean { return true @@ -377,9 +323,12 @@ export { verifyCredentialMount, seedClaudeOnboarding, checkDockerMemoryCapacity, + checkDockerDaemon, + isDockerRunning, + runDocker, } from './docker-management.js' -export type { SpawnStageError, EnsureDockerContainerResult } from './docker-management.js' +export type { SpawnStageError, EnsureDockerContainerResult, DockerDaemonStatus } from './docker-management.js' export { buildIntegrationCommandsSection, @@ -405,6 +354,8 @@ export { ExecutionConfig, DEFAULT_EXECUTION_CONFIG, normalizeEnvironment, + RunnerResult, + Runner, getSetTitleCommands, readDevcontainerJson, generateOrchestratorDockerfile, diff --git a/apps/cli/src/lib/execution/types.ts b/apps/cli/src/lib/execution/types.ts index 7d5011a54..c2d7762c0 100644 --- a/apps/cli/src/lib/execution/types.ts +++ b/apps/cli/src/lib/execution/types.ts @@ -205,6 +205,25 @@ export interface WorkspaceManifest { action?: string } +// ============================================================================= +// Runner Result +// ============================================================================= + +export interface RunnerResult { + success: boolean + pid?: string + containerId?: string + sessionId?: string + logPath?: string + error?: string +} + +export type Runner = ( + context: ExecutionContext, + executor: ExecutorType, + config: ExecutionConfig +) => Promise + // ============================================================================= // Execution Context // ============================================================================= diff --git a/apps/cli/test/unit/docker-single-mount-path.test.ts b/apps/cli/test/unit/docker-single-mount-path.test.ts index 2c3f79f32..48d2f0e31 100644 --- a/apps/cli/test/unit/docker-single-mount-path.test.ts +++ b/apps/cli/test/unit/docker-single-mount-path.test.ts @@ -1,47 +1,54 @@ /** - * PRLT-1364: Single source of truth for Docker container mounts + * PRLT-1364 / PRLT-1365: Single source of truth for Docker container mounts + * and consolidated Docker runner code path. * * Ensures: - * 1. docker.ts (simple runner) uses buildContainerMounts() — not inline mounts + * 1. runDocker (simple detached runner) lives in docker-management.ts and uses buildContainerMounts() * 2. docker-management.ts createDockerContainer() uses buildContainerMounts() * 3. devcontainer.ts calls ensureDockerContainerDetailed() (which calls buildContainerMounts()) * 4. buildContainerMounts() includes ~/.claude bind-mount for claude-code executor * 5. buildContainerMounts() does NOT include ~/.claude for non-claude executors - * 6. docker.ts is marked as deprecated + * 6. The legacy docker.ts file no longer exists (PRLT-1365) * 7. No Docker code path builds its own mount list inline * - * Regression guard: if docker.ts is reverted to inline mounts, or if a new - * Docker code path is added that skips buildContainerMounts(), these tests fail. + * Regression guard: if a separate Docker runner file is reintroduced, or if a + * new Docker code path is added that skips buildContainerMounts(), these tests fail. */ import { expect } from 'chai' import * as fs from 'node:fs' import * as path from 'node:path' import { fileURLToPath } from 'node:url' -import { buildContainerMounts } from '../../src/lib/execution/runners/docker-management.js' +import { buildContainerMounts, runDocker } from '../../src/lib/execution/runners/docker-management.js' import type { ExecutionContext } from '../../src/lib/execution/types.js' const __dirname = path.dirname(fileURLToPath(import.meta.url)) const RUNNERS_DIR = path.resolve(__dirname, '../../src/lib/execution/runners') -describe('Single mount code path (PRLT-1364)', () => { +describe('Single mount code path (PRLT-1364) + consolidated runner (PRLT-1365)', () => { // ========================================================================= - // 1. docker.ts uses buildContainerMounts() — not inline mounts + // 1. runDocker lives in docker-management.ts and uses buildContainerMounts() // ========================================================================= - describe('docker.ts — delegates to buildContainerMounts()', () => { - const dockerFile = path.join(RUNNERS_DIR, 'docker.ts') + describe('runDocker — consolidated into docker-management.ts (PRLT-1365)', () => { + const managementFile = path.join(RUNNERS_DIR, 'docker-management.ts') let content: string before(() => { - content = fs.readFileSync(dockerFile, 'utf-8') + content = fs.readFileSync(managementFile, 'utf-8') + }) + + it('runDocker is exported as a function', () => { + expect(typeof runDocker).to.equal('function') }) - it('should import buildContainerMounts from shared', () => { - expect(content).to.include('buildContainerMounts') + it('runDocker is defined inside docker-management.ts (not a separate file)', () => { + const fnMatch = content.match( + /export async function runDocker\([\s\S]*?^}/m + ) + expect(fnMatch, 'runDocker function not found in docker-management.ts').to.exist }) - it('should call buildContainerMounts(context, executor)', () => { - // Look for the actual call — not just the import + it('runDocker calls buildContainerMounts(context, executor)', () => { const fnMatch = content.match( /export async function runDocker\([\s\S]*?^}/m ) @@ -50,19 +57,19 @@ describe('Single mount code path (PRLT-1364)', () => { expect(fnBody).to.include('buildContainerMounts(context, executor)') }) - it('should NOT have inline -v worktreePath:/workspace mount', () => { + it('runDocker does NOT have inline -v worktreePath:/workspace mount', () => { const fnMatch = content.match( /export async function runDocker\([\s\S]*?^}/m ) expect(fnMatch, 'runDocker function not found').to.exist const fnBody = fnMatch![0] - - // The old inline mount that was replaced: + // The old inline mount that was replaced by PRLT-1364: expect(fnBody).to.not.include('-v "${context.worktreePath}:/workspace"') }) - it('should be marked as @deprecated', () => { - expect(content).to.include('@deprecated') + it('the legacy docker.ts file has been removed', () => { + const legacyFile = path.join(RUNNERS_DIR, 'docker.ts') + expect(fs.existsSync(legacyFile), 'legacy docker.ts should be deleted').to.equal(false) }) }) @@ -174,13 +181,18 @@ describe('Single mount code path (PRLT-1364)', () => { // 5. No other Docker runner builds inline mounts (regression guard) // ========================================================================= describe('Regression guard — no inline mount building in runners', () => { - it('docker.ts should not contain -v "${context. patterns outside of comments', () => { - const dockerContent = fs.readFileSync( - path.join(RUNNERS_DIR, 'docker.ts'), + it('runDocker in docker-management.ts should not contain -v "${context. patterns', () => { + const managementContent = fs.readFileSync( + path.join(RUNNERS_DIR, 'docker-management.ts'), 'utf-8' ) + const fnMatch = managementContent.match( + /export async function runDocker\([\s\S]*?^}/m + ) + expect(fnMatch, 'runDocker function not found').to.exist + const fnBody = fnMatch![0] // Strip comments to check only code - const codeLines = dockerContent.split('\n') + const codeLines = fnBody.split('\n') .filter(line => !line.trim().startsWith('//') && !line.trim().startsWith('*')) .join('\n') From 9108adcb171d7c8cef15836bdb46a80b72096c6e Mon Sep 17 00:00:00 2001 From: Proletariat Agent Date: Wed, 6 May 2026 15:51:28 -0600 Subject: [PATCH 2/2] feat(PRLT-1365): fix lint: drop duplicate RunnerResult/Runner re-exports from runners/index --- apps/cli/src/lib/execution/runners/index.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/apps/cli/src/lib/execution/runners/index.ts b/apps/cli/src/lib/execution/runners/index.ts index 35e2864fa..0f9e0cb48 100644 --- a/apps/cli/src/lib/execution/runners/index.ts +++ b/apps/cli/src/lib/execution/runners/index.ts @@ -78,10 +78,9 @@ export async function runExecution( // ============================================================================= // Shared utilities +// Note: RunnerResult and Runner come from types.js (re-exported via execution/index.ts) +// to avoid duplicate-export lint errors. export { - // Runner types - RunnerResult, - Runner, // Session/title helpers buildSessionName, buildWindowTitle,