Prevent AI credits rate-limit false positives from MCP echoes - #61425
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This reverts commit 005a093. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation addresses the reported false-positive paths with focused regression coverage.
Pull request overview
Fixes #61141 by preventing echoed MCP payloads from falsely triggering AI credits rate-limit failure handling.
Changes:
- Bounds keyword proximity and excludes MCP tool-result lines.
- Requires an agent failure before acting on detected signals.
- Adds engine-neutral guidance and regression tests.
File summaries
| File | Description |
|---|---|
actions/setup/md/ai_credits_rate_limit_throttle.md |
Makes guidance engine-neutral. |
actions/setup/js/parse_mcp_gateway_log.cjs |
Hardens rate-limit detection. |
actions/setup/js/parse_mcp_gateway_log.test.cjs |
Tests echo filtering and keyword bounds. |
actions/setup/js/handle_agent_failure.cjs |
Gates signals on failed conclusions. |
actions/setup/js/handle_agent_failure.test.cjs |
Tests successful-conclusion handling. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
@copilot this PR looks close, but it has not moved since the latest review. Please do a quick branch refresh if needed, re-run the 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
- "github.com"See Network Configuration for more information.
|
Branch refresh confirmed it is 4 commits ahead and 0 behind |
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ 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
- "registry.npmjs.org"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. 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
- "ab.chatgpt.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Reviewed with a correctness/security focus (Go/JS backend change; the Impeccable UI modes don't apply here since there's no UI in this diff).
Findings: No blocking issues. The changes are small, well-scoped, and covered by new regression tests:
- Bounding
AI_CREDITS_RATE_LIMIT_PATTERNSto 80 chars and excludingtool_resultecho lines correctly prevents false positives from MCP payload echoes without narrowing genuine nearby signals. - Gating
aiCreditsRateLimitErroronagentConclusion === "failure"matches the existing pattern used by sibling flags (maxCacheMissesExceeded,missingModelPricingError,shellExpansionGuardRejected), so it's consistent with the codebase's conventions. - The engine-neutral wording change in
ai_credits_rate_limit_throttle.mdis a reasonable, low-risk copy fix. - New tests in both
.test.cjsfiles directly cover the fixed scenarios (echoed titles, distant keywords, success conclusion, nearby genuine signal).
Note: I was unable to execute vitest in this sandbox (dependency install did not complete), so test results are based on static review of the added test cases rather than a live run.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
codeload.github.comgithub.laiyagushi.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "codeload.github.com"
- "github.com"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 31 AIC · ⌖ 14.6 AIC · ⊞ 8.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix correctly hardens parse_mcp_gateway_log.cjs, but the same false-positive pattern class remains unpatched in a sibling detection path.
📋 Key Themes & Highlights
Key Themes
- Root cause only partially addressed:
hasAICreditsRateLimitErrorinparse_mcp_gateway_log.cjsnow bounds keyword distance to 80 chars and filterstool_resultechoes. Howeverai_credits_context.cjs'sAI_CREDITS_RATE_LIMIT_PATTERNS(used byresolveAICreditsFailureState(), the functionhandle_agent_failure.cjsgates onagentConclusion === "failure") still uses unbounded.*matching and has no echo filtering — the same false-positive risk can leak through the firewall audit-log path. - Test coverage gap: the new regression test only covers the
agentConclusion === "success"case; there's no companion test pinning down that the rate-limit signal is still honored whenagentConclusion === "failure".
Positive Highlights
- ✅ Clean, minimal conclusion-gating change in
handle_agent_failure.cjs - ✅ Good regression coverage for the echoed-title and distant-keyword false positives in
parse_mcp_gateway_log.test.cjs - ✅ Engine-neutral wording update in the throttle template is a nice correctness improvement independent of the bug fix
Posted 2 inline comments with specific suggestions.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 197.1 AIC · ⌖ 15.2 AIC · ⊞ 10.4K
Comment /matt to run again
| delete process.env.GH_AW_AI_CREDITS_RATE_LIMIT_ERROR; | ||
| delete process.env.GH_AW_AIC; | ||
| } | ||
| }); |
There was a problem hiding this comment.
[/tdd] Good regression test for the success case, but there's no test asserting the opposite: that aiCreditsRateLimitError is still honored when agentConclusion === "failure". Without it, a future refactor could silently invert or drop the agentConclusion === "failure" guard and this suite would still pass.
💡 Suggested test
it("still handles an AI credits rate-limit signal when the agent failed", async () => {
// ...same setup as the success test but with:
process.env.GH_AW_AGENT_CONCLUSION = "failure";
process.env.GH_AW_AI_CREDITS_RATE_LIMIT_ERROR = "true";
process.env.GH_AW_AIC = "1";
// assert a failure issue / ai_credits_rate_limit_error context IS produced
});This closes the loop on the fix and pins down both directions of the new conclusion gate as a spec, per the red-green-refactor principle.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 7f2a775: added a failed-conclusion regression that verifies the AI credits rate-limit signal remains actionable.
|
Please address the unresolved review feedback:
Also review the failing check from the candidate scan: If the branch is behind or stale, refresh it, then continue with the 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
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the GitHub Actions review in |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Rate-limit detection scanned echoed MCP payloads and could self-seed from prior failure issue titles, causing failure handling despite successful agent conclusions.
Detection hardening
tool_resultpayload echoes.Conclusion handling
failure.Failure guidance
Regression coverage
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.laiyagushi.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.