Report terminal Claude failures after recovered continue retries - #52057
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52057 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
There was a problem hiding this comment.
Pull request overview
Filters recovered Claude --continue marker failures so reports surface the terminal engine error.
Changes:
- Detects and suppresses recovered no-deferred-marker errors.
- Adds regression coverage for recovered and standalone marker failures.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/handle_agent_failure.cjs |
Filters recovered retry errors from failure context. |
actions/setup/js/handle_agent_failure.test.cjs |
Tests recovered and unrecovered marker handling. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| const recoveredNoDeferredMarker = /No deferred tool marker found/i.test(logContent) && /no deferred tool marker on --continue.*retrying as fresh run/i.test(logContent) && /--continue disabled permanently/i.test(logContent); | ||
| const isRecoveredNoDeferredMarkerLine = line => recoveredNoDeferredMarker && /No deferred tool marker found/i.test(line); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with two minor suggestions; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Whole-log boolean assumption (
recoveredNoDeferredMarker): the suppression flag is derived from the entire log, not per-occurrence. This is likely fine in practice but is undocumented — a comment or a mixed-scenario test would make the intent explicit and prevent future confusion. - Fragile test assertion: asserting on the
"Error details:"heading string is an implementation detail; asserting on the marker message itself is both sufficient and more resilient to future heading renames.
Positive Highlights
- ✅ Clean, minimal diff — the fix is surgical and well-scoped.
- ✅ Both branches (recovered and unrecovered) are covered by dedicated test cases.
- ✅ The regex triple-guard (
No deferred tool marker found+ harness recovery log +--continue disabled permanently) makes the suppression condition narrow and precise. - ✅
isRecoveredNoDeferredMarkerLineis applied consistently in both theerrorMessagesloop and theagentLinesfilter.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 27.4 AIC · ⌖ 6.7 AIC · ⊞ 7.1K
Comment /matt to run again
| @@ -2767,6 +2767,8 @@ function buildEngineFailureContext(options = {}) { | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[/diagnosing-bugs] recoveredNoDeferredMarker is a whole-log boolean — if a future log contains both a recovered marker and a second, separate unrecovered No deferred tool marker error, every marker line will be silently suppressed, hiding a real failure.
💡 Suggested improvement
If the whole-log behaviour is intentional (one recovery means all markers are suppressed), add a short comment to document that assumption:
// Once the harness logs a successful recovery (--continue disabled permanently),
// ALL "No deferred tool marker" lines are treated as noise from that recovery chain.
const recoveredNoDeferredMarker = ...;Otherwise consider a mixed-scenario test:
it("still surfaces an unrecovered marker that appears independently of a recovered one", () => { ... });@copilot please address this.
| expect(result).toContain("Engine Failure"); | ||
| expect(result).toContain("No deferred tool marker found"); | ||
| expect(result).toContain("Error details:"); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The recovered-marker test asserts not.toContain("Error details:") but the exact heading string is an implementation detail. If the section heading ever changes wording, this test passes falsely.
💡 Suggestion
Assert on the absence of the marker error message itself rather than the section heading, since that's the actual behaviour being protected:
expect(result).not.toContain("No deferred tool marker");
// already present — the Error details check is redundant and fragileDropping the Error details: assertion removes brittleness without weakening the coverage.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (2 tests)
|
There was a problem hiding this comment.
The change is well-structured and correct. recoveredNoDeferredMarker is computed once from the full log requiring all three signals — appropriately conservative. isRecoveredNoDeferredMarkerLine re-checks at the individual line level so only the specific noisy error line is dropped. Regex patterns use .test() not /g with .exec() so no infinite-loop risk per the error-pattern-safety guidelines. Both the suppressed (recovered) and non-suppressed (unrecovered) paths are covered by new tests. LGTM
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.5 AIC · ⌖ 6.14 AIC · ⊞ 5.4K
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the pr-finisher pass. Reviews: addressed the outstanding reviewer thread — the whole-log recovered flag is replaced with per-line correlation. Each Checks (local): Not merging, per skill rules. |
Daily AW Cross-Repo Compile Check surfaced a recovered Claude
--continueretry error as the workflow failure. The harness had already fallen back to fresh retries, so the issue body hid the actual terminal failure.Failure classification
No deferred tool marker foundlines only when the harness logs that it recovered by retrying fresh with--continuepermanently disabled.Regression coverage
Error details.Now the recovered marker is filtered and the terminal failure remains visible.