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 is reviewing this pull request using Matt Pocock's engineering skills... |
|
✅ 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.
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