diff --git a/docs/iloom-commands.md b/docs/iloom-commands.md index d2d7ae1d..4fcd2fb7 100644 --- a/docs/iloom-commands.md +++ b/docs/iloom-commands.md @@ -654,6 +654,79 @@ il dev-server feat/my-branch - Use Ctrl+C to stop the server - Respects `sourceEnvOnStart` setting for environment loading +#### Docker Dev Server Mode + +By default, `il dev-server` runs your dev server as a native process. For frameworks that ignore the `PORT` environment variable (e.g., Angular CLI), you can use Docker mode to remap ports via Docker's `-p` flag. + +**Requirements:** +- Docker must be installed and the Docker daemon must be running +- A `Dockerfile` in your project (or a custom path configured) + +**Configuration:** + +Set `capabilities.web.devServer` to `"docker"` in your `.iloom/settings.json`: + +```json +{ + "capabilities": { + "web": { + "devServer": "docker", + "dockerFile": "./Dockerfile", + "containerPort": 4200, + "dockerBuildArgs": { + "NODE_ENV": "development" + }, + "dockerRunArgs": ["-v", "./src:/app/src"] + } + } +} +``` + +**Configuration Fields:** + +| Field | Type | Default | Description | +|-------|------|---------|-------------| +| `devServer` | `"process"` \| `"docker"` | `"process"` | Dev server execution mode. `"process"` runs natively, `"docker"` runs inside a Docker container with port mapping. | +| `dockerFile` | `string` | `"./Dockerfile"` | Path to the Dockerfile relative to the worktree root. Only used when `devServer` is `"docker"`. | +| `containerPort` | `number` | Auto-detected | Port the application listens on inside the Docker container. If not set, iloom attempts to detect it from `EXPOSE` directives in the built Docker image (via `docker image inspect`), falling back to Dockerfile parsing. | +| `dockerBuildArgs` | `Record` | - | Build arguments passed to `docker build` (e.g., `{"NODE_ENV": "development"}`). | +| `dockerRunArgs` | `string[]` | - | Additional flags passed to `docker run`. Use this for volume mounts, environment variables, user mapping, and other Docker options. | + +**How Port Mapping Works:** + +Each loom workspace gets a unique port calculated as `basePort + issue/PR number` (e.g., issue #25 gets port 3025). In Docker mode: + +1. The Docker image is built from your Dockerfile +2. The container runs with `-p :` +3. Your app runs on `containerPort` inside the container (e.g., 4200 for Angular) +4. Docker maps that to the workspace port on the host (e.g., 3025) +5. You access the app at `http://localhost:3025` as usual + +This means frameworks that hardcode their listen port work correctly -- Docker handles the port translation transparently. + +**Container Naming:** + +Containers are named `iloom-dev-` where the identifier is derived from the issue/PR number or branch name. Special characters (slashes, etc.) are replaced with hyphens. + +**Known Limitations:** + +- **macOS volume mount performance:** Docker Desktop on macOS has known slow file-watching performance through bind mounts. This can affect hot reload when using volume mounts via `dockerRunArgs`. Consider using tools like `mutagen` or Docker's `synchronized` file sharing if performance is an issue. +- **Linux file permissions:** Docker containers typically run as root, so files created by the container via volume mounts may be owned by root on the host. Mitigate by passing `--user` via `dockerRunArgs`: + ```json + { + "dockerRunArgs": ["--user", "1000:1000"] + } + ``` +- **Container orphaning on crash:** If the `il dev-server` process is killed ungracefully (e.g., SIGKILL), the Docker container may keep running. Use `docker stop` to clean up orphaned containers: + ```bash + # List any orphaned iloom containers + docker ps --filter "name=iloom-dev-" + + # Stop all orphaned iloom containers + docker stop $(docker ps -q --filter "name=iloom-dev-") + ``` + Normal cleanup via `il cleanup`, `il finish`, or Ctrl+C handles container shutdown automatically. + --- ### il build diff --git a/src/commands/dev-server.test.ts b/src/commands/dev-server.test.ts index a565181e..16d24073 100644 --- a/src/commands/dev-server.test.ts +++ b/src/commands/dev-server.test.ts @@ -3,6 +3,7 @@ import { DevServerCommand } from './dev-server.js' import { GitWorktreeManager } from '../lib/GitWorktreeManager.js' import { ProjectCapabilityDetector } from '../lib/ProjectCapabilityDetector.js' import { DevServerManager } from '../lib/DevServerManager.js' +import { DockerManager } from '../lib/DockerManager.js' import { SettingsManager } from '../lib/SettingsManager.js' import { IdentifierParser } from '../utils/IdentifierParser.js' import { loadWorkspaceEnv, isNoEnvFilesFoundError } from '../utils/env.js' @@ -14,6 +15,7 @@ import fs from 'fs-extra' vi.mock('../lib/GitWorktreeManager.js') vi.mock('../lib/ProjectCapabilityDetector.js') vi.mock('../lib/DevServerManager.js') +vi.mock('../lib/DockerManager.js') vi.mock('../utils/IdentifierParser.js') vi.mock('fs-extra') vi.mock('../lib/SettingsManager.js') @@ -209,7 +211,8 @@ describe('DevServerCommand', () => { 3087, false, expect.any(Function), - expect.any(Object) + expect.any(Object), + undefined ) }) @@ -293,7 +296,8 @@ describe('DevServerCommand', () => { 4500, false, expect.any(Function), - expect.any(Object) + expect.any(Object), + undefined ) }) @@ -315,7 +319,8 @@ describe('DevServerCommand', () => { 3087, false, expect.any(Function), - expect.any(Object) + expect.any(Object), + undefined ) }) }) @@ -402,7 +407,8 @@ describe('DevServerCommand', () => { 3087, false, expect.any(Function), - expect.any(Object) + expect.any(Object), + undefined ) }) @@ -417,7 +423,8 @@ describe('DevServerCommand', () => { 3087, true, expect.any(Function), - expect.any(Object) + expect.any(Object), + undefined ) }) }) @@ -458,7 +465,8 @@ describe('DevServerCommand', () => { 3087, false, expect.any(Function), - { DATABASE_URL: 'postgres://test', API_KEY: 'secret' } + { DATABASE_URL: 'postgres://test', API_KEY: 'secret' }, + undefined ) }) @@ -475,7 +483,8 @@ describe('DevServerCommand', () => { 3087, false, expect.any(Function), - {} + {}, + undefined ) }) @@ -490,7 +499,8 @@ describe('DevServerCommand', () => { 3087, false, expect.any(Function), - {} + {}, + undefined ) }) @@ -513,7 +523,8 @@ describe('DevServerCommand', () => { 3087, false, expect.any(Function), - {} + {}, + undefined ) }) @@ -533,4 +544,211 @@ describe('DevServerCommand', () => { expect(mockDevServerManager.runServerForeground).toHaveBeenCalled() }) }) + + describe('Docker mode', () => { + beforeEach(() => { + vi.mocked(mockIdentifierParser.parseForPatternDetection).mockResolvedValue({ + type: 'issue', + number: 87, + originalInput: '87', + }) + + vi.mocked(mockGitWorktreeManager.findWorktreeForIssue).mockResolvedValue(mockWorktree) + + const mockCapabilities: ProjectCapabilities = { + capabilities: ['web'], + binEntries: {}, + } + vi.mocked(mockCapabilityDetector.detectCapabilities).mockResolvedValue(mockCapabilities) + + vi.mocked(fs.pathExists).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue('PORT=3087\n') + + // Docker is available by default in Docker mode tests + vi.mocked(DockerManager.assertAvailable).mockResolvedValue(undefined) + }) + + it('should construct DockerConfig and pass to isServerRunning and runServerForeground when devServer is "docker"', async () => { + const expectedDockerConfig = { + dockerFile: './Dockerfile.dev', + containerPort: 4200, + dockerBuildArgs: { NODE_ENV: 'development' }, + dockerRunArgs: ['-v', './src:/app/src'], + identifier: '87', + } + + vi.mocked(mockSettingsManager.loadSettings).mockResolvedValue({ + capabilities: { + web: { + devServer: 'docker', + dockerFile: './Dockerfile.dev', + containerPort: 4200, + dockerBuildArgs: { NODE_ENV: 'development' }, + dockerRunArgs: ['-v', './src:/app/src'], + }, + }, + }) + + vi.mocked(DockerManager.buildDockerConfigFromSettings).mockReturnValue(expectedDockerConfig) + + await command.execute({ identifier: '87' }) + + expect(DockerManager.assertAvailable).toHaveBeenCalled() + expect(mockDevServerManager.isServerRunning).toHaveBeenCalledWith( + 3087, + expectedDockerConfig + ) + expect(mockDevServerManager.runServerForeground).toHaveBeenCalledWith( + mockWorktree.path, + 3087, + false, + expect.any(Function), + {}, + expectedDockerConfig + ) + }) + + it('should use default dockerFile when not specified in settings', async () => { + const expectedDockerConfig = { + dockerFile: './Dockerfile', + containerPort: undefined, + dockerBuildArgs: undefined, + dockerRunArgs: undefined, + identifier: '87', + } + + vi.mocked(mockSettingsManager.loadSettings).mockResolvedValue({ + capabilities: { + web: { + devServer: 'docker', + }, + }, + }) + + vi.mocked(DockerManager.buildDockerConfigFromSettings).mockReturnValue(expectedDockerConfig) + + await command.execute({ identifier: '87' }) + + expect(mockDevServerManager.runServerForeground).toHaveBeenCalledWith( + mockWorktree.path, + 3087, + false, + expect.any(Function), + {}, + expect.objectContaining({ + dockerFile: './Dockerfile', + identifier: '87', + }) + ) + }) + + it('should use branchName as identifier when number is not available', async () => { + vi.mocked(mockIdentifierParser.parseForPatternDetection).mockResolvedValue({ + type: 'branch', + branchName: 'feat/docker-support', + originalInput: 'feat/docker-support', + }) + + vi.mocked(mockGitWorktreeManager.findWorktreeForBranch).mockResolvedValue(mockWorktree) + + vi.mocked(mockSettingsManager.loadSettings).mockResolvedValue({ + capabilities: { + web: { + devServer: 'docker', + }, + }, + }) + + vi.mocked(DockerManager.buildDockerConfigFromSettings).mockReturnValue({ + dockerFile: './Dockerfile', + containerPort: undefined, + dockerBuildArgs: undefined, + dockerRunArgs: undefined, + identifier: 'feat/docker-support', + }) + + await command.execute({ identifier: 'feat/docker-support' }) + + expect(mockDevServerManager.runServerForeground).toHaveBeenCalledWith( + mockWorktree.path, + 3087, + false, + expect.any(Function), + {}, + expect.objectContaining({ + identifier: 'feat/docker-support', + }) + ) + }) + + it('should throw when Docker is not available in Docker mode', async () => { + vi.mocked(DockerManager.assertAvailable).mockRejectedValue( + new Error( + 'Docker is not available. Please ensure Docker is installed and the Docker daemon is running.' + ) + ) + + vi.mocked(mockSettingsManager.loadSettings).mockResolvedValue({ + capabilities: { + web: { + devServer: 'docker', + }, + }, + }) + + // Return a config so that the code path reaches assertAvailable + vi.mocked(DockerManager.buildDockerConfigFromSettings).mockReturnValue({ + dockerFile: './Dockerfile', + containerPort: undefined, + dockerBuildArgs: undefined, + dockerRunArgs: undefined, + identifier: '87', + }) + + await expect(command.execute({ identifier: '87' })).rejects.toThrow( + 'Docker is not available' + ) + + // Should not proceed to run server + expect(mockDevServerManager.runServerForeground).not.toHaveBeenCalled() + }) + + it('should not call DockerManager.assertAvailable when devServer is "process"', async () => { + vi.mocked(mockSettingsManager.loadSettings).mockResolvedValue({ + capabilities: { + web: { + devServer: 'process', + }, + }, + }) + + await command.execute({ identifier: '87' }) + + expect(DockerManager.assertAvailable).not.toHaveBeenCalled() + expect(mockDevServerManager.isServerRunning).toHaveBeenCalledWith( + 3087, + undefined + ) + expect(mockDevServerManager.runServerForeground).toHaveBeenCalledWith( + mockWorktree.path, + 3087, + false, + expect.any(Function), + {}, + undefined + ) + }) + + it('should not call DockerManager.assertAvailable when devServer is not configured', async () => { + vi.mocked(mockSettingsManager.loadSettings).mockResolvedValue({}) + + await command.execute({ identifier: '87' }) + + expect(DockerManager.assertAvailable).not.toHaveBeenCalled() + expect(mockDevServerManager.isServerRunning).toHaveBeenCalledWith( + 3087, + undefined + ) + }) + }) }) diff --git a/src/commands/dev-server.ts b/src/commands/dev-server.ts index c1739f72..80685619 100644 --- a/src/commands/dev-server.ts +++ b/src/commands/dev-server.ts @@ -2,6 +2,7 @@ import path from 'path' import { GitWorktreeManager } from '../lib/GitWorktreeManager.js' import { ProjectCapabilityDetector } from '../lib/ProjectCapabilityDetector.js' import { DevServerManager } from '../lib/DevServerManager.js' +import { DockerManager } from '../lib/DockerManager.js' import { SettingsManager } from '../lib/SettingsManager.js' import { IdentifierParser } from '../utils/IdentifierParser.js' import { loadWorkspaceEnv, isNoEnvFilesFoundError } from '../utils/env.js' @@ -65,10 +66,22 @@ export class DevServerCommand { logger.debug(`Found worktree at: ${worktree.path}`) - // 3. Load settings to check sourceEnvOnStart + // 3. Load settings to check sourceEnvOnStart and Docker config const settings = await this.settingsManager.loadSettings() const shouldLoadEnv = settings.sourceEnvOnStart ?? false + // 3a. Extract Docker configuration if Docker mode is enabled + const identifier = parsed.number?.toString() ?? parsed.branchName ?? parsed.originalInput + const dockerConfig = DockerManager.buildDockerConfigFromSettings( + settings.capabilities?.web, + identifier + ) + + if (dockerConfig) { + await DockerManager.assertAvailable() + logger.debug(`Docker mode enabled with config: ${JSON.stringify(dockerConfig)}`) + } + // Build environment variables let envOverrides: Record = {} @@ -117,7 +130,7 @@ export class DevServerCommand { const url = `http://localhost:${port}` // 6. Check if server already running - const isRunning = await this.devServerManager.isServerRunning(port) + const isRunning = await this.devServerManager.isServerRunning(port, dockerConfig) if (isRunning) { const message = `Dev server already running at ${url}` @@ -165,7 +178,8 @@ export class DevServerCommand { this.outputJson(finalResult) } }, - envOverrides + envOverrides, + dockerConfig ) if (processInfo.pid) { diff --git a/src/commands/open.test.ts b/src/commands/open.test.ts index cedf63db..a5bbd189 100644 --- a/src/commands/open.test.ts +++ b/src/commands/open.test.ts @@ -657,7 +657,8 @@ describe('OpenCommand', () => { expect(mockDevServerManager.ensureServerRunning).toHaveBeenCalledWith( mockWorktree.path, - 3087 + 3087, + undefined ) }) @@ -687,7 +688,8 @@ describe('OpenCommand', () => { // Should calculate port as 3000 + 87 = 3087 expect(mockDevServerManager.ensureServerRunning).toHaveBeenCalledWith( mockWorktree.path, - 3087 + 3087, + undefined ) }) @@ -703,7 +705,8 @@ describe('OpenCommand', () => { // Should use PORT from .env expect(mockDevServerManager.ensureServerRunning).toHaveBeenCalledWith( mockWorktree.path, - 4500 + 4500, + undefined ) }) diff --git a/src/commands/open.ts b/src/commands/open.ts index f94ed2d7..59f886f3 100644 --- a/src/commands/open.ts +++ b/src/commands/open.ts @@ -4,6 +4,7 @@ import { execa } from 'execa' import { GitWorktreeManager } from '../lib/GitWorktreeManager.js' import { ProjectCapabilityDetector } from '../lib/ProjectCapabilityDetector.js' import { DevServerManager } from '../lib/DevServerManager.js' +import { DockerManager } from '../lib/DockerManager.js' import { SettingsManager } from '../lib/SettingsManager.js' import { IdentifierParser } from '../utils/IdentifierParser.js' import { openBrowser } from '../utils/browser.js' @@ -225,10 +226,24 @@ export class OpenCommand { checkEnvFile: true, }) + // Extract Docker configuration if Docker mode is enabled + const issueNumber = extractIssueNumber(worktree.branch) + const dockerIdentifier = issueNumber?.toString() ?? worktree.branch + const dockerConfig = DockerManager.buildDockerConfigFromSettings( + settings.capabilities?.web, + dockerIdentifier + ) + + if (dockerConfig) { + await DockerManager.assertAvailable() + logger.debug(`Docker mode enabled with config: ${JSON.stringify(dockerConfig)}`) + } + // Ensure dev server is running on the port const serverReady = await this.devServerManager.ensureServerRunning( worktree.path, - port + port, + dockerConfig ) if (!serverReady) { diff --git a/src/commands/run.test.ts b/src/commands/run.test.ts index a56ae10b..5fdb675c 100644 --- a/src/commands/run.test.ts +++ b/src/commands/run.test.ts @@ -592,7 +592,8 @@ describe('RunCommand', () => { expect(mockDevServerManager.ensureServerRunning).toHaveBeenCalledWith( mockWorktree.path, - 3087 + 3087, + undefined ) }) diff --git a/src/commands/run.ts b/src/commands/run.ts index 7bf83334..f609968f 100644 --- a/src/commands/run.ts +++ b/src/commands/run.ts @@ -4,6 +4,7 @@ import { execa } from 'execa' import { GitWorktreeManager } from '../lib/GitWorktreeManager.js' import { ProjectCapabilityDetector } from '../lib/ProjectCapabilityDetector.js' import { DevServerManager } from '../lib/DevServerManager.js' +import { DockerManager } from '../lib/DockerManager.js' import { SettingsManager } from '../lib/SettingsManager.js' import { IdentifierParser } from '../utils/IdentifierParser.js' import { openBrowser } from '../utils/browser.js' @@ -267,10 +268,24 @@ export class RunCommand { checkEnvFile: true, }) + // Extract Docker configuration if Docker mode is enabled + const issueNumber = extractIssueNumber(worktree.branch) + const dockerIdentifier = issueNumber?.toString() ?? worktree.branch + const dockerConfig = DockerManager.buildDockerConfigFromSettings( + settings.capabilities?.web, + dockerIdentifier + ) + + if (dockerConfig) { + await DockerManager.assertAvailable() + logger.debug(`Docker mode enabled with config: ${JSON.stringify(dockerConfig)}`) + } + // Ensure dev server is running on the port const serverReady = await this.devServerManager.ensureServerRunning( worktree.path, - port + port, + dockerConfig ) if (!serverReady) { diff --git a/src/lib/DevServerManager.test.ts b/src/lib/DevServerManager.test.ts index 5d045b36..258a808c 100644 --- a/src/lib/DevServerManager.test.ts +++ b/src/lib/DevServerManager.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' -import { DevServerManager } from './DevServerManager.js' +import { DevServerManager, type DockerConfig } from './DevServerManager.js' import { ProcessManager } from './process/ProcessManager.js' +import { DockerManager } from './DockerManager.js' import { execa, type ExecaChildProcess } from 'execa' import { setTimeout } from 'timers/promises' import * as devServerUtils from '../utils/dev-server.js' @@ -11,6 +12,7 @@ import * as packageJsonUtils from '../utils/package-json.js' vi.mock('execa') vi.mock('timers/promises') vi.mock('./process/ProcessManager.js') +vi.mock('./DockerManager.js') vi.mock('../utils/dev-server.js') vi.mock('../utils/package-manager.js') vi.mock('../utils/package-json.js') @@ -582,6 +584,397 @@ describe('DevServerManager', () => { }) }) + describe('Docker mode', () => { + const dockerConfig: DockerConfig = { + dockerFile: './Dockerfile', + containerPort: 4200, + identifier: '548', + } + + beforeEach(() => { + // Set up default return values for DockerManager static methods + vi.mocked(DockerManager.buildContainerName).mockReturnValue('iloom-dev-548') + vi.mocked(DockerManager.buildImageName).mockReturnValue('iloom-dev-548') + }) + + describe('ensureServerRunning', () => { + it('should detect running container and skip start', async () => { + const port = 3548 + + vi.mocked(DockerManager.isContainerRunning).mockResolvedValue(true) + + const result = await manager.ensureServerRunning(mockWorktreePath, port, dockerConfig) + + expect(result).toBe(true) + expect(DockerManager.isContainerRunning).toHaveBeenCalledWith('iloom-dev-548') + expect(DockerManager.buildImage).not.toHaveBeenCalled() + }) + + it('should build image then run container in background when not running', async () => { + const port = 3548 + + vi.mocked(DockerManager.isContainerRunning).mockResolvedValue(false) + vi.mocked(DockerManager.buildImage).mockResolvedValue(undefined) + vi.mocked(DockerManager.resolveContainerPort).mockResolvedValue(4200) + vi.mocked(DockerManager.runDetached).mockResolvedValue('abc123') + + // Server becomes ready on first poll + vi.mocked(mockProcessManager.detectDevServer).mockResolvedValueOnce({ + pid: 12345, + name: 'docker-proxy', + command: 'docker', + port, + isDevServer: true, + }) + + const result = await manager.ensureServerRunning(mockWorktreePath, port, dockerConfig) + + expect(result).toBe(true) + expect(DockerManager.buildImage).toHaveBeenCalledWith( + mockWorktreePath, + 'iloom-dev-548', + './Dockerfile', + undefined + ) + expect(DockerManager.resolveContainerPort).toHaveBeenCalledWith( + 4200, + expect.stringContaining('Dockerfile'), + 'iloom-dev-548' + ) + expect(DockerManager.runDetached).toHaveBeenCalledWith( + 'iloom-dev-548', + 'iloom-dev-548', + port, + 4200, + undefined + ) + }) + + it('should return false when Docker build fails', async () => { + const port = 3548 + + vi.mocked(DockerManager.isContainerRunning).mockResolvedValue(false) + vi.mocked(DockerManager.buildImage).mockRejectedValue(new Error('Build failed')) + + const result = await manager.ensureServerRunning(mockWorktreePath, port, dockerConfig) + + expect(result).toBe(false) + }) + + it('should clean up container and return false when server fails to start within timeout', async () => { + const port = 3548 + + manager = new DevServerManager(mockProcessManager, { + startupTimeout: 500, + checkInterval: 100, + }) + + vi.mocked(DockerManager.isContainerRunning).mockResolvedValue(false) + vi.mocked(DockerManager.buildImage).mockResolvedValue(undefined) + vi.mocked(DockerManager.resolveContainerPort).mockResolvedValue(4200) + vi.mocked(DockerManager.runDetached).mockResolvedValue('abc123') + vi.mocked(DockerManager.stopAndRemoveContainer).mockResolvedValue(true) + + // Server never starts + vi.mocked(mockProcessManager.detectDevServer).mockResolvedValue(null) + vi.mocked(setTimeout).mockResolvedValue(undefined) + + const result = await manager.ensureServerRunning(mockWorktreePath, port, dockerConfig) + + expect(result).toBe(false) + // Should clean up the container on timeout + expect(DockerManager.stopAndRemoveContainer).toHaveBeenCalledWith('iloom-dev-548') + }) + + it('should pass build args and run args to Docker', async () => { + const port = 3548 + const configWithArgs: DockerConfig = { + ...dockerConfig, + dockerBuildArgs: { NODE_ENV: 'development' }, + dockerRunArgs: ['-v', './src:/app/src'], + } + + vi.mocked(DockerManager.isContainerRunning).mockResolvedValue(false) + vi.mocked(DockerManager.buildImage).mockResolvedValue(undefined) + vi.mocked(DockerManager.resolveContainerPort).mockResolvedValue(4200) + vi.mocked(DockerManager.runDetached).mockResolvedValue('abc123') + + vi.mocked(mockProcessManager.detectDevServer).mockResolvedValueOnce({ + pid: 12345, + name: 'docker-proxy', + command: 'docker', + port, + isDevServer: true, + }) + + await manager.ensureServerRunning(mockWorktreePath, port, configWithArgs) + + expect(DockerManager.buildImage).toHaveBeenCalledWith( + mockWorktreePath, + 'iloom-dev-548', + './Dockerfile', + { NODE_ENV: 'development' } + ) + expect(DockerManager.runDetached).toHaveBeenCalledWith( + 'iloom-dev-548', + 'iloom-dev-548', + port, + 4200, + ['-v', './src:/app/src'] + ) + }) + + it('should not use process-based detection in Docker mode', async () => { + const port = 3548 + + vi.mocked(DockerManager.isContainerRunning).mockResolvedValue(true) + + await manager.ensureServerRunning(mockWorktreePath, port, dockerConfig) + + // Process-based detection should NOT be called in Docker mode + expect(mockProcessManager.detectDevServer).not.toHaveBeenCalled() + }) + }) + + describe('isServerRunning', () => { + it('should check Docker container status when dockerConfig provided', async () => { + const port = 3548 + vi.mocked(DockerManager.isContainerRunning).mockResolvedValue(true) + + const result = await manager.isServerRunning(port, dockerConfig) + + expect(result).toBe(true) + expect(DockerManager.buildContainerName).toHaveBeenCalledWith('548') + expect(mockProcessManager.detectDevServer).not.toHaveBeenCalled() + }) + + it('should return false when Docker container is not running', async () => { + const port = 3548 + vi.mocked(DockerManager.isContainerRunning).mockResolvedValue(false) + + const result = await manager.isServerRunning(port, dockerConfig) + + expect(result).toBe(false) + }) + + it('should fall back to process detection when no dockerConfig', async () => { + const port = 3548 + vi.mocked(mockProcessManager.detectDevServer).mockResolvedValue({ + pid: 12345, + name: 'node', + command: 'pnpm dev', + port, + isDevServer: true, + }) + + const result = await manager.isServerRunning(port) + + expect(result).toBe(true) + expect(mockProcessManager.detectDevServer).toHaveBeenCalledWith(port) + expect(DockerManager.isContainerRunning).not.toHaveBeenCalled() + }) + }) + + describe('runServerForeground', () => { + it('should build image then run container in foreground', async () => { + const port = 3548 + + vi.mocked(DockerManager.buildImage).mockResolvedValue(undefined) + vi.mocked(DockerManager.resolveContainerPort).mockResolvedValue(4200) + vi.mocked(DockerManager.runForeground).mockResolvedValue(undefined) + + const result = await manager.runServerForeground( + mockWorktreePath, port, false, undefined, undefined, dockerConfig + ) + + expect(result).toEqual({}) + expect(DockerManager.buildImage).toHaveBeenCalledWith( + mockWorktreePath, + 'iloom-dev-548', + './Dockerfile', + undefined + ) + expect(DockerManager.runForeground).toHaveBeenCalledWith( + 'iloom-dev-548', + 'iloom-dev-548', + port, + 4200, + undefined, + false + ) + }) + + it('should pass redirectToStderr to DockerManager.runForeground', async () => { + const port = 3548 + + vi.mocked(DockerManager.buildImage).mockResolvedValue(undefined) + vi.mocked(DockerManager.resolveContainerPort).mockResolvedValue(4200) + vi.mocked(DockerManager.runForeground).mockResolvedValue(undefined) + + await manager.runServerForeground( + mockWorktreePath, port, true, undefined, undefined, dockerConfig + ) + + expect(DockerManager.runForeground).toHaveBeenCalledWith( + 'iloom-dev-548', + 'iloom-dev-548', + port, + 4200, + undefined, + true + ) + }) + + it('should pass dockerRunArgs to container', async () => { + const port = 3548 + const configWithRunArgs: DockerConfig = { + ...dockerConfig, + dockerRunArgs: ['-v', './src:/app/src', '--env', 'DEBUG=true'], + } + + vi.mocked(DockerManager.buildImage).mockResolvedValue(undefined) + vi.mocked(DockerManager.resolveContainerPort).mockResolvedValue(4200) + vi.mocked(DockerManager.runForeground).mockResolvedValue(undefined) + + await manager.runServerForeground( + mockWorktreePath, port, false, undefined, undefined, configWithRunArgs + ) + + expect(DockerManager.runForeground).toHaveBeenCalledWith( + 'iloom-dev-548', + 'iloom-dev-548', + port, + 4200, + ['-v', './src:/app/src', '--env', 'DEBUG=true'], + false + ) + }) + + it('should use containerPort from config or Dockerfile EXPOSE', async () => { + const port = 3548 + const configWithoutPort: DockerConfig = { + dockerFile: './Dockerfile', + identifier: '548', + } + + vi.mocked(DockerManager.buildImage).mockResolvedValue(undefined) + vi.mocked(DockerManager.resolveContainerPort).mockResolvedValue(8080) + vi.mocked(DockerManager.runForeground).mockResolvedValue(undefined) + + await manager.runServerForeground( + mockWorktreePath, port, false, undefined, undefined, configWithoutPort + ) + + // resolveContainerPort should receive undefined for containerPort + expect(DockerManager.resolveContainerPort).toHaveBeenCalledWith( + undefined, + expect.stringContaining('Dockerfile'), + 'iloom-dev-548' + ) + // runForeground should use the resolved port + expect(DockerManager.runForeground).toHaveBeenCalledWith( + 'iloom-dev-548', + 'iloom-dev-548', + port, + 8080, + undefined, + false + ) + }) + + it('should call onProcessStarted with undefined pid in Docker mode', async () => { + const port = 3548 + const onStart = vi.fn() + + vi.mocked(DockerManager.buildImage).mockResolvedValue(undefined) + vi.mocked(DockerManager.resolveContainerPort).mockResolvedValue(4200) + vi.mocked(DockerManager.runForeground).mockResolvedValue(undefined) + + await manager.runServerForeground( + mockWorktreePath, port, false, onStart, undefined, dockerConfig + ) + + // Docker containers don't have a host PID to report + expect(onStart).toHaveBeenCalledWith(undefined) + }) + + it('should not use runScript or buildDevServerCommand in Docker mode', async () => { + const port = 3548 + + vi.mocked(DockerManager.buildImage).mockResolvedValue(undefined) + vi.mocked(DockerManager.resolveContainerPort).mockResolvedValue(4200) + vi.mocked(DockerManager.runForeground).mockResolvedValue(undefined) + + await manager.runServerForeground( + mockWorktreePath, port, false, undefined, undefined, dockerConfig + ) + + expect(packageManagerUtils.runScript).not.toHaveBeenCalled() + expect(devServerUtils.buildDevServerCommand).not.toHaveBeenCalled() + expect(execa).not.toHaveBeenCalled() + }) + }) + + describe('cleanup', () => { + it('should stop and remove tracked Docker containers', async () => { + const port = 3548 + + vi.mocked(DockerManager.isContainerRunning).mockResolvedValue(false) + vi.mocked(DockerManager.buildImage).mockResolvedValue(undefined) + vi.mocked(DockerManager.resolveContainerPort).mockResolvedValue(4200) + vi.mocked(DockerManager.runDetached).mockResolvedValue('abc123') + vi.mocked(DockerManager.stopAndRemoveContainer).mockResolvedValue(true) + + // Server becomes ready + vi.mocked(mockProcessManager.detectDevServer).mockResolvedValueOnce({ + pid: 12345, + name: 'docker-proxy', + command: 'docker', + port, + isDevServer: true, + }) + + await manager.ensureServerRunning(mockWorktreePath, port, dockerConfig) + + // Reset mock to track cleanup call + vi.mocked(DockerManager.stopAndRemoveContainer).mockClear() + vi.mocked(DockerManager.stopAndRemoveContainer).mockResolvedValue(true) + + await manager.cleanup() + + expect(DockerManager.stopAndRemoveContainer).toHaveBeenCalledWith('iloom-dev-548') + }) + + it('should handle Docker cleanup errors gracefully', async () => { + const port = 3548 + + vi.mocked(DockerManager.isContainerRunning).mockResolvedValue(false) + vi.mocked(DockerManager.buildImage).mockResolvedValue(undefined) + vi.mocked(DockerManager.resolveContainerPort).mockResolvedValue(4200) + vi.mocked(DockerManager.runDetached).mockResolvedValue('abc123') + + // Server becomes ready + vi.mocked(mockProcessManager.detectDevServer).mockResolvedValueOnce({ + pid: 12345, + name: 'docker-proxy', + command: 'docker', + port, + isDevServer: true, + }) + + await manager.ensureServerRunning(mockWorktreePath, port, dockerConfig) + + // Make cleanup fail + vi.mocked(DockerManager.stopAndRemoveContainer).mockRejectedValue( + new Error('Docker daemon not responding') + ) + + // Should not throw + await expect(manager.cleanup()).resolves.not.toThrow() + }) + }) + }) + describe('default options', () => { it('should use default timeout (180s) and interval if not specified', () => { const defaultManager = new DevServerManager() diff --git a/src/lib/DevServerManager.ts b/src/lib/DevServerManager.ts index b225863d..4464a88a 100644 --- a/src/lib/DevServerManager.ts +++ b/src/lib/DevServerManager.ts @@ -1,6 +1,8 @@ import { execa, type ExecaChildProcess } from 'execa' +import path from 'path' import { setTimeout } from 'timers/promises' import { ProcessManager } from './process/ProcessManager.js' +import { DockerManager, type DockerConfig } from './DockerManager.js' import { buildDevServerCommand } from '../utils/dev-server.js' import { runScript } from '../utils/package-manager.js' import { getPackageScripts } from '../utils/package-json.js' @@ -38,6 +40,9 @@ export interface DevServerManagerOptions { checkInterval?: number } +// Re-export DockerConfig from DockerManager for backward compatibility +export type { DockerConfig } from './DockerManager.js' + /** * DevServerManager handles auto-starting and monitoring dev servers * Used by open/run commands to ensure dev server is running before opening browser @@ -46,6 +51,7 @@ export class DevServerManager { private readonly processManager: ProcessManager private readonly options: Required private runningServers: Map = new Map() + private runningDockerContainers: Map = new Map() constructor( processManager?: ProcessManager, @@ -64,12 +70,34 @@ export class DevServerManager { * * @param worktreePath - Path to the worktree * @param port - Port the server should run on + * @param dockerConfig - Optional Docker configuration for container-based server * @returns true if server is ready, false if startup failed/timed out */ - async ensureServerRunning(worktreePath: string, port: number): Promise { + async ensureServerRunning(worktreePath: string, port: number, dockerConfig?: DockerConfig): Promise { logger.debug(`Checking if dev server is running on port ${port}...`) - // Check if already running + // Docker mode: check if container is already running + if (dockerConfig) { + const containerName = DockerManager.buildContainerName(dockerConfig.identifier) + const isRunning = await DockerManager.isContainerRunning(containerName) + if (isRunning) { + logger.debug(`Docker container "${containerName}" already running on port ${port}`) + return true + } + + logger.info(`Docker dev server not running on port ${port}, starting...`) + try { + await this.startDockerServer(worktreePath, port, dockerConfig) + return true + } catch (error) { + logger.error( + `Failed to start Docker dev server: ${error instanceof Error ? error.message : 'Unknown error'}` + ) + return false + } + } + + // Process mode: check if a process is listening on the port const existingProcess = await this.processManager.detectDevServer(port) if (existingProcess) { logger.debug( @@ -139,6 +167,59 @@ export class DevServerManager { logger.success(`Dev server started successfully on port ${port}`) } + /** + * Start dev server in Docker container (background) and wait for it to be ready. + * Builds the image, resolves the container port, starts the container detached, + * and polls the host port for readiness. + */ + private async startDockerServer(worktreePath: string, port: number, dockerConfig: DockerConfig): Promise { + const imageName = DockerManager.buildImageName(dockerConfig.identifier) + const containerName = DockerManager.buildContainerName(dockerConfig.identifier) + const dockerfilePath = path.resolve(worktreePath, dockerConfig.dockerFile) + + // Build image + await DockerManager.buildImage( + worktreePath, + imageName, + dockerConfig.dockerFile, + dockerConfig.dockerBuildArgs + ) + + // Resolve container port (config > image inspect > Dockerfile EXPOSE) + const containerPort = await DockerManager.resolveContainerPort( + dockerConfig.containerPort, + dockerfilePath, + imageName + ) + + // Run container detached + await DockerManager.runDetached( + imageName, + containerName, + port, + containerPort, + dockerConfig.dockerRunArgs + ) + + // Track for cleanup + this.runningDockerContainers.set(port, containerName) + + // Wait for server to be ready (Docker proxy listens on host port) + logger.info(`Waiting for Docker dev server to start on port ${port}...`) + const ready = await this.waitForServerReady(port) + + if (!ready) { + // Clean up the container if startup failed + await DockerManager.stopAndRemoveContainer(containerName) + this.runningDockerContainers.delete(port) + throw new Error( + `Docker dev server failed to start within ${this.options.startupTimeout}ms timeout` + ) + } + + logger.success(`Docker dev server started successfully on port ${port}`) + } + /** * Wait for server to be ready by polling the port */ @@ -174,9 +255,14 @@ export class DevServerManager { * Check if a dev server is running on the specified port * * @param port - Port to check + * @param dockerConfig - Optional Docker configuration; when provided, checks container status * @returns true if server is running, false otherwise */ - async isServerRunning(port: number): Promise { + async isServerRunning(port: number, dockerConfig?: DockerConfig): Promise { + if (dockerConfig) { + const containerName = DockerManager.buildContainerName(dockerConfig.identifier) + return DockerManager.isContainerRunning(containerName) + } const existingProcess = await this.processManager.detectDevServer(port) return existingProcess !== null } @@ -196,8 +282,56 @@ export class DevServerManager { port: number, redirectToStderr = false, onProcessStarted?: (pid?: number) => void, - envOverrides?: Record + envOverrides?: Record, + dockerConfig?: DockerConfig ): Promise<{ pid?: number }> { + // Docker mode: build image and run container in foreground + if (dockerConfig) { + logger.debug(`Starting Docker dev server in foreground on port ${port}`) + + const imageName = DockerManager.buildImageName(dockerConfig.identifier) + const containerName = DockerManager.buildContainerName(dockerConfig.identifier) + const dockerfilePath = path.resolve(worktreePath, dockerConfig.dockerFile) + + // Build image + await DockerManager.buildImage( + worktreePath, + imageName, + dockerConfig.dockerFile, + dockerConfig.dockerBuildArgs + ) + + // Resolve container port + const containerPort = await DockerManager.resolveContainerPort( + dockerConfig.containerPort, + dockerfilePath, + imageName + ) + + if (onProcessStarted) { + onProcessStarted(undefined) + } + + // Track container for cleanup + this.runningDockerContainers.set(port, containerName) + try { + // Run container in foreground (blocks until stopped) + // DockerManager.runForeground handles signal forwarding internally + await DockerManager.runForeground( + imageName, + containerName, + port, + containerPort, + dockerConfig.dockerRunArgs, + redirectToStderr + ) + } finally { + this.runningDockerContainers.delete(port) + } + + return {} + } + logger.debug(`Starting dev server in foreground on port ${port}`) // Use runScript for foreground mode to support multi-language projects @@ -244,6 +378,7 @@ export class DevServerManager { * This should be called when the manager is being disposed */ async cleanup(): Promise { + // Clean up process-based servers for (const [port, serverProcess] of this.runningServers.entries()) { try { logger.debug(`Cleaning up server process on port ${port}`) @@ -255,5 +390,18 @@ export class DevServerManager { } } this.runningServers.clear() + + // Clean up Docker containers + for (const [port, containerName] of this.runningDockerContainers.entries()) { + try { + logger.debug(`Cleaning up Docker container "${containerName}" on port ${port}`) + await DockerManager.stopAndRemoveContainer(containerName) + } catch (error) { + logger.warn( + `Failed to stop Docker container "${containerName}" on port ${port}: ${error instanceof Error ? error.message : 'Unknown error'}` + ) + } + } + this.runningDockerContainers.clear() } } diff --git a/src/lib/DockerManager.test.ts b/src/lib/DockerManager.test.ts new file mode 100644 index 00000000..4b3fa9f7 --- /dev/null +++ b/src/lib/DockerManager.test.ts @@ -0,0 +1,796 @@ +import { describe, it, expect, vi } from 'vitest' +import { DockerManager } from './DockerManager.js' +import { execa } from 'execa' +import { readFile } from 'fs/promises' + +// Mock dependencies +vi.mock('execa') +vi.mock('fs/promises') + +// Mock the logger +vi.mock('../utils/logger.js', () => ({ + logger: { + info: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + debug: vi.fn(), + success: vi.fn(), + }, +})) + +describe('DockerManager', () => { + describe('isAvailable', () => { + it('should return true when docker CLI is accessible', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 0, + stdout: '', + stderr: '', + } as never) + + const result = await DockerManager.isAvailable() + + expect(result).toBe(true) + expect(execa).toHaveBeenCalledWith('docker', ['info'], { reject: false }) + }) + + it('should return false when docker CLI is not found', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 1, + stdout: '', + stderr: 'Cannot connect to the Docker daemon', + } as never) + + const result = await DockerManager.isAvailable() + + expect(result).toBe(false) + }) + + it('should return false when execa throws', async () => { + vi.mocked(execa).mockRejectedValue(new Error('command not found: docker')) + + const result = await DockerManager.isAvailable() + + expect(result).toBe(false) + }) + }) + + describe('assertAvailable', () => { + it('should not throw when Docker is available', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 0, + stdout: '', + stderr: '', + } as never) + + await expect(DockerManager.assertAvailable()).resolves.toBeUndefined() + }) + + it('should throw with install instructions when Docker is not available', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 1, + stdout: '', + stderr: '', + } as never) + + await expect(DockerManager.assertAvailable()).rejects.toThrow( + 'Docker is not available' + ) + await expect(DockerManager.assertAvailable()).rejects.toThrow( + 'https://docs.docker.com/get-docker/' + ) + }) + }) + + describe('buildImage', () => { + it('should build with default Dockerfile path', async () => { + vi.mocked(execa).mockResolvedValue({ exitCode: 0 } as never) + + await DockerManager.buildImage('/test/worktree', 'my-image', './Dockerfile') + + expect(execa).toHaveBeenCalledWith( + 'docker', + ['build', '-t', 'my-image', '-f', './Dockerfile', '.'], + { cwd: '/test/worktree', stdio: 'inherit' } + ) + }) + + it('should build with custom Dockerfile path', async () => { + vi.mocked(execa).mockResolvedValue({ exitCode: 0 } as never) + + await DockerManager.buildImage('/test/worktree', 'my-image', './docker/Dockerfile.dev') + + expect(execa).toHaveBeenCalledWith( + 'docker', + ['build', '-t', 'my-image', '-f', './docker/Dockerfile.dev', '.'], + { cwd: '/test/worktree', stdio: 'inherit' } + ) + }) + + it('should pass build args when configured', async () => { + vi.mocked(execa).mockResolvedValue({ exitCode: 0 } as never) + + await DockerManager.buildImage( + '/test/worktree', + 'my-image', + './Dockerfile', + { NODE_ENV: 'development', API_URL: 'http://localhost:3000' } + ) + + expect(execa).toHaveBeenCalledWith( + 'docker', + [ + 'build', '-t', 'my-image', '-f', './Dockerfile', + '--build-arg', 'NODE_ENV=development', + '--build-arg', 'API_URL=http://localhost:3000', + '.', + ], + { cwd: '/test/worktree', stdio: 'inherit' } + ) + }) + + it('should throw on build failure with clear error message', async () => { + vi.mocked(execa).mockRejectedValue(new Error('Step 3/7 : RUN npm install\n---> Running in abc123\nnpm ERR! code E404')) + + await expect( + DockerManager.buildImage('/test/worktree', 'my-image', './Dockerfile') + ).rejects.toThrow('Docker build failed for image "my-image"') + }) + }) + + describe('runDetached', () => { + it('should run with port mapping -p hostPort:containerPort', async () => { + // First call: force-remove existing container (reject: false) + // Second call: docker run + vi.mocked(execa) + .mockResolvedValueOnce({ exitCode: 0 } as never) // rm -f + .mockResolvedValueOnce({ exitCode: 0, stdout: 'abc123def456' } as never) // run -d + + const containerId = await DockerManager.runDetached( + 'my-image', 'iloom-dev-123', 3123, 4200 + ) + + expect(containerId).toBe('abc123def456') + // First call: force-remove + expect(execa).toHaveBeenNthCalledWith(1, 'docker', ['rm', '-f', 'iloom-dev-123'], { reject: false }) + // Second call: run + expect(execa).toHaveBeenNthCalledWith(2, 'docker', [ + 'run', '-d', + '--name', 'iloom-dev-123', + '-p', '3123:4200', + 'my-image', + ]) + }) + + it('should pass additional dockerRunArgs', async () => { + vi.mocked(execa) + .mockResolvedValueOnce({ exitCode: 0 } as never) // rm -f + .mockResolvedValueOnce({ exitCode: 0, stdout: 'abc123' } as never) // run + + await DockerManager.runDetached( + 'my-image', 'iloom-dev-123', 3123, 4200, + ['-v', './src:/app/src', '--env', 'DEBUG=true'] + ) + + expect(execa).toHaveBeenNthCalledWith(2, 'docker', [ + 'run', '-d', + '--name', 'iloom-dev-123', + '-p', '3123:4200', + '-v', './src:/app/src', '--env', 'DEBUG=true', + 'my-image', + ]) + }) + + it('should use named container iloom-dev-{identifier}', async () => { + vi.mocked(execa) + .mockResolvedValueOnce({ exitCode: 0 } as never) // rm -f + .mockResolvedValueOnce({ exitCode: 0, stdout: 'containerid' } as never) // run + + await DockerManager.runDetached( + 'my-image', 'iloom-dev-548', 3548, 4200 + ) + + expect(execa).toHaveBeenNthCalledWith(2, 'docker', expect.arrayContaining([ + '--name', 'iloom-dev-548', + ])) + }) + + it('should throw on run failure with error details', async () => { + vi.mocked(execa) + .mockResolvedValueOnce({ exitCode: 0 } as never) // rm -f + .mockRejectedValueOnce(new Error('port already allocated')) + + await expect( + DockerManager.runDetached('my-image', 'iloom-dev-123', 3123, 4200) + ).rejects.toThrow('Failed to start Docker container "iloom-dev-123"') + }) + }) + + describe('runForeground', () => { + it('should run attached with --rm flag and port mapping', async () => { + vi.mocked(execa) + .mockResolvedValueOnce({ exitCode: 0 } as never) // rm -f + .mockResolvedValueOnce({ exitCode: 0 } as never) // run (foreground) + + await DockerManager.runForeground( + 'my-image', 'iloom-dev-123', 3123, 4200 + ) + + // First call: force-remove + expect(execa).toHaveBeenNthCalledWith(1, 'docker', ['rm', '-f', 'iloom-dev-123'], { reject: false }) + // Second call: run in foreground + expect(execa).toHaveBeenNthCalledWith(2, 'docker', [ + 'run', + '--name', 'iloom-dev-123', + '--rm', + '-p', '3123:4200', + 'my-image', + ], { stdio: 'inherit' }) + }) + + it('should redirect to stderr when redirectToStderr is true', async () => { + vi.mocked(execa) + .mockResolvedValueOnce({ exitCode: 0 } as never) // rm -f + .mockResolvedValueOnce({ exitCode: 0 } as never) // run + + await DockerManager.runForeground( + 'my-image', 'iloom-dev-123', 3123, 4200, + undefined, true + ) + + expect(execa).toHaveBeenNthCalledWith(2, 'docker', expect.any(Array), { + stdio: [process.stdin, process.stderr, process.stderr], + }) + }) + + it('should pass additional args', async () => { + vi.mocked(execa) + .mockResolvedValueOnce({ exitCode: 0 } as never) // rm -f + .mockResolvedValueOnce({ exitCode: 0 } as never) // run + + await DockerManager.runForeground( + 'my-image', 'iloom-dev-123', 3123, 4200, + ['-v', './src:/app/src'] + ) + + expect(execa).toHaveBeenNthCalledWith(2, 'docker', [ + 'run', + '--name', 'iloom-dev-123', + '--rm', + '-p', '3123:4200', + '-v', './src:/app/src', + 'my-image', + ], { stdio: 'inherit' }) + }) + + it('should set up signal forwarding for SIGINT and SIGTERM', async () => { + const processOnSpy = vi.spyOn(process, 'on') + const processRemoveListenerSpy = vi.spyOn(process, 'removeListener') + + vi.mocked(execa) + .mockResolvedValueOnce({ exitCode: 0 } as never) // rm -f + .mockResolvedValueOnce({ exitCode: 0 } as never) // run + + await DockerManager.runForeground( + 'my-image', 'iloom-dev-123', 3123, 4200 + ) + + // Should have registered signal handlers + expect(processOnSpy).toHaveBeenCalledWith('SIGINT', expect.any(Function)) + expect(processOnSpy).toHaveBeenCalledWith('SIGTERM', expect.any(Function)) + + // Should have cleaned up signal handlers + expect(processRemoveListenerSpy).toHaveBeenCalledWith('SIGINT', expect.any(Function)) + expect(processRemoveListenerSpy).toHaveBeenCalledWith('SIGTERM', expect.any(Function)) + }) + + it('should clean up signal handlers even when docker run fails', async () => { + const processRemoveListenerSpy = vi.spyOn(process, 'removeListener') + + vi.mocked(execa) + .mockResolvedValueOnce({ exitCode: 0 } as never) // rm -f + .mockRejectedValueOnce(new Error('container failed')) // run + + await expect( + DockerManager.runForeground('my-image', 'iloom-dev-123', 3123, 4200) + ).rejects.toThrow('container failed') + + // Signal handlers should still be cleaned up + expect(processRemoveListenerSpy).toHaveBeenCalledWith('SIGINT', expect.any(Function)) + expect(processRemoveListenerSpy).toHaveBeenCalledWith('SIGTERM', expect.any(Function)) + }) + }) + + describe('stopAndRemoveContainer', () => { + it('should force-remove a running container', async () => { + // isContainerRunning check + vi.mocked(execa) + .mockResolvedValueOnce({ + exitCode: 0, + stdout: 'iloom-dev-123', + } as never) // docker ps + .mockResolvedValueOnce({ exitCode: 0 } as never) // docker rm -f + + const result = await DockerManager.stopAndRemoveContainer('iloom-dev-123') + + expect(result).toBe(true) + // Should have used docker rm -f (atomic force-remove) + expect(execa).toHaveBeenNthCalledWith(2, 'docker', ['rm', '-f', 'iloom-dev-123'], { reject: false }) + }) + + it('should not throw if container does not exist', async () => { + // isContainerRunning returns false + vi.mocked(execa) + .mockResolvedValueOnce({ + exitCode: 0, + stdout: '', + } as never) // docker ps (empty = not running) + .mockResolvedValueOnce({ exitCode: 1 } as never) // docker rm -f (not found, ok) + + const result = await DockerManager.stopAndRemoveContainer('iloom-dev-nonexistent') + + expect(result).toBe(false) + }) + + it('should still try rm -f for stopped containers', async () => { + // isContainerRunning returns false (container stopped but exists) + vi.mocked(execa) + .mockResolvedValueOnce({ + exitCode: 0, + stdout: '', + } as never) // docker ps (not running) + .mockResolvedValueOnce({ exitCode: 0 } as never) // docker rm -f (removes stopped container) + + const result = await DockerManager.stopAndRemoveContainer('iloom-dev-stopped') + + expect(result).toBe(false) + expect(execa).toHaveBeenNthCalledWith(2, 'docker', ['rm', '-f', 'iloom-dev-stopped'], { reject: false }) + }) + }) + + describe('isContainerRunning', () => { + it('should return true for running named container', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 0, + stdout: 'iloom-dev-123', + } as never) + + const result = await DockerManager.isContainerRunning('iloom-dev-123') + + expect(result).toBe(true) + expect(execa).toHaveBeenCalledWith( + 'docker', + ['ps', '--filter', 'name=^iloom-dev-123$', '--format', '{{.Names}}'], + { reject: false } + ) + }) + + it('should return false for stopped/missing container', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 0, + stdout: '', + } as never) + + const result = await DockerManager.isContainerRunning('iloom-dev-missing') + + expect(result).toBe(false) + }) + + it('should return false when docker ps fails', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 1, + stdout: '', + } as never) + + const result = await DockerManager.isContainerRunning('iloom-dev-123') + + expect(result).toBe(false) + }) + + it('should return false when execa throws', async () => { + vi.mocked(execa).mockRejectedValue(new Error('Docker not available')) + + const result = await DockerManager.isContainerRunning('iloom-dev-123') + + expect(result).toBe(false) + }) + + it('should use exact name matching with anchored regex', async () => { + // Ensure "iloom-dev-1" doesn't match "iloom-dev-123" + vi.mocked(execa).mockResolvedValue({ + exitCode: 0, + stdout: 'iloom-dev-123', // returned name does not match queried name + } as never) + + const result = await DockerManager.isContainerRunning('iloom-dev-1') + + expect(result).toBe(false) // Names don't match + }) + }) + + describe('parseExposeFromDockerfile', () => { + it('should extract port from EXPOSE directive', async () => { + vi.mocked(readFile).mockResolvedValue( + 'FROM node:18\nWORKDIR /app\nCOPY . .\nEXPOSE 4200\nCMD ["npm", "start"]' + ) + + const port = await DockerManager.parseExposeFromDockerfile('/test/Dockerfile') + + expect(port).toBe(4200) + }) + + it('should return first EXPOSE if multiple exist', async () => { + vi.mocked(readFile).mockResolvedValue( + 'FROM node:18\nEXPOSE 4200\nEXPOSE 8080\nCMD ["npm", "start"]' + ) + + const port = await DockerManager.parseExposeFromDockerfile('/test/Dockerfile') + + expect(port).toBe(4200) + }) + + it('should return null if no EXPOSE directive', async () => { + vi.mocked(readFile).mockResolvedValue( + 'FROM node:18\nWORKDIR /app\nCOPY . .\nCMD ["npm", "start"]' + ) + + const port = await DockerManager.parseExposeFromDockerfile('/test/Dockerfile') + + expect(port).toBeNull() + }) + + it('should handle EXPOSE with protocol (e.g., 4200/tcp)', async () => { + vi.mocked(readFile).mockResolvedValue( + 'FROM node:18\nEXPOSE 4200/tcp\nCMD ["npm", "start"]' + ) + + const port = await DockerManager.parseExposeFromDockerfile('/test/Dockerfile') + + expect(port).toBe(4200) + }) + + it('should handle EXPOSE with udp protocol', async () => { + vi.mocked(readFile).mockResolvedValue( + 'FROM node:18\nEXPOSE 8080/udp\nCMD ["npm", "start"]' + ) + + const port = await DockerManager.parseExposeFromDockerfile('/test/Dockerfile') + + expect(port).toBe(8080) + }) + + it('should return null if Dockerfile does not exist', async () => { + vi.mocked(readFile).mockRejectedValue( + Object.assign(new Error('ENOENT'), { code: 'ENOENT' }) + ) + + const port = await DockerManager.parseExposeFromDockerfile('/nonexistent/Dockerfile') + + expect(port).toBeNull() + }) + + it('should not match EXPOSE in comments', async () => { + vi.mocked(readFile).mockResolvedValue( + 'FROM node:18\n# EXPOSE 4200\nCMD ["npm", "start"]' + ) + + const port = await DockerManager.parseExposeFromDockerfile('/test/Dockerfile') + + expect(port).toBeNull() + }) + }) + + describe('inspectImagePorts', () => { + it('should detect ports from docker image inspect', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 0, + stdout: '{"4200/tcp":{}}', + } as never) + + const port = await DockerManager.inspectImagePorts('my-image') + + expect(port).toBe(4200) + expect(execa).toHaveBeenCalledWith( + 'docker', + ['image', 'inspect', 'my-image', '--format', '{{json .Config.ExposedPorts}}'], + { reject: false } + ) + }) + + it('should return first port when multiple are exposed', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 0, + stdout: '{"4200/tcp":{},"8080/tcp":{}}', + } as never) + + const port = await DockerManager.inspectImagePorts('my-image') + + expect(port).toBe(4200) + }) + + it('should return null when no ports are exposed', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 0, + stdout: 'null', + } as never) + + const port = await DockerManager.inspectImagePorts('my-image') + + expect(port).toBeNull() + }) + + it('should return null for empty exposed ports', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 0, + stdout: '{}', + } as never) + + const port = await DockerManager.inspectImagePorts('my-image') + + expect(port).toBeNull() + }) + + it('should return null when inspect fails', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 1, + stdout: '', + } as never) + + const port = await DockerManager.inspectImagePorts('nonexistent-image') + + expect(port).toBeNull() + }) + + it('should return null for output', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 0, + stdout: '', + } as never) + + const port = await DockerManager.inspectImagePorts('my-image') + + expect(port).toBeNull() + }) + + it('should handle execa throwing', async () => { + vi.mocked(execa).mockRejectedValue(new Error('Docker not available')) + + const port = await DockerManager.inspectImagePorts('my-image') + + expect(port).toBeNull() + }) + }) + + describe('resolveContainerPort', () => { + it('should return config port when provided', async () => { + const port = await DockerManager.resolveContainerPort(4200, '/test/Dockerfile') + + expect(port).toBe(4200) + // Should not call execa or readFile since config port is provided + expect(execa).not.toHaveBeenCalled() + expect(readFile).not.toHaveBeenCalled() + }) + + it('should use image inspect when config port is not set and imageName provided', async () => { + vi.mocked(execa).mockResolvedValue({ + exitCode: 0, + stdout: '{"8080/tcp":{}}', + } as never) + + const port = await DockerManager.resolveContainerPort( + undefined, '/test/Dockerfile', 'my-image' + ) + + expect(port).toBe(8080) + }) + + it('should fall back to Dockerfile regex when image inspect returns nothing', async () => { + // Image inspect returns null + vi.mocked(execa).mockResolvedValue({ + exitCode: 0, + stdout: 'null', + } as never) + + // Dockerfile has EXPOSE + vi.mocked(readFile).mockResolvedValue( + 'FROM node:18\nEXPOSE 3000\nCMD ["npm", "start"]' + ) + + const port = await DockerManager.resolveContainerPort( + undefined, '/test/Dockerfile', 'my-image' + ) + + expect(port).toBe(3000) + }) + + it('should use Dockerfile regex when no imageName provided', async () => { + vi.mocked(readFile).mockResolvedValue( + 'FROM node:18\nEXPOSE 4200\nCMD ["npm", "start"]' + ) + + const port = await DockerManager.resolveContainerPort(undefined, '/test/Dockerfile') + + expect(port).toBe(4200) + }) + + it('should throw when no port source provides a port', async () => { + // Image inspect returns null + vi.mocked(execa).mockResolvedValue({ + exitCode: 0, + stdout: 'null', + } as never) + + // Dockerfile has no EXPOSE + vi.mocked(readFile).mockResolvedValue( + 'FROM node:18\nCMD ["npm", "start"]' + ) + + await expect( + DockerManager.resolveContainerPort(undefined, '/test/Dockerfile', 'my-image') + ).rejects.toThrow('Cannot determine container port') + }) + + it('should include helpful error message with resolution steps', async () => { + vi.mocked(readFile).mockResolvedValue('FROM node:18\nCMD ["npm", "start"]') + + await expect( + DockerManager.resolveContainerPort(undefined, '/test/Dockerfile') + ).rejects.toThrow('capabilities.web') + }) + }) + + describe('sanitizeContainerName', () => { + it('should replace slashes with hyphens', () => { + expect(DockerManager.sanitizeContainerName('feat/issue-548')).toBe('feat-issue-548') + }) + + it('should remove invalid characters', () => { + expect(DockerManager.sanitizeContainerName('my@container!name')).toBe('my-container-name') + }) + + it('should handle branch names like feat/issue-548__docker', () => { + // Double underscores are valid Docker container name characters + expect(DockerManager.sanitizeContainerName('iloom-dev-feat/issue-548__docker')) + .toBe('iloom-dev-feat-issue-548__docker') + }) + + it('should collapse consecutive hyphens', () => { + expect(DockerManager.sanitizeContainerName('a---b---c')).toBe('a-b-c') + }) + + it('should remove leading non-alphanumeric characters', () => { + expect(DockerManager.sanitizeContainerName('---my-container')).toBe('my-container') + }) + + it('should remove trailing hyphens', () => { + expect(DockerManager.sanitizeContainerName('my-container---')).toBe('my-container') + }) + + it('should truncate to 63 characters', () => { + const longName = 'a'.repeat(100) + const result = DockerManager.sanitizeContainerName(longName) + + expect(result.length).toBeLessThanOrEqual(63) + }) + + it('should clean trailing hyphen after truncation', () => { + // Create a name that will have a hyphen at position 63 after replacement + const name = 'a'.repeat(62) + '/b' + const result = DockerManager.sanitizeContainerName(name) + + expect(result).not.toMatch(/-$/) + expect(result.length).toBeLessThanOrEqual(63) + }) + + it('should preserve dots and underscores', () => { + expect(DockerManager.sanitizeContainerName('my_container.v1')).toBe('my_container.v1') + }) + + it('should handle empty string with fallback', () => { + expect(DockerManager.sanitizeContainerName('---')).toBe('iloom-container') + }) + + it('should handle string with only invalid characters', () => { + expect(DockerManager.sanitizeContainerName('@@@')).toBe('iloom-container') + }) + + it('should handle spaces in names', () => { + expect(DockerManager.sanitizeContainerName('my container name')).toBe('my-container-name') + }) + + it('should preserve double underscores (valid Docker name chars)', () => { + // Underscores are valid in Docker container names + expect(DockerManager.sanitizeContainerName('feat__docker-server')) + .toBe('feat__docker-server') + }) + }) + + describe('buildContainerName', () => { + it('should build name with numeric identifier', () => { + expect(DockerManager.buildContainerName(548)).toBe('iloom-dev-548') + }) + + it('should build name with string identifier', () => { + expect(DockerManager.buildContainerName('my-branch')).toBe('iloom-dev-my-branch') + }) + + it('should sanitize branch names with slashes', () => { + // Slashes replaced with hyphens, underscores preserved + expect(DockerManager.buildContainerName('feat/issue-548__docker')) + .toBe('iloom-dev-feat-issue-548__docker') + }) + }) + + describe('buildImageName', () => { + it('should build image name with numeric identifier', () => { + expect(DockerManager.buildImageName(548)).toBe('iloom-dev-548') + }) + + it('should build image name with string identifier', () => { + expect(DockerManager.buildImageName('my-branch')).toBe('iloom-dev-my-branch') + }) + }) + + describe('buildDockerConfigFromSettings', () => { + it('should return undefined when webSettings is undefined', () => { + const result = DockerManager.buildDockerConfigFromSettings(undefined, '548') + expect(result).toBeUndefined() + }) + + it('should return undefined when devServer is "process"', () => { + const result = DockerManager.buildDockerConfigFromSettings( + { devServer: 'process' }, + '548' + ) + expect(result).toBeUndefined() + }) + + it('should return undefined when devServer is not set', () => { + const result = DockerManager.buildDockerConfigFromSettings({}, '548') + expect(result).toBeUndefined() + }) + + it('should return DockerConfig when devServer is "docker"', () => { + const result = DockerManager.buildDockerConfigFromSettings( + { devServer: 'docker' }, + '548' + ) + + expect(result).toEqual({ + dockerFile: './Dockerfile', + containerPort: undefined, + dockerBuildArgs: undefined, + dockerRunArgs: undefined, + identifier: '548', + }) + }) + + it('should use custom dockerFile when provided', () => { + const result = DockerManager.buildDockerConfigFromSettings( + { devServer: 'docker', dockerFile: './docker/Dockerfile.dev' }, + '548' + ) + + expect(result?.dockerFile).toBe('./docker/Dockerfile.dev') + }) + + it('should pass through containerPort, buildArgs, and runArgs', () => { + const result = DockerManager.buildDockerConfigFromSettings( + { + devServer: 'docker', + containerPort: 4200, + dockerBuildArgs: { NODE_ENV: 'development' }, + dockerRunArgs: ['-v', './src:/app/src'], + }, + 'my-branch' + ) + + expect(result).toEqual({ + dockerFile: './Dockerfile', + containerPort: 4200, + dockerBuildArgs: { NODE_ENV: 'development' }, + dockerRunArgs: ['-v', './src:/app/src'], + identifier: 'my-branch', + }) + }) + }) +}) diff --git a/src/lib/DockerManager.ts b/src/lib/DockerManager.ts new file mode 100644 index 00000000..552c1aa5 --- /dev/null +++ b/src/lib/DockerManager.ts @@ -0,0 +1,491 @@ +import { execa } from 'execa' +import { readFile } from 'fs/promises' +import { logger } from '../utils/logger.js' + +/** + * Configuration for Docker-based dev server mode. + * When provided to DevServerManager methods, the server runs inside a Docker container + * with port mapping instead of as a local process. + */ +export interface DockerConfig { + /** Path to Dockerfile (relative to worktree) */ + dockerFile: string + /** Port inside the container (auto-detected from image inspect or Dockerfile EXPOSE if not set) */ + containerPort?: number | undefined + /** Build arguments passed as --build-arg to docker build */ + dockerBuildArgs?: Record | undefined + /** Additional docker run flags (e.g., volume mounts) */ + dockerRunArgs?: string[] | undefined + /** Identifier for container naming (issue number, branch name) */ + identifier: string +} + +/** + * Web settings shape accepted by buildDockerConfigFromSettings. + * Matches the capabilities.web section of IloomSettings. + * Uses `| undefined` for optional properties to satisfy exactOptionalPropertyTypes. + */ +interface WebSettings { + devServer?: 'process' | 'docker' | undefined + dockerFile?: string | undefined + containerPort?: number | undefined + dockerBuildArgs?: Record | undefined + dockerRunArgs?: string[] | undefined +} + +/** + * Maximum length for Docker container names. + * Docker allows up to 63 characters for container names. + */ +const MAX_CONTAINER_NAME_LENGTH = 63 + +/** + * DockerManager encapsulates all Docker CLI interactions. + * Used by DevServerManager and ResourceCleanup for Docker-based dev server operations. + * + * All methods are static since no instance state is needed -- Docker CLI is stateless. + */ +export class DockerManager { + /** + * Check if Docker CLI is available and daemon is running. + * @returns true if Docker is ready, false otherwise + */ + static async isAvailable(): Promise { + try { + const result = await execa('docker', ['info'], { reject: false }) + return result.exitCode === 0 + } catch { + return false + } + } + + /** + * Assert that Docker is available, throwing a clear error if not. + * Call this early in any workflow that requires Docker. + */ + static async assertAvailable(): Promise { + const available = await DockerManager.isAvailable() + if (!available) { + throw new Error( + 'Docker is not available. Please ensure Docker is installed and the Docker daemon is running.\n' + + 'Install Docker: https://docs.docker.com/get-docker/' + ) + } + } + + /** + * Build a Docker image. + * + * @param cwd - Working directory (worktree path) + * @param imageName - Image tag name + * @param dockerFile - Path to Dockerfile (relative to cwd) + * @param buildArgs - Optional build arguments passed as --build-arg + */ + static async buildImage( + cwd: string, + imageName: string, + dockerFile: string, + buildArgs?: Record + ): Promise { + const args = ['build', '-t', imageName, '-f', dockerFile] + + if (buildArgs) { + for (const [key, value] of Object.entries(buildArgs)) { + args.push('--build-arg', `${key}=${value}`) + } + } + + // Context directory is always cwd + args.push('.') + + logger.info(`Building Docker image "${imageName}" from ${dockerFile}...`) + + try { + await execa('docker', args, { + cwd, + stdio: 'inherit', + }) + } catch (error) { + const message = error instanceof Error ? error.message : 'Unknown error' + throw new Error(`Docker build failed for image "${imageName}": ${message}`) + } + + logger.success(`Docker image "${imageName}" built successfully`) + } + + /** + * Run a container in detached mode (background). + * If a container with the same name already exists, it is force-removed first. + * + * @param imageName - Image to run + * @param containerName - Container name (must be sanitized) + * @param hostPort - Port on the host to map + * @param containerPort - Port inside the container + * @param additionalArgs - Additional docker run flags (e.g., volume mounts) + * @returns Container ID + */ + static async runDetached( + imageName: string, + containerName: string, + hostPort: number, + containerPort: number, + additionalArgs?: string[] + ): Promise { + // Force-remove any existing container with same name to avoid name collision + await DockerManager.forceRemoveContainer(containerName) + + const args = [ + 'run', '-d', + '--name', containerName, + '-p', `${hostPort}:${containerPort}`, + ] + + if (additionalArgs) { + args.push(...additionalArgs) + } + + args.push(imageName) + + logger.info(`Starting Docker container "${containerName}" (${hostPort}:${containerPort})...`) + + try { + const result = await execa('docker', args) + const containerId = result.stdout.trim() + logger.success(`Docker container "${containerName}" started (ID: ${containerId.substring(0, 12)})`) + return containerId + } catch (error) { + const message = error instanceof Error ? error.message : 'Unknown error' + throw new Error(`Failed to start Docker container "${containerName}": ${message}`) + } + } + + /** + * Run a container in foreground mode (attached, blocking). + * The container is automatically removed on exit (--rm flag). + * Stdout/stderr are streamed to the terminal. + * + * Signal forwarding: Captures SIGINT/SIGTERM on the host process and + * forwards them to the container via `docker kill --signal` for graceful + * shutdown of the framework running inside the container. + * + * @param imageName - Image to run + * @param containerName - Container name (must be sanitized) + * @param hostPort - Port on the host to map + * @param containerPort - Port inside the container + * @param additionalArgs - Additional docker run flags + * @param redirectToStderr - If true, redirect stdout/stderr to stderr + */ + static async runForeground( + imageName: string, + containerName: string, + hostPort: number, + containerPort: number, + additionalArgs?: string[], + redirectToStderr?: boolean + ): Promise { + // Force-remove any existing container with same name to avoid name collision + await DockerManager.forceRemoveContainer(containerName) + + const args = [ + 'run', + '--name', containerName, + '--rm', + '-p', `${hostPort}:${containerPort}`, + ] + + if (additionalArgs) { + args.push(...additionalArgs) + } + + args.push(imageName) + + logger.info(`Running Docker container "${containerName}" in foreground (${hostPort}:${containerPort})...`) + + const stdio = redirectToStderr + ? [process.stdin, process.stderr, process.stderr] as const + : 'inherit' as const + + // Set up signal forwarding to gracefully shut down the container + const forwardSignal = (signal: string): void => { + logger.debug(`Forwarding ${signal} to container "${containerName}"`) + void execa('docker', ['kill', `--signal=${signal}`, containerName], { reject: false }) + } + + const onSigint = (): void => forwardSignal('SIGINT') + const onSigterm = (): void => forwardSignal('SIGTERM') + + process.on('SIGINT', onSigint) + process.on('SIGTERM', onSigterm) + + try { + await execa('docker', args, { stdio }) + } finally { + // Clean up signal handlers to avoid leaks + process.removeListener('SIGINT', onSigint) + process.removeListener('SIGTERM', onSigterm) + } + } + + /** + * Stop and remove a container by name using atomic force-remove. + * No-op if the container doesn't exist or is already removed. + * Uses `docker rm -f` which handles both running and stopped containers + * atomically, eliminating race conditions between stop and remove. + * + * @param containerName - Name of the container to remove + * @returns true if a container was removed, false if no container was found + */ + static async stopAndRemoveContainer(containerName: string): Promise { + const isRunning = await DockerManager.isContainerRunning(containerName) + + if (!isRunning) { + logger.debug(`No running container found with name "${containerName}"`) + // Still try to remove in case container exists but is stopped + await execa('docker', ['rm', '-f', containerName], { reject: false }) + return false + } + + logger.info(`Removing Docker container "${containerName}"...`) + + // Atomic force-remove: handles running and stopped containers + await execa('docker', ['rm', '-f', containerName], { reject: false }) + + logger.success(`Docker container "${containerName}" removed`) + return true + } + + /** + * Check if a named container is currently running. + * + * @param containerName - Name of the container to check + * @returns true if the container is running, false otherwise + */ + static async isContainerRunning(containerName: string): Promise { + try { + const result = await execa('docker', [ + 'ps', + '--filter', `name=^${containerName}$`, + '--format', '{{.Names}}', + ], { reject: false }) + + return result.exitCode === 0 && result.stdout.trim() === containerName + } catch { + return false + } + } + + /** + * Parse the EXPOSE directive from a Dockerfile. + * Returns the first exposed port number, or null if none found. + * + * Handles formats like: + * - EXPOSE 4200 + * - EXPOSE 4200/tcp + * - EXPOSE 8080/udp + * + * @param dockerfilePath - Absolute path to the Dockerfile + * @returns First exposed port number, or null if no EXPOSE directive found + */ + static async parseExposeFromDockerfile( + dockerfilePath: string + ): Promise { + try { + const content = await readFile(dockerfilePath, 'utf-8') + const match = /^EXPOSE\s+(\d+)/m.exec(content) + + if (match?.[1]) { + const port = parseInt(match[1], 10) + if (!isNaN(port) && port >= 1 && port <= 65535) { + return port + } + } + + return null + } catch { + return null + } + } + + /** + * Sanitize a string for use as a Docker container name. + * + * Docker container names must match: [a-zA-Z0-9][a-zA-Z0-9_.-] + * - Replace slashes, spaces, and other invalid characters with hyphens + * - Ensure the name starts with an alphanumeric character + * - Truncate to a maximum of 63 characters + * + * @param name - Raw name to sanitize + * @returns Sanitized container name + */ + static sanitizeContainerName(name: string): string { + // Replace any character that isn't alphanumeric, underscore, dot, or hyphen with a hyphen + let sanitized = name.replace(/[^a-zA-Z0-9_.-]/g, '-') + + // Collapse consecutive hyphens into one + sanitized = sanitized.replace(/-{2,}/g, '-') + + // Remove leading non-alphanumeric characters (Docker requires starting with [a-zA-Z0-9]) + sanitized = sanitized.replace(/^[^a-zA-Z0-9]+/, '') + + // Remove trailing hyphens + sanitized = sanitized.replace(/-+$/, '') + + // Truncate to max length + if (sanitized.length > MAX_CONTAINER_NAME_LENGTH) { + sanitized = sanitized.substring(0, MAX_CONTAINER_NAME_LENGTH) + // Remove trailing hyphen if truncation created one + sanitized = sanitized.replace(/-+$/, '') + } + + // Fallback if everything was stripped + if (sanitized.length === 0) { + sanitized = 'iloom-container' + } + + return sanitized + } + + /** + * Build the standard container name for an iloom dev server. + * + * @param identifier - Issue number, branch name, or other identifier + * @returns Sanitized container name in the format "iloom-dev-{identifier}" + */ + static buildContainerName(identifier: string | number): string { + return DockerManager.sanitizeContainerName(`iloom-dev-${identifier}`) + } + + /** + * Build the standard image name for an iloom dev server. + * + * @param identifier - Issue number, branch name, or other identifier + * @returns Sanitized image name + */ + static buildImageName(identifier: string | number): string { + return DockerManager.sanitizeContainerName(`iloom-dev-${identifier}`) + } + + /** + * Detect exposed ports from a built Docker image using `docker image inspect`. + * This is the source of truth for exposed ports as it handles multi-stage builds, + * inherited images, and runtime configuration that Dockerfile regex cannot capture. + * + * @param imageName - Name of the built Docker image to inspect + * @returns First exposed port number, or null if none found + */ + static async inspectImagePorts(imageName: string): Promise { + try { + const result = await execa('docker', [ + 'image', 'inspect', imageName, + '--format', '{{json .Config.ExposedPorts}}', + ], { reject: false }) + + if (result.exitCode !== 0 || !result.stdout.trim()) { + return null + } + + const output = result.stdout.trim() + // Output format: {"4200/tcp":{},"8080/tcp":{}} or null + if (output === 'null' || output === '' || output === '{}') { + return null + } + + const parsed: unknown = JSON.parse(output) + if (typeof parsed !== 'object' || parsed === null) { + return null + } + + // Get first key (e.g., "4200/tcp") and extract port number + const firstKey = Object.keys(parsed)[0] + if (!firstKey) { + return null + } + + const portMatch = /^(\d+)/.exec(firstKey) + if (portMatch?.[1]) { + const port = parseInt(portMatch[1], 10) + if (!isNaN(port) && port >= 1 && port <= 65535) { + return port + } + } + + return null + } catch { + return null + } + } + + /** + * Resolve the container port from config, image inspection, or Dockerfile EXPOSE directive. + * Priority: config > docker image inspect > Dockerfile regex + * Throws a clear error if no source provides a port. + * + * @param configPort - Port from settings (containerPort field) + * @param dockerfilePath - Absolute path to the Dockerfile + * @param imageName - Optional built image name for inspection (preferred over Dockerfile regex) + * @returns Resolved container port number + */ + static async resolveContainerPort( + configPort: number | undefined, + dockerfilePath: string, + imageName?: string + ): Promise { + if (configPort !== undefined) { + return configPort + } + + // Try image inspection first (most accurate - handles multi-stage builds, inherited images) + if (imageName) { + const inspectedPort = await DockerManager.inspectImagePorts(imageName) + if (inspectedPort !== null) { + logger.debug(`Auto-detected container port ${inspectedPort} from Docker image inspect`) + return inspectedPort + } + } + + // Fall back to Dockerfile regex + const exposedPort = await DockerManager.parseExposeFromDockerfile(dockerfilePath) + if (exposedPort !== null) { + logger.debug(`Auto-detected container port ${exposedPort} from Dockerfile EXPOSE directive`) + return exposedPort + } + + throw new Error( + `Cannot determine container port. No "containerPort" configured in settings and no EXPOSE directive found in ${dockerfilePath}.\n` + + 'Either add an EXPOSE directive to your Dockerfile or set "containerPort" in capabilities.web settings.' + ) + } + + /** + * Force-remove a container by name (no-op if it doesn't exist). + * Used internally before running a new container to avoid name collisions. + */ + private static async forceRemoveContainer(containerName: string): Promise { + await execa('docker', ['rm', '-f', containerName], { reject: false }) + } + + /** + * Build a DockerConfig from iloom web settings and an identifier. + * Centralizes the Docker config extraction logic used by dev-server, open, and run commands. + * + * @param webSettings - The capabilities.web section from IloomSettings + * @param identifier - Identifier for container naming (issue number, branch name, etc.) + * @returns DockerConfig if Docker mode is enabled, undefined otherwise + */ + static buildDockerConfigFromSettings( + webSettings: WebSettings | undefined, + identifier: string + ): DockerConfig | undefined { + if (!webSettings || webSettings.devServer !== 'docker') { + return undefined + } + + return { + dockerFile: webSettings.dockerFile ?? './Dockerfile', + containerPort: webSettings.containerPort, + dockerBuildArgs: webSettings.dockerBuildArgs, + dockerRunArgs: webSettings.dockerRunArgs, + identifier, + } + } +} diff --git a/src/lib/ResourceCleanup.test.ts b/src/lib/ResourceCleanup.test.ts index 34a95387..56ec2db8 100644 --- a/src/lib/ResourceCleanup.test.ts +++ b/src/lib/ResourceCleanup.test.ts @@ -4,6 +4,7 @@ import { GitWorktreeManager } from './GitWorktreeManager.js' import { DatabaseManager } from './DatabaseManager.js' import { ProcessManager } from './process/ProcessManager.js' import { SettingsManager } from './SettingsManager.js' +import { DockerManager } from './DockerManager.js' import type { GitWorktree } from '../types/worktree.js' import type { ResourceCleanupOptions } from '../types/cleanup.js' import { executeGitCommand, findMainWorktreePathWithSettings, hasUncommittedChanges, isBranchMergedIntoMain, checkRemoteBranchStatus, getMergeTargetBranch, findWorktreeForBranch } from '../utils/git.js' @@ -14,6 +15,7 @@ vi.mock('./GitWorktreeManager.js') vi.mock('./DatabaseManager.js') vi.mock('./process/ProcessManager.js') vi.mock('./SettingsManager.js') +vi.mock('./DockerManager.js') // Mock MetadataManager to prevent real file creation during tests vi.mock('./MetadataManager.js', () => ({ @@ -467,6 +469,215 @@ describe('ResourceCleanup', () => { /may still be running/ ) }) + + it('should stop Docker container when dockerIdentifier is provided and container is running', async () => { + vi.mocked(DockerManager.buildContainerName).mockReturnValue('iloom-dev-25') + vi.mocked(DockerManager.isContainerRunning).mockResolvedValueOnce(true) + vi.mocked(DockerManager.stopAndRemoveContainer).mockResolvedValueOnce(true) + + const result = await resourceCleanup.terminateDevServer(3025, 25) + + expect(DockerManager.buildContainerName).toHaveBeenCalledWith(25) + expect(DockerManager.isContainerRunning).toHaveBeenCalledWith('iloom-dev-25') + expect(DockerManager.stopAndRemoveContainer).toHaveBeenCalledWith('iloom-dev-25') + expect(result).toBe(true) + // Should NOT fall through to process detection + expect(mockProcessManager.detectDevServer).not.toHaveBeenCalled() + }) + + it('should fall back to process detection when dockerIdentifier is provided but no container found', async () => { + vi.mocked(DockerManager.buildContainerName).mockReturnValue('iloom-dev-25') + vi.mocked(DockerManager.isContainerRunning).mockResolvedValueOnce(false) + vi.mocked(mockProcessManager.detectDevServer).mockResolvedValueOnce({ + pid: 12345, + name: 'node', + command: 'node next dev', + port: 3025, + isDevServer: true, + }) + vi.mocked(mockProcessManager.terminateProcess).mockResolvedValueOnce(true) + vi.mocked(mockProcessManager.verifyPortFree).mockResolvedValueOnce(true) + + const result = await resourceCleanup.terminateDevServer(3025, 25) + + expect(DockerManager.isContainerRunning).toHaveBeenCalledWith('iloom-dev-25') + expect(mockProcessManager.detectDevServer).toHaveBeenCalledWith(3025) + expect(result).toBe(true) + }) + + it('should return false when dockerIdentifier provided but no container and no process found', async () => { + vi.mocked(DockerManager.buildContainerName).mockReturnValue('iloom-dev-42') + vi.mocked(DockerManager.isContainerRunning).mockResolvedValueOnce(false) + vi.mocked(mockProcessManager.detectDevServer).mockResolvedValueOnce(null) + + const result = await resourceCleanup.terminateDevServer(3042, 42) + + expect(DockerManager.isContainerRunning).toHaveBeenCalledWith('iloom-dev-42') + expect(result).toBe(false) + }) + + it('should not check Docker when no dockerIdentifier is provided', async () => { + vi.mocked(mockProcessManager.detectDevServer).mockResolvedValueOnce(null) + + const result = await resourceCleanup.terminateDevServer(3025) + + expect(DockerManager.isContainerRunning).not.toHaveBeenCalled() + expect(DockerManager.stopAndRemoveContainer).not.toHaveBeenCalled() + expect(result).toBe(false) + }) + + it('should handle string dockerIdentifier (branch name)', async () => { + vi.mocked(DockerManager.buildContainerName).mockReturnValue('iloom-dev-feat-issue-25') + vi.mocked(DockerManager.isContainerRunning).mockResolvedValueOnce(true) + vi.mocked(DockerManager.stopAndRemoveContainer).mockResolvedValueOnce(true) + + const result = await resourceCleanup.terminateDevServer(3025, 'feat/issue-25') + + expect(DockerManager.buildContainerName).toHaveBeenCalledWith('feat/issue-25') + expect(result).toBe(true) + }) + }) + + describe('cleanupWorktree - Docker mode', () => { + const mockWorktree: GitWorktree = { + path: '/path/to/worktree', + branch: 'feat/issue-25', + commit: 'abc123', + bare: false, + detached: false, + locked: false, + } + + it('should pass dockerIdentifier to terminateDevServer when Docker mode is configured', async () => { + // Configure Docker mode in settings + mockSettingsManager.loadSettings = vi.fn().mockResolvedValue({ + capabilities: { + web: { + basePort: 3000, + devServer: 'docker', + }, + }, + }) + + vi.mocked(mockGitWorktree.findWorktreeForIssue).mockResolvedValueOnce(mockWorktree) + vi.mocked(mockProcessManager.calculatePort).mockReturnValue(3025) + + // Docker container is found and removed + vi.mocked(DockerManager.buildContainerName).mockReturnValue('iloom-dev-25') + vi.mocked(DockerManager.isContainerRunning).mockResolvedValueOnce(true) + vi.mocked(DockerManager.stopAndRemoveContainer).mockResolvedValueOnce(true) + + vi.mocked(mockGitWorktree.removeWorktree).mockResolvedValueOnce(undefined) + + const parsedInput = { + type: 'issue' as const, + number: 25, + originalInput: 'issue-25', + } + + const result = await resourceCleanup.cleanupWorktree(parsedInput, { + keepDatabase: true, + }) + + expect(result.success).toBe(true) + expect(DockerManager.buildContainerName).toHaveBeenCalledWith('25') + expect(DockerManager.isContainerRunning).toHaveBeenCalledWith('iloom-dev-25') + expect(DockerManager.stopAndRemoveContainer).toHaveBeenCalledWith('iloom-dev-25') + expect(result.operations[0]?.type).toBe('dev-server') + expect(result.operations[0]?.success).toBe(true) + expect(result.operations[0]?.message).toContain('terminated') + }) + + it('should NOT pass dockerIdentifier when process mode is configured', async () => { + // Configure process mode (default) + mockSettingsManager.loadSettings = vi.fn().mockResolvedValue({ + capabilities: { + web: { + basePort: 3000, + devServer: 'process', + }, + }, + }) + + vi.mocked(mockGitWorktree.findWorktreeForIssue).mockResolvedValueOnce(mockWorktree) + vi.mocked(mockProcessManager.calculatePort).mockReturnValue(3025) + vi.mocked(mockProcessManager.detectDevServer).mockResolvedValueOnce(null) + vi.mocked(mockGitWorktree.removeWorktree).mockResolvedValueOnce(undefined) + + const parsedInput = { + type: 'issue' as const, + number: 25, + originalInput: 'issue-25', + } + + await resourceCleanup.cleanupWorktree(parsedInput, { + keepDatabase: true, + }) + + // Docker methods should NOT be called + expect(DockerManager.isContainerRunning).not.toHaveBeenCalled() + expect(DockerManager.stopAndRemoveContainer).not.toHaveBeenCalled() + }) + + it('should NOT pass dockerIdentifier when devServer setting is not configured (defaults to process)', async () => { + // Settings without devServer field - should default to 'process' + mockSettingsManager.loadSettings = vi.fn().mockResolvedValue({}) + + vi.mocked(mockGitWorktree.findWorktreeForIssue).mockResolvedValueOnce(mockWorktree) + vi.mocked(mockProcessManager.calculatePort).mockReturnValue(3025) + vi.mocked(mockProcessManager.detectDevServer).mockResolvedValueOnce(null) + vi.mocked(mockGitWorktree.removeWorktree).mockResolvedValueOnce(undefined) + + const parsedInput = { + type: 'issue' as const, + number: 25, + originalInput: 'issue-25', + } + + await resourceCleanup.cleanupWorktree(parsedInput, { + keepDatabase: true, + }) + + // Docker methods should NOT be called + expect(DockerManager.isContainerRunning).not.toHaveBeenCalled() + }) + + it('should fall back to process cleanup when Docker mode is configured but no container found', async () => { + // Configure Docker mode in settings + mockSettingsManager.loadSettings = vi.fn().mockResolvedValue({ + capabilities: { + web: { + basePort: 3000, + devServer: 'docker', + }, + }, + }) + + vi.mocked(mockGitWorktree.findWorktreeForIssue).mockResolvedValueOnce(mockWorktree) + vi.mocked(mockProcessManager.calculatePort).mockReturnValue(3025) + + // Docker container not found + vi.mocked(DockerManager.buildContainerName).mockReturnValue('iloom-dev-25') + vi.mocked(DockerManager.isContainerRunning).mockResolvedValueOnce(false) + + // Falls back to process detection - no process found either + vi.mocked(mockProcessManager.detectDevServer).mockResolvedValueOnce(null) + + vi.mocked(mockGitWorktree.removeWorktree).mockResolvedValueOnce(undefined) + + const parsedInput = { + type: 'issue' as const, + number: 25, + originalInput: 'issue-25', + } + + const result = await resourceCleanup.cleanupWorktree(parsedInput, { + keepDatabase: true, + }) + + expect(result.success).toBe(true) + expect(result.operations[0]?.message).toContain('No dev server running') + }) }) describe('deleteBranch', () => { diff --git a/src/lib/ResourceCleanup.ts b/src/lib/ResourceCleanup.ts index b081c159..566e0f11 100644 --- a/src/lib/ResourceCleanup.ts +++ b/src/lib/ResourceCleanup.ts @@ -5,6 +5,7 @@ import { ProcessManager } from './process/ProcessManager.js' import { CLIIsolationManager } from './CLIIsolationManager.js' import { SettingsManager } from './SettingsManager.js' import { MetadataManager } from './MetadataManager.js' +import { DockerManager } from './DockerManager.js' import { getLogger } from '../utils/logger-context.js' import { hasUncommittedChanges, executeGitCommand, findMainWorktreePathWithSettings, extractIssueNumber, isBranchMergedIntoMain, checkRemoteBranchStatus, getMergeTargetBranch, findWorktreeForBranch, type RemoteBranchStatus } from '../utils/git.js' import { calculatePortFromIdentifier } from '../utils/port.js' @@ -56,15 +57,20 @@ export class ResourceCleanup { const displayIdentifier = parsed.branchName ?? parsed.number?.toString() ?? parsed.originalInput getLogger().info(`Starting cleanup for: ${displayIdentifier}`) - // Extract number from ParsedInput for port calculation - const number = parsed.number + // Derive identifier for port calculation and Docker container lookup + // Uses the same fallback logic as dev-server.ts: number > branchName > originalInput + const portIdentifier = parsed.number ?? parsed.branchName ?? parsed.originalInput // Step 1: Terminate dev server if applicable - if (number !== undefined) { - // Load settings to get basePort + { + // Load settings to get basePort and Docker mode const settings = await this.settingsManager.loadSettings() const basePort = settings?.capabilities?.web?.basePort ?? 3000 - const port = calculatePortFromIdentifier(number, basePort) + const port = calculatePortFromIdentifier(portIdentifier, basePort) + const devServerMode = settings?.capabilities?.web?.devServer ?? 'process' + const dockerIdentifier = devServerMode === 'docker' + ? (parsed.number?.toString() ?? parsed.branchName ?? parsed.originalInput) + : undefined if (options.dryRun) { operations.push({ @@ -74,7 +80,7 @@ export class ResourceCleanup { }) } else { try { - const terminated = await this.terminateDevServer(port) + const terminated = await this.terminateDevServer(port, dockerIdentifier) operations.push({ type: 'dev-server', success: true, @@ -472,11 +478,29 @@ export class ResourceCleanup { } /** - * Terminate dev server on specified port + * Terminate dev server on specified port. + * When a dockerIdentifier is provided, attempts Docker container cleanup first + * before falling back to process-based detection. + * + * @param port - Port the dev server is running on + * @param dockerIdentifier - Optional identifier for Docker container lookup (issue number, branch name, etc.) */ - async terminateDevServer(port: number): Promise { + async terminateDevServer(port: number, dockerIdentifier?: string | number): Promise { getLogger().debug(`Checking for dev server on port ${port}`) + // Try Docker container cleanup first if identifier provided + if (dockerIdentifier !== undefined) { + const containerName = DockerManager.buildContainerName(dockerIdentifier) + const isRunning = await DockerManager.isContainerRunning(containerName) + if (isRunning) { + getLogger().info(`Terminating Docker container: ${containerName}`) + await DockerManager.stopAndRemoveContainer(containerName) + return true + } + getLogger().debug(`No Docker container found with name "${containerName}", falling back to process detection`) + } + + // Existing process-based detection const processInfo = await this.processManager.detectDevServer(port) if (!processInfo) { diff --git a/src/lib/SettingsManager.ts b/src/lib/SettingsManager.ts index 9afd4d59..f0d7348f 100644 --- a/src/lib/SettingsManager.ts +++ b/src/lib/SettingsManager.ts @@ -181,6 +181,28 @@ export const CapabilitiesSettingsSchema = z .max(65535, 'Base port must be <= 65535') .optional() .describe('Base port for web workspace port calculations (default: 3000)'), + devServer: z + .enum(['process', 'docker']) + .default('process') + .describe('Dev server mode: "process" runs natively, "docker" runs inside a Docker container with port mapping'), + dockerFile: z + .string() + .default('./Dockerfile') + .describe('Path to Dockerfile relative to worktree root (only used when devServer is "docker")'), + containerPort: z + .number() + .min(1, 'Container port must be >= 1') + .max(65535, 'Container port must be <= 65535') + .optional() + .describe('Port the app runs on inside the Docker container (auto-detected from EXPOSE directive if not set)'), + dockerBuildArgs: z + .record(z.string()) + .optional() + .describe('Build arguments to pass to docker build (e.g., {"NODE_ENV": "development"})'), + dockerRunArgs: z + .array(z.string()) + .optional() + .describe('Additional arguments for docker run (e.g., ["-v", "./src:/app/src"] for volume mounts)'), }) .optional(), database: z @@ -214,6 +236,28 @@ export const CapabilitiesSettingsSchemaNoDefaults = z .max(65535, 'Base port must be <= 65535') .optional() .describe('Base port for web workspace port calculations (default: 3000)'), + devServer: z + .enum(['process', 'docker']) + .optional() + .describe('Dev server mode: "process" runs natively, "docker" runs inside a Docker container with port mapping'), + dockerFile: z + .string() + .optional() + .describe('Path to Dockerfile relative to worktree root (only used when devServer is "docker")'), + containerPort: z + .number() + .min(1, 'Container port must be >= 1') + .max(65535, 'Container port must be <= 65535') + .optional() + .describe('Port the app runs on inside the Docker container (auto-detected from EXPOSE directive if not set)'), + dockerBuildArgs: z + .record(z.string()) + .optional() + .describe('Build arguments to pass to docker build (e.g., {"NODE_ENV": "development"})'), + dockerRunArgs: z + .array(z.string()) + .optional() + .describe('Additional arguments for docker run (e.g., ["-v", "./src:/app/src"] for volume mounts)'), }) .optional(), database: z