Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds an opt-in diagnostic logging feature to capture container failure details (logs/state/mounts + sanitized compose) before teardown, improving debuggability when containers fail to reach steady state.
Changes:
- Introduces
--diagnostic-logs/features.awf-diagnostic-logsconfig to trigger diagnostics on non-zero exit. - Implements
collectDiagnosticLogs(workDir)and preserves diagnostics duringcleanup()(to audit dir or/tmp). - Adds workflow and CLI wiring plus unit tests for diagnostics collection and preservation.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Documents and exposes diagnosticLogs config flag on WrapperConfig. |
| src/docker-manager.ts | Implements diagnostics collection and preservation during cleanup. |
| src/docker-manager.test.ts | Adds unit tests covering diagnostics collection + cleanup preservation behavior. |
| src/cli.ts | Adds --diagnostic-logs option and passes config/dependency through. |
| src/cli-workflow.ts | Calls diagnostics collection before cleanup on non-zero exit. |
| src/cli-workflow.test.ts | Tests call ordering and conditional invocation of diagnostics collection. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 5
| const result = await execa('docker', ['logs', container], { reject: false }); | ||
| const combined = [result.stdout, result.stderr].filter(Boolean).join('\n').trim(); | ||
| if (combined) { | ||
| fs.writeFileSync(path.join(diagnosticsDir, `${container}.log`), combined + '\n'); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 85.85% | 85.85% | ➡️ +0.00% |
| Statements | 85.76% | 85.73% | 📉 -0.03% |
| Functions | 87.54% | 87.70% | 📈 +0.16% |
| Branches | 78.56% | 78.60% | 📈 +0.04% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
86.3% → 86.2% (-0.08%) | 85.9% → 85.7% (-0.13%) |
src/cli-workflow.ts |
92.0% → 92.6% (+0.59%) | 92.0% → 92.6% (+0.59%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 85.85% | 85.86% | 📈 +0.01% |
| Statements | 85.76% | 85.74% | 📉 -0.02% |
| Functions | 87.54% | 87.70% | 📈 +0.16% |
| Branches | 78.56% | 78.60% | 📈 +0.04% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
86.3% → 86.2% (-0.05%) | 85.9% → 85.8% (-0.10%) |
src/cli-workflow.ts |
92.0% → 92.6% (+0.59%) | 92.0% → 92.6% (+0.59%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 85.85% | 85.84% | ➡️ -0.01% |
| Statements | 85.76% | 85.72% | 📉 -0.04% |
| Functions | 87.54% | 87.70% | 📈 +0.16% |
| Branches | 78.56% | 78.60% | 📈 +0.04% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli-workflow.ts |
92.0% → 89.7% (-2.35%) | 92.0% → 89.7% (-2.35%) |
src/docker-manager.ts |
86.3% → 86.2% (-0.05%) | 85.9% → 85.8% (-0.10%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
🔥 Smoke Test Results
Overall: PASS Author:
|
Smoke Test Results
Overall: PASS
|
|
PR titles (latest merged):
Results: 1✅ 2❌ 3✅ 4❌ 5✅ 6✅ 7❌ 8✅
|
Chroot Version Comparison Results
Result: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
Smoke Test: GitHub Actions Services Connectivity ✅All checks passed:
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
When AWF containers crash before reaching steady state (e.g. Squid exits code 1 in DinD environments), application-level logs are never written, leaving engineers with no visibility into the root cause. This adds opt-in diagnostic collection triggered on non-zero exit.
Changes
New:
--diagnostic-logsflag--diagnostic-logsCLI flag orfeatures.awf-diagnostic-logs: trueworkflow frontmatterdocker compose down -vso container state is still accessiblecollectDiagnosticLogs(workDir)(src/docker-manager.ts)Runs against
awf-squid,awf-agent,awf-api-proxy,awf-iptables-init(containers that never started are silently skipped):<container>.log— stdout+stderr viadocker logs<container>.state— exit code + error string viadocker inspect --format '{{.State.ExitCode}} {{.State.Error}}'<container>.mounts.json— mount metadata viadocker inspect --format '{{json .Mounts}}'docker-compose.yml— sanitized copy with env var values matching\w*(?:TOKEN|KEY|SECRET)\w*redactedExplicitly NOT collected: raw env vars, full
docker inspectoutput, host filesystem contents.Preservation (
cleanup())--audit-dir: diagnostics land at<audit-dir>/diagnostics/(single upload path for CI artifacts)/tmp/awf-diagnostics-<timestamp>/Workflow integration (
src/cli-workflow.ts)collectDiagnosticLogsadded as optionalWorkflowDependenciesfield; called betweenrunAgentCommandandperformCleanuponly whendiagnosticLogs && exitCode !== 0.