Fix false positive AI credits rate limit and agentic engine timeout detections - #49750
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes false-positive AI credit rate-limit and agent timeout reporting from MCP payloads and watchdog termination logs.
Changes:
- Excludes raw MCP JSONL payloads from rate-limit detection.
- Distinguishes watchdog termination from genuine engine timeouts.
- Adds regression coverage for both scenarios.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/parse_mcp_gateway_log.cjs |
Restricts rate-limit scanning sources. |
actions/setup/js/parse_mcp_gateway_log.test.cjs |
Tests MCP payload false positives. |
actions/setup/js/detect_agent_errors.cjs |
Adds watchdog-aware timeout detection. |
actions/setup/js/detect_agent_errors.test.cjs |
Tests watchdog, timeout, and mixed cases. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| // Do NOT scan gateway.jsonl / rpc-messages.jsonl for AI credits rate limit errors. | ||
| // These files contain full MCP tool call request/response payloads including arbitrary | ||
| // repository data (branch names, commit messages, file contents) that can false-positively | ||
| // match the rate-limit patterns. Real AI credits rate limit errors from the inference API | ||
| // appear in gateway.log / stderr.log / gateway.md, not in MCP RPC message logs. |
PR Triage
Automated triage — see full report issue for details. Structured data: {
"action": "batch_review",
"category": "bug",
"pr_number": 49750,
"risk": "medium"
}
|
|
✅ 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 (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Fix is correct and well-tested. isAgenticEngineTimeout correctly distinguishes post-result watchdog SIGTERMs from genuine step-timeout kills. Removing hasAICreditsRateLimitError scans from JSONL files eliminates false positives from arbitrary repository content. Test coverage is comprehensive. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 29 AIC · ⌖ 7.38 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (50 sampled tests)
i️ Inflation Note (advisory only)Both test files exceed the 2:1 ratio of test lines added vs. production lines added:
This is expected and appropriate for a false-positive fix PR. The production change is a targeted regex refinement ( Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two issues.
📋 Key Themes & Highlights
Key Themes
- Incomplete false-positive fix:
hasUnknownModelAICreditsErrorstill scansgateway.jsonlandrpc-messages.jsonl, the same false-positive vector just fixed forhasAICreditsRateLimitError. - Ambiguous SIGTERM fallback:
isAgenticEngineTimeoutreturnstruefor any SIGTERM not on aprocess closedline, which may be too broad. - Missing
detectErrorsintegration test for the mixed watchdog + step-timeout case.
Positive Highlights
- ✅ Excellent root-cause analysis and clear PR description
- ✅ Good regression tests for both fixes, including the exact failing run scenario
- ✅ Clean separation of
isAgenticEngineTimeoutwith well-documented JSDoc - ✅ Exporting the new patterns and function enables direct unit testing
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 49.4 AIC · ⌖ 8.24 AIC · ⊞ 7.1K
Comment /matt to run again
| // repository data (branch names, commit messages, file contents) that can false-positively | ||
| // match the rate-limit patterns. Real AI credits rate limit errors from the inference API | ||
| // appear in gateway.log / stderr.log / gateway.md, not in MCP RPC message logs. | ||
| unknownModelAICredits ||= hasUnknownModelAICreditsError([jsonlContent]); |
| // repository data (branch names, commit messages, file contents) that can false-positively | ||
| // match the rate-limit patterns. Real AI credits rate limit errors from the inference API | ||
| // appear in gateway.log / stderr.log / gateway.md, not in MCP RPC message logs. | ||
| unknownModelAICredits ||= hasUnknownModelAICreditsError([jsonlContent]); |
There was a problem hiding this comment.
[/diagnosing-bugs] hasUnknownModelAICreditsError still scans gateway.jsonl and rpc-messages.jsonl — the same false-positive vector that motivated removing the hasAICreditsRateLimitError calls. Branch names or commit messages containing unknown_model_ai_credits could trigger this.
💡 Suggested fix
Remove lines 850 and 870 (the JSONL scans) for hasUnknownModelAICreditsError, mirroring what was done for hasAICreditsRateLimitError. Real unknown_model_ai_credits errors originate from the inference API and appear in gateway.log, stderr.log, or gateway.md.
@copilot please address this.
| * @returns {boolean} | ||
| */ | ||
| function isAgenticEngineTimeout(logContent) { | ||
| // Always detect SDK idle-timeout (distinct from the step timeout). |
There was a problem hiding this comment.
[/diagnosing-bugs] The AGENTIC_ENGINE_TIMEOUT_PATTERN regex is stateful (lastIndex) but used with both .test() and the new isAgenticEngineTimeout function. Since it has no g flag here the lastIndex issue doesn't apply, but calling AGENTIC_ENGINE_TIMEOUT_PATTERN.test(logContent) inside isAgenticEngineTimeout then falling through to return true means a bare signal=SIGTERM in a non-process closed context (e.g. a process exit event line) will also be classified as a step timeout — even though that line alone cannot distinguish watchdog vs step.
💡 Suggested improvement
The fallthrough at line ~97 (return true when no watchdog-specific line is found) will fire on any SIGTERM that isn't on a process closed line. Consider narrowing the final fallback to only match process closed lines, or document explicitly that non-process-closed SIGTERMs are intentionally treated as step timeouts.
@copilot please address this.
| }); | ||
|
|
||
| it("detects max cache misses exceeded (JSON error type form)", () => { | ||
| const result = detectErrors('{"error":{"type":"max_cache_misses_exceeded","message":"Maximum consecutive cache misses exceeded (6 / 5).","consecutive_cache_misses":6,"max_cache_misses":5}}'); |
There was a problem hiding this comment.
[/tdd] The test for watchdogFired=false (line 554) uses a process closed line — good. But there is no test for a multiline log where a watchdog SIGTERM on attempt 1 is followed by a step-timeout SIGTERM on attempt 2 via detectErrors() (the public API). The isAgenticEngineTimeout unit tests cover this, but a detectErrors integration test for the mixed case would confirm the wiring is correct end-to-end.
💡 Suggested test
it('reports engine timeout when both watchdog and step-timeout SIGTERMs are present', () => {
const log = [
'[copilot-harness] attempt 1: process closed exitCode=1 signal=SIGTERM duration=10m watchdogFired=true',
'[copilot-harness] attempt 2: process closed exitCode=1 signal=SIGTERM duration=20m watchdogFired=false',
].join('
');
expect(detectErrors(log).agenticEngineTimeout).toBe(true);
});@copilot please address this.
|
@copilot run pr-finisher skill |
|
@copilot Please address the remaining review feedback on this PR, then run the pr-finisher skill. Outstanding signals to close:
Branch refresh was also requested for this PR. After updates, run the pr-finisher skill. Run details: https://github.com/github/gh-aw/actions/runs/30750494960
|
…SIGTERM detection Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed a PR-finisher pass in commit |
|
🎉 This pull request is included in a new release. Release: |
Workflow failure reports were simultaneously claiming "AI credits rate limit (HTTP 429)" and "Agent Timed Out" for runs where neither actually occurred — the real failure was
authentication_failedwith a post-result watchdog SIGTERM.False positive 1: AI credits rate limit (
parse_mcp_gateway_log.cjs)hasAICreditsRateLimitError()was being called onrpc-messages.jsonlandgateway.jsonl, which contain raw MCP tool call response payloads (branch listings, commit messages). Repository data like branch namesschema-coverage-rate-limit-*and commit messages mentioning "AI credits throughput rate-limit errors" matched the patterns.Fix: Remove
hasAICreditsRateLimitErrorcalls on MCP RPC message files. Real Copilot API 429s only appear ingateway.md,gateway.log, andstderr.log.False positive 2: Agentic engine timeout (
detect_agent_errors.cjs)AGENTIC_ENGINE_TIMEOUT_PATTERNmatched anysignal=SIGTERM, including the copilot-harness post-result watchdog — which fires when the agent process sits idle ~20s after completing work, not because the step timed out. The distinguishing markerwatchdogFired=trueexists on theprocess closedlog line but was never checked.Fix: Add
isAgenticEngineTimeout()with two new patterns:If all
process closedSIGTERM lines havewatchdogFired=true, the function returnsfalse(not a timeout).detectErrors()now callsisAgenticEngineTimeout()instead of the raw pattern.Regression tests added for both fixes covering watchdog-only, step-timeout, mixed, and the rpc-messages.jsonl branch-name false positive scenario.
Run: https://github.com/github/gh-aw/actions/runs/30750494960