Repository navigation
Stop Copilot retries on tool-call ID schema errors - #65761
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The targeted non-transient failure is correctly handled and covered across both required retry scenarios.
Review effort: Balanced
Findings: None
What changed in this PR
Stops futile Copilot retries when malformed tool-call IDs poison session state.
Changes:
- Detects and classifies the specific
ctc_call_/fcschema mismatch. - Fails immediately without resuming or restarting.
- Adds initial and resumed-attempt regression coverage.
| File | Description |
|---|---|
actions/setup/js/copilot_harness.cjs |
Adds terminal error detection and handling. |
actions/setup/js/copilot_harness.test.cjs |
Tests detection, classification, and retry prevention. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🔍 Design Decision Gate 🏗️ is checking for design decision records on this pull request... |
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The retry guard looks right, but the new tool_call_id_schema_error classification never leaves the harness: downstream outputs still expose only the generic HTTP 400 bucket, so the separate-classification part of the fix is still incomplete.
Blocking theme
- The added branch stops retries locally, but
detectCopilotErrors, the aggregateddetectedCopilotErrorsstate, andwriteCopilotOutputsstill have notool_call_id_schema_errorfield, so workflow consumers cannot distinguish this condition from any otherhttp_400_response_error.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 47 AIC · ⌖ 7.14 AIC · ⊞ 19.4K
Comment /review to run again
| isMCPPolicy, | ||
| isModelNotSupported, | ||
| isHTTP400ResponseError: hasHTTP400ResponseError, | ||
| isToolCallIdSchemaError: hasToolCallIdSchemaError, |
There was a problem hiding this comment.
This only creates an internal failure class; the workflow outputs still collapse the same error into http_400_response_error, so downstream steps cannot actually distinguish or handle the new non-retryable case.
💡 Why this blocks
The PR description says this mismatch should be classified separately from other HTTP 400s, but the new flag never gets wired into the existing detection/output plumbing (detectCopilotErrors, detectedCopilotErrors, writeCopilotOutputs). Right now anything consuming $GITHUB_OUTPUT / needs.agent.outputs.* still only sees the generic http_400_response_error=true, so the new classification is invisible outside this local retry branch.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — this is a clean, well-targeted fix with solid regression coverage. Approving with two minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Root cause correctly addressed: the fix distinguishes the
ctc_call_...→fcID-schema mismatch from genericpartial_execution/HTTP 400 handling and stops retrying instead of resuming poisoned--continuestate — exactly matching the root cause described in #65756. - Pattern specificity vs. generality:
TOOL_CALL_ID_SCHEMA_ERROR_PATTERNhardcodes the exact observed prefixes (ctc_call_→fc). A more general "ID prefix/schema mismatch" pattern would be more robust to future variants of the same underlying bug class (see inline comment). - Cross-file pattern sync: other classification patterns (
HTTP_400_RESPONSE_ERROR_PATTERN) carry an explicit "keep in sync with detect_agent_errors.cjs" comment; the new pattern has no such note. Confirmed this gap also exists for the pre-existingnull_type_tool_callclass, so it's consistent with precedent rather than a new regression — flagged for awareness only.
Positive Highlights
- ✅ Regression tests cover both the first-attempt failure and the attempt resumed after partial execution (
it.each([false, true])), which is exactly the scenario from the linked issue reproduction. - ✅ New check is placed correctly ahead of the generic
hasHTTP400ResponseErrorbranch so it takes precedence without disturbing existing HTTP-400/null-type-tool-call handling. - ✅ Doc comment block at the top of the file is updated to describe the new non-retryable class alongside the existing null-type-tool-call precedent — good practice for a file with many special-cased failure modes.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 76.9 AIC · ⌖ 13.6 AIC · ⊞ 10K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/copilot_harness.cjs:198
[/diagnosing-bugs] This pattern hardcodes the exact ctc_call_ → fc prefix pair from the one observed error. If the backend emits the same ID-schema-mismatch shape with a different expected prefix (e.g. a future model/tool-call ID scheme), this regex silently falls through to the generic HTTP_400_RESPONSE_ERROR_PATTERN/partial_execution path and regresses to the exact retry-storm bug this PR fixes.
<details>
<summary>💡 Suggested generalization</summary>
Consider loosening the seco…
actions/setup/js/copilot_harness.cjs:130
[/codebase-design] HTTP_400_RESPONSE_ERROR_PATTERN carries a // NOTE: keep in sync with ... detect_agent_errors.cjs comment (mirrored in agent_error_patterns.cjs), signaling that these failure-classification patterns are intentionally duplicated/tracked across files for the top-level error-detection surface. The new TOOL_CALL_ID_SCHEMA_ERROR_PATTERN has no such cross-reference, so there's no explicit record of whether it should also be surfaced there for consistent top-level failure…
There was a problem hiding this comment.
Reviewed with a correctness/security focus (backend Node.js harness change, not UI — Impeccable modes don't apply).
Findings: None blocking. The new TOOL_CALL_ID_SCHEMA_ERROR_PATTERN / isToolCallIdSchemaError correctly matches the ctc_call_... → fc ID-mismatch error, is placed above the generic hasHTTP400ResponseError check in the retry decision tree (so it takes precedence and the harness now stops immediately instead of burning retries on poisoned --continue state), and classifyCopilotFailure orders it consistently with other non-retryable classes. Verified the regex and classification manually by requiring the module and exercising both functions directly (vitest isn't installable in this sandbox — no network access to fetch deps — so I validated via direct node -e calls against the exported functions instead).
Tests cover both the direct-failure and partial-execution-then-schema-error paths, matching the issue's reproduction scenario (#65756). Documentation comment at the top of the file was updated accordingly.
No actionable changes requested.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 73.6 AIC · ⌖ 14 AIC · ⊞ 8.1K
|
🎉 This pull request is included in a new release. Release: |
A
400 Invalid 'input[N].id'error caused by actc_call_...ID being rejected in favor of anfcID was treated as partial execution. Retrying with--continuereused the malformed conversation state and exhausted the retry budget.