Repository navigation
Fix/claude refresh contract - #10
Merged
Merged
Conversation
Align the built-in Claude OAuth callback default with the known Claude token flow host so the app stops mixing callback identities across login and refresh paths. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Echo Claude's OAuth scopes on the refresh token request so the token endpoint sees the same app identity and grant context as the working Claude auth flow. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Pull request overview
Updates the Claude OAuth token refresh flow to align with the expected upstream contract by including the configured scope during refresh requests, with unit tests updated to validate the new payload/encoding.
Changes:
- Include
scope(fromCLAUDE_CONFIG.scopes) inrefreshClaudeOAuthTokenrequests (JSON + form fallback). - Update Claude refresh unit tests to assert
scopeis present and properly form-encoded. - Add a config-shape assertion for Claude’s default
redirectUri.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
open-sse/services/tokenRefresh.ts |
Adds scope to Claude refresh payload using shared OAuth constants. |
tests/unit/token-refresh-service.test.mjs |
Extends Claude refresh tests to assert scope is sent (JSON + form). |
tests/unit/oauth-providers-config.test.mjs |
Adds an assertion around Claude redirect URI defaults. |
| }); | ||
|
|
||
| test("provider-specific config shapes remain valid for special cases", () => { | ||
| assert.equal(CLAUDE_CONFIG.redirectUri, "https://platform.claude.com/oauth/code/callback"); |
There was a problem hiding this comment.
This assertion hard-codes Claude’s default redirect URI, but the test setup does not clear process.env.CLAUDE_CODE_REDIRECT_URI. If that env var is set in the environment running the unit tests, CLAUDE_CONFIG.redirectUri will not equal the default and this test will fail. Consider either explicitly deleting/unsetting CLAUDE_CODE_REDIRECT_URI in the env setup for this file, or asserting CLAUDE_CONFIG.redirectUri equals process.env.CLAUDE_CODE_REDIRECT_URI || "https://platform.claude.com/oauth/code/callback" to keep the test hermetic.
Suggested change
| assert.equal(CLAUDE_CONFIG.redirectUri, "https://platform.claude.com/oauth/code/callback"); | |
| assert.equal( | |
| CLAUDE_CONFIG.redirectUri, | |
| process.env.CLAUDE_CODE_REDIRECT_URI || "https://platform.claude.com/oauth/code/callback" | |
| ); |
i1hwan
added a commit
that referenced
this pull request
May 7, 2026
Security: /api/sessions enrichment now uses listProviderConnectionMetadata() which selects only id/provider/name/display_name/email. Decryption of apiKey, accessToken, refreshToken, idToken via getProviderConnections() is no longer on this hot path. Fixes Copilot review #11. Routing memory: heuristicBreakHistory map now bounded by BOTH age (10min) AND hard size cap (1000). Re-set on insert moves the entry to the end of the insertion-ordered Map; size-cap eviction loop removes oldest entries when size exceeds 1000. >1000 distinct sessions inside the cooldown window can no longer grow the map without bound. Fixes Copilot review #1/#6. Routing perf: isAffinityValid heuristic-break #2 now finds the bound entry in scoredAlternatives and reuses its score instead of calling scoreAccount() again. Matches the 'score upfront once' contract of selectByEarliestResetFirst. Fixes Copilot review #2/#7. UI tooltip: RoutingBadge now flips below the badge when there isn't enough space above (TOOLTIP_HEIGHT_ESTIMATE_PX + gap + viewport padding). top anchors to r.bottom + gap on flip; transform switches from translateY(-100%) to translateY(0). Fixes Copilot review #3/#8. UI width: TOOLTIP_WIDTH_ESTIMATE_PX raised 240 -> 244 to match min-w-[220px] plus px-3 padding. Eliminates 4px overflow on narrow viewports. Fixes Copilot review #9. Comments: SSR mounted comment rewritten to describe the actual typeof window inline check and the eslint react-hooks/set-state-in-effect rule that blocks the useState+useEffect mounted pattern. Fixes Copilot review #10. Tests: SA-5/6 description corrected (re-test in same tick is strictly inside the 60s cooldown window; '30s later' wording removed). Fixes Copilot review #4. New tests/unit/api-sessions-route.test.mjs covers /api/sessions success enrichment with explicit secret-leak guard, orphan connectionId fallback, and simulated DB failure fallback. Fixes Copilot review #5. Version 3.8.2 -> 3.8.3. test:unit 2833/2833 PASS.
i1hwan
added a commit
that referenced
this pull request
May 7, 2026
* feat(dashboard): limits page polish + smart session affinity (4 issues) PR #25 (v3.1 + v7) merge follow-up. User feedback identified 4 polish items observed on /dashboard/limits after Docker deploy: 1. AutoRefreshControl raw <input type="checkbox"> + raw <select> rendered like unstyled HTML controls — replaced checkbox with Toggle from the shared design system, restyled the select wrapper to match the rest of the dashboard (rounded bg-surface border, focus ring, custom chevron). 2. RoutingBadge hover tooltip was clipped by the Account Model Quotas card's overflow-hidden ancestor (PR #25 introduced the clipping). Now uses createPortal(..., document.body) with position:fixed and viewport edge clamping, matching the codebase's existing portal pattern in providers/[id]/page.tsx. 3. Smart session affinity continuation viability (v8). 5min SESSION_AFFINITY_WINDOW_MS unchanged (matches Anthropic's prompt cache TTL — librarian-confirmed via docs.anthropic.com prompt-caching). Two new heuristic break rules layered on top of existing hard exclusions: - affinity_break_low_quota: bound's min known remaining < 15% AND a usable alt exists (Oracle: 5% hard-floor × 3 cushion). - affinity_break_p1_too_urgent: alt score >= bound × 3 AND >= bound + 250 absolute delta. The absolute delta neutralizes near-zero misfire (e.g. bound=20, alt=61 trips 3× alone but 41-point gap doesn't justify a cache write). 60s per-session cooldown gates oscillation; hard exclusions bypass cooldown. selectByEarliestResetFirst now scores upfront and passes the list to isAffinityValid (avoids double scoring). 4. SessionsTab account name. /api/sessions now joins active sessions against provider connections via getAccountDisplayName (graceful degradation: connection lookup failure returns sessions with accountName:null without breaking the API). UI replaces raw connectionId.slice(0,10) with the resolved account name + provider tag, falling back to "Account #xxxxxx" when name is unavailable. New i18n key 'usage.connectionFallback' added across 32 locales (en + ko hand-translated, 30 placeholder). Tests: 10 new SA-1..SA-10 RED tests cover smart affinity (low-Q break, heavy-gap break, no-alt edge, missing-track edge, cooldown, hard- exclusion bypass, score<=0 special case, backwards-compat third arg). Test cooldown reset hook (__resetAffinityHeuristicCooldownForTesting) prevents cross-test pollution. Full unit suite: 2830/2830 PASS (was 2821; +9 net). Verify: prettier ✓ eslint (errors 0, warnings unchanged from baseline) ✓ typecheck:core ✓ typecheck:noimplicit:core ✓ test:unit 2830/2830 ✓ docs:sync ✓. Plan: .sisyphus/plans/limits-dashboard-polish.md Oracle review: APPROVED with 7 revisions, all applied (bg_79458ad3) Momus reviews: v1 REJECT (QA executability) → v2 OKAY (bg_999bd379, bg_ebbedf5b) * fix(dashboard): restore reset countdown on session/weekly mini-bars + scoreAccount terminal guard QuotaVisualization MiniBar now renders the reset countdown next to the label (`⏱ 0h 34m` / `6d 11h`), mirroring the per-model bar format. PR #25 introduced the dual mini-bars but accidentally dropped the countdown for the overall Session/Weekly windows, leaving the row visually flat. scoreAccount() now excludes connections with terminal testStatus (expired / banned / credits_exhausted) at scoring time, not only inside isAffinityValid. Without this guard the fall-through path of selectByEarliestResetFirst could re-select a terminal connection if its cached quotas still looked healthy. Mirrors the auth.ts contract via the shared isTerminalConnectionStatus helper. Fixes a SA-7 CI flake on Linux runners (selected.id === 'bound' instead of 'alt') without changing local behavior. CHANGELOG documents the OAuth-lane prompt-cache reality: clients can request ttl: '1h' but the server downgrades to 5m on the OAuth path (observed in production responses; tracked upstream as anthropics/claude-code#46829), so SESSION_AFFINITY_WINDOW_MS = 5min is the maximum we can rely on, not a default. * fix(security,routing,ui): address Copilot PR #26 review (8 issues) Security: /api/sessions enrichment now uses listProviderConnectionMetadata() which selects only id/provider/name/display_name/email. Decryption of apiKey, accessToken, refreshToken, idToken via getProviderConnections() is no longer on this hot path. Fixes Copilot review #11. Routing memory: heuristicBreakHistory map now bounded by BOTH age (10min) AND hard size cap (1000). Re-set on insert moves the entry to the end of the insertion-ordered Map; size-cap eviction loop removes oldest entries when size exceeds 1000. >1000 distinct sessions inside the cooldown window can no longer grow the map without bound. Fixes Copilot review #1/#6. Routing perf: isAffinityValid heuristic-break #2 now finds the bound entry in scoredAlternatives and reuses its score instead of calling scoreAccount() again. Matches the 'score upfront once' contract of selectByEarliestResetFirst. Fixes Copilot review #2/#7. UI tooltip: RoutingBadge now flips below the badge when there isn't enough space above (TOOLTIP_HEIGHT_ESTIMATE_PX + gap + viewport padding). top anchors to r.bottom + gap on flip; transform switches from translateY(-100%) to translateY(0). Fixes Copilot review #3/#8. UI width: TOOLTIP_WIDTH_ESTIMATE_PX raised 240 -> 244 to match min-w-[220px] plus px-3 padding. Eliminates 4px overflow on narrow viewports. Fixes Copilot review #9. Comments: SSR mounted comment rewritten to describe the actual typeof window inline check and the eslint react-hooks/set-state-in-effect rule that blocks the useState+useEffect mounted pattern. Fixes Copilot review #10. Tests: SA-5/6 description corrected (re-test in same tick is strictly inside the 60s cooldown window; '30s later' wording removed). Fixes Copilot review #4. New tests/unit/api-sessions-route.test.mjs covers /api/sessions success enrichment with explicit secret-leak guard, orphan connectionId fallback, and simulated DB failure fallback. Fixes Copilot review #5. Version 3.8.2 -> 3.8.3. test:unit 2833/2833 PASS. * fix(routing,ui,api): address Copilot PR #26 round 3 review (3 issues, Oracle-verified) /api/sessions now fetches metadata only for active session connectionIds: listProviderConnectionMetadata(ids?) accepts an optional id filter, the route collects distinct connectionIds from getActiveSessions(), short-circuits to no DB call on empty, and otherwise issues a single WHERE id IN (?, ?, ...) with bound parameters. Endpoint work is now proportional to active sessions, not total connections. Fixes Copilot review #NEW-3. RoutingBadge useLayoutEffect cleanup no longer calls setCoords(null). The tooltip is already gated by 'open && coords' so the cleanup state update was unnecessary and risked extra renders / strict-mode noise. Oracle verified no stale-frame race (useLayoutEffect runs synchronously before paint, updateCoords sets fresh coords on reopen). Fixes Copilot review #NEW-1. formatCountdown extracted to ProviderLimits/utils.tsx and reused from both index.tsx (per-model bars) and QuotaVisualization.tsx (Session/Weekly mini bars). Behavior preserved: <24h => h h m m, >=24h => d d h h, invalid => null. Eliminates the drift risk between the two countdown call sites. Fixes Copilot review #NEW-2. Tests: +2 in tests/unit/api-sessions-route.test.mjs covering 'no active sessions skips DB query' and 'distinct connectionIds collapse to single bound parameter, unrelated secrets never leak'. 2835/2835 unit tests pass. Oracle pre-commit verification (ses_1fc62b147ffeUtgRRb2uvBKS4x): APPROVED all 3 fixes with high confidence. Version 3.8.3 -> 3.8.4. * fix(routing,ui): address Oracle audit + Copilot R4 review (2 defects + 2 nits) QuotaVisualization.pickWindow no longer absorbs per-model quotas (D1). The previous Pass 2 fallback matched any name starting with 'weekly ' or 'session ', so a connection that only had 'weekly Sonnet (7d)' (no canonical 'weekly' row) populated the overall mini-bar AND rendered as its own per-model bar simultaneously. Pass 2 removed; only canonical 'session'/'weekly' (with or without parenthesised window) match. New regression suite tests/unit/quota-visualization-pickwindow.test.mjs. RoutingBadge tooltip is now visible on viewports shorter than the tooltip estimate (D2). Previous fix flipped vertically when space-above ran out but never clamped into the viewport, so very short viewports or tooltips taller than viewH-2*pad still rendered partially off-screen. The new placement (1) prefers the side with more room (mirrors providers/[id]/page.tsx overlay rule), (2) clamps on-screen top with transform-aware math (above-anchored uses translateY(-100%), so we require top >= tooltipHeight + padding), (3) caps the rendered element with maxHeight: calc(100vh - 16px) + overflow: hidden so tooltips larger than the viewport degrade to a clipped frame, never off-screen. Fixes Copilot R4-1. isAffinityValid now guards against scoredAlternatives missing the bound entry (N1). Previously fell through to boundScore=0, letting any positive alt trip the urgent-break rule. Now returns { valid: true } on missing-bound. Production selectByEarliestResetFirst always includes bound, so this is a future-caller footgun guard. New SA-11 test. /api/sessions metadata-lookup catch now logs a sanitized warning instead of swallowing the error silently (N2). Format: '[sessions] connection metadata lookup failed for N ids: <message>'. Includes only the count and the error message text — no connectionIds, no SQL, no secrets. Oracle pre-commit verification (ses_1fc4cce1bffePsARTIZ6x3AxlY) returned NEEDS_REVISION on D1 and D2; both fixes applied per the audit's concrete revision instructions, plus the two NITs. Test count: 2841/2841 PASS (was 2835; +6 net). Version 3.8.4 -> 3.8.5.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Related Issues
Validation
npm run lintnpm run test:unitnpm run test:coverage>= 60%for statements, lines, functions, and branchesTests Added Or Updated
Coverage Notes
src/,open-sse/,electron/, orbin/, explain which tests cover the change.Reviewer Notes