[dklawren]

GitHub login failures now show the real cause, and the "messaging functions" wrapper page is gone. That doesn't mean login works yet: I haven't found out why GitHub's response wasn't JSON. The new test passes in the container: 8 tests, Result: PASS.

Changes (3 files, nothing committed)

1. extensions/GitHubAuth/lib/Client.pm
   • It checks the HTTP status first, then parses the JSON. A failed request now gives github_error with the HTTP status and reason instead of "Unable to parse JSON response".
   • A 200 response that isn't JSON now reports the status and Content-Type.
   • GitHub's {"error": "bad_verification_code", ...} replies now show GitHub's own description. Before, this failed later on a confusing call with an empty token.
   • The error variable is renamed from message to reason, so the header no longer treats it as a message tag.
   • Added an explicit use LWP::UserAgent.
2. code-error-errors.html.tmpl: shows the status and content type. When LWP itself failed (DNS, TLS or proxy), it adds "Bugzilla could not connect to GitHub".
3. extensions/GitHubAuth/t/bmo/client-response.t (new): mocks the user agent to cover the success, 403, LWP internal 500, 200-but-not-JSON and OAuth-error cases. It also renders the error page to confirm the old fallback text is gone.

★ Insight ─────────────────────────────────────

• The test turns on $USE_EXCEPTION_OBJECTS = 1 in Client::Error, which Login.pm also uses. Errors come back as objects you can inspect, so the test can check error and vars without rendering a page.
• I used decoded_content(charset => 'none') instead of plain decoded_content. decode_json expects UTF-8 bytes, and fully decoded text would break on non-ASCII names or emails.
  ─────────────────────────────────────────────────

What I picked, what I gave up, and why: I made the failure readable and left the root cause alone, because the code doesn't show which GitHub call fails or why. Once this is deployed, the next failed login will name the cause (proxy, TLS or GitHub). That may need a second small fix. Also, I didn't run perl scripts/perl-fmt.pl, and line 77 of Client.pm is longer than the usual width.

[Copilot]

🟡 Changes recommended

The rendering regression test bypasses the header responsible for the original messaging fallback and needs to exercise webpage mode.

1 open issue

• Render subtest in webpage mode to exercise header processing · New

What changed in this PR

This PR improves GitHub authentication diagnostics in Bugzilla without claiming to fix the underlying login failure.

Changes:

• Checks HTTP status before parsing JSON and surfaces OAuth error descriptions.
• Displays response details without triggering the messaging fallback.
• Adds mocked response tests and an error-page rendering regression test.

File	Description
extensions/​GitHubAuth/​template/​en/​default/​hook/​global/​code-error-errors.html.tmpl	Displays status, content type, and connection-failure context.
extensions/​GitHubAuth/​t/​bmo/​client-response.t	Adds response-handling and error-page rendering tests.
extensions/​GitHubAuth/​lib/​Client.pm	Improves HTTP, JSON, and OAuth error handling.

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The success-path test expects a scalar instead of a JSON boolean object, causing its deep comparison to fail for a correct response.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Expect JSON boolean instead of scalar 1

extensions/​GitHubAuth/​t/​bmo/​client-response.t:68

The decoded verified value is a JSON boolean object, not the plain scalar 1. is_deeply distinguishes these types, so this success-path assertion fails even when the client returns the correct response. Use JSON::true() in the expected structure, or check the flag separately as a boolean.

1 resolved since last review

• Render subtest in webpage mode to exercise header processing

_{Review effort: Balanced}