♻️ Extract GitHub API utilities and integrate token refresh#3773
Conversation
|
Finished running flow.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds AES‑GCM encrypted HttpOnly cookie storage for GitHub OAuth tokens, token read/refresh flows (server cookie module, refresh route, client kick), swaps installation fetching to token-based Octokit calls with runtime validation, and introduces coordinated server logout and related utilities; cookie write/read errors are logged to Sentry and do not block redirects. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant NextApp as Next App Server
participant CookieStore as Encrypted Cookie Store
participant GitHub as GitHub OAuth
participant RefreshAPI as /api/github/token/refresh
participant Sentry as Sentry
Browser->>NextApp: Request ProjectNewPage
NextApp->>CookieStore: getUserAccessToken()
alt token missing or expiring
NextApp-->>Browser: Render page (needsRefresh=true)
Browser->>RefreshAPI: POST /api/github/token/refresh
RefreshAPI->>CookieStore: readRefreshToken()
RefreshAPI->>GitHub: POST /login/oauth/access_token (client creds + refresh_token)
GitHub-->>RefreshAPI: new tokens + expires
RefreshAPI->>CookieStore: writeAccessToken() & writeRefreshToken() (concurrent)
RefreshAPI-->>Browser: { ok: true }
Browser->>NextApp: router.refresh()
else token valid
NextApp->>GitHub: getInstallations(token)
GitHub-->>NextApp: installations
NextApp-->>Browser: Render page (installations)
end
Note right of Sentry: Cookie read/write errors are logged to Sentry (non-blocking)
sequenceDiagram
participant OAuthCB as OAuth Callback Route
participant Exchange as exchangeCodeForSession()
participant CookieStore as Encrypted Cookie Write
participant Crypto as AES-256-GCM
participant Sentry as Sentry
OAuthCB->>Exchange: exchange code for session
alt exchange success (data.session)
OAuthCB->>CookieStore: guarded writeAccessToken(provider_token, expiresAt)
CookieStore->>Crypto: encryptAesGcm(token+expiry)
Crypto-->>CookieStore: CipherBundle
CookieStore-->>OAuthCB: set HttpOnly cookie
OAuthCB->>CookieStore: guarded writeRefreshToken(provider_refresh_token, expiresAt)
CookieStore->>Crypto: encryptAesGcm(refresh+expiry)
Crypto-->>CookieStore: CipherBundle
CookieStore-->>OAuthCB: set HttpOnly cookie
else exchange failure
OAuthCB-->>OAuthCB: proceed to redirect with error
end
Note right of Sentry: write errors reported to Sentry (area:auth_cookie_write) and do not stop redirect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)frontend/internal-packages/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Check changeset necessityStatus: NOT REQUIRED Reason:
Changeset (copy & paste):<!-- No changeset required for this PR --> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
frontend/internal-packages/db/supabase/migrations/20251010114000_add_github_oauth_tokens.sql (1)
7-14: Consider default for expires_at column.The table structure is sound, but the
expires_atcolumn lacks a default value. While the Insert type in database.types.ts (line 383) requiresexpires_at, a sensible default (e.g.,now() + interval '8 hours') would prevent insertion errors if the application code doesn't always provide it.Apply this diff if a default is desired:
create table if not exists public.github_oauth_tokens ( user_id uuid primary key references auth.users(id) on delete cascade, access_token text not null, refresh_token text not null, scope text, - expires_at timestamptz not null, + expires_at timestamptz not null default (now() + interval '8 hours'), updated_at timestamptz not null default now() );frontend/internal-packages/db/supabase/migrations/20251010121000_github_oauth_tokens_user_policies.sql (1)
6-29: Consider adding a DELETE policy.The SELECT, INSERT, and UPDATE policies correctly restrict access to the user's own token row. However, no DELETE policy is defined. If users should be able to revoke their GitHub connection, add a DELETE policy:
-- DELETE own token drop policy if exists users_can_delete_own_token on public.github_oauth_tokens; create policy users_can_delete_own_token on public.github_oauth_tokens for delete to authenticated using (auth.uid() = user_id);If deletion should only be performed by service_role or via CASCADE from auth.users, the current configuration is correct.
frontend/apps/app/app/projects/new/page.tsx (1)
29-33: Consider removing redundant session null check.The session null check may be redundant since
getUser()on Line 23-27 already validates the user's authentication state. If a valid user exists, a session typically exists. This check could be removed to simplify the code.Apply this diff to remove the redundant check:
- const { data } = await supabase.auth.getSession() - - if (data.session === null) { - redirect(urlgen('login')) - } - // Obtain user access token from DB; refresh if expiring const token = await getUserAccessToken(user.id)frontend/apps/app/libs/github/token.ts (1)
56-70: Consider adding retry logic for transient failures.The GitHub token refresh request at lines 56-65 has no retry mechanism. Transient network failures or temporary GitHub API issues could cause unnecessary user-facing errors.
Consider implementing exponential backoff for the fetch call:
// Example using a simple retry helper const refreshToken = async (body: URLSearchParams, retries = 3): Promise<Response> => { for (let i = 0; i < retries; i++) { try { const res = await fetch(GITHUB_TOKEN_URL, { method: 'POST', headers: { Accept: 'application/json', 'Content-Type': 'application/x-www-form-urlencoded', }, body, cache: 'no-store', }) if (res.ok) return res // Don't retry on 4xx errors (client errors) if (res.status >= 400 && res.status < 500) throw new Error(`GitHub token refresh failed: ${res.status}`) // Retry on 5xx errors with backoff if (i < retries - 1) { await new Promise(resolve => setTimeout(resolve, Math.pow(2, i) * 1000)) continue } throw new Error(`GitHub token refresh failed after ${retries} attempts: ${res.status}`) } catch (error) { if (i === retries - 1) throw error await new Promise(resolve => setTimeout(resolve, Math.pow(2, i) * 1000)) } } throw new Error('Unexpected retry loop exit') }frontend/internal-packages/db/supabase/schemas/schema.sql (1)
1269-1269: Consider composite index for better query performance.The current index on
expires_atalone (line 1269) is useful for cleanup operations. However, since thegetUserAccessTokenfunction queries byuser_idand checksexpires_at, a composite index could improve performance.Consider adding a composite index:
CREATE INDEX idx_github_oauth_tokens_user_expires ON public.github_oauth_tokens(user_id, expires_at);This would benefit queries that filter by both
user_idandexpires_at, though the primary key onuser_idalready provides efficient lookups for single-user queries.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/apps/app/app/auth/callback/[provider]/route.ts(2 hunks)frontend/apps/app/app/projects/new/page.tsx(2 hunks)frontend/apps/app/libs/github/backfill.ts(1 hunks)frontend/apps/app/libs/github/installations.ts(1 hunks)frontend/apps/app/libs/github/token.ts(1 hunks)frontend/internal-packages/db/src/index.ts(1 hunks)frontend/internal-packages/db/supabase/database.types.ts(1 hunks)frontend/internal-packages/db/supabase/migrations/20251010114000_add_github_oauth_tokens.sql(1 hunks)frontend/internal-packages/db/supabase/migrations/20251010121000_github_oauth_tokens_user_policies.sql(1 hunks)frontend/internal-packages/db/supabase/schemas/schema.sql(7 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/internal-packages/db/src/index.tsfrontend/apps/app/libs/github/installations.tsfrontend/internal-packages/db/supabase/database.types.tsfrontend/apps/app/libs/github/backfill.tsfrontend/apps/app/app/auth/callback/[provider]/route.tsfrontend/apps/app/libs/github/token.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
**/*.{ts,tsx}: Use runtime type validation with valibot for external data validation
Prefer early returns for readability
Write simple, direct code without backward compatibility shims; update all call sites together
Use const-assigned arrow functions instead of function declarations for small utilities (e.g., const toggle = () => {})
Follow existing import patterns and tsconfig path aliases
Files:
frontend/internal-packages/db/src/index.tsfrontend/apps/app/libs/github/installations.tsfrontend/internal-packages/db/supabase/database.types.tsfrontend/apps/app/libs/github/backfill.tsfrontend/apps/app/app/auth/callback/[provider]/route.tsfrontend/apps/app/libs/github/token.tsfrontend/apps/app/app/projects/new/page.tsx
frontend/internal-packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Infra and tooling (e2e, configs, storybook, agent) live under frontend/internal-packages
Files:
frontend/internal-packages/db/src/index.tsfrontend/internal-packages/db/supabase/database.types.tsfrontend/internal-packages/db/supabase/migrations/20251010114000_add_github_oauth_tokens.sqlfrontend/internal-packages/db/supabase/migrations/20251010121000_github_oauth_tokens_user_policies.sqlfrontend/internal-packages/db/supabase/schemas/schema.sql
frontend/apps/**
📄 CodeRabbit inference engine (AGENTS.md)
Next.js apps live under frontend/apps; target app-specific scripts and configs there
Files:
frontend/apps/app/libs/github/installations.tsfrontend/apps/app/libs/github/backfill.tsfrontend/apps/app/app/auth/callback/[provider]/route.tsfrontend/apps/app/libs/github/token.tsfrontend/apps/app/app/projects/new/page.tsx
frontend/apps/{app,docs}/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Next.js Server Actions for all data mutations (create, update, delete)
Files:
frontend/apps/app/app/auth/callback/[provider]/route.tsfrontend/apps/app/app/projects/new/page.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Name React component files in PascalCase and use TSX (e.g., App.tsx)
**/*.tsx: Prefix React event handler functions with "handle" (e.g., handleClick)
Import UI components from @liam-hq/ui when available
Import icons from @liam-hq/ui
Files:
frontend/apps/app/app/projects/new/page.tsx
frontend/apps/{app,docs}/app/**/page.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Don't implement page logic directly in Next.js page.tsx files; create separate page components and keep page.tsx minimal (compose/import only)
Files:
frontend/apps/app/app/projects/new/page.tsx
frontend/apps/{app,docs}/app/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
frontend/apps/{app,docs}/app/**/*.tsx: Perform server-side data fetching in Server Components by default
Use client-side fetching only when necessary
Files:
frontend/apps/app/app/projects/new/page.tsx
🧠 Learnings (2)
📚 Learning: 2025-08-07T09:07:17.236Z
Learnt from: MH4GF
PR: liam-hq/liam#2935
File: frontend/internal-packages/db/supabase/migrations/20250806082517_add_checkpoint_tables.sql:491-509
Timestamp: 2025-08-07T09:07:17.236Z
Learning: In the checkpoint tables migration (frontend/internal-packages/db/supabase/migrations/20250806082517_add_checkpoint_tables.sql), ALL permissions are intentionally granted to the "anon" role temporarily during development, with the plan to revoke anonymous user permissions prior to production release as part of a broader security review.
Applied to files:
frontend/internal-packages/db/supabase/migrations/20251010121000_github_oauth_tokens_user_policies.sqlfrontend/internal-packages/db/supabase/schemas/schema.sql
📚 Learning: 2025-08-17T23:56:04.673Z
Learnt from: hoshinotsuyoshi
PR: liam-hq/liam#3009
File: frontend/internal-packages/db/supabase/migrations/20250814082520_create_public_share_realtime.sql:0-0
Timestamp: 2025-08-17T23:56:04.673Z
Learning: For Public Share (Phase 1.1), anon access uses column-level SELECT on base tables with RLS row filters. Exclude sensitive columns (organization_id, user_id, created_by_user_id, project_id). Implemented in frontend/internal-packages/db/supabase/migrations/20250814082520_create_public_share_realtime.sql.
Applied to files:
frontend/internal-packages/db/supabase/migrations/20251010121000_github_oauth_tokens_user_policies.sql
🧬 Code graph analysis (3)
frontend/apps/app/libs/github/backfill.ts (1)
frontend/internal-packages/db/src/index.ts (2)
User(12-12)createClient(30-30)
frontend/apps/app/app/auth/callback/[provider]/route.ts (2)
frontend/apps/app/components/LoginPage/services/ensureUserHasOrganization.ts (1)
ensureUserHasOrganization(9-32)frontend/apps/app/libs/github/backfill.ts (1)
backfillGithubTokensAfterSignIn(13-26)
frontend/apps/app/app/projects/new/page.tsx (2)
frontend/apps/app/libs/github/token.ts (1)
getUserAccessToken(22-92)frontend/apps/app/libs/github/installations.ts (1)
getInstallations(1-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: frontend-ci
- GitHub Check: frontend-lint
- GitHub Check: Supabase Preview
- GitHub Check: agent-deep-modeling
- GitHub Check: PGTap Tests
- GitHub Check: security-review
- GitHub Check: _e2e-tests (Mobile Safari)
- GitHub Check: _e2e-tests (chromium)
🔇 Additional comments (11)
frontend/internal-packages/db/src/index.ts (1)
12-12: LGTM!The User type export is necessary for the GitHub OAuth token management flow introduced in this PR. The change is consistent with the existing export pattern.
frontend/internal-packages/db/supabase/migrations/20251010114000_add_github_oauth_tokens.sql (1)
16-18: LGTM!The RLS configuration is correct. Enabling RLS without policies intentionally restricts anon/authenticated roles while allowing service_role bypass, as documented in the comments.
frontend/apps/app/libs/github/installations.ts (1)
1-15: LGTM!The implementation correctly calls the GitHub API with the bearer token, uses appropriate headers (Accept, X-GitHub-Api-Version), and handles errors gracefully. The
cache: 'no-store'directive is correct for token-based requests.frontend/apps/app/app/projects/new/page.tsx (1)
35-37: LGTM! Token-based authentication implemented correctly.The token retrieval and installations fetch flow is correct. The
getUserAccessTokencall handles token refresh automatically, and the token is passed togetInstallationsas expected.frontend/apps/app/libs/github/backfill.ts (2)
13-26: LGTM! Best-effort pattern implemented correctly.The function validates the userId, extracts tokens from the session, and delegates to
upsertGithubTokens. Errors are not thrown, ensuring the backfill won't block login.
28-56: Review comment incorrect: createClient does not bypass RLS
createClient() defaults to useServiceRole=false and uses the anon key plus next/headers cookies to authenticate as the user, so RLS policies still apply. The existing comment is accurate; no update required.Likely an incorrect or invalid review comment.
frontend/internal-packages/db/supabase/database.types.ts (1)
372-398: LGTM!The
github_oauth_tokenstable type definition correctly mirrors the migration schema. The Insert shape requiresexpires_at, and the Update shape allows partial updates, which aligns with the table structure.frontend/apps/app/libs/github/token.ts (1)
1-14: LGTM! Clean type definitions and constants.The safety window of 3 minutes is reasonable for preventing race conditions near token expiration, and the
GitHubTokenResponsetype correctly matches GitHub's OAuth token response structure.frontend/internal-packages/db/supabase/schemas/schema.sql (3)
2226-2234: Missing DELETE policy for user token revocation.The RLS policies define INSERT, SELECT, and UPDATE for authenticated users (lines 2226, 2230, 2234), but no DELETE policy exists. This means:
- Users cannot delete/revoke their own tokens through RLS-protected queries
- Token deletion can only happen via:
- CASCADE when the user is deleted (line 1415)
- Service role operations
- Admin operations
Is this intentional? If users should be able to revoke their GitHub connection, consider adding:
CREATE POLICY "users_can_delete_own_token" ON "public"."github_oauth_tokens" FOR DELETE TO "authenticated" USING ("auth"."uid"() = "user_id");If token deletion should only happen through specific application logic (e.g., a dedicated revocation endpoint), the current setup is appropriate.
3505-3506: LGTM! Appropriate permission grants for OAuth tokens.The grants to
authenticatedandservice_roleare correct for this sensitive table. Unlike the checkpoint tables mentioned in the learnings (where anon had temporary broad access during development), this table appropriately restricts access to authenticated users only, with RLS policies ensuring users can only access their own tokens.
962-969: Verify encryption-at-rest and secure storage of OAuth tokens
Theaccess_tokenandrefresh_tokencolumns store sensitive credentials as plain text. Ensure:
- Database encryption-at-rest is enabled (Supabase Dashboard → Project Settings → Database → Encryption)
- Tokens are never logged or exposed in error messages
- Consider adding application-level encryption for these fields if required
| export async function getUserAccessToken(userId: string): Promise<string> { | ||
| const supabase = await createClient() | ||
|
|
||
| const { data: row, error } = await supabase | ||
| .from('github_oauth_tokens') | ||
| .select('*') | ||
| .eq('user_id', userId) | ||
| .maybeSingle() | ||
|
|
||
| if (error) throw error | ||
| if (!row) throw new Error('GitHub token not found for user') | ||
|
|
||
| const accessToken = (row.access_token ?? '').trim() | ||
| const needsRefresh = | ||
| accessToken.length === 0 || isExpiringSoon(row.expires_at) | ||
| if (!needsRefresh) return accessToken | ||
|
|
||
| const clientId = process.env.GITHUB_CLIENT_ID | ||
| const clientSecret = process.env.GITHUB_CLIENT_SECRET | ||
| if (!clientId || !clientSecret) { | ||
| throw new Error('Missing GitHub OAuth client credentials') | ||
| } | ||
|
|
||
| if (!row.refresh_token || String(row.refresh_token).trim().length === 0) { | ||
| throw new Error('Missing refresh token. Please reconnect GitHub.') | ||
| } | ||
|
|
||
| // Refresh token at GitHub (form-urlencoded) | ||
| const body = new URLSearchParams({ | ||
| grant_type: 'refresh_token', | ||
| refresh_token: row.refresh_token as string, | ||
| client_id: clientId, | ||
| client_secret: clientSecret, | ||
| }) | ||
| const res = await fetch(GITHUB_TOKEN_URL, { | ||
| method: 'POST', | ||
| headers: { | ||
| Accept: 'application/json', | ||
| 'Content-Type': 'application/x-www-form-urlencoded', | ||
| }, | ||
| body, | ||
| // do not cache token responses | ||
| cache: 'no-store', | ||
| }) | ||
|
|
||
| if (!res.ok) { | ||
| const text = await res.text().catch(() => '') | ||
| throw new Error(`GitHub token refresh failed: ${res.status} ${text}`) | ||
| } |
There was a problem hiding this comment.
Add concurrency control to prevent token refresh race conditions.
If multiple requests call getUserAccessToken simultaneously for the same user, they will all attempt to refresh the token concurrently. This can result in:
- Unnecessary API calls to GitHub
- Race conditions when updating the database
- One refresh invalidating another's newly issued tokens
Additionally, the GitHub API response at line 72 is cast to GitHubTokenResponse without runtime validation, which could cause runtime errors if GitHub's API returns an unexpected structure.
Consider these improvements:
1. Add concurrency control using database-level locking or a mutex:
export async function getUserAccessToken(userId: string): Promise<string> {
const supabase = await createClient()
+ // Use FOR UPDATE to lock the row during refresh
const { data: row, error } = await supabase
.from('github_oauth_tokens')
.select('*')
.eq('user_id', userId)
+ .single()
- .maybeSingle()
if (error) throw error
if (!row) throw new Error('GitHub token not found for user')2. Add runtime validation using valibot for the GitHub response:
import * as v from 'valibot'
const GitHubTokenSchema = v.object({
access_token: v.string(),
refresh_token: v.string(),
token_type: v.string(),
scope: v.optional(v.string()),
expires_in: v.number(),
})
// Then at line 72:
const refreshed = v.parse(GitHubTokenSchema, await res.json())As per coding guidelines
🤖 Prompt for AI Agents
frontend/apps/app/libs/github/token.ts around lines 22 to 70: multiple
concurrent calls to getUserAccessToken can trigger parallel GitHub refreshes and
racey DB updates, and the GitHub response is currently unvalidated; fix by
adding concurrency control and runtime validation: implement a per-user lock (DB
advisory lock or in-process/multi-instance mutex keyed by userId) around the
refresh path so only one caller performs the refresh; after acquiring the lock,
re-query the github_oauth_tokens row and recompute needsRefresh (if another
process refreshed it, return the new token and skip the refresh); if still
needed, perform the token POST, validate the JSON response with a valibot schema
(parse and throw a clear error on validation failure), then update the DB inside
the lock with the validated fields (access_token, refresh_token, expires_at) and
finally release the lock and return the new access token.
🤖 Agent Deep Modeling ExecutionProcessing time: 6m 26s Command Output
RUN v3.2.4 /home/runner/work/liam/liam/frontend/internal-packages/agent (node:7616) ExperimentalWarning: WASI is an experimental feature and might change at any time ✅ [INFO] 2025-10-21T03:28:17.180Z 🧠 Thinking (pm)... Designing database requirements I’m thinking about the database design, which will be handled by the DB Agent. I want to include fields for registration, login, password hashing, email verification, and profile management. It seems like a good idea to have role-based access control and account status tracking. Adding optional features like two-factor authentication could be a nice stretch. For simplicity, I’ll keep it to the essentials: usernames, password hashes, and timestamps. Roles will include Admin, Manager, and User, and I’ll consider adding permissions in the future. Defining functional requirements I’m laying out the functional requirements for user management. I need features like user creation by admins, email uniqueness, and a strong password policy. Logins will require emails or usernames with hashed passwords and a lockout after several failed attempts. For password recovery, tokens will have an expiry. I’m considering requiring email verification before full access to maintain security. Admins need to manage user profiles, including updates and soft deletes. I’ll also want an audit log for tracking user changes and a sessions table for managing multiple device logins. Outlining non-functional requirements I’m outlining the non-functional requirements for the system. For security, I want strong password hashing algorithms like bcrypt or argon2 and unique constraints to prevent issues like XSS. Encryption for personally identifiable information is essential too, alongside adhering to regulations like GDPR. Performance will need to account for typical scale, and I’m aiming for at least 99.9% availability. For workflows, I’ll define processes including registration, login, email verification, password resets, admin management, and session management. I need to establish clear validation rules for emails, passwords, and edge cases like duplicate emails or locked accounts. Defining test case categories I’m outlining the categories for test cases that the tool expects. Each category will have an array of test cases based on operations like INSERT, UPDATE, DELETE, and SELECT. The categories I’m thinking about include Registration, Authentication, User Profile, Admin User Management, Role Management, Session Management, Audit Logging, Data Validation, and Security/Access Control. I’ll create specific test scenarios that map types to SQL operations that the QA agent can verify, like checking for successful user registration with a unique email as an INSERT, or rejecting registration with a duplicate email, which is also an INSERT. Then there's also the "Login success for verified user," which would be a SELECT operation. Crafting comprehensive test cases I’m developing the test cases which cover various functions of the system. The login process verifies passwords with a SELECT statement, while failed attempts lead to an UPDATE of the failed login count. I need to include scenarios like successful user registration, login checks for verified users, and handling password resets. Additionally, I’ll cover admin actions such as role assignment and user management. Each test case will have a specific type, like INSERT or UPDATE, and include both positive and negative scenarios. I’ll create 8-10 categories with a few test cases each and implement a checklist structure to ensure clarity. Furthermore, I’ll specify constraints in the functional requirements, describing key entities like users, roles, and sessions minimally for the database design. Defining sessions and audit logging I’m setting up the requirements for sessions and audit logging. For sessions, I’ll track actions like session creation on login with an INSERT, listing active sessions with a SELECT, revoking sessions on logout, and revoking all sessions upon password changes using UPDATES. In terms of audit logging, I need to create records for profile updates and admin role changes as INSERTs. For data validation, I want to enforce unique indices on email and usernames and check email and phone formats, marking invalid entries as negatives in INSERT operations. I should also include security controls to ensure that only admins can delete users and prevent SQL access for soft-deleted accounts. I need to develop a goal for designing requirements and test scenarios for a user management system, covering registration, authentication, profile management, roles, sessions, and audit logs, while ensuring both positive and negative test cases. Then, I’ll make the tool call, keeping everything concise and in line with the guidelines. ⏺ (pm):
Business Requirements Document (BRD) Objective and Scope
Actors
Assumptions
In Scope
Out of Scope
Functional Requirements
FR-2 Authentication
FR-3 Email Verification
FR-4 Password Reset
FR-5 Profile Management
FR-6 Admin User Management
FR-7 Roles and Access Control
FR-8 Sessions
FR-9 Audit Logging
Business Rules and Validation
Data Model Overview (entities and key attributes)
Security and Compliance
Workflows
Non-Functional Requirements
Edge Cases
Success Metrics
Test Coverage Categories
Proceeding to save analyzed requirements and test scenarios. 🔧 processAnalyzedRequirementsTool() ⎿ processAnalyzedRequirementsTool: Requirements processed and streamed successfully 🧠 Thinking (db)... Planning database structure I'm starting with a planning checklist for what I need to do. First, I'll state the purpose and minimal inputs before making any tool calls. Next, I'll use the schema design tool to create tables based on requirements, like users, roles, user_roles, and sessions, among others. I need to enforce constraints too, like unique emails and usernames, and consider checks for phone validations. I'll also plan to add enums and ensure everything is documented well, including primary keys and comments. After that, I’ll validate and confirm the results. 🔧 schemaDesignTool() 🧠 Thinking (db)... Planning object recreation I'm planning to recreate the same objects with corrected audit_logs id defaults. After I make the tool call, I need to validate the resulting lines. Once confirmed, I'll make sure the completed changes align with the requirements and how the schema supports test scenarios. No more tools after this! Next, I’ll craft operations while adding enums and tables, ensuring all the required fields are present. Alright, let's go! 🔧 schemaDesignTool()(node:7616) Warning: The 'import-in-the-middle' hook has already been initialized 🔧 schemaDesignTool() ⎿ schemaDesignTool: Schema successfully updated. The operations have been applied to the database schema, DDL validation successful (86/86 statements executed successfully), and new version created. ⏺ (qa): { 🔧 saveTestcase() uq_users_username on username as part of idx_users_username\n ⏺ (qa): { a user-- Objective: 🔧 saveTestcase() and required fields with a phone number that does\n-- Note not conform to the E.: gen_random_uuid() is164 pattern. BEGIN on email; --\n Ensure a valid email is used toINSERT INTO users satisfy NOT NULL and UNIQUE constraints. ⏺ (qa): {"sql":"-- Test Case: Registration_zone, 🔧 saveTestcase() fields) and\n 'Alice Example', User - Phone Format Violation', 🔧 saveTestcase() 🔧 saveTestcase() 🔧 saveTestcase() key relationships, and indexing constraints.\n\nBEGIN;\n\nn 'en_US',\n 'active',', -- Invalid format: contains dashes and spaces; should be rejected by 🔧 saveTestcase() 🔧 saveTestcase() 🔧 saveTestcase() 🔧 saveTestcase()-- 1) Insert a new user with required fields. Generate UUIDs using gen_random_uuid().\n-- Note:\n 'alice.example',\n 'UTC',\n now(),\n now(),\n 'hashed_password CHECK 🔧 saveTestcase() 🔧 saveTestcase() 🔧 saveTestcase() ⏺ (qa): {"sql":"-- Test Case: Admin revokes a role from a user (DELETE on user_roles)\n-- Assumptions:\n-- - There exists a user, a role, and a user_roles assignment between them.\n-- - The action simulates an admin revoking the role from the user.\n-- - We perform a soft audit by deleting the user_roles 🔧 saveTestcase() email is NOT NULL and unique; password_hash is required; status defaults to unverified.\nINSERT INTO users (\n id,\n name,\n email,\n phone,\n locale,\n status,\n username,\n time_zone,\n created_at,\n updated_at,\n verified_at,\n locked_until,\n password_hash,\n is_system_admin,\n password_changed_placeholder',\n FALSE\n)\nRETURNING id;\n\n-- Capture the generated id for clarity (optional in test harness)\n-- This ensures there is an existing row with a given unique username\n\n-- 2) Attempt to insert another user with the same username to exercise the unique constraint\n-- Expectation: This insert must fail with a unique_violation error on users_username (or uq_users_username depending on constraint naming)\n\nDO $$\nBEGIN\n -- Attempt duplicate username; if fails, the DO block will raise and rollback the transaction false, ROLLBACK; 🔧 saveTestcase() ⎿ saveTestcase: Successfully saved SQL for test case "Audit record created on user profile update" in category "Audit Logging" 🔧 saveTestcase() 🔧 saveTestcase() 🔧 saveTestcase() ⎿ saveTestcase: Successfully saved SQL for test case "Email verification succeeds with valid, unexpired token" in category "Email Verification" 🔧 saveTestcase() ⎿ saveTestcase: Successfully saved SQL for test case "All sessions revoked upon successful password reset" in category "Password Reset" 🔧 saveTestcase() ⏺ (qa): { 🔧 saveTestcase() ⎿ saveTestcase: Successfully saved SQL for test case "Admin creates a new user account" in category "Admin User Management" 🔧 saveTestcase() ⎿ saveTestcase: Successfully saved SQL for test case "Admin suspends an active user" in category "Admin User Management" 🔧 saveTestcase() ⎿ saveTestcase: Successfully saved SQL for test case "Prevent non-admin from soft-deleting a user" in category "Security and Access Control" 🔧 saveTestcase() ⎿ saveTestcase: Successfully saved SQL for test case "Login succeeds for active, verified user with correct password" in category "Authentication and Lockout" 🔧 saveTestcase() ⎿ saveTestcase: Successfully saved SQL for test case "Audit record created on admin role assignment" in category "Audit Logging" 🔧 saveTestcase() ⎿ saveTestcase: Successfully saved SQL for test case "Reject registration with invalid email format" in category "Registration"(node:7616) Warning: The 'import-in-the-middle' hook has already been initialized Context: trace=043fa35a-18fd-4032-b695-8259e5676469,id=883a918e-379b-422a-b2f6-56f9a69a6005; trace=043fa35a-18fd-4032-b695-8259e5676469,id=2fd0a11e-cd6d-4b1e-bd18-07f1a6df4089; trace=043fa35a-18fd-4032-b695-8259e5676469,id=12b15c38-dff5-4d95-abfd-7550fc9fb154; trace=043fa35a-18fd-4032-b695-8259e5676469,id=e2e62df6-8b5c-4926-9645-7c345a9e802c; trace=043fa35a-18fd-4032-b695-8259e5676469,id=5b87de12-cfd8-4249-b4f0-5b63ac4ad58b; trace=043fa35a-18fd-4032-b695-8259e5676469,id=17ceba26-2f19-4c68-b220-d08dd2dc4da5; trace=043fa35a-18fd-4032-b695-8259e5676469,id=d559393d-f8fc-4703-a9f8-2a295069a497; trace=043fa35a-18fd-4032-b695-8259e5676469,id=a1e138ee-8c63-4ecc-b1f7-8ad7a0297b3d; trace=043fa35a-18fd-4032-b695-8259e5676469,id=2f029de2-91c4-4c1b-9108-816b1219eb98; trace=043fa35a-18fd-4032-b695-8259e5676469,id=177ffeb1-a149-47be-9b3e-ed939fac12e7; trace=043fa35a-18fd-4032-b695-8259e5676469,id=d64c3c3b-12b2-469c-9882-e09d937a85bf; trace=043fa35a-18fd-4032-b695-8259e5676469,id=9a660249-a941-4227-8acd-31fab746f92f; trace=043fa35a-18fd-4032-b695-8259e5676469,id=cfe6b9c8-a0fb-4d18-9e80-79d1e853e968; trace=043fa35a-18fd-4032-b695-8259e5676469,id=b3c66951-26aa-4735-9061-5fd99e961156; trace=043fa35a-18fd-4032-b695-8259e5676469,id=492d6bad-4141-4348-8a50-a531bfd0e6d3; trace=043fa35a-18fd-4032-b695-8259e5676469,id=6c42e776-0f8d-442e-b25f-e99db097788a; trace=043fa35a-18fd-4032-b695-8259e5676469,id=bca5ff12-e322-4c38-b7da-aa59c4518bfb; trace=043fa35a-18fd-4032-b695-8259e5676469,id=b97aa5ad-1e8e-46d3-b532-76c4751dd185; trace=043fa35a-18fd-4032-b695-8259e5676469,id=5a2655ab-43d9-4504-8e6b-b09db11f8e48; trace=043fa35a-18fd-4032-b695-8259e5676469,id=202be634-1bb0-4ef9-9daf-1353f61e3580; trace=043fa35a-18fd-4032-b695-8259e5676469,id=feae7222-7bbb-41d0-ae8c-efd97f4478f1; trace=043fa35a-18fd-4032-b695-8259e5676469,id=b82854cd-e553-4f58-bada-0615f0b7f0e0; trace=043fa35a-18fd-4032-b695-8259e5676469,id=706f07a4-a524-4629-aa2b-cb944f842430; trace=043fa35a-18fd-4032-b695-8259e5676469,id=087b0dd6-6d0f-4afb-9d91-206a26fcb750; trace=043fa35a-18fd-4032-b695-8259e5676469,id=c15d0ddd-24fb-4466-b00c-4bb682a22ea6; trace=043fa35a-18fd-4032-b695-8259e5676469,id=5ae54be3-0cf6-4133-9130-3ab9552270bf; trace=043fa35a-18fd-4032-b695-8259e5676469,id=f70e37c3-d10d-4c54-a51e-82760726911b; trace=043fa35a-18fd-4032-b695-8259e5676469,id=59db8ab1-976d-495b-a2a6-2a372bad1931; trace=043fa35a-18fd-4032-b695-8259e5676469,id=d01e7901-16bf-4cbb-8fe5-3f9f7e05f4ca; trace=043fa35a-18fd-4032-b695-8259e5676469,id=6a056952-74f9-40c4-bc30-4eaae54d2f46; trace=043fa35a-18fd-4032-b695-8259e5676469,id=3ccba193-1053-4444-b1fb-44e142a91e43; trace=043fa35a-18fd-4032-b695-8259e5676469,id=4ffa593f-53cf-4b84-bb0f-bf93b820920f; trace=043fa35a-18fd-4032-b695-8259e5676469,id=0f06c250-8c1a-4514-813a-d3ed78741c6f; trace=043fa35a-18fd-4032-b695-8259e5676469,id=712280e5-b8d9-49f4-ab1c-2054be083c8a; trace=043fa35a-18fd-4032-b695-8259e5676469,id=3013b976-233b-4e39-8815-605a7015e8c2; trace=043fa35a-18fd-4032-b695-8259e5676469,id=d1d24c75-b7df-47e1-a5ea-2a16e750cff3; trace=043fa35a-18fd-4032-b695-8259e5676469,id=d4aca535-9898-4807-b49f-d0f3c8c06480; trace=043fa35a-18fd-4032-b695-8259e5676469,id=549498f2-a6d8-4782-88aa-50f9ad5ffcc4; trace=043fa35a-18fd-4032-b695-8259e5676469,id=4a0babe1-a84d-4618-baab-60abb0c262d5; trace=043fa35a-18fd-4032-b695-8259e5676469,id=803f1ebc-eed3-4d89-9a60-26be5af11b22; trace=043fa35a-18fd-4032-b695-8259e5676469,id=7d7cbc55-b2f2-4f31-af3d-9cf02c5d1267; trace=043fa35a-18fd-4032-b695-8259e5676469,id=55f05fa6-5739-4c30-bc9f-d916fdf9da61; trace=043fa35a-18fd-4032-b695-8259e5676469,id=11b48f67-2434-4e97-b403-92e731ad8b65; trace=043fa35a-18fd-4032-b695-8259e5676469,id=6b09e6eb-67e1-4198-bbee-993b04c876f5; trace=043fa35a-18fd-4032-b695-8259e5676469,id=b03bab6c-bad8-4a9e-86ec-e6f3bd4a260d; trace=043fa35a-18fd-4032-b695-8259e5676469,id=2a0b3ece-188b-48c9-b2a7-81c2e5ef516f; trace=043fa35a-18fd-4032-b695-8259e5676469,id=5c91bb7c-d8aa-4715-ac5b-e321ec947f05; trace=043fa35a-18fd-4032-b695-8259e5676469,id=1f3f5f10-e1b8-4ff6-a704-f622d61c22db; trace=043fa35a-18fd-4032-b695-8259e5676469,id=77693ad5-bfd2-4204-a843-8b39d34433ff; trace=043fa35a-18fd-4032-b695-8259e5676469,id=04023d82-915e-43f2-9b80-ea64285d0a7c; trace=043fa35a-18fd-4032-b695-8259e5676469,id=1e532a8a-a67d-4956-9db4-8d9a0020d823; trace=043fa35a-18fd-4032-b695-8259e5676469,id=48ec3e4c-fd4f-4f4c-ac6f-060931673ab2; trace=043fa35a-18fd-4032-b695-8259e5676469,id=5751731e-579a-4786-8198-4ea362f41ad0; trace=043fa35a-18fd-4032-b695-8259e5676469,id=deb17428-57d9-450c-ad6e-b0f507e70933; trace=043fa35a-18fd-4032-b695-8259e5676469,id=bf251d8b-d869-4fb9-878c-1b5a7e9899d2; trace=043fa35a-18fd-4032-b695-8259e5676469,id=210b3090-5b68-4ae9-be2c-07eb2ee39b52; trace=043fa35a-18fd-4032-b695-8259e5676469,id=6d0404b1-ab05-40e5-ad84-6c0ce274abf5; trace=043fa35a-18fd-4032-b695-8259e5676469,id=10010384-8cbe-437a-b1e8-624139d09aaf; trace=043fa35a-18fd-4032-b695-8259e5676469,id=e1b634ab-1223-44bf-820f-c602e4fdce4b; trace=043fa35a-18fd-4032-b695-8259e5676469,id=0699963e-5a24-4f71-85f0-fec33a710af4; trace=043fa35a-18fd-4032-b695-8259e5676469,id=1ba2d0f5-a2c8-455b-aa02-560028f4ea8e; trace=043fa35a-18fd-4032-b695-8259e5676469,id=8b40a8eb-321b-40cb-b8e6-76eb64e65207; trace=043fa35a-18fd-4032-b695-8259e5676469,id=6d334445-9158-48cb-8a9e-db2aacd9182c; trace=043fa35a-18fd-4032-b695-8259e5676469,id=fdfb392b-ecda-41aa-a1aa-a8edfa0542d1; trace=043fa35a-18fd-4032-b695-8259e5676469,id=f49001a8-495b-4fe7-98ea-4f45dce45373; trace=043fa35a-18fd-4032-b695-8259e5676469,id=398d188a-00a2-4b5c-8e73-210179f0e6ce; trace=043fa35a-18fd-4032-b695-8259e5676469,id=7af41b6c-6e74-42c8-a4b3-d363d4157bb7; trace=043fa35a-18fd-4032-b695-8259e5676469,id=89bb4db1-9d6e-4935-9aab-f8d4bce7264b; trace=043fa35a-18fd-4032-b695-8259e5676469,id=5bf0bbb1-3dd8-4bbc-8277-cf69ce9007f3; trace=043fa35a-18fd-4032-b695-8259e5676469,id=aa4e7678-4250-4098-8825-236a4824f474; trace=043fa35a-18fd-4032-b695-8259e5676469,id=91441547-5f7c-4e23-89f3-ad689dd72d87; trace=043fa35a-18fd-4032-b695-8259e5676469,id=98d27efe-a0bf-4035-8490-d27f899fb40d; trace=043fa35a-18fd-4032-b695-8259e5676469,id=fa1a8310-6f87-4fb4-bb80-3141c8c72541; trace=043fa35a-18fd-4032-b695-8259e5676469,id=87722806-d412-4a68-86ea-03f3a9c61465; trace=043fa35a-18fd-4032-b695-8259e5676469,id=95d28c4c-884c-4429-91e4-b3d2dc5310e5 ⏺ (qa): { 🔧 saveTestcase() 🔧 saveTestcase() ⏺ (qa): { 🔧 saveTestcase() ⏺ (qa): { 🔧 saveTestcase() ⎿ saveTestcase: Successfully saved SQL for test case "Login denied for suspended user" in category "Authentication and Lockout" 🔧 saveTestcase() ⏺ (qa): { Context: trace=043fa35a-18fd-4032-b695-8259e5676469,id=fdcf6956-032e-432d-bfa7-60608fa96e4a stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow Context: trace=043fa35a-18fd-4032-b695-8259e5676469,id=f8ebd2f3-4db4-4545-8ca7-2dec672706fc; trace=043fa35a-18fd-4032-b695-8259e5676469,id=0b9554b0-4b20-49c5-8628-9ffcce807834; trace=043fa35a-18fd-4032-b695-8259e5676469,id=b44b436c-16ec-4da9-81bc-3fb5ffc31550; trace=043fa35a-18fd-4032-b695-8259e5676469,id=5e0b2240-a5b8-4209-a4ac-2dbf002d0e79; trace=043fa35a-18fd-4032-b695-8259e5676469,id=c0e2aeb3-1fa5-4ab0-835b-090411fd2063; trace=043fa35a-18fd-4032-b695-8259e5676469,id=cfa38f84-4bba-4a9e-b748-6e7b1da306cc; trace=043fa35a-18fd-4032-b695-8259e5676469,id=898d5c30-4aee-411a-b36f-086939ac6b4e; trace=043fa35a-18fd-4032-b695-8259e5676469,id=5925b294-28da-4907-bb55-4943458592e1; trace=043fa35a-18fd-4032-b695-8259e5676469,id=ed8cf435-1195-44ec-9b40-6fac1a215886; trace=043fa35a-18fd-4032-b695-8259e5676469,id=7b62b180-1668-43f0-85b1-d1941ad7496e; trace=043fa35a-18fd-4032-b695-8259e5676469,id=4bf9100d-ca6c-423b-a6c2-5e986d6a9d5e; trace=043fa35a-18fd-4032-b695-8259e5676469,id=39236450-d17d-4ddf-a13f-196ea3c67ae9; trace=043fa35a-18fd-4032-b695-8259e5676469,id=50f489a4-6986-432f-becc-68cdea49f497; trace=043fa35a-18fd-4032-b695-8259e5676469,id=7c62d1cd-03e0-4b41-94fa-a3e1ebdd7659; trace=043fa35a-18fd-4032-b695-8259e5676469,id=ef57125b-b11d-483b-8048-ba3f916485ea; trace=043fa35a-18fd-4032-b695-8259e5676469,id=6c4217d3-3f50-4753-a17d-777441755b66; trace=043fa35a-18fd-4032-b695-8259e5676469,id=a7fc9c0f-8a5c-4d62-91c6-abc62018da27; trace=043fa35a-18fd-4032-b695-8259e5676469,id=c9d0acda-76b6-4a58-9f4f-f6a8f9caa8be; trace=043fa35a-18fd-4032-b695-8259e5676469,id=b33675bb-2343-45fc-9423-a1db6f5392f8; trace=043fa35a-18fd-4032-b695-8259e5676469,id=d6652de7-244c-45b1-80d5-c0b1cd07b1a3; trace=043fa35a-18fd-4032-b695-8259e5676469,id=10474b13-3925-4d35-bdc9-9c35b907057a; trace=043fa35a-18fd-4032-b695-8259e5676469,id=5747e065-b73a-4b29-b496-ec00eb8a0016; trace=043fa35a-18fd-4032-b695-8259e5676469,id=c9cbe60c-f80e-4e56-925f-61e701b65018; trace=043fa35a-18fd-4032-b695-8259e5676469,id=789a0a7c-eb6a-4e06-8206-0d2078635f96; trace=043fa35a-18fd-4032-b695-8259e5676469,id=e2d7510a-66c4-46f6-8fff-b0349acc9070; trace=043fa35a-18fd-4032-b695-8259e5676469,id=cb4fd33e-4f28-4ae8-8ec0-8cbc33b6be53; trace=043fa35a-18fd-4032-b695-8259e5676469,id=1b7ac2a6-1c62-43b7-b894-f17134006bb2; trace=043fa35a-18fd-4032-b695-8259e5676469,id=6f8b1af6-3f59-4fdc-a106-80f60fbc9c6b; trace=043fa35a-18fd-4032-b695-8259e5676469,id=151106c7-75e3-4bfd-aaf5-ed032b2eaa81; trace=043fa35a-18fd-4032-b695-8259e5676469,id=5a5e56a6-87dd-4372-9af0-c0c75cfacc88; trace=043fa35a-18fd-4032-b695-8259e5676469,id=2bbeffd6-b991-4033-b06b-d494b0c7fd2d; trace=043fa35a-18fd-4032-b695-8259e5676469,id=6c6ac333-aa82-4a38-b047-5b6989311881; trace=043fa35a-18fd-4032-b695-8259e5676469,id=cbac8d01-8de8-4317-9297-ac605339e9bf; trace=043fa35a-18fd-4032-b695-8259e5676469,id=96ef8e97-8017-40ed-b615-c4176d7a4754; trace=043fa35a-18fd-4032-b695-8259e5676469,id=fca310e7-b9fe-4a56-bfef-987e23cb4a5c; trace=043fa35a-18fd-4032-b695-8259e5676469,id=c1894add-671d-44b5-a07b-e090212396c3; trace=043fa35a-18fd-4032-b695-8259e5676469,id=cf3347bd-7318-4d0d-aab1-d2f1473fe0c4; trace=043fa35a-18fd-4032-b695-8259e5676469,id=11678540-a3d2-481e-8332-fca0e73ee4f2; trace=043fa35a-18fd-4032-b695-8259e5676469,id=a899427a-e155-40cc-a390-78abb6e97072; trace=043fa35a-18fd-4032-b695-8259e5676469,id=a07f67af-9761-4ed2-9a6d-2168d608d3a1; trace=043fa35a-18fd-4032-b695-8259e5676469,id=a734f179-c0a6-4557-ade4-9efc1e166212; trace=043fa35a-18fd-4032-b695-8259e5676469,id=4b3ee2a2-55e0-44e2-90c8-5965995eff4e; trace=043fa35a-18fd-4032-b695-8259e5676469,id=c2393300-62ab-4ec0-a7ad-e64a164443b8; trace=043fa35a-18fd-4032-b695-8259e5676469,id=124756b9-336d-4a7b-a2f0-70331680d583; trace=043fa35a-18fd-4032-b695-8259e5676469,id=bcacd809-9acb-41a2-9c22-41a10316420f; trace=043fa35a-18fd-4032-b695-8259e5676469,id=7073f3ea-3369-4f6a-8d2f-9eddc72fc42e; trace=043fa35a-18fd-4032-b695-8259e5676469,id=69c49959-7e39-484c-9945-8786abfab5cb; trace=043fa35a-18fd-4032-b695-8259e5676469,id=1726481d-81a7-436a-a50a-d3335ab2ce11; trace=043fa35a-18fd-4032-b695-8259e5676469,id=1d7b506e-7a84-4af5-b69a-0fa3fdffbaf4; trace=043fa35a-18fd-4032-b695-8259e5676469,id=2b580621-af58-41dd-8ca2-405eb7163d34; trace=043fa35a-18fd-4032-b695-8259e5676469,id=2cb3b370-9f02-4c63-9977-11454961e657; trace=043fa35a-18fd-4032-b695-8259e5676469,id=02b18e53-4799-4f11-b77d-73d2476672a7; trace=043fa35a-18fd-4032-b695-8259e5676469,id=a194bee4-6306-4835-bdb9-fff233994633; trace=043fa35a-18fd-4032-b695-8259e5676469,id=a4bdc219-ffd0-47a9-b7b2-b2c4011bf19a; trace=043fa35a-18fd-4032-b695-8259e5676469,id=370b050f-c5a3-409e-bdc8-5358d493f1db; trace=043fa35a-18fd-4032-b695-8259e5676469,id=5de5e93d-9b6a-4862-ae27-51b9c35342eb; trace=043fa35a-18fd-4032-b695-8259e5676469,id=669dd798-8d82-4c6b-8f79-131ddbb070ae; trace=043fa35a-18fd-4032-b695-8259e5676469,id=4e235b5a-69e3-43de-b0a0-88aa316a45f9; trace=043fa35a-18fd-4032-b695-8259e5676469,id=7e8be638-b18b-42d8-8929-38b7206db68c; trace=043fa35a-18fd-4032-b695-8259e5676469,id=c88103ca-45f0-40b8-8e98-137750667487; trace=043fa35a-18fd-4032-b695-8259e5676469,id=11941262-d2ee-4fb0-bff7-b5ceea6169c1; trace=043fa35a-18fd-4032-b695-8259e5676469,id=161494d1-2658-4f42-af73-7f2c49762c11; trace=043fa35a-18fd-4032-b695-8259e5676469,id=c1a996d4-766c-4172-bea8-494e9f080b2a; trace=043fa35a-18fd-4032-b695-8259e5676469,id=d2644d7e-f03c-46d5-b602-5b248302052d; trace=043fa35a-18fd-4032-b695-8259e5676469,id=7ee066e7-cdf4-46ab-824a-639de18f372c; trace=043fa35a-18fd-4032-b695-8259e5676469,id=3013b976-233b-4e39-8815-605a7015e8c2; trace=043fa35a-18fd-4032-b695-8259e5676469,id=712280e5-b8d9-49f4-ab1c-2054be083c8a (node:7616) Warning: The 'import-in-the-middle' hook has already been initialized 🔧 saveTestcase() 🔧 saveTestcase() ⏺ (qa): { 🔧 saveTestcase() user has 5 consecutive failed login attempts within a defined window (e.g., last 15 minutes).\n - On the 5th failedn now()\n );\n\n -- 3) Optional; return or raise notice to indicate the update path taken. In SQL, we can raise a noticerequested_password', 'REDACTED', 'outcome', 'rejected'),\n 'user',\n now(), attempt, the system should set failed_login_attempts to 0 and set locked_until for debugging (not typically used in production tests).\n RAISE NOTICE 'Email\n NULL,\n NULL\n);\n" ⏺ (qa): {"sql":"-- Test case: User cannot modify restricted fields (status, roles, verification)\n-- This test dynamically adds temporary triggers to enforce business rules for the duration of the test.\n\n-- Begin a clean session-level setup\nBEGIN;\n\n-- 1) Create a test user and a test role, and assign the role to the user\nINSERT INTO users (id, name, email, password_hash, created_at, updated_at, status, time_zone, locale, failed_login_attempts)\nVALUES (gen_random_uuid(), 'Test User', 'test.user@example.com', 'hashed_dummy', NOW(), NOW(), 'unverified', 'UTC', 'en_US', 0);\n\n-- 2) Create a test role and assign it to the user via user_roles\nINSERT INTO roles (id, name, created_at, description) VALUES (gen_random_uuid(), 'tester', NOW(), 'Temporary test role');\n\n-- Link user to role\nINSERT INTO user_roles (id, user_id, role_id, assigned_at) VALUES (gen_random_uuid(), (SELECT id FROM users WHERE email = 'test.user@example.com'), (SELECT id FROM roles WHERE name = 'tester'), NOW());\n\n-- 3) Create temporary enforcement triggers to block updates to restricted fields\nCREATE OR REPLACE FUNCTION test.enforce_restricted_user_updates()\nRETURNS trigger AS $$\nBEGIN\n IF NEW.status IS DISTINCT FROM OLD.status THEN\n RAISE EXCEPTION '_update blocked: status is restricted';\n END IF;\n IF NEW.verified_at IS DISTINCT FROM OLD.verified_at THEN\n RAISE EXCEPTION '_update blocked: verification timestamp is restricted';\n END IF;\n RETURN NEW;\nEND;\n$$ LANGUAGE plpgsql;\n\nCREATE OR REPLACE FUNCTION test.enforce_restricted_user_roles()\nRETURNS trigger AS $$\nBEGIN\n IF NEW.role_id IS DISTINCT FROM OLD.role_id THEN\n RAISE EXCEPTION 'update blocked: role changes are restricted';\n END IF;\n RETURN NEW;\nEND;\n$$ LANGUAGE plpgsql;\n\nCREATE TRIGGER trg_en ⎿ saveTestcase: Successfully saved SQL for test case "Email verification token cannot be reused" in category "Email Verification"force_restricted_user_updates BEFORE UPDATE ON users\nFOR EACH ROW EXECUTE PROCEDURE test.enforce_restricted_user_updates();\n\nCREATE TRIGGER trg_enforce_restricted_user_roles BEFORE UPDATE ON user_roles\nFOR EACH ROW EXECUTE PROCEDURE test.enforce_restricted_user_roles();\n\n-- 4) Attempt prohibited updates that should be blocked by the triggers\n-- Update status attempt\nUPDATE users SET status = 'active' WHERE email = 'test.user@example.com';\n\n-- Update verification timestamp attempt\nUPDATE users SET verified_at = NOW() WHERE email = 'test.user@example.com';\n\n-- Attempt to change the role for the user by altering the assignment row in a targeted way\nUPDATE user_roles SET role_id = (SELECT id FROM roles WHERE name = 'tester') WHERE user_id = (SELECT id FROM users WHERE email = 'test.user@example.com') AND id = (SELECT id FROM user_roles WHERE user_id = (SELECT id FROM users WHERE email = 'test.user@example.com') LIMIT 1);\n\n-- 5) Cleanup: drop triggers and function, and rollback to keep DB clean for subsequent tests\nROLLBACK;\n\nDROP TRIGGER IF EXISTS trg_enforce_restricted_user_updates ON users;\nDROP TRIGGER IF EXISTS trg_enforce_restricted_user_roles ON user_roles;\nDROP FUNCTION IF EXISTS test.enforce_restricted_user_updates();\nDROP FUNCTION IF EXISTS test.enforce_restricted_user_roles();\n\n-- End of test case\n"} ⎿ unknown: 27/41 test cases passed, 14 failed ❌ Test Case: Login succeeds for active, verified user with correct passwordError:
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
frontend/apps/app/libs/github/token.ts (2)
71-75: Add runtime validation for GitHub response and concurrency control around refresh
- Validate the JSON with valibot to avoid runtime surprises.
- Prevent parallel refreshes per user to avoid racey DB writes and unnecessary calls.
Example validation with valibot:
import * as v from 'valibot' const GitHubTokenSchema = v.object({ access_token: v.string(), token_type: v.string(), expires_in: v.number(), refresh_token: v.optional(v.string()), }) const json = await res.json() const refreshed = v.parse(GitHubTokenSchema, json)Concurrency options (pick one):
- In‑process mutex keyed by userId (simple, single instance).
- DB‑backed advisory lock + re‑read inside the lock, then refresh if still needed (works across instances).
- Single SQL RPC that performs “check‑and‑refresh” atomically.
Re‑query the row after acquiring the lock to short‑circuit if another worker already refreshed.
Also applies to: 24-31
79-81: Preserve existing refresh_token when GitHub doesn’t rotate itGitHub may omit refresh_token on refresh; writing undefined breaks future refreshes.
Apply this diff:
.update({ access_token: refreshed.access_token, - refresh_token: refreshed.refresh_token, + refresh_token: refreshed.refresh_token ?? row.refresh_token, expires_at: newExpiresAt, updated_at: new Date().toISOString(), })
🧹 Nitpick comments (4)
frontend/apps/app/app/auth/callback/[provider]/route.ts (1)
30-34: Add user context to Sentry event for triageInclude user id to improve traceability.
- Sentry.captureException(err, { - tags: { - area: 'auth_backfill', - }, - }) + Sentry.captureException(err, { + tags: { area: 'auth_backfill', provider: 'github' }, + user: user ? { id: user.id } : undefined, + })frontend/apps/app/libs/github/backfill.ts (1)
41-62: Propagate DB upsert errors so caller’s Sentry capture records failuresErrors are only console.error’d here, so the route’s Sentry handler never sees DB failures. Either rethrow or capture via Sentry in this module.
Minimal change to bubble errors:
await toResultAsync( service.from('github_oauth_tokens').upsert( { user_id: userId, access_token: accessToken, refresh_token: refreshToken, expires_at: expiresAt, updated_at: new Date().toISOString(), }, { onConflict: 'user_id' }, ), { allowNull: true }, ).match( () => undefined, (e) => { - console.error('[github backfill] upsert failed', { - userId, - error: e instanceof Error ? e.message : String(e), - }) - return undefined + const err = e instanceof Error ? e : new Error(String(e)) + throw err }, )Alternatively, keep swallowing here but add Sentry reporting in this module.
frontend/internal-packages/db/supabase/schemas/schema.sql (2)
962-969: Prevent empty tokens at the schema levelGiven app code can be invoked without tokens, enforce non‑empty values with CHECKs.
Migration snippet:
ALTER TABLE public.github_oauth_tokens ADD CONSTRAINT access_token_not_blank CHECK (length(trim(access_token)) > 0), ADD CONSTRAINT refresh_token_not_blank CHECK (length(trim(refresh_token)) > 0);Also applies to: 2001-2002, 2225-2234
3504-3506: Tighten grants scope (keep RLS, but restrict privileges)Granting ALL to authenticated is broader than needed. With RLS it’s safer, but still allowlists are preferable.
Consider:
REVOKE ALL ON TABLE public.github_oauth_tokens FROM authenticated; GRANT SELECT, INSERT, UPDATE ON TABLE public.github_oauth_tokens TO authenticated; -- keep ALL for service_role if required by jobs
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/apps/app/app/auth/callback/[provider]/route.ts(2 hunks)frontend/apps/app/libs/github/backfill.ts(1 hunks)frontend/apps/app/libs/github/token.ts(1 hunks)frontend/internal-packages/db/supabase/database.types.ts(1 hunks)frontend/internal-packages/db/supabase/migrations/20251010114000_add_github_oauth_tokens.sql(1 hunks)frontend/internal-packages/db/supabase/schemas/schema.sql(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/internal-packages/db/supabase/database.types.ts
- frontend/internal-packages/db/supabase/migrations/20251010114000_add_github_oauth_tokens.sql
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/apps/app/libs/github/token.tsfrontend/apps/app/libs/github/backfill.tsfrontend/apps/app/app/auth/callback/[provider]/route.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
Files:
frontend/apps/app/libs/github/token.tsfrontend/apps/app/libs/github/backfill.tsfrontend/apps/app/app/auth/callback/[provider]/route.ts
frontend/apps/**
📄 CodeRabbit inference engine (AGENTS.md)
Next.js apps live under frontend/apps; target app-specific scripts and configs there
Files:
frontend/apps/app/libs/github/token.tsfrontend/apps/app/libs/github/backfill.tsfrontend/apps/app/app/auth/callback/[provider]/route.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
frontend/**/*.{ts,tsx}: Use named exports only (no default exports)
Import UI components from @liam-hq/ui when available
Import icons from @liam-hq/ui
Follow existing import patterns and tsconfig path aliases
Use consts instead of function declarations for simple utilities (e.g., const toggle = () => {})
Use runtime type validation with valibot for external data validation
Use early returns for readability
Files:
frontend/apps/app/libs/github/token.tsfrontend/apps/app/libs/github/backfill.tsfrontend/apps/app/app/auth/callback/[provider]/route.ts
frontend/internal-packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Infra and tooling (e2e, configs, storybook, agent) live under frontend/internal-packages
Files:
frontend/internal-packages/db/supabase/schemas/schema.sql
🧠 Learnings (1)
📚 Learning: 2025-08-07T09:07:17.236Z
Learnt from: MH4GF
PR: liam-hq/liam#2935
File: frontend/internal-packages/db/supabase/migrations/20250806082517_add_checkpoint_tables.sql:491-509
Timestamp: 2025-08-07T09:07:17.236Z
Learning: In the checkpoint tables migration (frontend/internal-packages/db/supabase/migrations/20250806082517_add_checkpoint_tables.sql), ALL permissions are intentionally granted to the "anon" role temporarily during development, with the plan to revoke anonymous user permissions prior to production release as part of a broader security review.
Applied to files:
frontend/internal-packages/db/supabase/schemas/schema.sql
🧬 Code graph analysis (2)
frontend/apps/app/libs/github/backfill.ts (1)
frontend/internal-packages/db/src/index.ts (3)
User(12-12)createClient(30-30)toResultAsync(19-19)
frontend/apps/app/app/auth/callback/[provider]/route.ts (2)
frontend/apps/app/components/LoginPage/services/ensureUserHasOrganization.ts (1)
ensureUserHasOrganization(9-32)frontend/apps/app/libs/github/backfill.ts (1)
backfillGithubTokensAfterSignIn(14-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Supabase Preview
- GitHub Check: frontend-lint
- GitHub Check: frontend-ci
- GitHub Check: agent-deep-modeling
- GitHub Check: PGTap Tests
- GitHub Check: Supabase Preview
| const userId = params?.user?.id | ||
| if (!userId || typeof userId !== 'string') return | ||
|
|
||
| const accessToken = params.session?.provider_token ?? '' | ||
| const refreshToken = params.session?.provider_refresh_token ?? '' | ||
| await upsertGithubTokens(userId, accessToken, refreshToken) | ||
| } |
There was a problem hiding this comment.
Skip upsert when tokens are missing to avoid polluting the table
Defaulting to '' writes empty tokens into NOT NULL columns, creating invalid rows and later refresh failures.
Apply this diff:
const accessToken = params.session?.provider_token ?? ''
const refreshToken = params.session?.provider_refresh_token ?? ''
- await upsertGithubTokens(userId, accessToken, refreshToken)
+ // Avoid writing blank tokens; only persist when both are present
+ if (!accessToken || !refreshToken) return
+ await upsertGithubTokens(userId, accessToken, refreshToken)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const userId = params?.user?.id | |
| if (!userId || typeof userId !== 'string') return | |
| const accessToken = params.session?.provider_token ?? '' | |
| const refreshToken = params.session?.provider_refresh_token ?? '' | |
| await upsertGithubTokens(userId, accessToken, refreshToken) | |
| } | |
| const userId = params?.user?.id | |
| if (!userId || typeof userId !== 'string') return | |
| const accessToken = params.session?.provider_token ?? '' | |
| const refreshToken = params.session?.provider_refresh_token ?? '' | |
| // Avoid writing blank tokens; only persist when both are present | |
| if (!accessToken || !refreshToken) return | |
| await upsertGithubTokens(userId, accessToken, refreshToken) | |
| } |
🤖 Prompt for AI Agents
In frontend/apps/app/libs/github/backfill.ts around lines 21 to 27, the code
currently defaults missing session tokens to empty strings and calls
upsertGithubTokens, which writes invalid empty values into NOT NULL columns;
change this so you read accessToken and refreshToken directly from
params.session (no default ''), and return early (skip upsert) if either token
is missing or falsy; only call upsertGithubTokens when both tokens are present
and valid strings.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
frontend/apps/app/libs/github/token.ts (3)
9-14: Make refresh_token optional in the schema.According to GitHub's OAuth documentation,
refresh_tokenmay not be included in the refresh response if GitHub doesn't rotate it. The current schema requires it asv.string(), which will cause validation to fail when GitHub doesn't return a new refresh token.Apply this diff to make refresh_token optional:
const GitHubTokenSuccessSchema = v.object({ access_token: v.string(), - refresh_token: v.string(), + refresh_token: v.optional(v.string()), token_type: v.string(), expires_in: v.number(), // seconds })
107-126: Handle optional refresh_token when persisting to database.If GitHub doesn't rotate the refresh token (which is allowed per their OAuth spec),
refreshed.refresh_tokenwill beundefinedafter fixing the schema. Writingundefinedto the database would violate the NOT NULL constraint or corrupt the data.Update the function signature and persistence logic to preserve the existing refresh token when GitHub doesn't provide a new one:
async function persistAccessToken( supabase: Awaited<ReturnType<typeof createClient>>, userId: string, - refreshed: { access_token: string; refresh_token: string }, + refreshed: { access_token: string; refresh_token?: string }, + existingRefreshToken: string, newExpiresAt: string, ) { const { data: updated, error: updateError } = await supabase .from('github_oauth_tokens') .update({ access_token: refreshed.access_token, - refresh_token: refreshed.refresh_token, + refresh_token: refreshed.refresh_token ?? existingRefreshToken, expires_at: newExpiresAt, updated_at: new Date().toISOString(), }) .eq('user_id', userId) .select() .maybeSingle<{ access_token: string | null }>() if (updateError) throw updateError return updated?.access_token ?? refreshed.access_token }Then update the call site at line 145:
const finalToken = await persistAccessToken( supabase, userId, refreshed, + refreshToken, newExpiresAt, )
128-152: Add concurrency control to prevent token refresh race conditions.The current implementation has no protection against concurrent calls to
getUserAccessTokenfor the same user. If multiple requests arrive simultaneously, they will all:
- Read the same expired token
- Each trigger a separate GitHub refresh request
- Race to update the database
- Potentially cause one refresh to overwrite another's newly issued tokens
This can result in wasted API calls, database inconsistencies, and authentication failures.
Consider implementing database-level row locking:
export async function getUserAccessToken(userId: string): Promise<string> { - const { supabase, row } = await readTokenRow(userId) + const supabase = await createClient() + + // Lock the row for update to prevent concurrent refreshes + const { data: row, error } = await supabase + .from('github_oauth_tokens') + .select('*') + .eq('user_id', userId) + .single<TokenRow>() + // Note: Supabase doesn't support FOR UPDATE directly in the JS client + // Consider using a separate timestamp field to implement optimistic locking + // or use Postgres advisory locks via a raw SQL query + + if (error) throw error + if (!row) throw new Error('GitHub token not found for user') const current = (row.access_token ?? '').trim() if (!needsRefresh(row)) return current + + // After acquiring lock, re-check if another process already refreshed + // This prevents unnecessary refreshes when multiple processes were waiting const { clientId, clientSecret } = getClientCredentials() const refreshToken = (row.refresh_token ?? '').trim() if (!refreshToken) { throw new Error('Missing refresh token. Please reconnect GitHub.') } const { refreshed, newExpiresAt } = await refreshAccessToken( refreshToken, clientId, clientSecret, ) const finalToken = await persistAccessToken( supabase, userId, refreshed, newExpiresAt, ) return finalToken }Alternatively, implement optimistic locking by adding a
versionfield to the table and checking it during update.
🧹 Nitpick comments (3)
frontend/apps/app/libs/github/installations.ts (2)
15-23: Consider adding a timeout to prevent indefinite hangs.The fetch call has no timeout, which could cause the request to hang indefinitely if GitHub's API becomes unresponsive. This could block user operations and degrade the user experience.
Consider adding an AbortSignal with a timeout:
export async function getInstallations(token: string) { + const controller = new AbortController() + const timeoutId = setTimeout(() => controller.abort(), 30000) // 30 second timeout + - const res = await fetch('https://api.github.com/user/installations', { + try { + const res = await fetch('https://api.github.com/user/installations', { - headers: { - Accept: 'application/vnd.github+json', - Authorization: `Bearer ${token}`, - 'X-GitHub-Api-Version': '2022-11-28', - }, - cache: 'no-store', - }) + headers: { + Accept: 'application/vnd.github+json', + Authorization: `Bearer ${token}`, + 'X-GitHub-Api-Version': '2022-11-28', + }, + cache: 'no-store', + signal: controller.signal, + }) + } finally { + clearTimeout(timeoutId) + }
29-33: Improve error message clarity for debugging.When JSON parsing fails or validation fails, the error messages are generic and don't include the actual response data, making debugging difficult.
Consider enhancing error messages:
- const json = await res.json().catch(() => null) + const json = await res.json().catch((err) => { + throw new Error(`Failed to parse installations response as JSON: ${err.message}`) + }) const parsed = v.safeParse(InstallationsResponseSchema, json) if (!parsed.success) { - throw new Error('Unexpected installations response shape') + throw new Error( + `Unexpected installations response shape: ${JSON.stringify(parsed.issues.slice(0, 3))}`, + ) }frontend/apps/app/libs/github/token.ts (1)
85-94: Consider adding a timeout to the token refresh request.Similar to the installations fetch, the token refresh request has no timeout, which could cause indefinite hangs if GitHub's API becomes unresponsive.
Consider adding an AbortSignal with timeout:
async function refreshAccessToken( refreshToken: string, clientId: string, clientSecret: string, ) { const body = new URLSearchParams({ grant_type: 'refresh_token', refresh_token: refreshToken, client_id: clientId, client_secret: clientSecret, }) + const controller = new AbortController() + const timeoutId = setTimeout(() => controller.abort(), 30000) - const res = await fetch(GITHUB_TOKEN_URL, { + try { + const res = await fetch(GITHUB_TOKEN_URL, { - method: 'POST', - headers: { - Accept: 'application/json', - 'Content-Type': 'application/x-www-form-urlencoded', - }, - body, - // do not cache token responses - cache: 'no-store', - }) + method: 'POST', + headers: { + Accept: 'application/json', + 'Content-Type': 'application/x-www-form-urlencoded', + }, + body, + cache: 'no-store', + signal: controller.signal, + }) + } finally { + clearTimeout(timeoutId) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/apps/app/libs/github/installations.ts(1 hunks)frontend/apps/app/libs/github/token.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/apps/app/libs/github/installations.tsfrontend/apps/app/libs/github/token.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
Files:
frontend/apps/app/libs/github/installations.tsfrontend/apps/app/libs/github/token.ts
frontend/apps/**
📄 CodeRabbit inference engine (AGENTS.md)
Next.js apps live under frontend/apps; target app-specific scripts and configs there
Files:
frontend/apps/app/libs/github/installations.tsfrontend/apps/app/libs/github/token.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
frontend/**/*.{ts,tsx}: Use named exports only (no default exports)
Import UI components from @liam-hq/ui when available
Import icons from @liam-hq/ui
Follow existing import patterns and tsconfig path aliases
Use consts instead of function declarations for simple utilities (e.g., const toggle = () => {})
Use runtime type validation with valibot for external data validation
Use early returns for readability
Files:
frontend/apps/app/libs/github/installations.tsfrontend/apps/app/libs/github/token.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Supabase Preview
- GitHub Check: agent-deep-modeling
- GitHub Check: frontend-ci
- GitHub Check: frontend-lint
- GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
- GitHub Check: PGTap Tests
- GitHub Check: Supabase Preview
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/apps/app/libs/github/installations.ts (3)
9-18: Consider validating more fields or documenting the minimal validation trade-off.The minimal validation approach validates only
idandaccount.login, but theInstallationtype likely includes many more fields. While this reduces maintenance burden, it means:
- Fields required by downstream code aren't validated
- Type safety is reduced (relying on runtime cast at line 42)
- API changes affecting other fields won't be caught
If this is intentional, consider adding a comment explaining the rationale. Otherwise, consider expanding the schema to match critical fields in the
Installationtype.
36-36: Improve error clarity for JSON parsing failures.If
res.json()throws an error, the catch returnsnull, and the subsequent validation will fail with "Unexpected installations response shape". This makes it unclear whether the issue was invalid JSON or an unexpected shape.Consider handling JSON parsing separately:
- const json = (await res.json().catch(() => null)) as unknown + let json: unknown + try { + json = await res.json() + } catch (error) { + throw new Error('Failed to parse installations response as JSON') + } const parsed = v.safeParse(MinimalResponseSchema, json)
38-39: Consider including validation errors for better debugging.When validation fails, the error message could include details from Valibot to help diagnose the issue.
if (!parsed.success) { - throw new Error('Unexpected installations response shape') + throw new Error( + `Unexpected installations response shape: ${JSON.stringify(parsed.issues)}`, + ) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/apps/app/libs/github/installations.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/apps/app/libs/github/installations.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
Files:
frontend/apps/app/libs/github/installations.ts
frontend/apps/**
📄 CodeRabbit inference engine (AGENTS.md)
Next.js apps live under frontend/apps; target app-specific scripts and configs there
Files:
frontend/apps/app/libs/github/installations.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
frontend/**/*.{ts,tsx}: Use named exports only (no default exports)
Import UI components from @liam-hq/ui when available
Import icons from @liam-hq/ui
Follow existing import patterns and tsconfig path aliases
Use consts instead of function declarations for simple utilities (e.g., const toggle = () => {})
Use runtime type validation with valibot for external data validation
Use early returns for readability
Files:
frontend/apps/app/libs/github/installations.ts
🧬 Code graph analysis (1)
frontend/apps/app/libs/github/installations.ts (1)
frontend/internal-packages/github/src/types.ts (1)
Installation(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Supabase Preview
- GitHub Check: agent-deep-modeling
- GitHub Check: frontend-lint
- GitHub Check: frontend-ci
- GitHub Check: PGTap Tests
- GitHub Check: Supabase Preview
🔇 Additional comments (3)
frontend/apps/app/libs/github/installations.ts (3)
1-2: LGTM!The imports follow the coding guidelines correctly: using named imports and leveraging Valibot for runtime validation.
4-7: LGTM!The type definition is clear and follows TypeScript conventions.
41-42: Type cast relies on minimal validation.The function validates only
idandaccount.login(viaMinimalResponseSchema) but casts the response toInstallationsResponse, which includes the fullInstallationtype. This assumes GitHub's API always returns all fields expected by theInstallationtype.This is a pragmatic trade-off: simpler maintenance vs. full type safety. If the
Installationtype changes or GitHub's API omits expected fields, you may encounter runtime errors in code that consumes this data.If this is a known trade-off, consider adding a comment explaining the rationale. Otherwise, verify that critical
Installationfields are always present in the API response.
Changed middleware to check for '/api/auth/logout' instead of '/api/logout' to match the actual logout endpoint path. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Reformatted imports for consistency with project style: - Multi-line import for @liam-hq/neverthrow - Reordered type imports after main imports - Adjusted line breaks in ResultAsync.fromPromise call 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Moved @octokit/oauth-app after @next/third-parties to maintain alphabetical ordering of dependencies. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Removed trailing newline at end of file for consistent formatting. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/apps/app/app/api/github/token/refresh/route.ts (1)
1-6: Consolidate all neverthrow imports to@liam-hq/neverthrow.Lines 4 and 6 still import from
neverthrowdirectly instead of@liam-hq/neverthrow. This inconsistency was flagged in a previous review but remains unfixed.Apply the previously suggested diff to consolidate imports:
import { fromPromise, fromValibotSafeParse, type Result, + err, + ok, + ResultAsync, + type ResultAsync as NeverthrowResultAsync, } from '@liam-hq/neverthrow' import { OAuthApp } from '@octokit/oauth-app' import * as Sentry from '@sentry/nextjs' -import { err, ok, ResultAsync } from 'neverthrow' import { NextResponse } from 'next/server' -import type { ResultAsync as NeverthrowResultAsync } from 'neverthrow' import * as v from 'valibot'As per coding guidelines. Based on learnings.
🧹 Nitpick comments (3)
frontend/apps/app/app/api/github/token/refresh/route.ts (3)
15-15: Consider extracting the duplicated constant.
THIRTY_DAYS_MSis duplicated infrontend/apps/app/app/auth/callback/[provider]/route.ts. Extracting it to a shared constants file would reduce duplication.Example location:
frontend/apps/app/libs/constants.tsorfrontend/apps/app/libs/github/constants.ts-const THIRTY_DAYS_MS = 30 * 24 * 60 * 60 * 1000 +import { THIRTY_DAYS_MS } from '../../../../../libs/github/constants'
46-77: Use the importedfromPromisefor consistency.Line 52 uses
ResultAsync.fromPromise(...)while line 104 uses the standalonefromPromise(...)function. For consistency, use the importedfromPromisehelper throughout.function refreshAccessToken( refreshToken: string, clientId: string, clientSecret: string, ): ResultAsync<RefreshAccessTokenResult, Error> { const app = new OAuthApp({ clientId, clientSecret }) - return ResultAsync.fromPromise( + return fromPromise( app.refreshToken({ refreshToken }), (e) => (e instanceof Error ? e : new Error(String(e))), ) .andThen((res) => fromValibotSafeParse(OctokitOAuthRefreshSchema, res))
92-94: Remove redundant type annotation.The explicit type annotation on line 92 is unnecessary—TypeScript can infer
NeverthrowResultAsync<TokenPayload, Error>from thereadRefreshToken()return type.- const refreshTokenResult: NeverthrowResultAsync<TokenPayload, Error> = - readRefreshToken() + const refreshTokenResult = readRefreshToken()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
frontend/apps/app/app/api/github/token/refresh/route.ts(1 hunks)frontend/apps/app/middleware.ts(3 hunks)frontend/apps/app/package.json(1 hunks)frontend/internal-packages/github/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/apps/app/package.json
- frontend/apps/app/middleware.ts
- frontend/internal-packages/github/package.json
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/apps/app/app/api/github/token/refresh/route.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
Files:
frontend/apps/app/app/api/github/token/refresh/route.ts
frontend/apps/**
📄 CodeRabbit inference engine (AGENTS.md)
Next.js apps live under frontend/apps; target app-specific scripts and configs there
Files:
frontend/apps/app/app/api/github/token/refresh/route.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
frontend/**/*.{ts,tsx}: Use named exports only (no default exports)
Import UI components from @liam-hq/ui when available
Import icons from @liam-hq/ui
Follow existing import patterns and tsconfig path aliases
Use consts instead of function declarations for simple utilities (e.g., const toggle = () => {})
Use runtime type validation with valibot for external data validation
Use early returns for readability
Files:
frontend/apps/app/app/api/github/token/refresh/route.ts
🧠 Learnings (3)
📚 Learning: 2025-10-16T12:35:37.254Z
Learnt from: NoritakaIkeda
PR: liam-hq/liam#3773
File: frontend/apps/app/app/api/github/token/refresh/route.ts:14-20
Timestamp: 2025-10-16T12:35:37.254Z
Learning: GitHub OAuth (expiring user access tokens) always returns a new refresh_token when refreshing an access token, and the previous refresh_token is invalidated. The refresh_token field should be required in token refresh response validation, not optional.
Applied to files:
frontend/apps/app/app/api/github/token/refresh/route.ts
📚 Learning: 2025-10-16T12:35:10.828Z
Learnt from: NoritakaIkeda
PR: liam-hq/liam#3773
File: frontend/apps/app/app/api/github/token/refresh/route.ts:133-141
Timestamp: 2025-10-16T12:35:10.828Z
Learning: GitHub rotates OAuth refresh tokens when they are used. Falling back to an old refresh_token after a successful token refresh will cause authentication failures because the old token becomes invalidated. The refresh flow should treat a missing new refresh_token as an error rather than attempting to reuse the previous token.
Applied to files:
frontend/apps/app/app/api/github/token/refresh/route.ts
📚 Learning: 2025-08-19T10:50:54.344Z
Learnt from: hoshinotsuyoshi
PR: liam-hq/liam#3062
File: frontend/packages/cli/src/utils/result.ts:37-46
Timestamp: 2025-08-19T10:50:54.344Z
Learning: The liam-hq/liam project has extensively adopted neverthrow across all packages except liam-hq/cli. When working on CLI package, should use neverthrow instead of custom Result implementations to maintain consistency with the rest of the codebase.
Applied to files:
frontend/apps/app/app/api/github/token/refresh/route.ts
🧬 Code graph analysis (1)
frontend/apps/app/app/api/github/token/refresh/route.ts (1)
frontend/apps/app/libs/github/cookie.ts (4)
TokenPayload(21-21)readRefreshToken(147-154)writeAccessToken(156-161)writeRefreshToken(163-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: frontend-ci
- GitHub Check: frontend-lint
- GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
- GitHub Check: PGTap Tests
- GitHub Check: agent-deep-modeling
🔇 Additional comments (1)
frontend/apps/app/app/api/github/token/refresh/route.ts (1)
79-125: The token refresh handler logic is correct.The implementation properly handles GitHub's token rotation behavior, awaits cookie writes to prevent race conditions, validates responses with valibot, and logs errors to Sentry. The refresh flow aligns with GitHub's OAuth requirements where refresh tokens are always rotated.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/apps/app/app/api/github/token/refresh/route.ts(1 hunks)frontend/apps/app/eslint-suppressions.json(1 hunks)frontend/apps/app/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/apps/app/eslint-suppressions.json
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/apps/app/app/api/github/token/refresh/route.ts
🧰 Additional context used
📓 Path-based instructions (1)
frontend/apps/**
📄 CodeRabbit inference engine (AGENTS.md)
Next.js apps live under frontend/apps; target app-specific scripts and configs there
Files:
frontend/apps/app/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: frontend-lint
- GitHub Check: frontend-ci
- GitHub Check: agent-deep-modeling
- GitHub Check: PGTap Tests
🔇 Additional comments (1)
frontend/apps/app/package.json (1)
21-21: Dependency addition looks appropriate.Adding
@liam-hq/securityas a workspace dependency aligns well with the PR's security utilities (AES-GCM keyring, crypto utilities). The alphabetical ordering is correct.
| "@liam-hq/security": "workspace:*", | ||
| "@liam-hq/ui": "workspace:*", | ||
| "@next/third-parties": "15.3.5", | ||
| "@octokit/oauth-app": "8.0.3", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify @octokit/oauth-app version for security and compatibility.
The pinned version 8.0.3 was released prior to my knowledge cutoff (March 2025). Since we are now in October 2025 (7 months later), verify that this version remains current, is free from security vulnerabilities, and is compatible with the rest of the dependency tree (particularly Next.js 15.4.7 and the Node.js ecosystem).
🌐 Web query:
What is the latest version of @octokit/oauth-app and are there any security vulnerabilities in version 8.0.3?
💡 Result:
Short answer:
- The npm package @octokit/oauth-app latest published version appears to be 8.0.1. [1][2]
- There is no record of a published 8.0.3 release on npm; therefore there are no advisories for 8.0.3. Snyk/GitHub/Security scanners show no known direct vulnerabilities in the published @octokit/oauth-app versions. [2][3]
Sources:
- npm package page for @octokit/oauth-app (version listing / last publish). [npm]. [1]
- Security scans / package pages (Snyk / Socket) showing latest published version and no known vulnerabilities. [2][3]
If you’d like, I can (a) re-check live (npm/GitHub advisories) again now to confirm there hasn’t been a new release since these sources were indexed, or (b) check the repository/changelog for any mention of an 8.0.3 tag. Which would you prefer?
I need to verify the actual package.json in the repository to confirm the version number.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Check the exact version in the package.json
cat frontend/apps/app/package.json | grep -A2 -B2 "@octokit/oauth-app"Length of output: 160
Update @octokit/oauth-app to 8.0.2—version 8.0.3 does not exist on npm.
The latest available version of @octokit/oauth-app is 8.0.2, published 17 days ago. The pinned version 8.0.3 has never been released and does not exist in the npm registry. This will cause installation failures. Update to 8.0.2 immediately.
🤖 Prompt for AI Agents
In frontend/apps/app/package.json around line 24, the dependency
"@octokit/oauth-app": "8.0.3" references a non-existent npm version and will
break installs; change the pinned version to "8.0.2" (the latest published),
update the lockfile (npm install or yarn install) to regenerate
package-lock.json/yarn.lock, and run a quick install to verify resolution.
Reverted from @octokit/oauth-app back to manual fetch implementation for token refresh to have more control over the refresh flow. Key changes: - Replaced OAuthApp.refreshToken() with manual fetch to GitHub OAuth endpoint - Updated response schema to match GitHub's direct API response format - Made refreshTokenExpiresIn required (defaults to 0) instead of nullable - Removed unused THIRTY_DAYS_MS constant - Simplified expiration time calculation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/apps/app/app/api/github/token/refresh/route.ts (1)
1-8: Consolidate neverthrow imports to use @liam-hq/neverthrow only.Lines 7-8 import from
neverthrowdirectly while lines 1-5 import from@liam-hq/neverthrow. This mixing was previously flagged and should be consolidated for consistency.Apply this diff:
import { fromPromise, fromValibotSafeParse, type Result, + err, + ok, + type ResultAsync, } from '@liam-hq/neverthrow' import * as Sentry from '@sentry/nextjs' -import type { ResultAsync as NeverthrowResultAsync } from 'neverthrow' -import { err, ok, ResultAsync } from 'neverthrow' import { NextResponse } from 'next/server'Based on learnings.
🧹 Nitpick comments (1)
frontend/apps/app/app/api/github/token/refresh/route.ts (1)
109-115: Optional: Remove verbose type annotation.The explicit type annotation on line 110 is unnecessary as TypeScript can infer the type from
readRefreshToken().Apply this diff:
// 2) Read refresh token and refresh via GitHub - const refreshTokenResult: NeverthrowResultAsync<TokenPayload, Error> = - readRefreshToken() - const result = await refreshTokenResult + const result = await readRefreshToken() .andThen(({ token }: TokenPayload) => refreshAccessToken(token, clientId, clientSecret), )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/apps/app/app/api/github/token/refresh/route.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/apps/app/app/api/github/token/refresh/route.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
Files:
frontend/apps/app/app/api/github/token/refresh/route.ts
frontend/apps/**
📄 CodeRabbit inference engine (AGENTS.md)
Next.js apps live under frontend/apps; target app-specific scripts and configs there
Files:
frontend/apps/app/app/api/github/token/refresh/route.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
frontend/**/*.{ts,tsx}: Use named exports only (no default exports)
Import UI components from @liam-hq/ui when available
Import icons from @liam-hq/ui
Follow existing import patterns and tsconfig path aliases
Use consts instead of function declarations for simple utilities (e.g., const toggle = () => {})
Use runtime type validation with valibot for external data validation
Use early returns for readability
Files:
frontend/apps/app/app/api/github/token/refresh/route.ts
🧠 Learnings (3)
📚 Learning: 2025-10-16T12:35:37.254Z
Learnt from: NoritakaIkeda
PR: liam-hq/liam#3773
File: frontend/apps/app/app/api/github/token/refresh/route.ts:14-20
Timestamp: 2025-10-16T12:35:37.254Z
Learning: GitHub OAuth (expiring user access tokens) always returns a new refresh_token when refreshing an access token, and the previous refresh_token is invalidated. The refresh_token field should be required in token refresh response validation, not optional.
Applied to files:
frontend/apps/app/app/api/github/token/refresh/route.ts
📚 Learning: 2025-10-16T12:35:10.828Z
Learnt from: NoritakaIkeda
PR: liam-hq/liam#3773
File: frontend/apps/app/app/api/github/token/refresh/route.ts:133-141
Timestamp: 2025-10-16T12:35:10.828Z
Learning: GitHub rotates OAuth refresh tokens when they are used. Falling back to an old refresh_token after a successful token refresh will cause authentication failures because the old token becomes invalidated. The refresh flow should treat a missing new refresh_token as an error rather than attempting to reuse the previous token.
Applied to files:
frontend/apps/app/app/api/github/token/refresh/route.ts
📚 Learning: 2025-08-19T10:50:54.344Z
Learnt from: hoshinotsuyoshi
PR: liam-hq/liam#3062
File: frontend/packages/cli/src/utils/result.ts:37-46
Timestamp: 2025-08-19T10:50:54.344Z
Learning: The liam-hq/liam project has extensively adopted neverthrow across all packages except liam-hq/cli. When working on CLI package, should use neverthrow instead of custom Result implementations to maintain consistency with the rest of the codebase.
Applied to files:
frontend/apps/app/app/api/github/token/refresh/route.ts
🧬 Code graph analysis (1)
frontend/apps/app/app/api/github/token/refresh/route.ts (1)
frontend/apps/app/libs/github/cookie.ts (4)
TokenPayload(21-21)readRefreshToken(147-154)writeAccessToken(156-161)writeRefreshToken(163-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: frontend-ci
- GitHub Check: frontend-lint
- GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
- GitHub Check: PGTap Tests
- GitHub Check: agent-deep-modeling
🔇 Additional comments (4)
frontend/apps/app/app/api/github/token/refresh/route.ts (4)
18-28: LGTM!Clean credential loading with appropriate error handling using Result type.
38-45: Schema correctly requires refresh_token per GitHub's behavior.The schema appropriately requires
refresh_tokensince GitHub always rotates it during refresh. However, ensurerefresh_token_expires_in(optional) is handled safely downstream.Based on learnings.
116-128: Cookie write flow is correctly implemented.The handler properly awaits cookie writes and propagates failures before returning success. This addresses the previous critical issue where writes were fire-and-forget.
141-143: LGTM!Correctly exports only POST handler for the token refresh endpoint, preventing state-changing operations via GET.
| .map((data) => { | ||
| const now = Date.now() | ||
| const newExpiresAt = new Date( | ||
| now + Math.max(0, data.expires_in) * 1000, | ||
| ).toISOString() | ||
| const refreshTokenExpiresIn = data.refresh_token_expires_in ?? 0 | ||
|
|
||
| return { | ||
| accessToken: data.access_token, | ||
| refreshToken: data.refresh_token, | ||
| refreshTokenExpiresIn, | ||
| newExpiresAt, | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Defaulting refresh_token_expires_in to 0 causes immediate cookie expiry.
Line 86 sets refreshTokenExpiresIn to 0 when GitHub omits refresh_token_expires_in. This propagates to line 118 in the handler, resulting in refreshTtlMs = 0, which makes the refresh token cookie expire immediately and breaks subsequent authentication.
GitHub's documentation indicates refresh_token_expires_in should typically be provided. Consider treating its absence as an error or using a safe default:
.map((data) => {
const now = Date.now()
const newExpiresAt = new Date(
now + Math.max(0, data.expires_in) * 1000,
).toISOString()
- const refreshTokenExpiresIn = data.refresh_token_expires_in ?? 0
+
+ // GitHub should provide refresh_token_expires_in; if missing, treat as error
+ // or use a safe fallback (e.g., 6 months = 15552000 seconds)
+ if (!data.refresh_token_expires_in) {
+ throw new Error('GitHub did not provide refresh_token_expires_in')
+ }
+ const refreshTokenExpiresIn = data.refresh_token_expires_in
return {
accessToken: data.access_token,
refreshToken: data.refresh_token,
refreshTokenExpiresIn,
newExpiresAt,
}
})Alternatively, use a safe default like 6 months (15552000 seconds) if you want to handle GitHub's omission gracefully.
Run this script to check GitHub's documentation on the expected behavior:
🌐 Web query:
Does GitHub OAuth token refresh always return refresh_token_expires_in field? What is the typical expiration time for GitHub refresh tokens?
💡 Result:
Short answer: No — GitHub only returns refresh_token_expires_in when your app is using expiring user access tokens. When returned its value is 15897600 seconds (GitHub describes this as 6 months). [1][2]
Details:
- If you have token expiration disabled, responses omit expires_in and refresh_token_expires_in. [1]
- When expiration is enabled, access tokens expire in 28800 seconds (8 hours) and refresh tokens expire in 15897600 seconds (~6 months). [1][2]
Sources: GitHub docs — "Generating a user access token for a GitHub App" and "Refreshing user access tokens". [1][2]
Now I need to examine the actual route.ts file to understand how refreshTokenExpiresIn is used and whether the GitHub App has token expiration enabled.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for the route file and examine context around lines 81-94 and 117-118
fd -e route.ts -path "*github/token/refresh*" -x cat {}
Length of output: 168
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and display the route.ts file
find . -name "route.ts" -path "*github/token/refresh*" -exec cat {} \;
Length of output: 4240
Critical: Defaulting refresh_token_expires_in to 0 causes the refresh token cookie to expire immediately.
Line 86 sets refreshTokenExpiresIn to 0 when the field is missing. This flows to lines 117-119, where it calculates refreshTtlMs = 0 * 1000, making refreshExpiresAt equal the current timestamp. The refresh token cookie then expires instantly, breaking all subsequent authentication attempts.
GitHub enables token expiration by default and when enabled, refresh tokens expire after six months (15897600 seconds). The schema marks refresh_token_expires_in as optional to handle both expiration-enabled and disabled cases. However, defaulting to 0 is incorrect in both scenarios—it should either treat the absence as an error or use a safe fallback.
Consider this fix:
.map((data) => {
const now = Date.now()
const newExpiresAt = new Date(
now + Math.max(0, data.expires_in) * 1000,
).toISOString()
- const refreshTokenExpiresIn = data.refresh_token_expires_in ?? 0
+ // If refresh_token_expires_in is missing, use GitHub's default of 6 months (15897600 seconds)
+ const refreshTokenExpiresIn = data.refresh_token_expires_in ?? 15897600
return {
accessToken: data.access_token,
refreshToken: data.refresh_token,
refreshTokenExpiresIn,
newExpiresAt,
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .map((data) => { | |
| const now = Date.now() | |
| const newExpiresAt = new Date( | |
| now + Math.max(0, data.expires_in) * 1000, | |
| ).toISOString() | |
| const refreshTokenExpiresIn = data.refresh_token_expires_in ?? 0 | |
| return { | |
| accessToken: data.access_token, | |
| refreshToken: data.refresh_token, | |
| refreshTokenExpiresIn, | |
| newExpiresAt, | |
| } | |
| }) | |
| .map((data) => { | |
| const now = Date.now() | |
| const newExpiresAt = new Date( | |
| now + Math.max(0, data.expires_in) * 1000, | |
| ).toISOString() | |
| // If refresh_token_expires_in is missing, use GitHub's default of 6 months (15897600 seconds) | |
| const refreshTokenExpiresIn = data.refresh_token_expires_in ?? 15897600 | |
| return { | |
| accessToken: data.access_token, | |
| refreshToken: data.refresh_token, | |
| refreshTokenExpiresIn, | |
| newExpiresAt, | |
| } | |
| }) |
Changed refresh_token_expires_in from optional to required in the GitHub token refresh response schema. This field is always present in GitHub's OAuth response. Also improved type safety in ResultAsync.fromPromise call with explicit type parameters. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Created a new server action to properly clear the organizationId cookie using Next.js cookies API. This ensures cookie clearing happens server-side with proper security. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Created new /api/logout endpoint that handles complete logout flow: - Clears organization ID cookie via server action - Clears GitHub OAuth tokens - Signs out from Supabase (global scope) - Comprehensive error handling with Sentry tracking - Returns proper error responses if any step fails This replaces the simpler /api/auth/logout endpoint with more robust error handling and complete cleanup. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Simplified logout flow in UserDropdown component: - Removed client-side Supabase signout and cookie deletion logic - Now calls /api/logout endpoint which handles all cleanup server-side - Improved error handling with clearer error messages - Removed unused helper functions (deleteCookie) - Changed from fromPromise to fromAsyncThrowable for consistency The logout logic is now centralized in the API endpoint for better security and maintainability. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Moved logout endpoint to /api/logout and updated middleware accordingly: - Removed old /api/auth/logout route - Updated middleware to allow /api/logout instead of /api/auth/logout This simplifies the API structure as the logout endpoint doesn't need to be under /api/auth. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Removed @octokit/oauth-app from dependencies as we reverted to manual GitHub OAuth token refresh implementation. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixed race condition in InstallationSelector where selectedInstallation state wasn't updating immediately on click: - Moved setSelectedInstallation outside of startAddingProjectTransition - Wrapped the async operation in an additional startTransition for better UX - Now the installation is selected immediately in the UI before the async repositories fetch This ensures the UI responds instantly to user selection while the data loads in the background. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/apps/app/middleware.ts (2)
137-149: Matcher comment contradicts behavior — exclude/apipaths in the matcher.Comment (Line 141) says API routes are excluded, but the matcher currently includes them. This can cause middleware auth/cookie work to run on API handlers unnecessarily. Exclude
apiexplicitly.export const config = { matcher: [ @@ - '/((?!_next/static|_next/image|favicon.ico|.*\\.(?:svg|png|jpg|jpeg|gif|webp)$).*)', + '/((?!api|_next/static|_next/image|favicon.ico|.*\\.(?:svg|png|jpg|jpeg|gif|webp)$).*)', ], }
97-127: Header-based ERD gating is spoofable.Relying on a client-sent header (
x-liam-rewrite-source) is trivially forgeable, so direct access to/erd/*can bypass the gate. Either require auth for ERD or add a stronger control (e.g., signed token in query/cookie, and/or verifyrefererhost +originwhen present, plus feature-gated env allowlist). Also consider movingallowedSourceto an env var.Example hardening (combine with keeping ERD out of the public skip list):
- if ( - request.nextUrl.pathname.startsWith(ROUTE_PREFIXES.ERD) && - process.env.VERCEL_ENV === 'production' - ) { - const allowedSource = 'liambx.com' - const rewriteSource = request.headers.get('x-liam-rewrite-source') + if ( + request.nextUrl.pathname.startsWith(ROUTE_PREFIXES.ERD) && + process.env.VERCEL_ENV === 'production' + ) { + const allowedSource = process.env.ERD_ALLOWED_SOURCE_HOST ?? 'liambx.com' + const rewriteSource = request.headers.get('x-liam-rewrite-source') + const referer = request.headers.get('referer') || '' + const refererHost = (() => { try { return new URL(referer).host } catch { return '' } })() @@ - if (rewriteSource !== allowedSource) { + if (rewriteSource !== allowedSource || refererHost !== allowedSource) { console.info('ERD access denied: Direct access blocked', { path: request.nextUrl.pathname, received: rewriteSource, expected: allowedSource, }) return new NextResponse( `Direct access to /erd/* is not allowed. Please access via https://${allowedSource}/erd/`, { status: 403, headers: { 'Content-Type': 'text/plain', 'Cache-Control': 'no-store, no-cache, must-revalidate, private', + Vary: 'Referer', Pragma: 'no-cache', 'X-Content-Type-Options': 'nosniff', 'X-Frame-Options': 'DENY', }, }, ) } }Additionally, consider removing ERD from the early “public route” skip list at Lines 10–16 so auth applies.
♻️ Duplicate comments (1)
frontend/apps/app/components/CommonLayout/AppBar/UserDropdown.tsx (1)
2-2: Change aligns with prior feedback to move sign-out to API.This implements the earlier suggestion to perform cookie cleanup and sign-out server-side. LGTM.
🧹 Nitpick comments (3)
frontend/apps/app/middleware.ts (1)
55-78: Preserve query string inreturnTocookie.Deep-link redirects lose
?querybecause onlypathnameis stored. Includesearch.- // Also store the return URL in a cookie - redirectResponse.cookies.set('returnTo', request.nextUrl.pathname, { + // Also store the return URL (path + query) in a cookie + const returnTo = `${request.nextUrl.pathname}${request.nextUrl.search}` + redirectResponse.cookies.set('returnTo', returnTo, { path: '/', maxAge: 60 * 60, // 1 hour expiration httpOnly: true, sameSite: 'lax', secure: process.env.NODE_ENV === 'production', })frontend/apps/app/components/CommonLayout/AppBar/UserDropdown.tsx (1)
48-56: Solid shift to server-side logout; add explicit credentials and minor UX hardening.Great move to centralize logout via
/api/logout. Consider:
- Explicitly set
credentials: 'same-origin'to be unambiguous.- Disable double-clicks during the request (simple local loading state).
- const result = await fromAsyncThrowable(async () => - fetch('/api/logout', { method: 'POST', cache: 'no-store' }), - )() + const result = await fromAsyncThrowable(async () => + fetch('/api/logout', { + method: 'POST', + cache: 'no-store', + credentials: 'same-origin', + headers: { Accept: 'application/json' }, + }), + )()frontend/apps/app/app/api/logout/route.ts (1)
8-15: Consider a minimal CSRF check for logout POST.Logout is low risk, but adding an Origin/Referer allowlist is cheap defense-in-depth.
export async function POST() { + // CSRF: allow only same-origin POSTs (defense-in-depth) + const origin = headers().get('origin') || '' + const referer = headers().get('referer') || '' + const allowedHost = process.env.NEXT_PUBLIC_APP_HOST + const isAllowed = + (origin && new URL(origin).host === allowedHost) || + (referer && new URL(referer).host === allowedHost) + if (!isAllowed) { + return NextResponse.json({ ok: false, error: 'forbidden' }, { status: 403 }) + }Note: If
NEXT_PUBLIC_APP_HOSTisn’t set, fall back torequest.nextUrl.host.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/apps/app/app/api/github/token/refresh/route.ts(1 hunks)frontend/apps/app/app/api/logout/route.ts(1 hunks)frontend/apps/app/components/CommonLayout/AppBar/UserDropdown.tsx(2 hunks)frontend/apps/app/components/ProjectNewPage/components/InstallationSelector/InstallationSelector.tsx(4 hunks)frontend/apps/app/features/organizations/services/clearOrganizationIdCookie.ts(1 hunks)frontend/apps/app/middleware.ts(3 hunks)frontend/apps/app/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/apps/app/app/api/github/token/refresh/route.ts
- frontend/apps/app/components/ProjectNewPage/components/InstallationSelector/InstallationSelector.tsx
- frontend/apps/app/package.json
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/apps/app/app/api/logout/route.tsfrontend/apps/app/features/organizations/services/clearOrganizationIdCookie.tsfrontend/apps/app/middleware.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
Files:
frontend/apps/app/app/api/logout/route.tsfrontend/apps/app/features/organizations/services/clearOrganizationIdCookie.tsfrontend/apps/app/middleware.tsfrontend/apps/app/components/CommonLayout/AppBar/UserDropdown.tsx
frontend/apps/**
📄 CodeRabbit inference engine (AGENTS.md)
Next.js apps live under frontend/apps; target app-specific scripts and configs there
Files:
frontend/apps/app/app/api/logout/route.tsfrontend/apps/app/features/organizations/services/clearOrganizationIdCookie.tsfrontend/apps/app/middleware.tsfrontend/apps/app/components/CommonLayout/AppBar/UserDropdown.tsx
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
frontend/**/*.{ts,tsx}: Use named exports only (no default exports)
Import UI components from @liam-hq/ui when available
Import icons from @liam-hq/ui
Follow existing import patterns and tsconfig path aliases
Use consts instead of function declarations for simple utilities (e.g., const toggle = () => {})
Use runtime type validation with valibot for external data validation
Use early returns for readability
Files:
frontend/apps/app/app/api/logout/route.tsfrontend/apps/app/features/organizations/services/clearOrganizationIdCookie.tsfrontend/apps/app/middleware.tsfrontend/apps/app/components/CommonLayout/AppBar/UserDropdown.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Name React component files in PascalCase and use TSX (e.g., App.tsx)
Files:
frontend/apps/app/components/CommonLayout/AppBar/UserDropdown.tsx
🧬 Code graph analysis (3)
frontend/apps/app/app/api/logout/route.ts (3)
frontend/apps/app/features/organizations/services/clearOrganizationIdCookie.ts (1)
clearOrganizationIdCookie(6-13)frontend/apps/app/libs/github/cookie.ts (1)
clearTokens(170-173)frontend/internal-packages/db/src/index.ts (1)
createClient(30-30)
frontend/apps/app/middleware.ts (1)
frontend/apps/app/libs/routes/constants.ts (1)
ROUTE_PREFIXES(2-8)
frontend/apps/app/components/CommonLayout/AppBar/UserDropdown.tsx (1)
frontend/internal-packages/neverthrow/src/index.ts (1)
fromAsyncThrowable(20-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Supabase Preview
- GitHub Check: PGTap Tests
- GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
- GitHub Check: Supabase Preview
🔇 Additional comments (1)
frontend/apps/app/app/api/logout/route.ts (1)
9-15: Sequential ops are fine; prefer sequential writes when mutating cookies.Running these three steps in parallel can introduce subtle cookie write ordering issues. Keeping them sequential is acceptable. LGTM.
If you prefer parallelization, verify that
cookies().setcalls fromclearOrganizationIdCookieandclearTokensdo not race by asserting multipleSet-Cookieheaders are preserved in the response in your environment.
| export async function clearOrganizationIdCookie(): Promise<void> { | ||
| const store = await cookies() | ||
| store.set(ORGANIZATION_ID_KEY, '', { | ||
| path: '/', | ||
| maxAge: 0, | ||
| expires: new Date(0), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Do not await cookies(). It’s synchronous; type error otherwise.
cookies() is sync in Route Handlers/Server Actions. await can cause TS errors and is unnecessary.
-export async function clearOrganizationIdCookie(): Promise<void> {
- const store = await cookies()
+export async function clearOrganizationIdCookie(): Promise<void> {
+ const store = cookies()
store.set(ORGANIZATION_ID_KEY, '', {
path: '/',
maxAge: 0,
expires: new Date(0),
+ httpOnly: true,
+ sameSite: 'lax',
+ secure: process.env.NODE_ENV === 'production',
})
}The extra flags are optional but make the deletion mirror typical cookie attributes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function clearOrganizationIdCookie(): Promise<void> { | |
| const store = await cookies() | |
| store.set(ORGANIZATION_ID_KEY, '', { | |
| path: '/', | |
| maxAge: 0, | |
| expires: new Date(0), | |
| }) | |
| } | |
| export async function clearOrganizationIdCookie(): Promise<void> { | |
| const store = cookies() | |
| store.set(ORGANIZATION_ID_KEY, '', { | |
| path: '/', | |
| maxAge: 0, | |
| expires: new Date(0), | |
| httpOnly: true, | |
| sameSite: 'lax', | |
| secure: process.env.NODE_ENV === 'production', | |
| }) | |
| } |
🤖 Prompt for AI Agents
In
frontend/apps/app/features/organizations/services/clearOrganizationIdCookie.ts
around lines 6 to 13, remove the unnecessary and incorrect await before
cookies() (cookies() is synchronous in Route Handlers/Server Actions) and call
it directly; then delete the ORGANIZATION_ID_KEY using the synchronous cookie
API (store.delete) and pass the same cookie attributes (path: '/', maxAge: 0,
expires: new Date(0)) to mirror typical deletion behavior.
There was a problem hiding this comment.
@NoritakaIkeda Great! Thanks for fixing the cookies() await issue. 🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Removed trailing newline at end of file for consistent formatting. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Added neverthrow@8.2.0 as a direct dependency to @liam-hq/security package to use ok/err helpers alongside the custom neverthrow wrappers. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Refactored cryptoBox functions to follow neverthrow Result monad patterns: - Changed currentKey() to use ok/err directly instead of fromThrowable - Refactored encryptAesGcm to use andThen chaining for better error flow - Simplified decryptAesGcm to use andThen pattern - Removed unnecessary eslint-disable comments This improves code clarity and follows functional error handling patterns consistently. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Refactored GitHub token refresh implementation to use neverthrow Result monad patterns: - Introduced helper functions (tryFetch, tryParse, tryFormat) for better error flow - Changed from ResultAsync.fromPromise to fromAsyncThrowable for fetch operations - Added GITHUB_TOKEN_URL constant - Renamed schema to gitHubTokenSuccessSchema with type inference - Used okAsync/errAsync for clearer async Result construction - Improved error messages with statusText This makes the error handling flow more explicit and easier to follow. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/apps/app/features/organizations/services/clearOrganizationIdCookie.ts (1)
8-12: Consider usingdelete()for clearer intent.While the current approach works, Next.js provides a
delete()method as one of the three ways to delete cookies, which more clearly expresses the intent.- store.set(ORGANIZATION_ID_KEY, '', { - path: '/', - maxAge: 0, - expires: new Date(0), - }) + store.delete(ORGANIZATION_ID_KEY)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
frontend/apps/app/app/api/github/token/refresh/route.ts(1 hunks)frontend/apps/app/features/organizations/services/clearOrganizationIdCookie.ts(1 hunks)frontend/internal-packages/security/package.json(1 hunks)frontend/internal-packages/security/src/cryptoBox.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/apps/app/app/api/github/token/refresh/route.ts
- frontend/internal-packages/security/package.json
- frontend/internal-packages/security/src/cryptoBox.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/apps/app/features/organizations/services/clearOrganizationIdCookie.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
Files:
frontend/apps/app/features/organizations/services/clearOrganizationIdCookie.ts
frontend/apps/**
📄 CodeRabbit inference engine (AGENTS.md)
Next.js apps live under frontend/apps; target app-specific scripts and configs there
Files:
frontend/apps/app/features/organizations/services/clearOrganizationIdCookie.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
frontend/**/*.{ts,tsx}: Use named exports only (no default exports)
Import UI components from @liam-hq/ui when available
Import icons from @liam-hq/ui
Follow existing import patterns and tsconfig path aliases
Use consts instead of function declarations for simple utilities (e.g., const toggle = () => {})
Use runtime type validation with valibot for external data validation
Use early returns for readability
Files:
frontend/apps/app/features/organizations/services/clearOrganizationIdCookie.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Supabase Preview
- GitHub Check: frontend-ci
- GitHub Check: frontend-lint
- GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
- GitHub Check: agent-deep-modeling
- GitHub Check: PGTap Tests
- GitHub Check: Supabase Preview
🔇 Additional comments (1)
frontend/apps/app/features/organizations/services/clearOrganizationIdCookie.ts (1)
6-7: LGTM! The async/await usage is correct for Next.js 15+.The
cookies()function is asynchronous in Next.js 15+ and must be awaited, so this implementation is correct. The past review comment suggesting to removeawaitwas accurate for Next.js 14 but is now outdated.
|
@MH4GF I’ve also updated how neverthrow is used, so please take a look! |
MH4GF
left a comment
MH4GF
left a comment
There was a problem hiding this comment.
I only asked Claude to make minor corrections.
Removed path aliases for @liam-hq/security and @liam-hq/neverthrow as these packages are now properly configured in the workspace root and don't need local path overrides. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Removed outdated comment about API routes handling in middleware matcher config. API routes now follow the standard middleware flow without manual authentication handling. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Replaced fromPromise(Promise.resolve(...)) with direct okAsync call for cleaner code when returning a successful Result without async operations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Updates to Preview Branch (feat/github-oauth-token-management) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
- Regenerated pnpm-lock.yaml to resolve missing valibot@1.1.0 entry - Updated security package dependencies to match monorepo versions via syncpack 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
sasamuku
left a comment
sasamuku
left a comment
There was a problem hiding this comment.
Apologies for the late comment after the merge, but I've made two comments🙏
| // Access token expiration: 8 hours. | ||
| // GitHub's OAuth access tokens typically expire after 8 hours. See: https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps#step-3-github-redirects-back-to-your-site | ||
| const EIGHT_HOURS_MS = 8 * 60 * 60 * 1000 | ||
| const THIRTY_DAYS_MS = 30 * 24 * 60 * 60 * 1000 |
There was a problem hiding this comment.
GitHub refresh tokens are valid for 6 months (180 days), not 30 days. This hardcoded value causes refresh tokens to be considered expired after 30 days, even though they're still valid. Why?
https://docs.github.com/ja/apps/creating-github-apps/authenticating-with-a-github-app/refreshing-user-access-tokens
There was a problem hiding this comment.
I missed this point earlier.
I intended to align with any standard HttpOnly cookie storage duration, but since web standards and common security guidelines don't specifically mention it, I'll match the lifetime of GitHub tokens instead. I'll address this in the next PR.
There was a problem hiding this comment.
I intended to align with any standard HttpOnly cookie storage duration.
I don't think there's a problem with that itself, but using magic numbers is not good.
| method: 'POST', | ||
| cache: 'no-store', | ||
| }), | ||
| ).match(onOk, () => {}) |
There was a problem hiding this comment.
When the refresh token expires (after 6 months of inactivity), the refresh API returns 401, but the error is silently ignored. Users are left with a non-functional UI with no explanation.
Current user experience:
- User returns after 6 months
- UI becomes unresponsive (installations don't load)
- No error message shown
- User is confused
Please let me know if my understanding is wrong.
There was a problem hiding this comment.
Thank you. We acknowledge that we do not currently present the behavior after the refresh token expires to users.
We will implement one of the following solutions, or a better alternative, while referencing other systems' UX:
- Prompt the user to sign out and sign in again
- Have the user re-authorize the GitHub OAuth connection
Issue
Functional Test
https://github.com/route06/liam-internal/issues/5764#issuecomment-3410090541
I'd appreciate it if you could read my inline comments during the review.
my inline comments
Why is this change needed?
This change extracts GitHub API calls into dedicated utility modules and integrates automatic token refresh functionality. This improves code organization and ensures that expired GitHub OAuth tokens are automatically refreshed before making API calls, preventing authentication failures.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Chores