Harden Claude startup failure handling for empty-log Avenger runs#44332
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Status: DRAFT — needs undraft + CI before review. Improves Claude/Avenger startup observability and adds bounded recovery for empty-log failures. Complements #44354 — both target the same failure mode from different layers. Small diff (185+/6-) across 5 files. Run §28966928999
|
🤖 PR Triage
Score breakdown: Impact 32 + Urgency 20 + Quality 8 Rationale: DRAFT. Hardens Claude startup failure handling for empty-log Avenger runs — prevents silent failures from propagating. Medium risk; behaviour-protective. Needs undraft + CI. Run §28986426320
|
There was a problem hiding this comment.
Pull request overview
Improves observability and classification for Claude-engine “empty structured log” startup failures in gh-aw’s setup action code, and adds a bounded “fresh run” retry for no-output startup exits to reduce flakiness.
Changes:
- Added Claude-specific startup diagnostics extraction and failure classification when
logEntriesis empty, including step-summary diagnostics and ERR_API vs ERR_CONFIG selection. - Added bounded startup retry handling for no-output exits in
claude_harness.cjs, configurable viaGH_AW_CLAUDE_STARTUP_RETRIES(clamped to0..2). - Expanded JS tests to cover the new messages, transient classification/output flags, and startup retry behavior/clamping.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/parse_claude_log.test.cjs | Updates expectations for Claude empty-structured-log failure messaging. |
| actions/setup/js/log_parser_bootstrap.test.cjs | Adds coverage for empty-log diagnostics classification and output flags (e.g., rate-limit signatures). |
| actions/setup/js/log_parser_bootstrap.cjs | Implements Claude startup diagnostics extraction, step-summary emission, output flagging, and error-code classification when no structured logs are parsed. |
| actions/setup/js/claude_harness.test.cjs | Adds tests for default startup retry behavior and env var clamping. |
| actions/setup/js/claude_harness.cjs | Implements bounded fresh-run startup retries for no-output startup failures and adds retry-limit parsing/clamping. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Low
| const startupLines = lines.filter(line => line.includes("[claude-harness]") || STARTUP_DIAGNOSTIC_LINE_PATTERN.test(line)); | ||
| const diagnosticLines = startupLines.length > 0 ? startupLines : lines; |
| const summaryLine = `Claude startup failed before structured logging (exitCode=${exitCode}).`; | ||
| const summaryMarkdown = tailText ? `<details><summary>Claude startup diagnostics</summary>\n\n\`\`\`text\n${tailText}\n\`\`\`\n</details>` : ""; |
| const errorCode = diagnostics.inferenceAccessError || diagnostics.transientInferenceAvailabilityError ? ERR_API : ERR_CONFIG; | ||
| const failureKind = diagnostics.inferenceAccessError || diagnostics.transientInferenceAvailabilityError ? "transient inference availability signal detected" : "startup/configuration failure detected"; |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44332 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (3 tests)
Modified tests (string-literal updates only — not newly scored):
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Harden Claude startup failure handling
Overall the change is well-structured and the startup-retry mechanism is correctly bounded. Two issues need addressing before merge.
Blocking
failureKindis semantically wrong forinferenceAccessError(line 426,log_parser_bootstrap.cjs): When the matched pattern isAccess denied by policy settings|invalid access to inference, the error message says "transient inference availability signal detected" — but a policy denial is not transient. Operators and downstream tooling may incorrectly expect a retry to resolve it. See the inline comment for a 3-way fix.
Non-blocking (but worth fixing)
- Raw log content injected into markdown code fence (line 62,
log_parser_bootstrap.cjs): If a log line contains triple-backticks the step-summary code block breaks, leaking remaining content as rendered markdown. Escaping or replacing triple-backtick sequences intailTextbefore embedding would prevent this.
Everything else (retry loop logic, clamping, ERR_API vs ERR_CONFIG classification, output flag routing, test coverage) looks correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 149.6 AIC · ⌖ 6.37 AIC · ⊞ 4.8K
| } | ||
|
|
||
| const errorCode = diagnostics.inferenceAccessError || diagnostics.transientInferenceAvailabilityError ? ERR_API : ERR_CONFIG; | ||
| const failureKind = diagnostics.inferenceAccessError || diagnostics.transientInferenceAvailabilityError ? "transient inference availability signal detected" : "startup/configuration failure detected"; |
There was a problem hiding this comment.
Misleading failureKind for inferenceAccessError — policy denial is not transient
When inferenceAccessError is true the combined condition fires and sets:
failureKind = "transient inference availability signal detected";But INFERENCE_ACCESS_ERROR_PATTERN matches Access denied by policy settings|invalid access to inference — these are non-transient policy denials. Labelling them "transient" misleads operators (and any downstream automation parsing the error message) into expecting a retry will self-resolve the problem.
Suggested fix — split the two cases:
const failureKind = diagnostics.inferenceAccessError
? "inference access denied by policy"
: diagnostics.transientInferenceAvailabilityError
? "transient inference availability signal detected"
: "startup/configuration failure detected";The errorCode = ERR_API for both is fine; only the human-readable failureKind string needs to be differentiated.
@copilot please address this.
| const aiCreditsRateLimitError = CLAUDE_RATE_LIMIT_PATTERN.test(content) || CLAUDE_OVERLOAD_PATTERN.test(content); | ||
| const transientInferenceAvailabilityError = aiCreditsRateLimitError || CLAUDE_HTTP_5XX_STATUS_PATTERN.test(content); | ||
| const summaryLine = `Claude startup failed before structured logging (exitCode=${exitCode}).`; | ||
| const summaryMarkdown = tailText ? `<details><summary>Claude startup diagnostics</summary>\n\n\`\`\`text\n${tailText}\n\`\`\`\n</details>` : ""; |
There was a problem hiding this comment.
Embedded triple-backtick fences in raw log content can break the summaryMarkdown code block
tailText is raw log content inserted directly into a fenced code block:
const summaryMarkdown = tailText
? `<details>...\n\`\`\`text\n${tailText}\n\`\`\`\n</details>`
: "";If any log line itself contains ```` the fence closes prematurely, leaking the rest of the log content as rendered markdown in the GitHub Actions step summary. While this doesn't affect control flow, it can expose unescaped content in the step summary view.
Consider stripping or escaping triple-backtick sequences from tailText before embedding:
const safeTailText = tailText.replace(/```/g, "'''");\n// then use safeTailText in the template@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on three correctness issues before merge.
📋 Key Themes & Findings
Correctness Risks
- HTML injection in step summary (
log_parser_bootstrap.cjsline 62): raw harness log content is embedded unescaped into the<details>block. A log line containing</pre>or</details>would break the summary page structure. This is the highest-priority fix. inferenceAccessError→ai_credits_rate_limit_errorasymmetry (line 60): when policy-denial fires,ERR_APIis emitted butai_credits_rate_limit_erroroutput is not set, potentially confusinghandle_agent_failure.cjsdownstream consumers.- Exit code extraction priority (line 38): the
done: exitCode=prefix is checked first, falling back tofailed: exitCode=. Because harness retry logs emitfailed: exitCode=lines and only the final invocation emitsdone: exitCode=, multi-attempt startup failures will resolve the exit code correctly — but the code flow is fragile and lacks a test for thefailed:path.
Minor Issues
- Startup retry and non-retryable signals (
claude_harness.cjsline 554): silent auth failures can consume the startup retry budget — worth guarding or documenting. - Backoff delay not reset on startup retry (line 560): low risk now but worth an explicit reset for future safety.
- Overly broad
CLAUDE_HTTP_5XX_STATUS_PATTERN(line 10): can match non-HTTP error strings; consider tightening or adding a boundary test.
Test Coverage Gaps
- No integration test for
GH_AW_CLAUDE_STARTUP_RETRIES=0(disabling retries entirely). - No test asserting
inference_access_erroroutput is set on policy-denial empty-log startup.
Positive Highlights
- ✅
resolveStartupRetryLimitis clean: regex validation,Number.parseInt, and clamping are all present and well-tested. - ✅ The non-fatal safe-outputs warning path is correctly left unchanged.
- ✅ The
buildClaudeStartupDiagnosticsJSDoc return-type annotation is precise and helpful. - ✅ Bounded retry budget with an env-var override is the right design for a CI harness.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 119.6 AIC · ⌖ 6.61 AIC · ⊞ 6.6K
Comment /matt to run again
| const INFERENCE_ACCESS_ERROR_PATTERN = /Access denied by policy settings|invalid access to inference/i; | ||
| const CLAUDE_RATE_LIMIT_PATTERN = /rate_limit_error|429 Too Many Requests|"api_error_status"\s*:\s*429|request rejected \(429\)|rate limit/i; | ||
| const CLAUDE_OVERLOAD_PATTERN = /overloaded_error|"overloaded"/i; | ||
| const CLAUDE_HTTP_5XX_STATUS_PATTERN = /(?:HTTP(?:\/\d\.\d)?\s+5\d{2}\b|status(?:\s+code)?\s*[:=]\s*5\d{2}\b|error(?:\s+code)?\s*[:=]?\s*5\d{2}\b)/i; |
There was a problem hiding this comment.
[/diagnosing-bugs] CLAUDE_HTTP_5XX_STATUS_PATTERN can match plain text like error code: 500 in non-HTTP messages — this risks false-positive transient classification.
💡 Suggested tightening
The current alternation error(?:\s+code)?\s*[:=]?\s*5\d{2}\b matches things like "Error: exit status 500" or any string saying error code: 501. Consider requiring an HTTP-context word before the status, e.g.:
const CLAUDE_HTTP_5XX_STATUS_PATTERN =
/(?:HTTP(?:\/\d\.\d)?\s+5\d{2}\b|status(?:\s+code)?\s*[:=]\s*5\d{2}\b|(?:http|fetch|request)\s+(?:failed|error)[^\n]*\s5\d{2}\b)/i;Or add a targeted regression test for a non-HTTP error: 500 string to document the accepted false-positive boundary.
@copilot please address this.
|
|
||
| const inferenceAccessError = INFERENCE_ACCESS_ERROR_PATTERN.test(content); | ||
| const aiCreditsRateLimitError = CLAUDE_RATE_LIMIT_PATTERN.test(content) || CLAUDE_OVERLOAD_PATTERN.test(content); | ||
| const transientInferenceAvailabilityError = aiCreditsRateLimitError || CLAUDE_HTTP_5XX_STATUS_PATTERN.test(content); |
There was a problem hiding this comment.
[/diagnosing-bugs] inferenceAccessError is not included in aiCreditsRateLimitError but IS included in transientInferenceAvailabilityError — the asymmetry is subtle and may confuse future readers.
💡 Why this matters
When inferenceAccessError is true, ai_credits_rate_limit_error is NOT set (line 59 only covers rate-limit/overload patterns). But errorCode switches to ERR_API via the transientInferenceAvailabilityError branch. That means ERR_API is emitted without the ai_credits_rate_limit_error output being set, which could confuse downstream consumers (e.g. handle_agent_failure.cjs) that route on that flag.
If inference access denial is deliberately kept separate, add a comment explaining the intentional asymmetry. Otherwise, consider also setting ai_credits_rate_limit_error (or a new dedicated output) when inferenceAccessError is true.
@copilot please address this.
| const tailText = tailLines.join("\n"); | ||
|
|
||
| let exitCode = "unknown"; | ||
| const donePrefix = "done: exitCode="; |
There was a problem hiding this comment.
[/diagnosing-bugs] Exit code extraction searches for done: exitCode= first, then falls back to failed: exitCode=, but the harness only emits done: exitCode= at the very end of successful or non-retried exits. If the raw content contains only failed: exitCode= lines from retry attempts, lastIndexOf(donePrefix) will miss them and exitCode stays "unknown".
💡 Suggested fix
Reverse the priority — scan for the last failed: exitCode= or done: exitCode= in a single pass, whichever appears later in the string:
const prefixes = ["done: exitCode=", "failed: exitCode="];
for (const prefix of prefixes) {
const idx = content.lastIndexOf(prefix);
if (idx !== -1) {
const code = content.slice(idx + prefix.length).match(/^(\d+)/);
if (code && (exitCode === "unknown" || idx > bestIdx)) {
exitCode = code[1];
bestIdx = idx;
}
}
}Alternatively, add a test that verifies the exit code is extracted correctly when only failed: exitCode=N lines are present.
@copilot please address this.
| log(`all ${maxRetries} retries exhausted — giving up (exitCode=${lastExitCode})`); | ||
| } else { | ||
| log(`attempt ${attempt + 1}: no output produced — not retrying` + ` (possible causes: binary not found, permission denied, auth failure, or silent startup crash)`); | ||
| if (startupRetriesUsed < startupRetryLimit) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The startup retry fires for ALL no-output exits — including authentication failures (isAuthenticationFailed) and invalid model errors (isInvalidModel), which are non-retryable by design and checked explicitly above. Because the result.hasOutput guard on line 525 exits early for those cases with break, they only reach the startup retry block when !result.hasOutput is also true.
💡 Risk and suggestion
If an auth failure produces no stdout/stderr at all (silent binary crash), it will be retried once without benefit. The extra retry wastes time and, worse, the retry log says "no output produced — retrying startup as fresh run" which could mislead operators into thinking a transient condition occurred.
Consider threading the isAuthenticationFailed and isInvalidModel signals down into the no-output branch and short-circuiting the startup retry for them:
if (!result.hasOutput && (isAuthenticationFailed || isInvalidModel)) {
log(`attempt ${attempt + 1}: no output + non-retryable signal — not retrying`);
break;
}Alternatively, add a test for the silent-auth-failure path to document the accepted behavior.
@copilot please address this.
| // attempt is 0-based; logs are 1-based, so "next attempt" is +2. | ||
| const nextAttemptNumber = attempt + 2; | ||
| const totalAttempts = maxRetries + 1; | ||
| log(`attempt ${attempt + 1}: no output produced — retrying startup as fresh run ` + `(startup retry ${startupRetriesUsed}/${startupRetryLimit}, next attempt ${nextAttemptNumber} of ${totalAttempts} total attempts)`); |
There was a problem hiding this comment.
[/diagnosing-bugs] When a startup retry succeeds (exits 0), the delay variable has already been advanced by the backoff multiplier on the previous iteration. If a future code path reaches the backoff sleep block after a startup retry, it will start from an inflated delay rather than a fresh one.
💡 Suggestion
Reset delay to initialDelayMs alongside useContinueOnRetry = false when initiating a startup retry:
if (startupRetriesUsed < startupRetryLimit) {
startupRetriesUsed++;
useContinueOnRetry = false;
delay = initialDelayMs; // reset backoff for startup retry
...
}This is low risk now since startupRetryLimit defaults to 1, but worth making explicit for correctness and future readers.
@copilot please address this.
| process.exit(0); | ||
| `; | ||
| const { result, calls } = runHarnessWithStub({ stubScript }); | ||
| expect(result.status, result.stderr).toBe(0); |
There was a problem hiding this comment.
[/tdd] The integration test verifies the success path but has no coverage for: (a) startup retry budget exhausted after startupRetryLimit failures, and (b) GH_AW_CLAUDE_STARTUP_RETRIES=0 disabling the retry entirely.
💡 Suggested additional tests
it("does not retry startup failure when GH_AW_CLAUDE_STARTUP_RETRIES=0", () => {
const stubScript = `
const fs = require("fs");
fs.appendFileSync(process.env.CLAUDE_HARNESS_STUB_CALLS, JSON.stringify({ args: process.argv.slice(2) }) + "\\n");
process.exit(1);
`;
const { result, calls } = runHarnessWithStub({ stubScript,
extraEnv: { GH_AW_CLAUDE_STARTUP_RETRIES: "0" } });
expect(result.status).not.toBe(0);
expect(calls.length).toBe(1); // no retry
expect(result.stderr).toContain("startup retry budget exhausted");
});Without this, a regression that ignores the 0 value would go undetected.
@copilot please address this.
| fs.rmdirSync(tmpDir); | ||
| } | ||
| }), | ||
| it("should classify Claude empty-log startup rate-limit signatures as transient inference availability", async () => { |
There was a problem hiding this comment.
[/tdd] The rate-limit test only asserts ai_credits_rate_limit_error output, but not inference_access_error. There is no test asserting that inference_access_error=true is emitted when INFERENCE_ACCESS_ERROR_PATTERN matches (e.g. "Access denied by policy settings").
💡 Suggested additional test
it("should classify Claude empty-log startup inference-access denial", async () => {
fs.writeFileSync(logFile, `[claude-harness] attempt 1 failed: exitCode=1 hasOutput=false\nAccess denied by policy settings\n[claude-harness] done: exitCode=1`);
// ...
expect(mockCore.setOutput).toHaveBeenCalledWith("inference_access_error", "true");
expect(mockCore.setFailed).toHaveBeenCalledWith(
expect.stringContaining("ERR_API")
);
});Without this, a regression in INFERENCE_ACCESS_ERROR_PATTERN would not be caught by tests.
@copilot please address this.
| const aiCreditsRateLimitError = CLAUDE_RATE_LIMIT_PATTERN.test(content) || CLAUDE_OVERLOAD_PATTERN.test(content); | ||
| const transientInferenceAvailabilityError = aiCreditsRateLimitError || CLAUDE_HTTP_5XX_STATUS_PATTERN.test(content); | ||
| const summaryLine = `Claude startup failed before structured logging (exitCode=${exitCode}).`; | ||
| const summaryMarkdown = tailText ? `<details><summary>Claude startup diagnostics</summary>\n\n\`\`\`text\n${tailText}\n\`\`\`\n</details>` : ""; |
There was a problem hiding this comment.
[/diagnosing-bugs] summaryMarkdown embeds raw harness log lines directly into a GitHub step summary <pre> block without any sanitisation. If a log line contains </pre> or </details>, it can break the HTML structure in the summary page.
💡 Suggested fix
Escape <, >, and & in tailText before embedding:
function escapeHtml(str) {
return str.replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">");
}
const summaryMarkdown = tailText
? `<details><summary>Claude startup diagnostics</summary>\n\n\`\`\`text\n${escapeHtml(tailText)}\n\`\`\`\n</details>`
: "";This is especially important because the log content comes from an untrusted external process.
@copilot please address this.
| const { generatePlainTextSummary, generateCopilotCliStyleSummary, wrapAgentLogInSection, formatSafeOutputsPreview } = require("./log_parser_shared.cjs"); | ||
| const { getErrorMessage } = require("./error_helpers.cjs"); | ||
| const { ERR_API, ERR_CONFIG, ERR_VALIDATION } = require("./error_codes.cjs"); | ||
| const INFERENCE_ACCESS_ERROR_PATTERN = /Access denied by policy settings|invalid access to inference/i; |
There was a problem hiding this comment.
[/codebase-design] These six module-level regex constants are defined at the file top but are only used inside buildClaudeStartupDiagnostics. Moving them inside the function would make their locality explicit and reduce namespace pollution.
💡 Tradeoff
Module-scope is fine if you expect these patterns to be reused by future functions in this file (e.g. a live-tail classifier). If that is the intent, add a brief comment explaining their intended reuse. Otherwise, moving them inside buildClaudeStartupDiagnostics signals they are implementation details, which matches the deep-module principle: simple public interface, rich private behaviour.
@copilot please address this.
|
@copilot run pr-limiter skill |
I tried to invoke |
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran |
Claude-engine Avenger runs were failing at startup with zero structured log entries, collapsing distinct startup conditions into a generic
ERR_CONFIGand providing little actionable context. This change improves startup observability, adds bounded recovery for transient empty-start exits, and aligns empty-log classification with existing inference/rate-limit outputs.Startup failure diagnostics (empty structured logs)
log_parser_bootstrap.cjswhenlogEntriesis empty.Failure classification alignment
inference_access_error=trueai_credits_rate_limit_error=trueERR_APIfor transient inference availability signatures; retainsERR_CONFIGfor true startup/config cases.Bounded startup retry for zero-output exits
claude_harness.cjsfor no-output startup failures (non---continuepath).1; configurable viaGH_AW_CLAUDE_STARTUP_RETRIES(clamped to0..2).Targeted parser/harness updates