Improve PR failure issue title specificity for engine HTTP 429 rate limits - #49411
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds engine HTTP 429 detection to produce more specific failure issue titles.
Changes:
- Detects rate-limit signals in agent logs and OTLP telemetry.
- Adds a dedicated engine-rate-limit issue title.
- Extends title unit tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/handle_agent_failure.cjs |
Detects and titles engine HTTP 429 failures. |
actions/setup/js/handle_agent_failure.test.cjs |
Tests the new title classification. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| hasStaleLockFileFailed, | ||
| hasDailyAICExceeded, | ||
| aiCreditsRateLimitError, | ||
| hasEngineRateLimit429, |
There was a problem hiding this comment.
Fixed. engine_rate_limit_429 is now pushed to buildFailureMatchCategories (alongside the existing ai_credits_rate_limit_error entry) and hasEngineRateLimit429 is passed at the buildFailureMatchCategories call site in main(). A corresponding unit test confirms the new category is emitted and that agent_failure is not also added when this flag is true.
| const logContent = fs.readFileSync(stdioLogPath, "utf8"); | ||
| if (hasEngineRateLimit429Signal(logContent)) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed (this thread is now outdated — the line was modified). detectEngineRateLimit429Failure now reads the stdio log and checks for terminal_reason: "completed" before accepting either the stdio 429 signal or the OTEL fallback. If the agent completed, the function returns false immediately, preventing a transient mid-session 429 from producing a rate-limit title on an unrelated post-processing failure. Unit tests covering the completed guard were added.
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49411 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. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe 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 threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /tdd — requesting changes on guard condition completeness and missing precedence test.
📋 Key Themes & Highlights
Issues to Address
- Incomplete guard condition (line 3412):
hasEngineRateLimit429is not excluded whenaiCreditsRateLimitErroris true, so the detector runs unnecessarily and the flag can be set alongside an AI-credits signal — adding!aiCreditsRateLimitErrorto the guard closes this. - Missing precedence test (test line 111): No table case verifies that
aiCreditsRateLimitErrorbeatshasEngineRateLimit429when both are true; the title chain is load-bearing and should be regression-tested. buildFailureMatchCategoriesgap (existing comment #3694142669):hasEngineRateLimit429is not plumbed into the deduplication categories, so a 429 still falls back toagent_failure.- Transient 429 guard (existing comment #3694142686): The detector doesn't respect the
terminal_reason: completedguard, so a job that logs a transient 429 but ultimately completes can incorrectly trigger the failure title.
Positive Highlights
- ✅ Clean signal-plumbing pattern — consistent with how other flags are threaded through to
buildFailureIssueTitle - ✅ Dual-source detection (stdio log + OTLP mirror) with proper fallback
- ✅ Table-driven test structure makes it easy to add more cases
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.9 AIC · ⊞ 7K
Comment /matt to run again
| @@ -109,6 +110,7 @@ describe("handle_agent_failure", () => { | |||
| { flag: "hasDailyAICExceeded", expected: "[aw] Test Workflow exceeded daily AI credits budget" }, | |||
| { flag: "maxAICreditsExceeded", expected: "[aw] Test Workflow exceeded max AI credits" }, | |||
There was a problem hiding this comment.
[/tdd] Precedence ordering is not tested — add a case where both hasEngineRateLimit429 and aiCreditsRateLimitError are true to confirm the AI-credits title wins.
💡 Suggested test case
{
flags: { aiCreditsRateLimitError: true, hasEngineRateLimit429: true },
expected: "[aw] Test Workflow hit AI credits rate limit",
label: "aiCreditsRateLimitError takes precedence over hasEngineRateLimit429"
}The precedence chain in buildFailureIssueTitle is load-bearing; a regression here would silently mis-classify engine rate-limit failures as an AI-credits problem (or vice-versa).
@copilot please address this.
There was a problem hiding this comment.
Added. A new test case "aiCreditsRateLimitError takes precedence over hasEngineRateLimit429 when both are true" confirms the AI-credits title wins when both flags are set, covering the load-bearing precedence ordering in buildFailureIssueTitle.
| @@ -3389,6 +3412,7 @@ | |||
| if (hasToolDenialsExceeded) { | |||
There was a problem hiding this comment.
[/tdd] The guard !maxAICreditsExceeded is the only exclusion — !aiCreditsRateLimitError is not excluded. This means when aiCreditsRateLimitError is true and maxAICreditsExceeded is false, hasEngineRateLimit429 can still be true, causing the detector to run unnecessarily (and potentially log a false positive in buildFailureMatchCategories).
💡 Suggested fix
const hasEngineRateLimit429 = agentConclusion === "failure"
&& !maxAICreditsExceeded
&& !aiCreditsRateLimitError
&& detectEngineRateLimit429Failure();This mirrors the intent of the title precedence chain and avoids running the detector when an AI-credits signal already classifies the failure.
@copilot please address this.
There was a problem hiding this comment.
Fixed. !aiCreditsRateLimitError is now added to the hasEngineRateLimit429 guard condition alongside the existing !maxAICreditsExceeded check, so the detector is skipped when an AI-credits rate-limit signal already classifies the failure — mirroring the title precedence chain.
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Request changes
The title-classification addition is well-scoped and tested, but two gaps undermine its correctness/purpose: the new category is missing from the dedup key (buildFailureMatchCategories), and the 429 detector lacks the terminal_reason: completed guard that the equivalent context-builder already has, risking mislabeled issues for unrelated failures.
Themes
- Deduplication gap:
hasEngineRateLimit429only feeds the title, not the categorization/dedup key, so 429 issues will still bucket under genericagent_failurefor grouping purposes. - Inconsistent completion guard:
detectEngineRateLimit429Failuredoesn't checkterminal_reason: completedthe waybuildEngineFailureContextdoes, allowing transient/early 429s to mislabel unrelated job failures. - Test coverage is title-only; the categorization behavior remains unverified.
🔎 Code quality review by PR Code Quality Reviewer · auto · 25.9 AIC · ⊞ 7.8K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/handle_agent_failure.cjs:61
hasEngineRateLimit429 drives the failure title but is never added to buildFailureMatchCategories, so dedup/categorization silently degrades to the generic agent_failure bucket for this new failure class.
<details><summary>💡 Details and fix</summary>
buildFailureIssueTitle gains a dedicated branch for hasEngineRateLimit429 (line 308), but buildFailureMatchCategories (lines 233-274) has no matching entry pushing an engine_rate_limit_429 category. Since categories are the deduplic…
actions/setup/js/handle_agent_failure.cjs:53
detectEngineRateLimit429Failure (called here to compute hasEngineRateLimit429) has no terminal_reason: completed guard, unlike buildEngineFailureContext, so a transient 429 earlier in the log can mislabel a job whose agent actually completed successfully.
<details><summary>💡 Details and fix</summary>
buildEngineFailureContext (lines 2681-2688) explicitly checks for terminal_reason: "completed" in the log and suppresses engine-failure classification if present, because a completed…
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review summary
Two blocking issues before merge — both inline comments have the exact fixes needed.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 26.4 AIC · ⊞ 5.3K
| if (hasToolDenialsExceeded) { | ||
| core.info(`Detected ${toolDenialsExceededEvents.length} guard.tool_denials_exceeded event(s) from Copilot SDK events.jsonl`); | ||
| } | ||
| const hasEngineRateLimit429 = agentConclusion === "failure" && !maxAICreditsExceeded && detectEngineRateLimit429Failure(); |
There was a problem hiding this comment.
Missing entry in buildFailureMatchCategories — hasEngineRateLimit429 has a dedicated buildFailureIssueTitle branch but no corresponding categories.push(...) in buildFailureMatchCategories. Without it a 429-caused run falls through to the agent_failure fallback and deduplication logic won't treat it as a distinct failure class. Add alongside the other rate-limit entries (~line 255):
if (options.hasEngineRateLimit429) categories.push("engine_rate_limit_429");@copilot please address this.
There was a problem hiding this comment.
Fixed. engine_rate_limit_429 is added to buildFailureMatchCategories at line 256 (alongside ai_credits_rate_limit_error), and hasEngineRateLimit429 is forwarded to the call site in main(). Tests confirm the category is emitted and that agent_failure is suppressed when the flag is set.
| } catch { | ||
| // Ignore read errors and continue with OTLP mirror fallback. | ||
| } | ||
| return hasEngineRateLimit429InOTELMirror(); |
There was a problem hiding this comment.
Transient 429 guard missing — detectEngineRateLimit429Failure scans the stdio log and OTEL mirror unconditionally. buildEngineFailureContext (lines 2681–2688) already suppresses the 429 signal when terminal_reason: "completed" is present, because a transient mid-session 429 that the agent recovered from should not produce a failure title. This new detector bypasses that guard.
Add the same check before returning true:
if (fs.existsSync(stdioLogPath)) {
const logContent = fs.readFileSync(stdioLogPath, "utf8");
// Suppress transient 429s that the agent recovered from.
if (/"terminal_reason"[ ]?:[ ]?"completed"/.test(logContent)) return false;
if (hasEngineRateLimit429Signal(logContent)) return true;
}@copilot please address this.
There was a problem hiding this comment.
Fixed. detectEngineRateLimit429Failure now checks for terminal_reason: "completed" in the stdio log before accepting the 429 signal (or falling through to the OTEL mirror). This matches the guard already present in buildEngineFailureContext. Unit tests covering the completed-guard path are included.
PR Triage
|
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot Please address the outstanding review feedback, refresh the branch, and run the
|
…s and guard against completed agent in detectEngineRateLimit429Failure - Add `engine_rate_limit_429` to `buildFailureMatchCategories` so that engine HTTP 429 rate-limit signals produce a distinct deduplication category instead of falling back to `agent_failure`. Pass `hasEngineRateLimit429` at the `buildFailureMatchCategories` call site in `main()`. - Add `terminal_reason: completed` guard in `detectEngineRateLimit429Failure` so that a transient 429 logged earlier in a run that ultimately succeeded does not cause a rate-limit title to be applied to an unrelated post-processing failure. - Export `detectEngineRateLimit429Failure` and add focused unit tests for the new completed-guard and the new `engine_rate_limit_429` category. Addresses review feedback from copilot-pull-request-reviewer (threads on handle_agent_failure.cjs:3564 and handle_agent_failure.cjs:2629-2633). Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…ecedence test - Exclude `aiCreditsRateLimitError` from the `hasEngineRateLimit429` guard so that when an AI-credits rate-limit signal already classifies the failure, the engine-429 detector is not run unnecessarily (mirroring the title precedence chain that places AI-credits above engine rate limits). - Add a test case confirming that `aiCreditsRateLimitError` takes precedence over `hasEngineRateLimit429` in `buildFailureIssueTitle` when both flags are true. Addresses github-actions review threads on handle_agent_failure.cjs:3419 and handle_agent_failure.test.cjs:111. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All outstanding review feedback has been addressed across two commits: From
|
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the
|
|
🎉 This pull request is included in a new release. Release: |
The PR Code Quality Reviewer failure issue for run
30647761131was titled generically asfaileddespite telemetry indicating an engine-side HTTP 429/rate-limit condition. This change makes the failure title reflect that condition directly for faster triage.Problem targeted
handle_agent_failuredid not distinguish engine throughput throttling (HTTP 429) from generic failures in the issue title.Title classification update
buildFailureIssueTitlefor engine rate-limit signals:"[aw] <workflow> hit engine rate limit (HTTP 429)"Signal plumbing
hasEngineRateLimit429as an explicit title input so title rendering can use the already-detected engine 429 signal path without changing unrelated failure handling.Focused unit coverage
buildFailureIssueTitletable-driven cases to cover the newhasEngineRateLimit429classification.Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
run: https://github.com/github/gh-aw/actions/runs/30676859249