From 69438fcd011d610a1da10020b86b3b7ee2d09caf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Apr 2026 18:57:58 +0000 Subject: [PATCH 1/3] Initial plan From 889b02090174e7858f69291e5e535c636c829436 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Apr 2026 19:04:47 +0000 Subject: [PATCH 2/3] feat: collect diagnostic logs on container startup failure Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/76309f9a-1e18-4e50-848e-0057b10dc6a1 --- src/cli-workflow.test.ts | 63 ++++++++++++++++++++++++++++++++++++++++ src/cli-workflow.ts | 15 +++++++++- src/docker-manager.ts | 4 +-- 3 files changed, 79 insertions(+), 3 deletions(-) diff --git a/src/cli-workflow.test.ts b/src/cli-workflow.test.ts index fe958bba..da0829dc 100644 --- a/src/cli-workflow.test.ts +++ b/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.toThrow(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.toThrow(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.toThrow(startError); + // performCleanup should NOT be called — that is the caller's (cli.ts) responsibility + expect(performCleanup).not.toHaveBeenCalled(); + }); }); diff --git a/src/cli-workflow.ts b/src/cli-workflow.ts index 91130c6c..8175f690 100644 --- a/src/cli-workflow.ts +++ b/src/cli-workflow.ts @@ -71,7 +71,20 @@ 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) { + // 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 diff --git a/src/docker-manager.ts b/src/docker-manager.ts index 94edf330..549a5693 100644 --- a/src/docker-manager.ts +++ b/src/docker-manager.ts @@ -2521,9 +2521,9 @@ export async function collectDiagnosticLogs(workDir: string): Promise { ]; 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) { From 36222402cd92bcbf9963113d87fb48601c7d4d62 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Sun, 12 Apr 2026 12:24:14 -0700 Subject: [PATCH 3/3] 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> --- src/cli-workflow.test.ts | 6 +++--- src/cli-workflow.ts | 5 +++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/cli-workflow.test.ts b/src/cli-workflow.test.ts index da0829dc..b7f1a3ad 100644 --- a/src/cli-workflow.test.ts +++ b/src/cli-workflow.test.ts @@ -328,7 +328,7 @@ describe('runMainWorkflow', () => { }; const logger = createLogger(); - await expect(runMainWorkflow(configWithDiagnostics, dependencies, { logger, performCleanup: jest.fn() })).rejects.toThrow(startError); + await expect(runMainWorkflow(configWithDiagnostics, dependencies, { logger, performCleanup: jest.fn() })).rejects.toBe(startError); expect(collectDiagnosticLogs).toHaveBeenCalledWith(configWithDiagnostics.workDir); expect(dependencies.runAgentCommand).not.toHaveBeenCalled(); @@ -347,7 +347,7 @@ describe('runMainWorkflow', () => { }; const logger = createLogger(); - await expect(runMainWorkflow(baseConfig, dependencies, { logger, performCleanup: jest.fn() })).rejects.toThrow(startError); + await expect(runMainWorkflow(baseConfig, dependencies, { logger, performCleanup: jest.fn() })).rejects.toBe(startError); expect(collectDiagnosticLogs).not.toHaveBeenCalled(); }); @@ -369,7 +369,7 @@ describe('runMainWorkflow', () => { }; const logger = createLogger(); - await expect(runMainWorkflow(configWithDiagnostics, dependencies, { logger, performCleanup })).rejects.toThrow(startError); + 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(); }); diff --git a/src/cli-workflow.ts b/src/cli-workflow.ts index 8175f690..46c4defb 100644 --- a/src/cli-workflow.ts +++ b/src/cli-workflow.ts @@ -74,6 +74,11 @@ export async function runMainWorkflow( 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) {