Uh oh!
There was an error while loading. Please reload this page.
Improve Copilot 403 auth guidance for copilot-requests mode - #40052
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| const stage = detectCopilotAuthFailureStage(output); | ||
| if (authFailure.statusCode === "403" && envFlagEnabled(env.S2STOKENS)) { | ||
| return ( | ||
| `Copilot requests authentication failed through the gh-aw API proxy (HTTP 403, model=${selectedModel}, stage=${stage}). ` + |
There was a problem hiding this comment.
@copilot refactor to template file in actions/setup/md use renderTemplate
There was a problem hiding this comment.
Addressed in ef58ece by moving the 403 copilot-requests guidance into an actions/setup/md template and rendering it with renderTemplateFromFile.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
✅ Ready to approve
The changes are narrowly scoped to diagnostics, include targeted regression tests, and align with the stated 403/Copilot-requests requirements without introducing risky behavioral changes.
Note: this review does not count toward required approvals for merging.
Pull request overview
This PR updates gh-aw’s Copilot CLI harness diagnostics so HTTP 403 failures that occur specifically in permissions.copilot-requests: write (Copilot requests / centralized billing) mode no longer show BYOK provider-token guidance, and instead direct users to org billing enablement and the billing reference docs.
Changes:
- Added a dedicated 403 diagnostic message for Copilot requests mode when the failure is coming from the local gh-aw API proxy.
- Hardened Copilot requests mode detection by treating
S2STOKENSas a truthy env flag (not only an exact"true"match). - Added regression tests covering proxy-backed 403 behavior in Copilot requests mode and
S2STOKENStruthy variants, while preserving existing 401 behavior.
File summaries
| File | Description |
|---|---|
actions/setup/md/copilot_requests_proxy_auth_403.md | New templated diagnostic message for proxy-backed HTTP 403 in Copilot requests mode, linking to billing guidance. |
actions/setup/js/copilot_harness.cjs | Adds truthy env-flag parsing and a 403-specific diagnostic path gated on S2STOKENS (Copilot requests mode). |
actions/setup/js/copilot_harness.test.cjs | Adds focused unit coverage for the new 403 rewrite behavior and truthy S2STOKENS handling. |
.changeset/patch-copilot-403-org-billing-message.md | Records the user-facing patch change in release notes. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 0
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40052 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold 100). |
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
✅ PR Code Quality Reviewer completed the code quality review. |
✅ Test Quality Sentinel completed test quality analysis. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
JavaScript: 2 ( 💡 Optional Improvements (non-blocking)
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnose — commenting with improvement suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps: The new branch discriminator (
envFlagEnabled(S2STOKENS)guards the 403 path) has no negative-case test. BYOK 403s should return empty, but removing the guard wouldn't be caught by the current suite. envFlagEnabledis untested at the unit level: Falsy variants ("false","0","no") are never exercised; only the truthy path is covered integration-style.- Second test underspecified: The truthy-normalization test checks a single property, while the first test checks five — worth aligning the coverage depth.
Positive Highlights
- ✅ Clean guard restructuring — splitting the compound
statusCode !== "401"condition into per-branch returns is more readable and extensible. - ✅
envFlagEnabledis a well-scoped pure function with a clear contract; the"yes"/"1"normalization pre-empts common CI flag variation. - ✅ The first regression test is thorough: six expectations lock down the message content, billing URL, and absence of wrong guidance in one shot.
- ✅ Changeset entry accurately scopes the change as
patchwith a clear one-sentence description.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
REQUEST_CHANGES — 3 test coverage gaps + 1 throw-safety issue
The diagnostic logic and envFlagEnabled implementation are correct. The issues below are about insufficient test coverage that would let regressions slip through and a silent contract change that turns a no-throw function into one that can crash the harness.
Blocking findings summary
| # | File | Severity | Finding |
|---|---|---|---|
| 1 | copilot_harness.test.cjs:987 | medium | No assertion verifies {selected_model}/{stage} placeholders are resolved — static-string checks pass even if substitution is broken |
| 2 | copilot_harness.test.cjs:996 | medium | No test for falsy S2STOKENS returning "" and no test for non-proxy 403 + S2STOKENS=true returning "" — both gate conditions are unverified on their off paths |
| 3 | copilot_harness.cjs:434 | medium | buildCopilotProxyAuthFailureDiagnostic gains a filesystem I/O call with no error handling; a missing template file now crashes the harness retry loop instead of returning a fallback string |
🔎 Code quality review by PR Code Quality Reviewer
Uh oh!
There was an error while loading. Please reload this page.
| }); | ||
| expect(diagnostic).toContain("Copilot requests authentication failed"); | ||
| }); |
There was a problem hiding this comment.
Falsy S2STOKENS and non-proxy-URL paths are completely untested: the test suite covers only truthy values, leaving two correctness gaps that would hide regressions.
💡 Suggested additions
Gap 1 — falsy S2STOKENS returns empty string (S2STOKENS gate correctness):
it("returns empty string for proxy 403 when S2STOKENS is not set",()=>{constdiagnostic=buildCopilotProxyAuthFailureDiagnostic("Authentication failed with provider at (172.30.0.30/redacted) (HTTP 403).",{COPILOT_MODEL: "claude-sonnet-4.5"}// no S2STOKENS);expect(diagnostic).toBe("");});Gap 2 — non-proxy URL with S2STOKENS=true returns empty string (proxy-URL gate correctness):
it("returns empty string for non-proxy 403 even when S2STOKENS is true",()=>{constdiagnostic=buildCopilotProxyAuthFailureDiagnostic("Authentication failed with provider at (api.anthropic.com/redacted) (HTTP 403).",{COPILOT_MODEL: "claude-sonnet-4.5",S2STOKENS: "true"});expect(diagnostic).toBe("");});Without these, accidentally inverting the envFlagEnabled check or removing the isLikelyAWFAPIProxyURL guard would silently pass the entire test suite.
Uh oh!
There was an error while loading. Please reload this page.
pelikhan
commented
Jun 18, 2026
@copilot run pr-finisher skill |
``
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Addressed all open
Local |
| try { | ||
| return renderTemplateFromFile(COPILOT_REQUESTS_PROXY_AUTH_403_TEMPLATE_PATH, { | ||
| selected_model: selectedModel, | ||
| stage, | ||
| }); | ||
| } catch { | ||
| return `Copilot requests authentication failed through the gh-aw API proxy (HTTP 403, model=${selectedModel}, stage=${stage}). Verify that copilot-requests: write is granted and that Copilot org billing is enabled.`; | ||
| } |
There was a problem hiding this comment.
@copilot remove try/catch. Assume this will not fail
There was a problem hiding this comment.
Done — try/catch removed in the latest commit.
…FailureDiagnostic Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot CLI failures in
copilot-requests: writemode were surfacing BYOK provider-token guidance even when the real problem was org-billing authorization. This updates the 403 path to point users at Copilot requests permissions and centralized billing docs instead.403 diagnostics for Copilot requests
COPILOT_PROVIDER_API_KEY/COPILOT_PROVIDER_BEARER_TOKENguidance with messaging tailored to:permissions.copilot-requests: writeMode detection hardening
S2STOKENSas a truthy env flag instead of requiring an exact"true"match.Regression coverage
S2STOKENSvariantsExample of the new guidance path: