feat: collect diagnostic logs on container startup failure (#1941)

* Initial plan

* feat: collect diagnostic logs on container startup failure

Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/76309f9a-1e18-4e50-848e-0057b10dc6a1

* fix: signal containersStarted on partial startup failure

Address review feedback:
- Call onContainersStarted() in the startContainers catch block so
  the caller's cleanup runs stopContainers() even on partial startup,
  preventing orphaned containers/networks.
- Use .rejects.toBe() instead of .rejects.toThrow() in tests to
  verify exact error identity (rethrow, not wrap).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Landon Cox <landon.cox@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: 3 people

src/cli-workflow.test.ts

@@ -310,4 +310,67 @@ describe('runMainWorkflow', () => {
 
     await expect(runMainWorkflow(configWithDiagnostics, dependencies, { logger, performCleanup: jest.fn() })).resolves.toBe(1);
   });
+
+  it('calls collectDiagnosticLogs on startContainers failure when diagnosticLogs is enabled', async () => {
+    const startError = new Error('Squid container is unhealthy');
+    const collectDiagnosticLogs = jest.fn().mockResolvedValue(undefined);
+    const configWithDiagnostics: WrapperConfig = {
+      ...baseConfig,
+      diagnosticLogs: true,
+    };
+    const dependencies: WorkflowDependencies = {
+      ensureFirewallNetwork: jest.fn().mockResolvedValue({ squidIp: '172.30.0.10' }),
+      setupHostIptables: jest.fn().mockResolvedValue(undefined),
+      writeConfigs: jest.fn().mockResolvedValue(undefined),
+      startContainers: jest.fn().mockRejectedValue(startError),
+      runAgentCommand: jest.fn(),
+      collectDiagnosticLogs,
+    };
+    const logger = createLogger();
+
+    await expect(runMainWorkflow(configWithDiagnostics, dependencies, { logger, performCleanup: jest.fn() })).rejects.toBe(startError);
+
+    expect(collectDiagnosticLogs).toHaveBeenCalledWith(configWithDiagnostics.workDir);
+    expect(dependencies.runAgentCommand).not.toHaveBeenCalled();
+  });
+
+  it('does not call collectDiagnosticLogs on startContainers failure when diagnosticLogs is disabled', async () => {
+    const startError = new Error('Squid container is unhealthy');
+    const collectDiagnosticLogs = jest.fn().mockResolvedValue(undefined);
+    const dependencies: WorkflowDependencies = {
+      ensureFirewallNetwork: jest.fn().mockResolvedValue({ squidIp: '172.30.0.10' }),
+      setupHostIptables: jest.fn().mockResolvedValue(undefined),
+      writeConfigs: jest.fn().mockResolvedValue(undefined),
+      startContainers: jest.fn().mockRejectedValue(startError),
+      runAgentCommand: jest.fn(),
+      collectDiagnosticLogs,
+    };
+    const logger = createLogger();
+
+    await expect(runMainWorkflow(baseConfig, dependencies, { logger, performCleanup: jest.fn() })).rejects.toBe(startError);
+
+    expect(collectDiagnosticLogs).not.toHaveBeenCalled();
+  });
+
+  it('rethrows startContainers error after collecting diagnostics', async () => {
+    const startError = new Error('docker compose failed');
+    const configWithDiagnostics: WrapperConfig = {
+      ...baseConfig,
+      diagnosticLogs: true,
+    };
+    const performCleanup = jest.fn().mockResolvedValue(undefined);
+    const dependencies: WorkflowDependencies = {
+      ensureFirewallNetwork: jest.fn().mockResolvedValue({ squidIp: '172.30.0.10' }),
+      setupHostIptables: jest.fn().mockResolvedValue(undefined),
+      writeConfigs: jest.fn().mockResolvedValue(undefined),
+      startContainers: jest.fn().mockRejectedValue(startError),
+      runAgentCommand: jest.fn(),
+      collectDiagnosticLogs: jest.fn().mockResolvedValue(undefined),
+    };
+    const logger = createLogger();
+
+    await expect(runMainWorkflow(configWithDiagnostics, dependencies, { logger, performCleanup })).rejects.toBe(startError);
+    // performCleanup should NOT be called — that is the caller's (cli.ts) responsibility
+    expect(performCleanup).not.toHaveBeenCalled();
+  });
 });

src/cli-workflow.ts

@@ -71,7 +71,25 @@ export async function runMainWorkflow(
   await dependencies.writeConfigs(config);
 
   // Step 2: Start containers
-  await dependencies.startContainers(config.workDir, config.allowedDomains, config.proxyLogsDir, config.skipPull);
+  try {
+    await dependencies.startContainers(config.workDir, config.allowedDomains, config.proxyLogsDir, config.skipPull);
+  } catch (startError) {
+    // Signal that containers may have been partially created so the caller's
+    // cleanup (stopContainers / docker compose down -v) will tear them down
+    // instead of leaving orphaned containers and networks.
+    onContainersStarted?.();
+
+    // Collect diagnostics for startup failures before containers are torn down.
+    // Must happen before performCleanup() / stopContainers() destroys them.
+    if (config.diagnosticLogs && dependencies.collectDiagnosticLogs) {
+      try {
+        await dependencies.collectDiagnosticLogs(config.workDir);
+      } catch (diagError) {
+        logger.warn('Failed to collect diagnostic logs; continuing with cleanup.', diagError);
+      }
+    }
+    throw startError;
+  }
   onContainersStarted?.();
 
   // Step 3: Wait for agent to complete

src/docker-manager.ts

@@ -2521,9 +2521,9 @@ export async function collectDiagnosticLogs(workDir: string): Promise<void> {
   ];
 
   for (const container of containers) {
-    // Collect stdout+stderr from docker logs
+    // Collect stdout+stderr from docker logs (last 200 lines to keep files manageable)
     try {
-      const result = await execa('docker', ['logs', container], { reject: false });
+      const result = await execa('docker', ['logs', '--tail', '200', container], { reject: false });
       if (result.exitCode === 0) {
         const combined = [result.stdout, result.stderr].filter(Boolean).join('\n').trim();
         if (combined) {