-
Notifications
You must be signed in to change notification settings - Fork 475
Improve PR failure issue title specificity for engine HTTP 429 rate limits #49411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8ac117e
df907c6
e8221db
b0e98e1
dc52947
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -253,6 +253,7 @@ function buildFailureMatchCategories(options) { | |
| if (options.modelNotSupportedError) categories.push("model_not_supported_error"); | ||
| if (options.http400ResponseError) categories.push("http_400_response_error"); | ||
| if (options.aiCreditsRateLimitError) categories.push("ai_credits_rate_limit_error"); | ||
| if (options.hasEngineRateLimit429) categories.push("engine_rate_limit_429"); | ||
| if (options.unknownModelAICredits) categories.push("unknown_model_ai_credits"); | ||
| if (options.missingModelPricingError) categories.push("missing_model_pricing"); | ||
| if (options.maxAICreditsExceeded) categories.push("max_ai_credits_exceeded"); | ||
|
|
@@ -291,6 +292,7 @@ function buildFailureMatchCategories(options) { | |
| * @param {boolean} options.hasStaleLockFileFailed | ||
| * @param {boolean} options.hasDailyAICExceeded | ||
| * @param {boolean} options.aiCreditsRateLimitError | ||
| * @param {boolean} options.hasEngineRateLimit429 | ||
| * @param {boolean} options.maxAICreditsExceeded | ||
| * @param {boolean} options.hasAssignmentErrors | ||
| * @param {boolean} options.http400ResponseError | ||
|
|
@@ -304,6 +306,7 @@ function buildFailureIssueTitle(options) { | |
| if (options.hasDailyAICExceeded) return `[aw] ${workflowName} exceeded daily AI credits budget`; | ||
| if (options.maxAICreditsExceeded) return `[aw] ${workflowName} exceeded max AI credits`; | ||
| if (options.aiCreditsRateLimitError) return `[aw] ${workflowName} hit AI credits rate limit`; | ||
| if (options.hasEngineRateLimit429) return `[aw] ${workflowName} hit engine rate limit (HTTP 429)`; | ||
| // Missing model pricing is surfaced by the proxy as HTTP 400, so prefer the | ||
| // specialized title before falling back to the generic transport-level error. | ||
| if (options.missingModelPricingError) { | ||
|
|
@@ -2614,6 +2617,33 @@ function detectAWFFirewallStartupFailureFromLog() { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Detect whether the agent failure was caused by engine HTTP 429/rate limiting. | ||
| * Checks agent-stdio.log first, then falls back to OTLP mirror payloads. | ||
| * @returns {boolean} | ||
| */ | ||
| function detectEngineRateLimit429Failure() { | ||
| const agentOutputFile = process.env.GH_AW_AGENT_OUTPUT; | ||
| const stdioLogPath = agentOutputFile ? path.join(path.dirname(agentOutputFile), "agent-stdio.log") : "/tmp/gh-aw/agent-stdio.log"; | ||
| try { | ||
| if (fs.existsSync(stdioLogPath)) { | ||
| const logContent = fs.readFileSync(stdioLogPath, "utf8"); | ||
| // If the agent completed successfully (terminal_reason: "completed"), the failure | ||
| // was caused by something other than the agent itself. Suppress the 429 signal to | ||
| // avoid giving a rate-limit title to an unrelated post-processing failure. | ||
| if (/"terminal_reason"[ ]?:[ ]?"completed"/.test(logContent)) { | ||
| return false; | ||
| } | ||
| if (hasEngineRateLimit429Signal(logContent)) { | ||
| return true; | ||
| } | ||
| } | ||
| } catch { | ||
| // Ignore read errors and continue with OTLP mirror fallback. | ||
| } | ||
| return hasEngineRateLimit429InOTELMirror(); | ||
| } | ||
|
|
||
| /** | ||
| * Extract terminal error messages from agent-stdio.log to surface engine failures. | ||
| * First tries to match known error patterns (ERROR:, Error:, Fatal:, panic:, Reconnecting...). | ||
|
|
@@ -3389,6 +3419,7 @@ async function main() { | |
| if (hasToolDenialsExceeded) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The guard π‘ Suggested fixconst 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
| core.info(`Detected ${toolDenialsExceededEvents.length} guard.tool_denials_exceeded event(s) from Copilot SDK events.jsonl`); | ||
| } | ||
| const hasEngineRateLimit429 = agentConclusion === "failure" && !maxAICreditsExceeded && !aiCreditsRateLimitError && detectEngineRateLimit429Failure(); | ||
|
|
||
| // Detect cache-miss misconfiguration: the agent reported a missing_data with reason | ||
| // "cache_memory_miss" after a cache restore matched. This indicates the prompt | ||
|
|
@@ -3537,6 +3568,7 @@ async function main() { | |
| hasStaleLockFileFailed, | ||
| hasDailyAICExceeded, | ||
| aiCreditsRateLimitError, | ||
| hasEngineRateLimit429, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
| maxAICreditsExceeded, | ||
| hasAssignmentErrors, | ||
| http400ResponseError, | ||
|
|
@@ -3566,6 +3598,7 @@ async function main() { | |
| modelNotSupportedError, | ||
| http400ResponseError, | ||
| aiCreditsRateLimitError, | ||
| hasEngineRateLimit429, | ||
| unknownModelAICredits, | ||
| missingModelPricingError, | ||
| maxAICreditsExceeded, | ||
|
|
@@ -4141,6 +4174,7 @@ module.exports = { | |
| hasEngineMaxRunsExceededSignal, | ||
| hasEngineRateLimit429Signal, | ||
| hasEngineRateLimit429InOTELMirror, | ||
| detectEngineRateLimit429Failure, | ||
| buildEngineMaxRunsExceededContext, | ||
| buildEngineRateLimit429Context, | ||
| hasEngineMaxCacheMissesExceededSignal, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,6 +97,7 @@ describe("handle_agent_failure", () => { | |
| hasStaleLockFileFailed: false, | ||
| hasDailyAICExceeded: false, | ||
| aiCreditsRateLimitError: false, | ||
| hasEngineRateLimit429: false, | ||
| maxAICreditsExceeded: false, | ||
| hasAssignmentErrors: false, | ||
| http400ResponseError: false, | ||
|
|
@@ -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" }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] Precedence ordering is not tested β add a case where both π‘ 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 @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added. A new test case |
||
| { flag: "aiCreditsRateLimitError", expected: "[aw] Test Workflow hit AI credits rate limit" }, | ||
| { flag: "hasEngineRateLimit429", expected: "[aw] Test Workflow hit engine rate limit (HTTP 429)" }, | ||
| { flag: "http400ResponseError", expected: "[aw] Test Workflow hit HTTP 400 bad request" }, | ||
| { flag: "unknownModelAICredits", expected: "[aw] Test Workflow has unknown model pricing" }, | ||
| { flag: "hasAppTokenMintingFailed", expected: "[aw] Test Workflow failed to mint GitHub App token" }, | ||
|
|
@@ -155,6 +157,10 @@ describe("handle_agent_failure", () => { | |
| "[aw] Test Workflow has no AI credits pricing for model (claude-opus-5)" | ||
| ); | ||
| }); | ||
|
|
||
| it("aiCreditsRateLimitError takes precedence over hasEngineRateLimit429 when both are true", () => { | ||
| expect(buildFailureIssueTitle({ ...baseOptions, aiCreditsRateLimitError: true, hasEngineRateLimit429: true })).toBe("[aw] Test Workflow hit AI credits rate limit"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("detection caution placement in main()", () => { | ||
|
|
@@ -2945,6 +2951,62 @@ describe("handle_agent_failure", () => { | |
| }); | ||
| }); | ||
|
|
||
| // ββββββββββββββββββββββββββββββββββββββββββββββββββββββ | ||
| // detectEngineRateLimit429Failure | ||
| // ββββββββββββββββββββββββββββββββββββββββββββββββββββββ | ||
|
|
||
| describe("detectEngineRateLimit429Failure", () => { | ||
| let detectEngineRateLimit429Failure; | ||
| const fs = require("fs"); | ||
| const path = require("path"); | ||
| const os = require("os"); | ||
|
|
||
| /** @type {string} */ | ||
| let tmpDir; | ||
| /** @type {string} */ | ||
| let stdioLogPath; | ||
|
|
||
| beforeEach(() => { | ||
| vi.resetModules(); | ||
| tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "aw-test-detect-429-")); | ||
| stdioLogPath = path.join(tmpDir, "agent-stdio.log"); | ||
| process.env.GH_AW_AGENT_OUTPUT = path.join(tmpDir, "agent_output.json"); | ||
| ({ detectEngineRateLimit429Failure } = require("./handle_agent_failure.cjs")); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| delete process.env.GH_AW_AGENT_OUTPUT; | ||
| delete process.env.GH_AW_OTEL_JSONL_PATH; | ||
| if (fs.existsSync(tmpDir)) { | ||
| fs.rmSync(tmpDir, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
|
|
||
| it("returns true when stdio log contains a 429 rate-limit signal", () => { | ||
| fs.writeFileSync(stdioLogPath, "Failed to get response from the AI model; retried 5 times. Last error: CAPIError: 429 429 Sorry, you've exceeded your rate limit.\n"); | ||
| expect(detectEngineRateLimit429Failure()).toBe(true); | ||
| }); | ||
|
|
||
| it("returns false when stdio log contains terminal_reason: completed even with a 429 signal", () => { | ||
| fs.writeFileSync(stdioLogPath, 'Failed to get response; CAPIError: 429 rate limit\n{"type":"result","subtype":"success","terminal_reason":"completed","num_turns":10}\n'); | ||
| expect(detectEngineRateLimit429Failure()).toBe(false); | ||
| }); | ||
|
|
||
| it("returns false when stdio log contains terminal_reason: completed and no 429 signal", () => { | ||
| fs.writeFileSync(stdioLogPath, '{"type":"result","subtype":"success","terminal_reason":"completed","num_turns":5}\n'); | ||
| expect(detectEngineRateLimit429Failure()).toBe(false); | ||
| }); | ||
|
|
||
| it("returns false when stdio log has no 429 signal and OTLP mirror is absent", () => { | ||
| fs.writeFileSync(stdioLogPath, "Agent exited normally.\n"); | ||
| expect(detectEngineRateLimit429Failure()).toBe(false); | ||
| }); | ||
|
|
||
| it("returns false when log file does not exist and OTLP mirror is absent", () => { | ||
| expect(detectEngineRateLimit429Failure()).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| // ββββββββββββββββββββββββββββββββββββββββββββββββββββββ | ||
| // hasEngineMaxCacheMissesExceededSignal | ||
| // ββββββββββββββββββββββββββββββββββββββββββββββββββββββ | ||
|
|
@@ -5175,6 +5237,23 @@ describe("handle_agent_failure", () => { | |
| }); | ||
| expect(categories).not.toContain("missing_model_pricing"); | ||
| }); | ||
|
|
||
| it("returns engine_rate_limit_429 category when hasEngineRateLimit429 is true", () => { | ||
| const categories = buildFailureMatchCategories({ | ||
| agentConclusion: "failure", | ||
| hasEngineRateLimit429: true, | ||
| }); | ||
| expect(categories).toContain("engine_rate_limit_429"); | ||
| expect(categories).not.toContain("agent_failure"); | ||
| }); | ||
|
|
||
| it("does not return engine_rate_limit_429 category when hasEngineRateLimit429 is false", () => { | ||
| const categories = buildFailureMatchCategories({ | ||
| agentConclusion: "failure", | ||
| hasEngineRateLimit429: false, | ||
| }); | ||
| expect(categories).not.toContain("engine_rate_limit_429"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("failure categories filter behavior", () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transient 429 guard missing β
detectEngineRateLimit429Failurescans the stdio log and OTEL mirror unconditionally.buildEngineFailureContext(lines 2681β2688) already suppresses the 429 signal whenterminal_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:@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
detectEngineRateLimit429Failurenow checks forterminal_reason: "completed"in the stdio log before accepting the 429 signal (or falling through to the OTEL mirror). This matches the guard already present inbuildEngineFailureContext. Unit tests covering the completed-guard path are included.