sc-308937: Fix disabled Allow Access button in Codex OAuth flow

[kschrader]

Summary

• Default OAuth authorize scope to openid when client omits scope
• Preserve explicitly provided scopes when present
• Add regression test for /authorize without a scope query param

Problem

Codex CLI OAuth flow can omit the scope query parameter. Our authorize proxy forwarded no scope upstream, which left the Shortcut auth page with a disabled Allow Access button.

Verification

• bun test src/auth/oauth.test.ts
• Pre-push checks ran automatically and passed (full bun test suite)

Summary by CodeRabbit

• New Features

  • Authorization requests now default to the openid scope when no scope is explicitly specified, ensuring consistent identity authentication behavior.

• Tests

  • Added test coverage for default scope behavior in authorization flow.

[coderabbitai]

📝 Walkthrough

Walkthrough

A new default authorization scope behavior is implemented in the OAuth provider, automatically using "openid" as the default scope when client requests omit the scope parameter. Test coverage validates this default behavior.

Changes

Cohort / File(s)	Summary
Default OAuth Scope src/auth/provider.ts, src/auth/oauth.test.ts	Added DEFAULT_AUTHORIZATION_SCOPES constant and conditional logic to apply "openid" scope when clients omit scope parameter. Includes test case validating redirect includes default scope=openid.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A scope without a name, now defaults so true,
To "openid" it hops, like morning dew!
No more empty requests, left to their fate,
The rabbit ensures scopes are never misplaced. ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title references fixing a disabled button in OAuth flow, but the actual changes implement a default scope behavior—addressing the root cause rather than the UI symptom.	Consider revising the title to reflect the actual change: 'Default OAuth authorize scope to openid when omitted' or 'Set default authorize scope to openid in OAuth flow'.
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch kurt/sc-308937/fix-disabled-allow-access-button-when

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/auth/oauth.test.ts (1)

358-386: Nice regression test; consider asserting the parsed scope exactly.

toContain("scope=openid") can pass even if extra scopes are appended. Parsing location as a URL and asserting searchParams.get("scope") === "openid" will make this test stricter and future-proof.

Suggested assertion update

 			const location = res.headers.get("location");
 			expect(location).toBeDefined();
-			expect(location).toContain("scope=openid");
+			const redirectUrl = new URL(location!);
+			expect(redirectUrl.searchParams.get("scope")).toBe("openid");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/auth/oauth.test.ts` around lines 358 - 386, Update the assertion in the
test "GET /authorize defaults scope to openid when client omits scope" to parse
the returned location as a URL and assert the exact scope value; specifically,
replace the loose expect(location).toContain("scope=openid") check with
constructing a new URL(location) and asserting url.searchParams.get("scope") ===
"openid" (use the existing location variable and the test's URL base such that
the parsed URL is valid) to ensure the scope equals exactly "openid".

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/auth/oauth.test.ts`:
- Around line 358-386: Update the assertion in the test "GET /authorize defaults
scope to openid when client omits scope" to parse the returned location as a URL
and assert the exact scope value; specifically, replace the loose
expect(location).toContain("scope=openid") check with constructing a new
URL(location) and asserting url.searchParams.get("scope") === "openid" (use the
existing location variable and the test's URL base such that the parsed URL is
valid) to ensure the scope equals exactly "openid".

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5b63ed2 and a9efe4a.

📒 Files selected for processing (2)

• src/auth/oauth.test.ts
• src/auth/provider.ts

[opoku]

Seems right to me.