Skip to content

Improve PR Sous Chef engine-failure context for AWF startup crashes #34524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
May 24, 2026
+37 −0
Merged

Improve PR Sous Chef engine-failure context for AWF startup crashes #34524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3a3d8f
Initial plan
Copilot May 24, 2026
e3dc0c0
Improve AWF startup error extraction in engine failure context
Copilot May 24, 2026
441f86e
Add missing stderr assertion for AWF startup failure test
Copilot May 24, 2026
e3bc7d9
Refine AWF [ERROR] startup parsing and test coverage naming
Copilot May 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions actions/setup/js/handle_agent_failure.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1571,13 +1571,30 @@ function buildEngineFailureContext() {
continue;
}

// AWF startup failures can appear as "[ERROR] Failed to start ...".
// Exclude generic wrapper [ERROR] lines (e.g., command completion/exit noise)
// because they are infrastructure output and don't provide actionable startup context.
const awfStartupBracketErrorMatch = line.match(/^\[ERROR\]\s*((?:Failed to start|dependency failed to start:).+)$/);
if (awfStartupBracketErrorMatch) {
errorMessages.add(awfStartupBracketErrorMatch[1].trim());
continue;
}

// Fatal errors: "Fatal: <message>" or "FATAL: <message>"
const fatalMatch = line.match(/^(?:FATAL|Fatal):\s*(.+)$/);
if (fatalMatch) {
errorMessages.add(fatalMatch[1].trim());
continue;
}

// AWF docker-compose dependency failures surface this root-cause line without
// an explicit log-level prefix.
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant string reconstruction: the template literal re-assembles the full original line unnecessarily.

💡 Suggested simplification

Instead of:

errorMessages.add(`dependency failed to start: ${awfDependencyFailureMatch[1].trim()}`);

Use the capture group directly or just trim the original line:

errorMessages.add(line.trim());
// or
errorMessages.add(`dependency failed to start: ${awfDependencyFailureMatch[1].trim()}`);

Actually the current form is fine, but note the inconsistency: the [ERROR] handler strips its prefix (bracketErrorMatch[1].trim()), while this handler preserves it (dependency failed to start: ...). If both are intentional, a brief comment on why [ERROR] is stripped would help readers understand the difference.

const awfDependencyFailureMatch = line.match(/^dependency failed to start:\s*(.+)$/);
if (awfDependencyFailureMatch) {
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] Minor: the dependency failure message is reconstructed from the capture group rather than using line.trim() or match[0]. This normalises whitespace between the prefix and container name, which is intentional but subtle.

💡 Simpler alternative

Using match[0] (the full matched string) or line.trim() avoids the reconstruction and is more obviously correct:

// Option A — use the full match (already trimmed by regex anchor)
errorMessages.add(awfDependencyFailureMatch[0]);

// Option B — trim the original line
errorMessages.add(line.trim());

The current approach is safe, but a comment noting the whitespace normalisation intent would help the next reader.

errorMessages.add(`dependency failed to start: ${awfDependencyFailureMatch[1].trim()}`);
continue;
}

// Go runtime panic: "panic: <message>"
const panicMatch = line.match(/^panic:\s*(.+)$/);
if (panicMatch) {
Expand Down
20 changes: 20 additions & 0 deletions actions/setup/js/handle_agent_failure.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1208,6 +1208,24 @@ describe("handle_agent_failure", () => {
expect(result).toContain("connect ECONNREFUSED 127.0.0.1:8080");
});

it("extracts AWF startup errors from dependency lines and container startup failures", () => {
const lines = [
" Container awf-squid Error",
"dependency failed to start: container awf-squid is unhealthy",
"[ERROR] Failed to start containers: Error: Command failed with exit code 1: docker compose up -d --pull never",
" stdout: undefined,",
" stderr: undefined,",
];
fs.writeFileSync(stdioLogPath, lines.join("\n") + "\n");
const result = buildEngineFailureContext();
expect(result).toContain("Engine Failure");
expect(result).toContain("dependency failed to start: container awf-squid is unhealthy");
expect(result).toContain("Failed to start containers: Error: Command failed with exit code 1: docker compose up -d --pull never");
expect(result).not.toContain("Last agent output");
expect(result).not.toContain("stdout: undefined");
expect(result).not.toContain("stderr: undefined");
});

it("detects Fatal: prefix pattern", () => {
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] not.toContain("Last agent output") ties the test to a specific heading string — if that section heading changes the assertion silently becomes weaker without failing.

💡 Consider a more structural assertion

If the intent is to assert the fallback path was not taken, a positive assertion on the structured output (e.g. that only the Engine Failure section is present and not the fallback block) is more durable:

// Assert the fallback section is absent by checking its structural marker
expect(result).not.toMatch(/Last agent output|Fallback/i);
// or: assert the result does NOT contain the fallback sentinel string you own

Alternatively, a short comment explaining what "Last agent output" refers to (the fallback section heading) would make the intent clear for future maintainers.

fs.writeFileSync(stdioLogPath, "Fatal: out of memory\n");
const result = buildEngineFailureContext();
Expand Down Expand Up @@ -1288,6 +1306,7 @@ describe("handle_agent_failure", () => {
"[SUCCESS] Containers stopped successfully",
"[INFO] Agent session state preserved at: /tmp/awf-agent-session-state-abc123",
"[INFO] API proxy logs available at: /tmp/gh-aw/sandbox/firewall/logs/api-proxy-logs",
"[ERROR] Command completed with exit code: 1",
"[WARN] Command completed with exit code: 1",
"Process exiting with code: 1",
];
Expand All @@ -1300,6 +1319,7 @@ describe("handle_agent_failure", () => {
expect(result).not.toContain("Last agent output");
expect(result).not.toContain("awf-squid");
expect(result).not.toContain("Command completed with exit code");
expect(result).not.toContain("[ERROR]");
expect(result).not.toContain("Process exiting with code");
});

Expand Down
Loading