agent-orchestrator/docs/REFACTORING_PLAN.md

27 KiB

CLI Refactoring Plan - Service-Based Architecture

Status: Proposed
Created: 2026-02-17
Related PR: #70 - Dashboard Config Discovery & Integration Tests

Executive Summary

Refactor CLI codebase from scattered, duplicated logic to a clean service-based architecture. This will eliminate code duplication, create single sources of truth, enable atomic operations, and make the codebase more testable and maintainable.


Current Problems 🔴

1. Port Management is Scattered

Location: Multiple files
Issue: No single source of truth for port allocation

  • findAvailablePort in port.ts (only used for dashboard)
  • Hardcoded ports in package.json scripts (3001, 3003)
  • Hardcoded ports in stopDashboard function (3001, 3003)
  • WebSocket servers don't support dynamic ports

Impact:

  • Port conflicts not preventable
  • Difficult to add new services
  • Testing requires specific ports

2. Config Handling is Duplicated

Location: Every command file
Issue: Repeated filesystem lookups, no caching

  • Every command calls findConfigFile() + loadConfig()
  • Config path resolved multiple times
  • No shared state between commands

Impact:

  • Performance overhead
  • Inconsistent error handling
  • Harder to test

3. Dashboard Startup Logic is Duplicated

Location: start.ts and dashboard.ts
Issue: Same logic implemented twice

  • Both spawn processes, set env vars, handle ports
  • Different error handling in each place
  • Code drift risk

Impact:

  • Bugs fixed in one place but not the other
  • Maintenance burden
  • Inconsistent UX

4. Metadata Operations are Unsafe

Location: Throughout codebase
Issue: Direct file operations with race conditions

  • Direct readMetadata + writeMetadata calls everywhere
  • No atomicity guarantees
  • No validation or type safety
  • Metadata updates in start.ts use read-modify-write pattern

Impact:

  • Race conditions when multiple processes update same session
  • Potential data corruption
  • Difficult to debug issues

5. Process Management is Ad-hoc

Location: Scattered across commands
Issue: No unified approach

  • spawn and exec called directly
  • Inconsistent error handling
  • No unified logging
  • No graceful shutdown logic

Impact:

  • Orphaned processes (current bug in PR #70)
  • Inconsistent error messages
  • Difficult to add process monitoring

Proposed Architecture

Directory Structure

packages/cli/src/
  services/
    ConfigService.ts           # Singleton config + path management
    PortManager.ts             # Centralized port allocation
    DashboardManager.ts        # Dashboard lifecycle (start/stop/status)
    MetadataService.ts         # Atomic metadata operations
    ProcessManager.ts          # Unified process spawning/killing

  commands/
    start.ts                   # Thin wrapper using services
    stop.ts                    # Thin wrapper using services
    dashboard.ts               # Thin wrapper using services

  lib/
    web-dir.ts                 # Keep as-is (utility)
    shell.ts                   # Keep as-is (low-level)
    port.ts                    # Move to services/PortManager.ts

Service Responsibilities

Service Responsibility State
ConfigService Load, cache, and provide config + path Singleton
PortManager Discover and allocate available ports Instance
DashboardManager Start/stop/check dashboard + WebSocket servers Instance
MetadataService Atomic metadata read/write operations Instance
ProcessManager Spawn, monitor, and kill processes gracefully Instance

Detailed Service Design

1. ConfigService (Singleton)

Purpose: Load config once, cache both config and its file path

// packages/cli/src/services/ConfigService.ts

import { loadConfig, findConfigFile, type OrchestratorConfig } from "@composio/ao-core";

class ConfigService {
  private static instance: ConfigService;
  private config?: OrchestratorConfig;
  private configPath?: string;

  private constructor() {}

  static getInstance(): ConfigService {
    if (!ConfigService.instance) {
      ConfigService.instance = new ConfigService();
    }
    return ConfigService.instance;
  }

  getConfig(): OrchestratorConfig {
    if (!this.config) {
      this.configPath = findConfigFile() ?? undefined;
      this.config = loadConfig();
    }
    return this.config;
  }

  getConfigPath(): string | null {
    this.getConfig(); // Ensure loaded
    return this.configPath ?? null;
  }

  reload(): void {
    this.config = undefined;
    this.configPath = undefined;
  }
}

export default ConfigService;

Benefits:

  • Config + path loaded once
  • No repeated filesystem lookups
  • Single source of truth
  • Easy to mock for testing

Migration:

// Before
const config = loadConfig();
const configPath = findConfigFile();

// After
const configService = ConfigService.getInstance();
const config = configService.getConfig();
const configPath = configService.getConfigPath();

2. PortManager

Purpose: Discover and allocate available ports for all services

// packages/cli/src/services/PortManager.ts

import { isPortAvailable } from "../lib/port.js";

export interface ServicePorts {
  dashboard: number;
  terminalWs: number;
  directTerminalWs: number;
}

export class PortManager {
  private allocatedPorts: Set<number> = new Set();

  /**
   * Allocate ports for all dashboard services
   */
  async allocateServicePorts(preferredDashboardPort: number): Promise<ServicePorts> {
    const dashboard = await this.findNextAvailable(preferredDashboardPort);
    const terminalWs = await this.findNextAvailable(3001);
    const directTerminalWs = await this.findNextAvailable(3003);

    return { dashboard, terminalWs, directTerminalWs };
  }

  /**
   * Find next available port starting from preferred
   */
  private async findNextAvailable(preferred: number, maxAttempts = 10): Promise<number> {
    for (let offset = 0; offset < maxAttempts; offset++) {
      const port = preferred + offset;

      if (this.allocatedPorts.has(port)) {
        continue; // Skip already allocated
      }

      if (await isPortAvailable(port)) {
        this.allocatedPorts.add(port);
        return port;
      }
    }

    throw new Error(
      `Could not find available port near ${preferred} after ${maxAttempts} attempts`
    );
  }

  /**
   * Release a port back to the pool
   */
  release(port: number): void {
    this.allocatedPorts.delete(port);
  }

  /**
   * Release all allocated ports
   */
  releaseAll(): void {
    this.allocatedPorts.clear();
  }
}

Benefits:

  • All service ports discovered dynamically
  • No hardcoded ports in stop logic
  • Prevents port conflicts
  • Easy to add new services

Migration:

// Before
const port = await findAvailablePort(config.port ?? 4000);
// WebSocket ports hardcoded in scripts

// After
const portManager = new PortManager();
const ports = await portManager.allocateServicePorts(config.port ?? 4000);
// ports.dashboard, ports.terminalWs, ports.directTerminalWs

3. DashboardManager

Purpose: Unified dashboard lifecycle management

// packages/cli/src/services/DashboardManager.ts

import { spawn, type ChildProcess } from "node:child_process";
import { existsSync, resolve } from "node:fs";
import chalk from "chalk";
import { findWebDir } from "../lib/web-dir.js";
import { exec } from "../lib/shell.js";
import type { ServicePorts } from "./PortManager.js";

export interface DashboardOptions {
  ports: ServicePorts;
  configPath: string | null;
  openBrowser?: boolean;
}

export class DashboardManager {
  /**
   * Start dashboard and all WebSocket servers
   */
  async start(options: DashboardOptions): Promise<ChildProcess> {
    const { ports, configPath, openBrowser = false } = options;
    const webDir = findWebDir();

    // Validate web package exists
    if (!existsSync(resolve(webDir, "package.json"))) {
      throw new Error(
        "Could not find @composio/ao-web package.\n" +
        "Ensure it is installed: pnpm install"
      );
    }

    // Build environment with all ports and config
    const env = {
      ...process.env,
      PORT: String(ports.dashboard),
      TERMINAL_WS_PORT: String(ports.terminalWs),
      DIRECT_TERMINAL_WS_PORT: String(ports.directTerminalWs),
    };

    // Add config path if available
    if (configPath) {
      env["AO_CONFIG_PATH"] = configPath;
    }

    console.log(
      chalk.dim(`Starting dashboard on http://localhost:${ports.dashboard}`)
    );
    console.log(
      chalk.dim(`  - Terminal WebSocket: ${ports.terminalWs}`)
    );
    console.log(
      chalk.dim(`  - Direct Terminal WebSocket: ${ports.directTerminalWs}`)
    );

    // Start unified dev server (Next.js + both WebSocket servers)
    const child = spawn("pnpm", ["run", "dev"], {
      cwd: webDir,
      stdio: "inherit",
      env,
    });

    // Handle errors
    child.on("error", (err) => {
      throw new Error(`Dashboard failed to start: ${err.message}`);
    });

    // Open browser after delay
    if (openBrowser) {
      this.openBrowser(ports.dashboard);
    }

    return child;
  }

  /**
   * Stop dashboard and all WebSocket servers
   */
  async stop(ports: ServicePorts): Promise<void> {
    const allPorts = [
      ports.dashboard,
      ports.terminalWs,
      ports.directTerminalWs,
    ];

    console.log(chalk.dim("Stopping dashboard and WebSocket servers..."));

    // Collect all PIDs across all ports
    const allPids: string[] = [];

    for (const port of allPorts) {
      try {
        const { stdout } = await exec("lsof", ["-ti", `:${port}`]);
        const pids = stdout
          .trim()
          .split("\n")
          .filter((pid) => pid.length > 0);
        allPids.push(...pids);
      } catch {
        // Port not in use, continue
      }
    }

    if (allPids.length === 0) {
      console.log(chalk.yellow("Dashboard not running"));
      return;
    }

    // Deduplicate PIDs (parent process may appear on multiple ports)
    const uniquePids = [...new Set(allPids)];

    try {
      await exec("kill", uniquePids);
      console.log(chalk.green("Dashboard and WebSocket servers stopped"));
    } catch (err) {
      console.log(
        chalk.yellow("Could not stop some processes (may have already exited)")
      );
    }
  }

  /**
   * Check if dashboard is running
   */
  async isRunning(port: number): Promise<boolean> {
    try {
      const response = await fetch(`http://localhost:${port}`);
      return response.ok;
    } catch {
      return false;
    }
  }

  /**
   * Open browser after delay
   */
  private openBrowser(port: number, delay = 3000): void {
    setTimeout(() => {
      const browser = spawn("open", [`http://localhost:${port}`], {
        stdio: "ignore",
      });
      browser.on("error", () => {
        // Best effort - ignore if open command fails
      });
    }, delay);
  }
}

Benefits:

  • Unified dashboard startup logic
  • No duplication between start and dashboard commands
  • Consistent error handling and logging
  • Easy to test and mock

Migration:

// Before (in start.ts and dashboard.ts - duplicated)
const env = { ...process.env, PORT: String(port) };
if (configPath) env["AO_CONFIG_PATH"] = configPath;
const child = spawn("pnpm", ["run", "dev"], { cwd: webDir, env });
// ... hardcoded port killing logic

// After (in both commands - DRY)
const dashboardManager = new DashboardManager();
await dashboardManager.start({ ports, configPath, openBrowser: true });
// ...
await dashboardManager.stop(ports);

4. MetadataService

Purpose: Atomic metadata operations with locking

// packages/cli/src/services/MetadataService.ts

import { readMetadata, writeMetadata, type SessionMetadata } from "@composio/ao-core";

export class MetadataService {
  private locks: Map<string, Promise<void>> = new Map();

  /**
   * Atomically update metadata for a session
   * Merges updates with existing metadata
   */
  async updateMetadata(
    dataDir: string,
    sessionId: string,
    updates: Partial<SessionMetadata>
  ): Promise<void> {
    await this.acquireLock(sessionId);

    try {
      const existing = readMetadata(dataDir, sessionId) ?? {};
      const merged = { ...existing, ...updates };
      writeMetadata(dataDir, sessionId, merged);
    } finally {
      this.releaseLock(sessionId);
    }
  }

  /**
   * Get metadata for a session
   */
  async getMetadata(
    dataDir: string,
    sessionId: string
  ): Promise<SessionMetadata | null> {
    return readMetadata(dataDir, sessionId);
  }

  /**
   * Acquire lock for a session (prevents concurrent updates)
   */
  private async acquireLock(sessionId: string): Promise<void> {
    // Wait for any existing lock to release
    while (this.locks.has(sessionId)) {
      await this.locks.get(sessionId);
    }

    // Create new lock
    let releaseFn: () => void;
    const lockPromise = new Promise<void>((resolve) => {
      releaseFn = resolve;
    });

    this.locks.set(sessionId, lockPromise);
  }

  /**
   * Release lock for a session
   */
  private releaseLock(sessionId: string): void {
    const lockPromise = this.locks.get(sessionId);
    this.locks.delete(sessionId);

    // Resolve to release waiters (if any)
    if (lockPromise) {
      // The promise was created with a resolve function
      // that we called releaseFn - this is a simplification
      // In practice, we'd store the resolve function
    }
  }
}

Benefits:

  • Atomic updates prevent race conditions
  • Simple API: updateMetadata(sessionId, { dashboardPort: 4000 })
  • Centralized validation
  • Easy to add caching

Migration:

// Before (race condition possible)
const existing = readMetadata(config.dataDir, sessionId);
if (existing) {
  writeMetadata(config.dataDir, sessionId, {
    ...existing,
    dashboardPort: port,
  });
}

// After (atomic)
const metadataService = new MetadataService();
await metadataService.updateMetadata(config.dataDir, sessionId, {
  dashboardPort: port,
});

5. ProcessManager

Purpose: Unified process spawning and killing with graceful shutdown

// packages/cli/src/services/ProcessManager.ts

import { spawn, type ChildProcess, type SpawnOptions } from "node:child_process";
import chalk from "chalk";

interface ManagedSpawnOptions extends SpawnOptions {
  description?: string;
}

export class ProcessManager {
  /**
   * Spawn a process with consistent logging and error handling
   */
  async spawn(
    command: string,
    args: string[],
    options: ManagedSpawnOptions = {}
  ): Promise<ChildProcess> {
    const { description, ...spawnOpts } = options;

    if (description) {
      console.log(chalk.dim(`Starting: ${description}`));
    }

    const child = spawn(command, args, spawnOpts);

    child.on("error", (err) => {
      const label = description || `${command} ${args.join(" ")}`;
      console.error(chalk.red(`Process error (${label}): ${err.message}`));
    });

    return child;
  }

  /**
   * Kill a process gracefully with fallback to force kill
   */
  async kill(pid: number, signal: NodeJS.Signals = "SIGTERM"): Promise<void> {
    try {
      process.kill(pid, signal);

      // Wait for graceful shutdown (5 seconds)
      await this.waitForExit(pid, 5000);
    } catch {
      // Graceful shutdown failed, force kill
      try {
        console.log(chalk.yellow(`Force killing process ${pid}`));
        process.kill(pid, "SIGKILL");
      } catch {
        // Already dead
      }
    }
  }

  /**
   * Wait for a process to exit
   */
  private waitForExit(pid: number, timeout: number): Promise<void> {
    return new Promise((resolve, reject) => {
      const timer = setTimeout(() => {
        reject(new Error(`Process ${pid} did not exit within ${timeout}ms`));
      }, timeout);

      // Poll for process exit
      const interval = setInterval(() => {
        try {
          process.kill(pid, 0); // Check if alive (throws if dead)
        } catch {
          // Process is dead
          clearInterval(interval);
          clearTimeout(timer);
          resolve();
        }
      }, 100);
    });
  }
}

Benefits:

  • Consistent logging across all process operations
  • Graceful shutdown with automatic fallback
  • Easy to add monitoring and metrics
  • Testable without spawning real processes

Migration:

// Before
const child = spawn("pnpm", ["run", "dev"], { cwd: webDir, stdio: "inherit" });
child.on("error", (err) => {
  console.error("Could not start dashboard");
});

// After
const processManager = new ProcessManager();
const child = await processManager.spawn("pnpm", ["run", "dev"], {
  cwd: webDir,
  stdio: "inherit",
  description: "Dashboard dev server",
});

Updated Commands (After Refactor)

start.ts Example

// packages/cli/src/commands/start.ts (simplified)

import ConfigService from "../services/ConfigService.js";
import { PortManager } from "../services/PortManager.js";
import { DashboardManager } from "../services/DashboardManager.js";
import { MetadataService } from "../services/MetadataService.js";

export function registerStart(program: Command): void {
  program
    .command("start [project]")
    .description("Start orchestrator agent and dashboard for a project")
    .action(async (projectId?: string, opts?) => {
      // Get config (cached, efficient)
      const configService = ConfigService.getInstance();
      const config = configService.getConfig();
      const configPath = configService.getConfigPath();

      // Allocate ports for all services
      const portManager = new PortManager();
      const ports = await portManager.allocateServicePorts(config.port ?? 4000);

      // Start dashboard
      const dashboardManager = new DashboardManager();
      await dashboardManager.start({
        ports,
        configPath,
        openBrowser: true,
      });

      // Create/update orchestrator session
      const sessionId = `${projectId}-orchestrator`;
      const exists = await hasTmuxSession(sessionId);

      const metadataService = new MetadataService();

      if (exists) {
        // Update existing session metadata
        await metadataService.updateMetadata(config.dataDir, sessionId, {
          dashboardPort: ports.dashboard,
        });
        console.log(chalk.yellow("Orchestrator session already running"));
      } else {
        // Create new session
        const runtime = getRuntime(config, projectId);
        const runtimeHandle = await runtime.create(sessionId, {
          /* ... */
        });

        // Write full metadata atomically
        await metadataService.updateMetadata(config.dataDir, sessionId, {
          worktree: project.path,
          branch: project.defaultBranch,
          status: "working",
          project: projectId,
          createdAt: new Date().toISOString(),
          runtimeHandle,
          dashboardPort: ports.dashboard,
        });
      }
    });
}

Benefits:

  • 50% less code - No duplication, services handle complexity
  • Clear intent - Each line has one responsibility
  • Easy to test - Mock services, not filesystem
  • Type safe - Services enforce correct usage

WebSocket Server Updates

Current (Hardcoded Ports)

// packages/web/package.json
{
  "scripts": {
    "dev:next": "next dev -p ${PORT:-3000}",
    "dev:terminal": "tsx watch server/terminal-websocket.ts",
    "dev:direct-terminal": "tsx watch server/direct-terminal-ws.ts"
  }
}
// packages/web/server/terminal-websocket.ts
const PORT = 3001; // Hardcoded!
server.listen(PORT);

After Refactor (Environment-Based)

// packages/web/package.json
{
  "scripts": {
    "dev:next": "next dev -p ${PORT:-3000}",
    "dev:terminal": "tsx watch server/terminal-websocket.ts",
    "dev:direct-terminal": "tsx watch server/direct-terminal-ws.ts"
  }
}
// packages/web/server/terminal-websocket.ts
const PORT = parseInt(process.env.TERMINAL_WS_PORT ?? "3001", 10);
server.listen(PORT);
console.log(`[Terminal] Server listening on port ${PORT}`);
// packages/web/server/direct-terminal-ws.ts
const PORT = parseInt(process.env.DIRECT_TERMINAL_WS_PORT ?? "3003", 10);
server.listen(PORT);
console.log(`[DirectTerminal] WebSocket server listening on port ${PORT}`);

Benefits:

  • Ports configurable via environment
  • No code changes needed, just env vars
  • Integration tests can use different ports

Migration Plan

Phase 1: Add Services (Non-Breaking)

Goal: Create service layer without breaking existing code

  1. Create packages/cli/src/services/ directory
  2. Implement ConfigService.ts
  3. Implement PortManager.ts
  4. Implement DashboardManager.ts
  5. Add comprehensive unit tests for each service

Validation:

  • All tests pass
  • Existing commands still work
  • Services tested in isolation

Phase 2: Update WebSocket Servers

Goal: Make WebSocket ports configurable

  1. Update terminal-websocket.ts to read TERMINAL_WS_PORT
  2. Update direct-terminal-ws.ts to read DIRECT_TERMINAL_WS_PORT
  3. Update DashboardManager to pass env vars
  4. Test with custom ports

Validation:

  • WebSocket servers start on configured ports
  • Integration tests pass with custom ports

Phase 3: Refactor Commands

Goal: Migrate commands to use services

  1. Update start.ts:
    • Use ConfigService.getInstance()
    • Use PortManager for port allocation
    • Use DashboardManager for dashboard lifecycle
  2. Update stop.ts:
    • Use DashboardManager.stop() instead of custom logic
  3. Update dashboard.ts:
    • Use DashboardManager.start() instead of duplicate logic

Validation:

  • All commands work with new services
  • Integration tests pass
  • Manual testing on macOS/Linux

Phase 4: Add Metadata & Process Services

Goal: Atomic metadata updates and unified process management

  1. Implement MetadataService.ts
  2. Implement ProcessManager.ts
  3. Replace all readMetadata + writeMetadata with MetadataService
  4. Replace all raw spawn/exec with ProcessManager

Validation:

  • No race conditions in metadata updates
  • Graceful process shutdown works
  • Error handling is consistent

Phase 5: Integration Test Updates (DONE)

Goal: Verify all improvements work in CI

  1. Port conflict test — PortManager.real-ports.test.ts (6 tests, real port binding)
  2. Metadata lifecycle test — metadata-lifecycle.integration.test.ts (13 tests, real filesystem)
  3. Concurrent access test — concurrent writes, updates, and last-write-wins verification
  4. Config→metadata service integration — config-metadata-service.integration.test.ts (5 tests)
  5. dashboardPort serialization verified end-to-end

Validation:

  • CLI: 182 tests pass (17 files)
  • Integration: 163 tests pass (17 files), 1 pre-existing codex binary failure
  • No flaky tests

Phase 6: Documentation & Cleanup (DONE)

Goal: Document new architecture and remove old code

  1. Remove dead findAvailablePort from port.ts (superseded by PortManager)
  2. Remove dead lib/metadata.ts and its test (superseded by MetadataService + core)
  3. All services have JSDoc (ConfigService, PortManager, DashboardManager, MetadataService, ProcessManager)
  4. port.ts streamlined to single isPortAvailable used by PortManager

Validation:

  • No dead code remains
  • All 164 CLI tests pass (16 files)
  • Typecheck and lint clean

Testing Strategy

Unit Tests (New)

// packages/cli/src/services/__tests__/ConfigService.test.ts
describe("ConfigService", () => {
  it("should cache config after first load", () => {
    const service = ConfigService.getInstance();
    const config1 = service.getConfig();
    const config2 = service.getConfig();
    expect(config1).toBe(config2); // Same instance
  });

  it("should reload config when requested", () => {
    const service = ConfigService.getInstance();
    const config1 = service.getConfig();
    service.reload();
    const config2 = service.getConfig();
    expect(config1).not.toBe(config2); // Different instance
  });
});

Integration Tests (Enhanced)

# Test port allocation
- Start dashboard with preferred port occupied
- Verify fallback port is used
- Verify all services start on correct ports

# Test race conditions
- Start two orchestrators concurrently
- Verify metadata is correct
- Verify no data corruption

# Test graceful shutdown
- Start dashboard
- Send SIGTERM
- Verify clean shutdown within 5s
- Verify no orphaned processes

Benefits Summary

Category Improvement Impact
Code Quality -50% duplication Easier maintenance
Reliability Atomic operations No race conditions
Testability Service isolation 3x faster tests
Performance Config caching Fewer filesystem ops
Flexibility All ports configurable Easy to add services
Debugging Unified logging Faster troubleshooting
Architecture Clear separation Easier onboarding

Success Metrics

Before Refactor

  • Dashboard startup duplicated in 2 places
  • 5 different locations with port hardcoding
  • Race conditions in metadata updates
  • 10+ direct spawn/exec calls
  • Config loaded 3-5 times per command

After Refactor

  • Dashboard startup in 1 place (DashboardManager)
  • All ports in 1 place (PortManager)
  • Atomic metadata updates (MetadataService)
  • Unified process management (ProcessManager)
  • Config loaded once (ConfigService)

Open Questions

  1. File Locking: Should we use actual file locks (fs.flock) instead of in-memory locks for metadata?

    • Pro: Works across processes
    • Con: More complex, OS-dependent
  2. Port Persistence: Should we persist allocated ports to disk?

    • Pro: Can restore port mappings after restart
    • Con: Adds complexity
  3. Service Discovery: Should services register themselves with a central registry?

    • Pro: Easy to list all running services
    • Con: Adds indirection
  4. Health Checks: Should we add periodic health checks for all services?

    • Pro: Can detect and restart failed services
    • Con: Adds overhead

References

  • Related PR: #70 - Dashboard Config Discovery & Integration Tests
  • Related Issues: Port conflicts, race conditions, code duplication
  • Architecture Pattern: Service Layer Pattern
  • Inspiration: Clean Architecture, SOLID principles

Next Steps:

  1. Review this plan with team
  2. Get consensus on phased approach
  3. Create tracking issues for each phase
  4. Begin Phase 1 implementation

Estimated Effort: 2-3 days for full implementation + testing