Treat Copilot CLI's own retry-exhaustion message as non-retryable in harness classification - #53569
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…e false positives Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has 0 new lines of code in business logic directories.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Reviewed diff for over-engineering: the regex extension in detect_agent_errors.cjs is a minimal single-pattern addition with clear justification, doc-comment update is proportional, and added tests (429/503/negative cases) are reasonable coverage, not bloat. The two lock.yml changes are generated files, out of scope. No unnecessary abstractions, dead flexibility, or reinventable stdlib usage found.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
No actionable blocking issues found in the changed lines.
### Review notes
The regex expansion is narrowly scoped to the Copilot CLI retry-exhaustion message, the negative case protects against overmatching, and the harness test closes the regression that caused the retry-loop amplification. The generated workflow lockfile changes are consistent with the strict-mode policy enforcement added by recompilation.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 3.33 AIC · ⌖ 6.74 AIC · ⊞ 4.5K
Comment /review to run again
There was a problem hiding this comment.
The change is clean and well-tested. The new regex arm is correctly bounded ([^ ]{0,300}?), eliminating ReDoS risk, and the lazy quantifier finds Last error: as early as possible. Tests cover 429, 5xx, the multi-line real-world output case, and a negative case for messages without the retry-exhaustion prefix. Lock file changes look correct. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.6 AIC · ⌖ 8.76 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor suggestion on test coverage boundary.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause correctly identified:
CAPI_QUOTA_EXCEEDED_PATTERNwas too narrow, missing the no-prefix CLI form. - ✅ Pattern change is minimal and surgical — one regex alternative, no new failure class needed.
- ✅ Good regression tests added for both
429and503, plus a negative case for the non-prefixed form. - ✅
shouldRetryFailedExecutionintegration test directly reproduces the issue scenario. - ✅ Module-level doc comment updated to document the new matched form.
Minor Gap
- The
(?:429|5\d{2})status code boundary lacks a test for a non-quota4xx(e.g.400), leaving it unverified that client errors won't accidentally match. See inline comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 26.2 AIC · ⌖ 9.95 AIC · ⊞ 7.8K
Comment /matt to run again
| }); | ||
|
|
||
| it("does not match a 'Failed to get response' message without retry-exhaustion context", () => { | ||
| expect(isCAPIQuotaExceededError("Failed to get response from the AI model due to a network error")).toBe(false); |
There was a problem hiding this comment.
[/tdd] Missing negative test for non-429, non-5xx status codes — the boundary (?:429|5\d{2}) is not regression-tested, so a future pattern edit could silently start treating client errors like 400 as non-retryable.
💡 Suggested test
it("does not match a retry-exhaustion message with a non-quota 4xx status (e.g. 400)", () => {
expect(
isCAPIQuotaExceededError(
"Failed to get response from the AI model; retried 5 times Last error: 400 Bad Request"
)
).toBe(false);
});This pins the intended semantics: only 429 and 5xx terminal statuses are treated as non-retryable quota errors. A 400 (malformed request) is a different failure class and should flow through normal retry logic.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Extends Copilot failure detection to stop harness retries after CLI-level retry exhaustion.
Changes:
- Detects retry-exhausted 429 and 5xx responses.
- Adds unit coverage for detection and retry prevention.
- Regenerates two unrelated workflow lock files.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/detect_agent_errors.cjs |
Expands terminal-error detection. |
actions/setup/js/detect_agent_errors.test.cjs |
Tests new message patterns. |
actions/setup/js/copilot_harness.test.cjs |
Tests non-retry behavior. |
.github/workflows/mcp-inspector.lock.yml |
Changes generated strict-mode behavior. |
.github/workflows/daily-team-evolution-insights.lock.yml |
Changes generated strict-mode behavior. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
| const CAPI_QUOTA_EXCEEDED_PATTERN = | ||
| /CAPIError:\s*(?:429\s+)?(?:429\s+quota exceeded|Too Many Requests)|Failed to get response from the AI model;\s*retried\s+\d+\s+times[^\n]{0,300}?Last error:\s*(?:429|5\d{2})\b/i; |
| GH_AW_INFO_CACHE_MEMORY: "true" | ||
| GH_AW_INFO_FRONTMATTER_EMOJI: "🔍" | ||
| GH_AW_COMPILED_STRICT: "true" | ||
| GH_AW_COMPILED_STRICT: "false" |
| GH_AW_INFO_AGENT_RUNTIME: "" | ||
| GH_AW_INFO_FRONTMATTER_EMOJI: "📊" | ||
| GH_AW_COMPILED_STRICT: "true" | ||
| GH_AW_COMPILED_STRICT: "false" |
|
🎉 This pull request is included in a new release. Release: |
When the Copilot CLI exhausts its internal retries against a rate-limited or unavailable model, it exits 1 with
Failed to get response from the AI model; retried N times ... Last error: 429/5xx— a message with noCAPIError:prefix.copilot_harness.cjsmisclassified this aspartial_executionand retried with--continue, even though the attempt made zero progress (tokenCount=0,Changes +0 -0), amplifying a ~6.5-minute failure into a 20-minute timeout.Root cause
isCAPIQuotaExceededError()only matched theCAPIError:-prefixed form of quota/rate-limit errors, so this variant fell through the non-retryable check and was retried against the same persistent condition.Fix
actions/setup/js/detect_agent_errors.cjs: extendedCAPI_QUOTA_EXCEEDED_PATTERNwith an alternative matching the CLI's own retry-exhaustion message, anchored to a single line and covering both429and general5xxterminal statuses:Since
shouldRetryFailedExecution()andclassifyCopilotFailure()already treatisCAPIQuotaExceededError()results as non-retryable (capi_quota_exceeded), this single pattern change is sufficient to stop the retry-loop amplification for both status-code families without introducing a new failure class.capi_quota_exceeded_errorto document the new matched form.detect_agent_errors.test.cjsandcopilot_harness.test.cjscovering the 429 and 503 message shapes, plus a negative case, and assertingshouldRetryFailedExecutionreturnsfalsefor the reproduction message from the issue.