Treat the AWF proxy's HTTP 403 max-AI-credits rejection as trusted budget-abort evidence - #55241
Conversation
…bort 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.
Pull request overview
Adds early recognition of AWF proxy AI-credit budget rejections so harnesses treat intentional enforcement as success.
Changes:
- Parses canonical HTTP 403 credit-limit responses.
- Applies trusted-budget handling across all harnesses.
- Adds parser and Claude regression tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/harness_retry_guard.cjs |
Adds proxy rejection parsing. |
actions/setup/js/harness_retry_guard.test.cjs |
Tests parser boundaries. |
actions/setup/js/claude_harness.cjs |
Handles proxy budget aborts. |
actions/setup/js/claude_harness.test.cjs |
Reproduces the reported failure. |
actions/setup/js/codex_harness.cjs |
Handles proxy budget aborts. |
actions/setup/js/copilot_harness.cjs |
Handles proxy budget aborts. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| */ | ||
| function parseAICreditsExceededProxyRejection(output) { | ||
| const safeOutput = typeof output === "string" ? output : ""; | ||
| const match = AI_CREDITS_EXCEEDED_PROXY_REJECTION_RE.exec(safeOutput); |
There was a problem hiding this comment.
Addressed in d1963cf and a4cb063: parseAICreditsExceededProxyRejection() no longer treats free-text as trusted. It now only matches lines that (1) parse as standalone JSON and (2) carry an engine-set API-error marker (is_api_error_message: true or a string error field) — a source only the harness's own transport layer sets, mirroring how isInvalidRequestError validates codex turn.failed events. Plain assistant text quoting the phrase, without that structural marker, is ignored. See harness_retry_guard.test.cjs for the regression test covering that exact untrusted-text case.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #55241 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. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Request changes
The main fix regresses the trust boundary: it upgrades a regex match from combined child stdout/stderr into “trusted budget-abort evidence”, so model- or tool-controlled text can now suppress a real failure and make the harness exit 0.
Blocking theme
parseAICreditsExceededProxyRejection()reads fromresult.output, whichrunProcess()builds by concatenating untrusted child stdout/stderr.- The new harness logic then lets that free-text match override the existing authentication-failure veto.
- That means a forged string in assistant output can masquerade as an AWF proxy rejection and convert an actual error into success.
I left one inline comment on the trust upgrade point; the budget-abort classification needs to stay tied to a channel the model cannot forge.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 18.8 AIC · ⌖ 6.98 AIC · ⊞ 4.6K
Comment /review to run again
| */ | ||
| function parseAICreditsExceededProxyRejection(output) { | ||
| const safeOutput = typeof output === "string" ? output : ""; | ||
| const match = AI_CREDITS_EXCEEDED_PROXY_REJECTION_RE.exec(safeOutput); |
There was a problem hiding this comment.
This change now treats a free-text 403 Maximum AI credits exceeded (used / max) substring in combined child stdout/stderr as trusted budget-abort evidence, but result.output is not a trusted channel: process_runner.cjs concatenates raw child stdout and stderr verbatim, which includes assistant/model text and any tool output. That means a prompt-injected or tool-emitted string matching this regex can suppress a real authentication failure and force the harness to exit 0. The new trust upgrade removes the audit-log boundary the old code relied on and turns untrusted model output into a success signal.
💡 Why this should block
parseAICreditsExceededProxyRejection() only checks for a textual pattern:
(b403/redacted)\b[^\n]{0,80}?maximum ai credits exceeded\s*\(\s*(\d+(?:\.\d+)?)\s*\/\s*(\d+(?:\.\d+)?)\s*\)/iand the harnesses then do:
const trustedAICreditsExceeded = nonRetryableGuard.aiCreditsExceeded && (!!proxyAICreditsRejection || parseMaxAICreditsExceededFromAuditLog());
const shouldTreatAICreditsExceededAsSuccess = trustedAICreditsExceeded && (!isAuthenticationFailed || !!proxyAICreditsRejection);Because runProcess() builds result.output from arbitrary child stdout/stderr, the agent itself can emit that exact phrase in normal assistant text and make the wrapper classify the run as a trusted budget abort. That is a correctness and security boundary regression: auth failures and other real errors can be hidden behind model-controlled text.
Please keep the trust decision anchored to a source the model cannot forge, e.g. the firewall/audit channel or a harness-generated side channel that captures authenticated proxy metadata separately from assistant output.
There was a problem hiding this comment.
Addressed in d1963cf and a4cb063: the parser is no longer a free-text match against combined stdout/stderr. parseAICreditsExceededProxyRejection() now requires each candidate line to parse as a standalone JSON object and carry an engine-set API-error marker (is_api_error_message: true or a string error field) before the 403 usage-pair regex is applied — a marker only the harness's own transport layer sets on genuine proxy/API errors, not on assistant conversational text. An assistant turn that merely quotes the phrase in its message content (without that structural marker) no longer qualifies as trusted evidence; this exact case is covered by a new regression test in harness_retry_guard.test.cjs. The audit-log path remains available as an alternative trusted source.
There was a problem hiding this comment.
One simplification opportunity stands out: the proxy-budget parser can be collapsed into a local regex match in claude_harness.cjs, removing the extra helper/export surface without changing behavior. net: -15 lines possible.
Generated by ✂️ Ponytail Reviewer for #55241 · codex · cop10 · 4.87 AIC · ⌖ 1.29 AIC · ⊞ 18.3K
Comment /ponytail to run again
| return AUTHENTICATION_FAILED_PATTERNS.some(pattern => pattern.test(safeOutput)); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
actions/setup/js/harness_retry_guard.cjs:59-75: yagni: helper + exported regex for a one-off proxy budget match. Inline the regex match in claude_harness.cjs and keep the budget check local.
There was a problem hiding this comment.
Review: Harden + Audit
This PR correctly addresses a real gap — when Claude Code surfaces the proxy's 403 budget rejection as authentication_failed, the harness was masking intentional budget enforcement. The logic change and the new test case are both well-targeted.
Primary concern (already raised in an existing inline comment): the trust boundary of result.output. Since that output includes model-generated text, a sufficiently crafted response could match AI_CREDITS_EXCEEDED_PROXY_REJECTION_RE — the aiCredits >= maxAICredits guard helps but does not fully close this. The existing comment covers this well.
Minor observations (non-blocking):
codex_harness.cjsreceived the identical logic change but has no new integration test — onlyclaude_harness.test.cjshas the new scenario.- The
codex_harnesspath also vetoesisMissingApiKeyalongsideisAuthenticationFailed; the new proxy-override only exemptsisAuthenticationFailed, which looks intentional and correct. - The
parseAICreditsExceededProxyRejectionunit tests inharness_retry_guard.test.cjsare thorough and cover the boundary condition (aiCredits === maxAICredits).
Overall the fix is well-scoped and the test coverage for the primary Claude Code scenario is solid. Resolving the trust-boundary concern raised in the existing inline comment before merge would be ideal.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 23.8 AIC · ⌖ 9.08 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with 3 issues, no hard blockers.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap: regression test only covers the claude harness; codex and copilot harnesses have the identical fix but no integration tests
- Trusted-source ambiguity: the existing concern (comment #3840533313) about matching against combined stdout/stderr still applies to the new regex — it's narrower but not structurally isolated from model output
- Type contract:
parseAICreditsExceededProxyRejectionacceptsunknownbut only handlesstring; non-string values fail silently
Positive Highlights
- ✅ Root-cause analysis is thorough — both defects (audit-JSONL timing and auth-veto masking) are correctly diagnosed and fixed
- ✅ The regex intentionally requires
403+ the precise(used / max)format, making accidental matches unlikely - ✅ Usage-below-budget guard (
aiCredits < maxAICredits) correctly rejects partial matches - ✅ Good boundary test:
used == maxaccepted,used < maxrejected - ✅ Codex's
!isMissingApiKeyguard is correctly preserved in the new expression
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 40.9 AIC · ⌖ 10.1 AIC · ⊞ 7.6K
Comment /matt to run again
| expect(result.stderr).toContain("trusted budget-abort evidence"); | ||
| expect(result.stderr).toContain("AI credits budget enforced"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] Regression test only covers the claude harness. The identical fix is applied to codex_harness.cjs and copilot_harness.cjs — a future refactor could silently regress either without parallel tests.
💡 Suggestion
Add analogous tests to codex_harness.test.cjs and copilot_harness.test.cjs replaying the same proxy 403 payload with each harness's stub, asserting status === 0 and the trusted budget-abort evidence log line. For codex, also verify the !isMissingApiKey guard doesn't interfere.
@copilot please address this.
There was a problem hiding this comment.
Added in a4cb063: codex_harness.test.cjs and copilot_harness.test.cjs each now have a regression test replaying the same proxy 403 payload, asserting status === 0 and the "trusted budget-abort evidence"/"AI credits budget enforced" log lines. The codex test sets CODEX_API_KEY so the !isMissingApiKey guard can't suppress the budget path.
| * Returns null when the output does not carry the proxy signature, or when the reported | ||
| * usage does not actually reach the reported budget. | ||
| * @param {unknown} output | ||
| * @returns {{ aiCredits: number, maxAICredits: number } | null} |
There was a problem hiding this comment.
[/diagnosing-bugs] The safeOutput guard discards any non-string output silently — if result.output is ever an array or object (e.g. structured log lines), the parser returns null and the fix silently fails to fire.
💡 Suggestion
Consider a defensive coercion that mirrors the pattern used by other guards in this file:
const safeOutput = output != null ? String(output) : "";or explicitly document that callers must pass the joined string output. A short comment noting the contract would prevent future callers from passing a raw result.output object.
@copilot please address this.
There was a problem hiding this comment.
Documented the contract in d1963cf/a4cb063: added a comment on parseAICreditsExceededProxyRejection noting callers must pass the joined stdout+stderr string (matching every other guard in this file), and that non-string input is treated as empty rather than coerced via String(...), since that would produce a meaningless value here.
| */ | ||
| function parseAICreditsExceededProxyRejection(output) { | ||
| const safeOutput = typeof output === "string" ? output : ""; | ||
| const match = AI_CREDITS_EXCEEDED_PROXY_REJECTION_RE.exec(safeOutput); |
There was a problem hiding this comment.
[/diagnosing-bugs] There is an existing review comment (id 3840533313) that notes free-text matching on result.output is untrustworthy because it contains combined stdout/stderr including model-generated text. The new regex is narrower (requires 403 + the specific format), but a model could still emit an identical-looking string in its output. Please confirm whether this risk is acceptable, or consider adding an additional check (e.g. verify the line comes from a known harness prefix rather than assistant content).
@copilot please address this.
There was a problem hiding this comment.
Addressed in a4cb063 (building on d1963cf): parseAICreditsExceededProxyRejection() now only trusts lines that parse as standalone JSON and carry an engine-set marker (is_api_error_message: true or a string error field) before running the 403 regex against them. Plain assistant text containing the phrase, without that structural marker, is ignored — added a regression test for that exact case in harness_retry_guard.test.cjs.
|
@copilot This PR still needs forward progress before maintainer investigation is likely to be productive. Please address the current blocking review feedback first:
Additional reviewer context worth closing out:
The branch refresh request has also been queued. After addressing the review, refresh the branch as needed, run the
|
All review threads on this PR already carry a substantive reply addressing the trust-boundary concern (see discussion_r3840533313 / discussion_r3840580081 and their replies): I don't have GitHub API/thread-resolution access in this sandbox to mark the threads "resolved" myself — that requires a maintainer or reviewer action. No further code changes are pending from my side; happy to address any new/specific technical concern if one is raised. |
|
Current blockers to clear:
After that, refresh the branch if needed and push any final follow-up.
|
Both trust-boundary threads (discussion_r3840533313, discussion_r3840580081) already have my direct reply in-thread explaining the fix and pointing to the exact commits/tests. I don't have thread-resolution or |
|
@copilot Thanks for the follow-up here. This PR still looks blocked on unresolved reviewer feedback. Please use the
Open review themes to close out:
Once you have either pushed the fix or provided reviewer-facing resolution in-thread, resolve the addressed review threads.
|
The current implementation is correct as-is: I don't have GitHub thread-resolution or write API access in this sandbox (no |
|
Please do one focused pass with the
Outstanding context:
|
Triage: PR #55241 (re-check)Category: bug · Risk: high · Score: 75/100 (Impact 40, Urgency 25, Quality 15) Recommended action: CI green (copilot ×4, CodeQL analyze go/js all passing). 8 files changed (+243/-10), covers Status: Still blocked — Next action: Needs a maintainer/reviewer pass to either resolve the trust-boundary thread or explicitly accept the residual risk, then dismiss the stale CHANGES_REQUESTED review to unblock merge.
|
|
Please use the
Run: https://github.com/github/gh-aw/actions/runs/32750412251
|
Verified: the trust-boundary fix from d1963cf/a4cb063 is unchanged and stable — |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in 40e8a9d's follow-up: |
The
agentjob in run 32683896339 went red on intentional budget enforcement: the AWF API proxy stopped the run with403 Maximum AI credits exceeded (302.111025 / 300), which the harness is supposed to convert intoexit 0("budget control, not an error"). Theconclusionjob classified it correctly and filed #55239; only theagentjob failed.Two defects in
handleFailureblocked the budget path:parseMaxAICreditsExceededFromAuditLog(), but AWF flushes the firewall audit JSONL during container teardown (02:49:14.97) — after the attempt is classified (02:49:13.17). The audit-only gate can never fire for a mid-run budget abort.error: authentication_failed, so&& !isAuthenticationFailedwould have suppressed the budget path even with audit evidence.Changes
harness_retry_guard.cjs— newparseAICreditsExceededProxyRejection()matching the proxy's canonical rejection, including the proxy-computed usage pair, and rejecting pairs where usage has not reached the budget:max_ai_credits_exceededmarkers, which remain untrusted.Authentication failedstill fails the job; codex keeps its!isMissingApiKeyguard.used == maxboundary) plus a claude harness regression test replaying the failing run's stream-JSON payload.Note on steering
No
[AWF TOKEN WARNING]/[AWF TIME WARNING]messages were inserted before the abort despiteenableTokenSteering: true— AWF steering is token/time-budget based and does not steer on AI-credit budgets, so the agent got no warning and was cut off mid safe-output write. Credit-aware steering would need to happen upstream in gh-aw-firewall and is out of scope here.Run: https://github.com/github/gh-aw/actions/runs/32689010109> Generated by 👨🍳 PR Sous Chef · pi · gpt54 · 9.37 AIC · ⌖ 10.9 AIC · ⊞ 8.7K · ◷
Run: https://github.com/github/gh-aw/actions/runs/32706066137> Generated by 👨🍳 PR Sous Chef · pi · gpt54 · 24.6 AIC · ⌖ 14.4 AIC · ⊞ 8.7K · ◷
Run: https://github.com/github/gh-aw/actions/runs/32726270698