feat(api): migrate GET /api/chats to session-scoped shape#626
Conversation
Reads chats joined to sessions via a new selectChatsWithSessions helper and
projects rows to { id, title, accountId, sessionId, updatedAt } so clients
can render canonical /sessions/{sid}/chats/{cid} URLs.
Recoup admin → all chats; personal/org → caller's account (or target with
canAccessAccount). artist_account_id is accepted but ignored for backward
compatibility. The MCP get_chats tool gets the same treatment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 13 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChats are now queried joined with their owning sessions. Validation returns a GetChatsScope (accountIds) enforcing canAccessAccount and RECOUP org-wide scope. HTTP handler and MCP tool consume selectChatsWithSessions and transform joined rows into response objects including sessionId and accountId. ChangesChats session-join refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found
Confidence score: 2/5
- There are two high-severity, high-confidence risks in the chats path, so this is likely high risk to merge without fixes.
- The most severe issue is in
lib/chats/__tests__/getChatsHandler.test.ts: it currently enforces leaking raw exception text in 500 responses, which conflicts with the required hardcoded "Internal server error" behavior and can expose internals. lib/chats/validateGetChatsRequest.tshas an unreachable Recoup-admin scope branch becauseorgIdis always null, which can incorrectly restrict admin access to a single account (clear user-facing authorization behavior risk).- Pay close attention to
lib/chats/__tests__/getChatsHandler.test.ts,lib/chats/validateGetChatsRequest.ts, andlib/mcp/tools/chats/registerGetChatsTool.ts- error-handling contract, admin scope logic, and duplicated MCP/API logic may diverge.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/mcp/tools/chats/registerGetChatsTool.ts">
<violation number="1" location="lib/mcp/tools/chats/registerGetChatsTool.ts:55">
P2: This tool now duplicates `/api/chats` scope and response-shaping logic instead of reusing shared chats domain logic, which makes MCP and API behavior likely to drift on future changes.</violation>
</file>
<file name="lib/chats/getChatsHandler.ts">
<violation number="1" location="lib/chats/getChatsHandler.ts:46">
P3: The null-session guard is dead code given the `sessions!inner` join and adds unnecessary branching in the response mapping.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as HTTP Client / MCP Client
participant API as GET /api/chats (route.ts)
participant Handler as getChatsHandler
participant Validator as validateGetChatsRequest
participant Auth as validateAuthContext
participant Access as canAccessAccount
participant DB as Supabase: selectChatsWithSessions
participant MCP as MCP Tool: get_chats (registerGetChatsTool)
Note over Client,DB: NEW: Chats are scoped via sessions.account_id
Client->>API: GET /api/chats (optional ?account_id / ?artist_account_id)
API->>Handler: Forward request
Handler->>Validator: validateGetChatsRequest(request)
Validator->>Auth: validateAuthContext (x-api-key / Bearer)
alt Auth failure
Auth-->>Validator: 401 Response
Validator-->>Handler: Error Response
Handler-->>Client: 401
else Auth success
Auth-->>Validator: { accountId, orgId }
alt Has target?account_id?
Validator->>Access: canAccessAccount(targetAccountId, currentAccountId)
alt Access denied
Access-->>Validator: false
Validator-->>Handler: 403 Response
Handler-->>Client: 403
else Access granted
Access-->>Validator: true
Validator-->>Handler: { accountIds: [targetAccountId] }
end
else No target?account_id
alt Recoup admin (orgId === RECOUP_ORG_ID)
Validator-->>Handler: { accountIds: undefined } (all chats)
else Personal / Org key
Validator-->>Handler: { accountIds: [accountId] }
end
end
end
Handler->>DB: selectChatsWithSessions({ accountIds })
Note over DB: Queries: chats ⋈ sessions!inner<br/>Filters on session.account_id<br/>Orders by chats.updated_at DESC<br/>Short-circuits [] for empty array
alt DB error
DB-->>Handler: null
Handler-->>Client: 500 Error
else Success
DB-->>Handler: rows[] with embedded session{id, account_id}
Handler->>Handler: Project rows to wire shape<br/>{ id, title, accountId, sessionId, updatedAt }
Handler-->>Client: 200 { status: "success", chats: [...] }
end
Note over Client,MCP: MCP Tool Flow (mirrors HTTP path)
MCP->>MCP: registerGetChatsTool(server)
Client->>MCP: Invoke get_chats({ account_id?, artist_account_id? })
alt No auth in context
MCP-->>Client: Error: "Authentication required"
else Authenticated
MCP->>MCP: Determine accountIds scope<br/>(same logic as Validator)
alt targetAccountId provided
MCP->>Access: canAccessAccount(targetAccountId, currentAccountId)
alt Denied
MCP-->>Client: Error: "Access denied"
end
else Recoup admin
MCP->>MCP: accountIds = undefined
else Personal / Org
MCP->>MCP: accountIds = [accountId]
end
MCP->>DB: selectChatsWithSessions({ accountIds })
alt DB error
DB-->>MCP: null
MCP-->>Client: Error: "Failed to retrieve chats"
else Success
DB-->>MCP: rows[] with session
MCP->>MCP: Project to wire shape
MCP-->>Client: { content: [{ type: "text", text: JSON }] }
end
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| if (error) { | ||
| return getToolResultError(error); | ||
| let accountIds: string[] | undefined; |
There was a problem hiding this comment.
P2: This tool now duplicates /api/chats scope and response-shaping logic instead of reusing shared chats domain logic, which makes MCP and API behavior likely to drift on future changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/mcp/tools/chats/registerGetChatsTool.ts, line 55:
<comment>This tool now duplicates `/api/chats` scope and response-shaping logic instead of reusing shared chats domain logic, which makes MCP and API behavior likely to drift on future changes.</comment>
<file context>
@@ -33,33 +40,52 @@ export function registerGetChatsTool(server: McpServer): void {
-
- if (error) {
- return getToolResultError(error);
+ let accountIds: string[] | undefined;
+ if (targetAccountId) {
+ const hasAccess = await canAccessAccount({
</file context>
| } | ||
|
|
||
| const chats = rows.flatMap(row => { | ||
| if (!row.session) return []; |
There was a problem hiding this comment.
P3: The null-session guard is dead code given the sessions!inner join and adds unnecessary branching in the response mapping.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chats/getChatsHandler.ts, line 46:
<comment>The null-session guard is dead code given the `sessions!inner` join and adds unnecessary branching in the response mapping.</comment>
<file context>
@@ -25,31 +33,31 @@ export async function getChatsHandler(request: NextRequest): Promise<NextRespons
}
+ const chats = rows.flatMap(row => {
+ if (!row.session) return [];
+ return [
+ {
</file context>
The typed supabase-js client infers the join row shape from
.from("chats").select("*, session:sessions!inner(id, account_id)")
directly. The ChatWithSession alias built from Tables<"chats"> &
Pick<Tables<"sessions">, ...> plus the trailing `as ChatWithSession[]`
cast were just paying tax to reach a shape the SDK was already giving
us. Let inference carry it through to the handler.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 4 unresolved issues from previous reviews.
Re-trigger cubic
The artist filter on GET /api/chats / get_chats is staying. It just has no rows to match until sessions.artist_id is populated. Reword the MCP tool schema, route handler JSDoc, and validator comment so callers know the filter is first-class and will start working once the database column lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked by 4 unresolved issues from previous reviews.
Re-trigger cubic
![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 (1)
lib/chats/validateGetChatsRequest.ts (1)
36-80: ⚖️ Poor tradeoffScope resolution is clean and correct — consider lifting the account-scope decision into a shared helper.
The auth → scope mapping here (target/
canAccessAccount,RECOUP_ORG_ID→ all, else caller account) is duplicated almost verbatim inregisterGetChatsTool.ts(L55-69). Extracting a pureresolveGetChatsScope({ accountId, orgId, targetAccountId })that both the route validator and the MCP tool call would keep the access-control contract in one place (SRP/DRY) and shrink this function below the 20-line guideline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/chats/validateGetChatsRequest.ts` around lines 36 - 80, Extract the duplicated auth→scope logic into a pure helper function (e.g., resolveGetChatsScope) that accepts { accountId, orgId, targetAccountId } and returns GetChatsScope or throws/returns an error indicator; update validateGetChatsRequest to call resolveGetChatsScope instead of doing the canAccessAccount/RECOUP_ORG_ID checks inline and likewise replace the duplicated block in registerGetChatsTool (L55-69) with the same helper; keep canAccessAccount and RECOUP_ORG_ID usage inside the new resolveGetChatsScope so access checks remain centralized and validateGetChatsRequest only handles parsing/validation and the single call to resolveGetChatsScope.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/mcp/tools/chats/registerGetChatsTool.ts`:
- Around line 45-69: In registerGetChatsTool replace the manual extraction of
accountId/orgId from extra.authInfo with a call to resolveAccountId({ authInfo:
extra.authInfo as McpAuthInfo | undefined, accountIdOverride: targetAccountId
}); if resolveAccountId returns an error, immediately return
getToolResultError(error); otherwise use the resolved accountId for subsequent
scope and access logic (retain the RECOUP_ORG_ID special-case for orgId if still
required) and remove the manual canAccessAccount/targetAccountId branching that
duplicated resolution behavior so authorization is centralized via
resolveAccountId.
In `@lib/supabase/chats/selectChatsWithSessions.ts`:
- Around line 27-29: The function signature for selectChatsWithSessions is split
across multiple lines and fails Prettier; collapse it into a single-line
declaration so the formatter/lint pass succeeds — e.g., change the multi-line
export async function selectChatsWithSessions( params:
SelectChatsWithSessionsParams = {}, ) { to a single-line signature for
selectChatsWithSessions with the same default param and body intact.
- Around line 12-15: The downstream code expects row.session to be a single
object but Supabase returns an array when the relationship is not one-to-one;
update selectChatsWithSessions to handle the array shape: in the SELECT/SELECT
constant and any consumer code (references like row.session.account_id and other
accesses around lines 36-40), treat session as session?.[0] (e.g., use
row.session?.[0]?.account_id) or normalize immediately after fetch (map
row.session = row.session?.[0] ?? null) so row.session becomes Sessions | null;
adjust all row.session.* references accordingly (or alternatively change
schema/relationship to one-to-one so SELECT in selectChatsWithSessions yields a
single object).
---
Nitpick comments:
In `@lib/chats/validateGetChatsRequest.ts`:
- Around line 36-80: Extract the duplicated auth→scope logic into a pure helper
function (e.g., resolveGetChatsScope) that accepts { accountId, orgId,
targetAccountId } and returns GetChatsScope or throws/returns an error
indicator; update validateGetChatsRequest to call resolveGetChatsScope instead
of doing the canAccessAccount/RECOUP_ORG_ID checks inline and likewise replace
the duplicated block in registerGetChatsTool (L55-69) with the same helper; keep
canAccessAccount and RECOUP_ORG_ID usage inside the new resolveGetChatsScope so
access checks remain centralized and validateGetChatsRequest only handles
parsing/validation and the single call to resolveGetChatsScope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fa5bd7b-d31e-429a-98a3-f4dc3d512c45
⛔ Files ignored due to path filters (4)
lib/chats/__tests__/getChatsHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/chats/__tests__/validateGetChatsRequest.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/mcp/tools/chats/__tests__/registerGetChatsTool.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/supabase/chats/__tests__/selectChatsWithSessions.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (5)
app/api/chats/route.tslib/chats/getChatsHandler.tslib/chats/validateGetChatsRequest.tslib/mcp/tools/chats/registerGetChatsTool.tslib/supabase/chats/selectChatsWithSessions.ts
| const authInfo = extra.authInfo as McpAuthInfo | undefined; | ||
| const accountId = authInfo?.extra?.accountId; | ||
| const orgId = authInfo?.extra?.orgId ?? null; | ||
|
|
||
| if (!accountId) { | ||
| return getToolResultError( | ||
| "Authentication required. Provide an API key via Authorization: Bearer header, or provide account_id from the system prompt context.", | ||
| ); | ||
| } | ||
|
|
||
| const { params, error } = await buildGetChatsParams({ | ||
| account_id: accountId, | ||
| target_account_id: account_id, | ||
| artist_id: artist_account_id, | ||
| }); | ||
|
|
||
| if (error) { | ||
| return getToolResultError(error); | ||
| let accountIds: string[] | undefined; | ||
| if (targetAccountId) { | ||
| const hasAccess = await canAccessAccount({ | ||
| targetAccountId, | ||
| currentAccountId: accountId, | ||
| }); | ||
| if (!hasAccess) { | ||
| return getToolResultError("Access denied to specified account_id"); | ||
| } | ||
| accountIds = [targetAccountId]; | ||
| } else if (orgId === RECOUP_ORG_ID) { | ||
| accountIds = undefined; | ||
| } else { | ||
| accountIds = [accountId]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect resolveAccountId signature/return to align this tool with the convention.
fd -i 'resolveAccountId*.ts' --exec cat {} \;
# See how sibling tools use resolveAccountId for the canonical pattern.
rg -nP -C4 'resolveAccountId\s*\(' --type=ts -g 'lib/mcp/tools/**'Repository: recoupable/api
Length of output: 8796
Use resolveAccountId() in registerGetChatsTool instead of manually reading accountId from extra.authInfo.
lib/mcp/tools/chats/registerGetChatsTool.ts derives accountId/auth error by hand (L45-47); MCP tools should route authentication + access validation through resolveAccountId() so the auth contract stays consistent across tools. Update the handler to call resolveAccountId({ authInfo, accountIdOverride: targetAccountId }), return getToolResultError(error) when it returns an error, and then base the accountIds scope logic on the resolved accountId (keeping any orgId === RECOUP_ORG_ID special-casing if still required).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/mcp/tools/chats/registerGetChatsTool.ts` around lines 45 - 69, In
registerGetChatsTool replace the manual extraction of accountId/orgId from
extra.authInfo with a call to resolveAccountId({ authInfo: extra.authInfo as
McpAuthInfo | undefined, accountIdOverride: targetAccountId }); if
resolveAccountId returns an error, immediately return getToolResultError(error);
otherwise use the resolved accountId for subsequent scope and access logic
(retain the RECOUP_ORG_ID special-case for orgId if still required) and remove
the manual canAccessAccount/targetAccountId branching that duplicated resolution
behavior so authorization is centralized via resolveAccountId.
| const SELECT = ` | ||
| *, | ||
| session:sessions!inner ( id, account_id ) | ||
| ` as const; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for the generated Database types and the chats/sessions relationship shape.
fd -i 'database.types.ts' -o -i 'supabase.ts' --exec rg -n -C2 'session_id|sessions' {} \;
# Inspect chats table relationships in generated types
rg -nP -C3 '\bchats\b' --type=ts -g '*database*'Repository: recoupable/api
Length of output: 1743
🌐 Web query:
In supabase-js v2.86, when selecting session:sessions!inner ( id, account_id ) from a table whose column references sessions, is the embedded result typed as a single object or an array?
💡 Result:
For supabase-js v2.86, the embedded relation result for session:sessions!inner ( id, account_id ) is typed as an array (i.e., ...: { id: ..., account_id: ... }[]), not a single object. This is because, in Supabase’s join/nesting typing, embedded relations are arrays by default (and !inner only changes join semantics/filtering of parent rows, not whether the embedded relation is represented as an array vs a single object) [1][2]. If you are expecting a single object, you’d typically need a “one row” pattern such as .single() on the top-level query; Supabase’s docs note that without .single() the top-level data is an array [3].
Citations:
- 1: https://supabase.com/docs/guides/database/joins-and-nesting
- 2: https://github.com/supabase/supabase/blob/master/apps/docs/content/guides/database/joins-and-nesting.mdx
- 3: Types incorrectly inferred as array on a single select query with multiple one to many joins to the same table supabase/supabase-js#1026
Fix session join shape in selectChatsWithSessions to match downstream row.session.account_id usage
Downstream dereferences row.session.account_id as a single object, but the generated DB relationship metadata for chats.session_id -> sessions.id marks it as isOneToOne: false, and Supabase-js join/nesting typing represents embedded relations as arrays by default when the relationship isn’t to-one—so session is likely ...[], making .account_id incompatible/undefined. Model the relationship as one-to-one (schema-level uniqueness) so session becomes Sessions | null, or update selectChatsWithSessions/callers to handle an array (e.g., row.session?.[0]?.account_id).
const SELECT = `
*,
session:sessions!inner ( id, account_id )
` as const;Also ensure any other row.session.* access (lines 36-40) follows the same shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/supabase/chats/selectChatsWithSessions.ts` around lines 12 - 15, The
downstream code expects row.session to be a single object but Supabase returns
an array when the relationship is not one-to-one; update selectChatsWithSessions
to handle the array shape: in the SELECT/SELECT constant and any consumer code
(references like row.session.account_id and other accesses around lines 36-40),
treat session as session?.[0] (e.g., use row.session?.[0]?.account_id) or
normalize immediately after fetch (map row.session = row.session?.[0] ?? null)
so row.session becomes Sessions | null; adjust all row.session.* references
accordingly (or alternatively change schema/relationship to one-to-one so SELECT
in selectChatsWithSessions yields a single object).
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 4 unresolved issues from previous reviews.
Re-trigger cubic
…e CLI `pnpm update-types` produces this exact output today. The previously committed file was hand-edited (per api#628) which drifted from the CLI's current style (semicolons vs trailing-comma multi-line). No schema deltas — purely formatting normalization plus a few JSON-path keys the newer CLI infers on JSONB columns. Going forward this file should always be CLI-generated, never hand-edited, so future regens are no-ops. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Matches the docs#227 contract that GET /api/chats responses include
`artistId` and the `artist_account_id` query param is a real filter.
- selectChatsWithSessions: project `session.artist_id` in SELECT;
accept `artistAccountId` param; `.eq("session.artist_id", id)` when set.
- validateGetChatsRequest: return `artistAccountId` on the scope.
Fix the unreachable Recoup-admin branch (cubic P1) — check
`account_organization_ids` membership instead of `auth.orgId`, which
is always null for Bearer tokens. Bearer-authed admins now get the
same admin scope as x-api-key org admins.
- getChatsHandler: pass `artistAccountId` to the select; include
`artistId` in the per-row projection. 500 catch block now returns
hardcoded "Internal server error" instead of echoing the raw
exception message (cubic P1 — no leaking internals on 500).
- registerGetChatsTool (MCP): same projection/filter changes; same
admin-via-membership fix.
- Tests: updated mocks for getAccountOrganizations, refreshed admin
scope cases, added composed filter + artistId-null cases, and
updated the 500 expectation to "Internal server error" (cubic).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
4 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/mcp/tools/chats/registerGetChatsTool.ts">
<violation number="1" location="lib/mcp/tools/chats/registerGetChatsTool.ts:55">
P2: This tool now duplicates `/api/chats` scope and response-shaping logic instead of reusing shared chats domain logic, which makes MCP and API behavior likely to drift on future changes.</violation>
<violation number="2" location="lib/mcp/tools/chats/registerGetChatsTool.ts:71">
P2: Admin scope can silently degrade on org-membership lookup failures, returning partial chat data instead of preserving admin scope or failing explicitly.</violation>
</file>
<file name="lib/chats/getChatsHandler.ts">
<violation number="1" location="lib/chats/getChatsHandler.ts:37">
P1: This now applies `artist_account_id` as an active filter, which breaks the accepted-but-ignored backward-compat behavior described for this route.</violation>
<violation number="2" location="lib/chats/getChatsHandler.ts:46">
P3: The null-session guard is dead code given the `sessions!inner` join and adds unnecessary branching in the response mapping.</violation>
</file>
<file name="lib/supabase/chats/__tests__/selectChatsWithSessions.test.ts">
<violation number="1" location="lib/supabase/chats/__tests__/selectChatsWithSessions.test.ts:61">
P3: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
This test file exceeds the repository's 100-line limit.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const rows = await selectChatsWithSessions({ | ||
| accountIds: validated.accountIds, | ||
| artistAccountId: validated.artistAccountId, | ||
| }); |
There was a problem hiding this comment.
P1: This now applies artist_account_id as an active filter, which breaks the accepted-but-ignored backward-compat behavior described for this route.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chats/getChatsHandler.ts, line 37:
<comment>This now applies `artist_account_id` as an active filter, which breaks the accepted-but-ignored backward-compat behavior described for this route.</comment>
<file context>
@@ -34,7 +34,10 @@ export async function getChatsHandler(request: NextRequest): Promise<NextRespons
}
- const rows = await selectChatsWithSessions({ accountIds: validated.accountIds });
+ const rows = await selectChatsWithSessions({
+ accountIds: validated.accountIds,
+ artistAccountId: validated.artistAccountId,
</file context>
| const rows = await selectChatsWithSessions({ | |
| accountIds: validated.accountIds, | |
| artistAccountId: validated.artistAccountId, | |
| }); | |
| const rows = await selectChatsWithSessions({ | |
| accountIds: validated.accountIds, | |
| }); |
| } | ||
| accountIds = [targetAccountId]; | ||
| } else { | ||
| const callerOrgs = await getAccountOrganizations({ accountId }); |
There was a problem hiding this comment.
P2: Admin scope can silently degrade on org-membership lookup failures, returning partial chat data instead of preserving admin scope or failing explicitly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/mcp/tools/chats/registerGetChatsTool.ts, line 71:
<comment>Admin scope can silently degrade on org-membership lookup failures, returning partial chat data instead of preserving admin scope or failing explicitly.</comment>
<file context>
@@ -62,13 +67,13 @@ export function registerGetChatsTool(server: McpServer): void {
- accountIds = undefined;
} else {
- accountIds = [accountId];
+ const callerOrgs = await getAccountOrganizations({ accountId });
+ const isRecoupAdmin = callerOrgs.some(m => m.organization_id === RECOUP_ORG_ID);
+ accountIds = isRecoupAdmin ? undefined : [accountId];
</file context>
| @@ -0,0 +1,123 @@ | |||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | |||
There was a problem hiding this comment.
P3: Custom agent: Enforce Clear Code Style and Maintainability Practices
This test file exceeds the repository's 100-line limit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/chats/__tests__/selectChatsWithSessions.test.ts, line 61:
<comment>This test file exceeds the repository's 100-line limit.</comment>
<file context>
@@ -47,12 +50,35 @@ describe("selectChatsWithSessions", () => {
expect(result).toEqual(rows);
});
+ it("composes accountIds + artistAccountId — both filters applied", async () => {
+ orderMock.mockResolvedValueOnce({ data: [], error: null });
+
</file context>
| // Recoup admin → all chats. Check membership directly so Bearer-authed | ||
| // admins get the same scope as x-api-key admins (auth.orgId is null for | ||
| // Bearer regardless of the caller's org memberships). | ||
| const callerOrgs = await getAccountOrganizations({ accountId }); | ||
| const isRecoupAdmin = callerOrgs.some(m => m.organization_id === RECOUP_ORG_ID); | ||
| if (isRecoupAdmin) { | ||
| return { accountIds: undefined, artistAccountId }; |
There was a problem hiding this comment.
DRY - do we have any existing, shared, libs to check isRecoupAdmin?
There was a problem hiding this comment.
Good call — extracted into lib/organizations/isRecoupAdmin.ts in fdc6ce5. Both validateGetChatsRequest and registerGetChatsTool now call it. canAccessAccount deliberately keeps its own inline check since it reuses the same org-list query for the subsequent shared-org check (calling the helper there would double the DB round-trip).
…heck Two callers were inlining the same `getAccountOrganizations(...).some(m => m.organization_id === RECOUP_ORG_ID)` pattern to detect Recoup-org membership. Extract into a shared helper so the admin-scope decision lives in one place. - New `lib/organizations/isRecoupAdmin.ts` (+ unit tests). - `validateGetChatsRequest` and `registerGetChatsTool` now call it. - `canAccessAccount` deliberately keeps its inline check — it reuses the same org-list query for the subsequent shared-org check, so calling this helper there would double the DB round-trip. - Tests now mock `isRecoupAdmin` directly (cleaner than mocking the Supabase fetch for every admin-scope case). Addresses sweetmantech's review comment on #626. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The supabase CLI emits the file without semicolons (it has its own inline formatter), but the project's prettier config requires them (`semi: true`), so `pnpm format:check` was failing in CI. This is mechanical — diff is purely whitespace + trailing semicolons. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 8 files (changes from recent commits).
Requires human review: Auto-approval blocked by 5 unresolved issues from previous reviews.
Re-trigger cubic
Preview verification —
|
| # | Scenario | Expected | Actual | ✓ |
|---|---|---|---|---|
| 1 | GET /api/chats (admin caller via membership) |
new shape + admin scope | 200, 1000 rows across 89 distinct accountIds, keys = accountId,artistId,id,sessionId,title,updatedAt |
✅ |
| 2 | ?account_id=<self> |
scoped to one account | 200, 64 rows, 1 distinct accountId, 0 leaks | ✅ |
| 3 | ?artist_account_id=<real artist> |
only rows in that artist context | 200, 9 rows, every row's artistId matches |
✅ |
| 4 | ?account_id=X&artist_account_id=Y (composed) |
rows matching both | 200, 9 rows, every row matches both | ✅ |
| 5 | ?artist_account_id=00000000-0000-4000-8000-000000000000 (random) |
0 rows | 200, 0 rows | ✅ |
| 6 | ?artist_account_id=not-a-uuid |
400 | 400, "artist_account_id must be a valid UUID" |
✅ |
| 7 | ?account_id=not-a-uuid |
400 | 400, "account_id must be a valid UUID" |
✅ |
Validates the PR's three substantive changes:
- ✅ New wire shape: every row carries
sessionId,accountId,artistId,updatedAt,id,title. - ✅ Cubic P1 fix api/image/generate - generate an image #1 — Recoup-admin via
account_organization_idsmembership now works for Bearer-authed callers (Test 1 returns 1000 rows across 89 accounts; previously theorgId === RECOUP_ORG_IDbranch was unreachable for Bearer). - ✅
artist_account_idis a real filter (was previously documented as accepted-but-no-effect).
Cubic P1 fix #2 (no leak of raw exception in 500) is covered by unit test returns 500 with a generic message when an exception is thrown (no leak of raw message).
Docs ↔ runtime shape validation — matches docs#227 spec ✅Cross-checked the preview's actual responses against the merged OpenAPI spec at Top-level response —
|
| Field | Docs spec | Preview returns | ✓ |
|---|---|---|---|
status |
required, enum: "success" |
"success" |
✅ |
chats |
required, ChatRoom[] |
array of objects | ✅ |
Each chat row — ChatRoom
| Field | Docs spec | Preview returns | ✓ |
|---|---|---|---|
id |
required, uuid | "bf16cfc8-fa49-42a0-8120-856317277ad2" |
✅ |
title |
required, string | "New chat" |
✅ |
accountId |
required, uuid | "2b27a850-c2ec-4a8b-877b-ece185989dd0" |
✅ |
sessionId |
required, uuid | "c4af76d0-2631-4b29-9864-bb2b0ee424e3" |
✅ |
updatedAt |
required, ISO date-time | "2026-05-30T22:04:30.340241+00:00" |
✅ |
artistId |
optional, uuid, nullable | null when session has no artist; uuid when it does (Test 3) |
✅ |
Returned keyset is exactly {accountId, artistId, id, sessionId, title, updatedAt} — no extras, no missing.
Query parameters
| Param | Docs spec | Preview behavior | ✓ |
|---|---|---|---|
account_id |
optional, uuid | Filters to that account (Test 2); rejects non-UUID with 400 (Test 7) | ✅ |
artist_account_id |
optional, uuid; composes with account_id |
Filters by sessions.artist_id (Test 3); composes (Test 4); 0 rows for unknown (Test 5); rejects non-UUID with 400 (Test 6) |
✅ |
Docs#227 contract is fully honored by the implementation in 0a5b275c.
2 participants
Rewrites
GET /api/chats(and the MCPget_chatstool) to read fromchats ⋈ sessionsand emit{ id, title, accountId, sessionId, updatedAt }so the chat sidebar can render canonical/sessions/{sid}/chats/{cid}URLs.artist_account_idis accepted-but-ignored for backward compat.Test plan
GET /api/chatswith a personal Bearer token and confirm the response carriessessionIdandaccountIdper row.GET /api/chats?artist_account_id=<uuid>and confirm the param is accepted (200) but does not filter.get_chatsMCP tool and confirm the same projection.Summary by CodeRabbit