feat(chat): add GET /api/chat/{chatId}/stream to resume an in-progress response - #809
Conversation
…s response The endpoint has been documented since the workflow cutover (api-reference/chat/workflow-stream.mdx) but was never implemented — app/api/chat/[chatId]/ contained only stop/. Documented-but-missing drift. It matters now because a long turn's SSE stream can end before the run does. Reproduced on prod 2026-08-02: the stream closed at ~123s with a clean [DONE] and no finish chunk while the workflow ran on to completion, so the client rendered 6 of 13 iterations and froze. The only recovery path was maybeResumeChatStream, which runs inside POST /api/chat — hence a refresh worked and sitting still did not. - app/api/chat/[chatId]/stream/route.ts — GET + OPTIONS, maxDuration 800 to match POST /api/chat, since a resumed stream lives as long as the turn it follows. - lib/chat/handleResumeChatStream.ts — 200 SSE + x-workflow-run-id when the run is live, 204 when there is nothing to resume (clearing a stale active_stream_id on the way), 502 when the status lookup throws. - lib/chat/parseStreamStartIndex.ts — the documented `integer, minimum 0` contract. Negative values are rejected even though the SDK accepts them: it reads those relative to the end of a live stream, which resolves to a different absolute position per call and cannot give a gap-free resume. - lib/chat/validateChatOwnership.ts — extracted from validateStopChatWorkflowRequest so both /stop and /stream enforce the same auth, chat-id and ownership rules from one place. The stop validator is now a thin alias; its behaviour is unchanged. A failed status read returns 502, not 204. Reporting "nothing to resume" for a transient workflow-api blip would tell a client with a live run to stop reconnecting — the exact silent truncation this route exists to prevent. Mirrors reconcileExistingActiveStream's conflict-over-clear stance. Implements docs#286 (adds the startIndex param + 400 to the published contract). Merge order: docs#286 → this → chat client reconnect. Refs recoupable/chat#1923 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdded a dynamic chat stream route that validates ownership, parses ChangesChat stream resumption
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StreamRoute
participant handleResumeChatStream
participant validateChatOwnership
participant WorkflowRun
Client->>StreamRoute: GET /api/chat/{chatId}/stream
StreamRoute->>handleResumeChatStream: Pass request and chatId
handleResumeChatStream->>validateChatOwnership: Validate chat ownership
validateChatOwnership-->>handleResumeChatStream: Return auth context and chat
handleResumeChatStream->>WorkflowRun: Read status and stream from startIndex
WorkflowRun-->>handleResumeChatStream: Return workflow data
handleResumeChatStream-->>Client: Return resumed SSE response
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/chat/validateChatOwnership.ts (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
z.uuid()for the chat ID schema.The project uses Zod 4, where string formats are top-level APIs. Replace the legacy
z.string().uuid(...)call withz.uuid(...)to avoid deprecated validator usage and keep schema style consistent.🤖 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/chat/validateChatOwnership.ts` at line 16, Update the chatIdSchema declaration to use Zod 4’s top-level z.uuid() validator instead of the deprecated z.string().uuid(...) chain, preserving the existing validation message.lib/chat/parseStreamStartIndex.ts (1)
22-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
startIndexparsing declarative and safe.Hand-rolling this already rejects blank input poorly:
?startIndex=passes, but?startIndex=%20is accepted as0. Move this into a Zod parser and apply the lower bound; add an upper bound while the docblock only saysinteger, minimum 0, especially since the current code accepts0x10,1e21, and unsafe large values.🤖 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/chat/parseStreamStartIndex.ts` around lines 22 - 29, Update parseStreamStartIndex to validate startIndex with a Zod schema instead of manual Number parsing: require a non-empty string representing a safe integer, enforce a minimum of 0, and add the intended maximum upper bound. Preserve the existing undefined result when the query parameter is absent and validationErrorResponse behavior for invalid values.
🤖 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/chat/handleResumeChatStream.ts`:
- Around line 69-75: Update the shared getCorsHeaders helper to include
x-workflow-run-id in Access-Control-Expose-Headers, then preserve that merged
CORS header configuration in the createUIMessageStreamResponse call within the
resume handler so cross-origin callers can read the workflow run ID.
In `@lib/chat/parseStreamStartIndex.ts`:
- Around line 22-29: Replace lib/chat/parseStreamStartIndex.ts with
lib/chat/validateChatStreamQuery.ts and rename the export to
validateChatStreamQuery. Use a Zod query-object schema with an optional
non-negative integer startIndex, returning validated data or NextResponse on
error, and export its inferred type. Update the import path in
app/api/chat/[chatId]/stream/route.ts as needed; no other route logic changes
are required.
---
Nitpick comments:
In `@lib/chat/parseStreamStartIndex.ts`:
- Around line 22-29: Update parseStreamStartIndex to validate startIndex with a
Zod schema instead of manual Number parsing: require a non-empty string
representing a safe integer, enforce a minimum of 0, and add the intended
maximum upper bound. Preserve the existing undefined result when the query
parameter is absent and validationErrorResponse behavior for invalid values.
In `@lib/chat/validateChatOwnership.ts`:
- Line 16: Update the chatIdSchema declaration to use Zod 4’s top-level z.uuid()
validator instead of the deprecated z.string().uuid(...) chain, preserving the
existing validation message.
🪄 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 Plus
Run ID: 927f5e11-96c1-4942-989d-bb6501466850
⛔ Files ignored due to path filters (2)
lib/chat/__tests__/handleResumeChatStream.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/chat/__tests__/parseStreamStartIndex.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (5)
app/api/chat/[chatId]/stream/route.tslib/chat/handleResumeChatStream.tslib/chat/parseStreamStartIndex.tslib/chat/validateChatOwnership.tslib/chat/validateStopChatWorkflowRequest.ts
| return createUIMessageStreamResponse({ | ||
| stream: wrapWorkflowStreamWatcher( | ||
| activeStreamId, | ||
| run.getReadable<UIMessageChunk>({ startIndex }), | ||
| ), | ||
| headers: { ...getCorsHeaders(), "x-workflow-run-id": activeStreamId }, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚖️ Poor tradeoff
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the CORS header helper and any existing expose-headers usage.
fd -t f 'getCorsHeaders.ts' -x cat -n {}
rg -nPi 'access-control-expose-headers|x-workflow-run-id' -g '!**/node_modules/**'Repository: recoupable/api
Length of output: 593
🏁 Script executed:
#!/bin/bash
set -u
echo "== locate target file =="
fd -t f 'handleResumeChatStream\.ts$' .
echo
echo "== file outline =="
ast-grep outline lib/chat/handleResumeChatStream.ts --view expanded || true
echo
echo "== target file lines 1-140 =="
cat -n lib/chat/handleResumeChatStream.ts | sed -n '1,140p'
echo
echo "== search for getReadable / resume / workflow run id / startIndex =="
rg -n "getReadable|resume|getResume|startIndex|x-workflow-run-id|Workflow|workflow|wrapWorkflowStreamWatcher" -g '!**/node_modules/**' .
echo
echo "== inspect files named getReadable or workflow stream chunks =="
rg -l "function getReadable|getReadable<" -g '!**/node_modules/**' .
rg -l "UIMessageChunk" -g '!**/node_modules/**' .Repository: recoupable/api
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -u
echo "== getCorsHeaders usage locations =="
rg -n "getCorsHeaders|Access-Control-Expose-Headers|x-workflow-run-id" lib app . --glob '!**/node_modules/**' --glob '!**/.next/**' | head -n 250
echo
echo "== route files using same helper =="
fd -t f '\.(ts|tsx)$' . --exclude node_modules --exclude .next | xargs rg -l "getCorsHeaders|options|optionsHandler|OPTIONS|preflight|preflightHeaders" 2>/dev/null | head -n 100Repository: recoupable/api
Length of output: 29812
Expose x-workflow-run-id to cross-origin callers.
getCorsHeaders() omits Access-Control-Expose-Headers, so browsers that cross-origin fetch this route cannot read x-workflow-run-id. Add x-workflow-run-id to the shared CORS helper and merge it on the resume handler response.
🤖 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/chat/handleResumeChatStream.ts` around lines 69 - 75, Update the shared
getCorsHeaders helper to include x-workflow-run-id in
Access-Control-Expose-Headers, then preserve that merged CORS header
configuration in the createUIMessageStreamResponse call within the resume
handler so cross-origin callers can read the workflow run ID.
| export function parseStreamStartIndex(url: URL): number | undefined | NextResponse { | ||
| const raw = url.searchParams.get("startIndex"); | ||
| if (raw === null) return undefined; | ||
|
|
||
| const parsed = Number(raw); | ||
| if (raw.trim() === "" || !Number.isInteger(parsed) || parsed < 0) { | ||
| return validationErrorResponse("startIndex must be a non-negative integer", ["startIndex"]); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The startIndex query validator does not follow the mandated validator contract. The coding guidelines require every API endpoint to parse input through a dedicated validate<Name>Body.ts or validate<Name>Query.ts function with Zod schema validation. GET /api/chat/{chatId}/stream accepts the startIndex query param, and parseStreamStartIndex.ts performs that parsing with hand-rolled Number checks under a non-conforming file name. This supersedes my earlier optional Zod suggestion at the same location.
lib/chat/parseStreamStartIndex.ts#L22-L29: rename the file and the exported function tovalidateChatStreamQuery, keep the file name matched to the export, and express the rule as a Zod schema. Export the inferred type for the validated data, per the validation-function instructions.app/api/chat/[chatId]/stream/route.ts#L37-L43: no change is required here once the helper is renamed, beyond the import path thathandleResumeChatStream.tsresolves.
One caveat worth confirming before you move it: the lib/**/validate*.ts instructions require a validator to return a NextResponse on error or validated data on success. parseStreamStartIndex currently returns a third state, undefined, for an absent param. Model that absence inside the schema, for example as an optional field on a validated-query object, so the return union stays two-valued.
As per coding guidelines: "All API endpoints should use a dedicated validate<Name>Body.ts or validate<Name>Query.ts function with Zod schema validation for input parsing." As per path instructions: "Use Zod for schema validation", "Return NextResponse on error or validated data on success", "Export inferred types for validated data", "Follow naming: validateBody.ts or validateQuery.ts".
📍 Affects 2 files
lib/chat/parseStreamStartIndex.ts#L22-L29(this comment)app/api/chat/[chatId]/stream/route.ts#L37-L43
🤖 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/chat/parseStreamStartIndex.ts` around lines 22 - 29, Replace
lib/chat/parseStreamStartIndex.ts with lib/chat/validateChatStreamQuery.ts and
rename the export to validateChatStreamQuery. Use a Zod query-object schema with
an optional non-negative integer startIndex, returning validated data or
NextResponse on error, and export its inferred type. Update the import path in
app/api/chat/[chatId]/stream/route.ts as needed; no other route logic changes
are required.
Sources: Coding guidelines, Path instructions
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 7 files
Confidence score: 3/5
- In
app/api/chat/[chatId]/stream/route.ts, resumed stream responses don’t exposex-workflow-run-idvia CORS, so cross-origin clients can’t read a documented header and resume flows can break in production integrations — addAccess-Control-Expose-Headersfor that header (or centralize it in shared CORS handling). - In
lib/chat/validateChatOwnership.ts, database/query failures are currently surfaced as 404, which can make clients stop retrying during transient outages and turn recoverable failures into user-visible dead ends — distinguish lookup misses from query errors and return a 5xx for backend failures. - In
lib/chat/handleResumeChatStream.ts, callinggetRunonactive_stream_idwithout thepending-*guard can hit transient placeholder values during the claim window and trigger avoidable resume errors — mirror the sibling stop-handler guard before invoking workflow lookups. - In
lib/chat/parseStreamStartIndex.ts,Number()-based integer validation accepts formats outside the documented contract (for example hex/exponential), and terminal-state constants are duplicated again inlib/chat/handleResumeChatStream.ts, increasing drift risk on future lifecycle changes — tighten parsing to decimal-only non-negative integers and reuse one shared terminal-status definition.
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="app/api/chat/[chatId]/stream/route.ts">
<violation number="1" location="app/api/chat/[chatId]/stream/route.ts:42">
P2: Cross-origin chat clients cannot read the documented `x-workflow-run-id` on resumed streams because the CORS response omits `Access-Control-Expose-Headers`. Expose that header on the 200 response (or centrally in `getCorsHeaders`) so clients can use the run identifier.</violation>
</file>
<file name="lib/chat/parseStreamStartIndex.ts">
<violation number="1" location="lib/chat/parseStreamStartIndex.ts:27">
P3: The validation gates on `Number(raw)` + `Number.isInteger(...)`, which `Number()` coerces too permissively for a documented "integer, minimum 0" contract: hex (`0x1A`→26), exponential (`1e3`→1000), binary (`0b101`→5) and `+42` all pass, and any value above `Number.MAX_SAFE_INTEGER` (e.g. `9007199254740993`) is silently rounded down yet still accepted — for a gap-free resume that means a client could be handed a wrong start index rather than a 400. Consider validating against a strict decimal digit pattern (e.g. `/^\d+$/` on the trimmed value) and rejecting results above `Number.MAX_SAFE_INTEGER` so out-of-spec input is rejected instead of coerced.</violation>
</file>
<file name="lib/chat/validateChatOwnership.ts">
<violation number="1" location="lib/chat/validateChatOwnership.ts:31">
P3: This validation function exceeds the configured 20-line SRP limit and combines parsing, data loading, and ownership authorization. Extract the chat/session ownership lookup into a focused helper to keep the validator small and independently testable.</violation>
<violation number="2" location="lib/chat/validateChatOwnership.ts:44">
P2: A failed chat lookup is reported as 404, so clients stop retrying while the database is temporarily unavailable. Make the chat selector expose query failures distinctly and return a 5xx here, as this helper already does for `selectSessions`.</violation>
</file>
<file name="lib/chat/handleResumeChatStream.ts">
<violation number="1" location="lib/chat/handleResumeChatStream.ts:11">
P3: Workflow terminal-state definitions now have a third independent copy, so a future lifecycle change can make resume cleanup disagree with stream watching or stop polling. Extract and reuse one shared terminal-status predicate/constant.</violation>
<violation number="2" location="lib/chat/handleResumeChatStream.ts:44">
P3: `getRun` is called on `active_stream_id` without the `pending-…` placeholder guard that the sibling stop handler uses. During `/api/chat`'s claim window the slot briefly holds `pending-<uuid>` before it is promoted to the real run id, and `getRun('pending-…')` throws — so a resume arriving in that window returns a spurious 502 rather than 204. Since this handler otherwise mirrors `handleStopChatWorkflow`'s status/CAS bookkeeping, add the same `activeStreamId.startsWith('pending-')` early-return (204) so a placeholder slot is treated as 'nothing to resume' instead of a workflow-api failure.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Client (Browser)
participant NextJS as Next.js Edge/Node
participant Route as GET /api/chat/{chatId}/stream
participant Auth as validateChatOwnership
participant ChatDB as Chats Table (Supabase)
participant SessionDB as Sessions Table (Supabase)
participant Parser as parseStreamStartIndex
participant CAS as compareAndSetChatActiveStreamId
participant WfAPI as Workflow API (getRun)
participant Watcher as wrapWorkflowStreamWatcher
participant Stream as SSE Stream (text/event-stream)
Note over Client,Stream: Resume Chat Stream Flow
Client->>Route: GET /api/chat/{chatId}/stream?startIndex=N
Route->>Auth: validateChatOwnership(request, chatId)
alt Auth/Validation Fails
Auth->>Auth: Validate auth context
Auth->>ChatDB: selectChats({ id: chatId })
ChatDB-->>Auth: Chat row (or null)
Auth->>SessionDB: selectSessions({ id: chat.session_id })
SessionDB-->>Auth: Session row (or null)
Auth-->>Route: 400/401/403/404 error response
Route-->>Client: Error (4xx)
else Auth Succeeds
Auth-->>Route: { auth, chat } object
end
Route->>Parser: parseStreamStartIndex(url)
alt Malformed startIndex (negative, non-integer, empty)
Parser-->>Route: 400 response
Route-->>Client: 400 Bad Request
else Valid or Absent
Parser-->>Route: number | undefined
end
alt No Active Stream (active_stream_id is null)
Route-->>Client: 204 No Content
else Active Stream Exists
Route->>WfAPI: getRun(activeStreamId)
alt Run Status Lookup Fails (transient error)
WfAPI-->>Route: Error thrown
Route-->>Client: 502 Bad Gateway
else Run Status Succeeds
WfAPI-->>Route: Run status
alt Run is Terminal (completed/cancelled/failed)
Route->>CAS: compareAndSetChatActiveStreamId(chatId, activeStreamId, null)
CAS-->>Route: Success or Error (best-effort clear)
Route-->>Client: 204 No Content
else Run is Live (running)
Route->>Watcher: wrapWorkflowStreamWatcher(activeStreamId, readable)
Watcher->>WfAPI: run.getReadable({ startIndex })
WfAPI-->>Watcher: ReadableStream<UIMessageChunk>
Watcher-->>Route: Wrapped readable stream
Route->>Stream: createUIMessageStreamResponse()
Note over Route,Stream: Sets headers: x-workflow-run-id, text/event-stream
Stream-->>Client: 200 SSE stream
end
end
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| options: { params: Promise<{ chatId: string }> }, | ||
| ): Promise<Response> { | ||
| const { chatId } = await options.params; | ||
| return handleResumeChatStream(request, chatId); |
There was a problem hiding this comment.
P2: Cross-origin chat clients cannot read the documented x-workflow-run-id on resumed streams because the CORS response omits Access-Control-Expose-Headers. Expose that header on the 200 response (or centrally in getCorsHeaders) so clients can use the run identifier.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/api/chat/[chatId]/stream/route.ts, line 42:
<comment>Cross-origin chat clients cannot read the documented `x-workflow-run-id` on resumed streams because the CORS response omits `Access-Control-Expose-Headers`. Expose that header on the 200 response (or centrally in `getCorsHeaders`) so clients can use the run identifier.</comment>
<file context>
@@ -0,0 +1,43 @@
+ options: { params: Promise<{ chatId: string }> },
+): Promise<Response> {
+ const { chatId } = await options.params;
+ return handleResumeChatStream(request, chatId);
+}
</file context>
| return validationErrorResponse(firstError.message, firstError.path); | ||
| } | ||
|
|
||
| const chats = await selectChats({ id: parsed.data }); |
There was a problem hiding this comment.
P2: A failed chat lookup is reported as 404, so clients stop retrying while the database is temporarily unavailable. Make the chat selector expose query failures distinctly and return a 5xx here, as this helper already does for selectSessions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chat/validateChatOwnership.ts, line 44:
<comment>A failed chat lookup is reported as 404, so clients stop retrying while the database is temporarily unavailable. Make the chat selector expose query failures distinctly and return a 5xx here, as this helper already does for `selectSessions`.</comment>
<file context>
@@ -0,0 +1,55 @@
+ return validationErrorResponse(firstError.message, firstError.path);
+ }
+
+ const chats = await selectChats({ id: parsed.data });
+ const chat = chats[0];
+ if (!chat) return errorResponse("Chat not found", 404);
</file context>
| if (raw === null) return undefined; | ||
|
|
||
| const parsed = Number(raw); | ||
| if (raw.trim() === "" || !Number.isInteger(parsed) || parsed < 0) { |
There was a problem hiding this comment.
P3: The validation gates on Number(raw) + Number.isInteger(...), which Number() coerces too permissively for a documented "integer, minimum 0" contract: hex (0x1A→26), exponential (1e3→1000), binary (0b101→5) and +42 all pass, and any value above Number.MAX_SAFE_INTEGER (e.g. 9007199254740993) is silently rounded down yet still accepted — for a gap-free resume that means a client could be handed a wrong start index rather than a 400. Consider validating against a strict decimal digit pattern (e.g. /^\d+$/ on the trimmed value) and rejecting results above Number.MAX_SAFE_INTEGER so out-of-spec input is rejected instead of coerced.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chat/parseStreamStartIndex.ts, line 27:
<comment>The validation gates on `Number(raw)` + `Number.isInteger(...)`, which `Number()` coerces too permissively for a documented "integer, minimum 0" contract: hex (`0x1A`→26), exponential (`1e3`→1000), binary (`0b101`→5) and `+42` all pass, and any value above `Number.MAX_SAFE_INTEGER` (e.g. `9007199254740993`) is silently rounded down yet still accepted — for a gap-free resume that means a client could be handed a wrong start index rather than a 400. Consider validating against a strict decimal digit pattern (e.g. `/^\d+$/` on the trimmed value) and rejecting results above `Number.MAX_SAFE_INTEGER` so out-of-spec input is rejected instead of coerced.</comment>
<file context>
@@ -0,0 +1,32 @@
+ if (raw === null) return undefined;
+
+ const parsed = Number(raw);
+ if (raw.trim() === "" || !Number.isInteger(parsed) || parsed < 0) {
+ return validationErrorResponse("startIndex must be a non-negative integer", ["startIndex"]);
+ }
</file context>
| * @returns The auth context + chat row, or an error response | ||
| * (400 malformed id, 401 unauthenticated, 403 not owned, 404 missing). | ||
| */ | ||
| export async function validateChatOwnership( |
There was a problem hiding this comment.
P3: This validation function exceeds the configured 20-line SRP limit and combines parsing, data loading, and ownership authorization. Extract the chat/session ownership lookup into a focused helper to keep the validator small and independently testable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chat/validateChatOwnership.ts, line 31:
<comment>This validation function exceeds the configured 20-line SRP limit and combines parsing, data loading, and ownership authorization. Extract the chat/session ownership lookup into a focused helper to keep the validator small and independently testable.</comment>
<file context>
@@ -0,0 +1,55 @@
+ * @returns The auth context + chat row, or an error response
+ * (400 malformed id, 401 unauthenticated, 403 not owned, 404 missing).
+ */
+export async function validateChatOwnership(
+ request: NextRequest,
+ chatId: string,
</file context>
| import { errorResponse } from "@/lib/networking/errorResponse"; | ||
| import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; | ||
|
|
||
| const TERMINAL_RUN_STATUSES: ReadonlySet<string> = new Set(["completed", "cancelled", "failed"]); |
There was a problem hiding this comment.
P3: Workflow terminal-state definitions now have a third independent copy, so a future lifecycle change can make resume cleanup disagree with stream watching or stop polling. Extract and reuse one shared terminal-status predicate/constant.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chat/handleResumeChatStream.ts, line 11:
<comment>Workflow terminal-state definitions now have a third independent copy, so a future lifecycle change can make resume cleanup disagree with stream watching or stop polling. Extract and reuse one shared terminal-status predicate/constant.</comment>
<file context>
@@ -0,0 +1,76 @@
+import { errorResponse } from "@/lib/networking/errorResponse";
+import { getCorsHeaders } from "@/lib/networking/getCorsHeaders";
+
+const TERMINAL_RUN_STATUSES: ReadonlySet<string> = new Set(["completed", "cancelled", "failed"]);
+
+/**
</file context>
| const activeStreamId = validated.chat.active_stream_id; | ||
| if (!activeStreamId) return new NextResponse(null, { status: 204, headers: getCorsHeaders() }); | ||
|
|
||
| const run = getRun(activeStreamId); |
There was a problem hiding this comment.
P3: getRun is called on active_stream_id without the pending-… placeholder guard that the sibling stop handler uses. During /api/chat's claim window the slot briefly holds pending-<uuid> before it is promoted to the real run id, and getRun('pending-…') throws — so a resume arriving in that window returns a spurious 502 rather than 204. Since this handler otherwise mirrors handleStopChatWorkflow's status/CAS bookkeeping, add the same activeStreamId.startsWith('pending-') early-return (204) so a placeholder slot is treated as 'nothing to resume' instead of a workflow-api failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chat/handleResumeChatStream.ts, line 44:
<comment>`getRun` is called on `active_stream_id` without the `pending-…` placeholder guard that the sibling stop handler uses. During `/api/chat`'s claim window the slot briefly holds `pending-<uuid>` before it is promoted to the real run id, and `getRun('pending-…')` throws — so a resume arriving in that window returns a spurious 502 rather than 204. Since this handler otherwise mirrors `handleStopChatWorkflow`'s status/CAS bookkeeping, add the same `activeStreamId.startsWith('pending-')` early-return (204) so a placeholder slot is treated as 'nothing to resume' instead of a workflow-api failure.</comment>
<file context>
@@ -0,0 +1,76 @@
+ const activeStreamId = validated.chat.active_stream_id;
+ if (!activeStreamId) return new NextResponse(null, { status: 204, headers: getCorsHeaders() });
+
+ const run = getRun(activeStreamId);
+
+ // A failed status read must not be reported as "nothing to resume" — that
</file context>
Preview testing of the resume route turned up that GET /api/chat/{chatId}/stream
returns 204 for a live headless run: lib/chat/runs/ never sets
chats.active_stream_id, and the route keys on it.
That contradicts the published contract, which cross-references the two
in both directions — POST /api/chat/runs says "read the result via GET
/api/chat/{chatId}/stream (resume the stream)", and the stream endpoint
says "start a headless run, then pass the returned chatId here to watch
its output live". handleStartChatRun's own comment says the same. The
intent was always there; only the slot claim was missing.
Claims the slot right after start(). The chat is freshly provisioned so
nothing contends for it, and the workflow's clearChatActiveStream already
releases it on run end — so this just closes the loop symmetrically with
the interactive path. Best-effort: a failed claim costs resumability, not
the run.
Refs recoupable/chat#1923
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Preview verification — every documented path exercised, plus one real bug foundPreview The bug the preview caughtThe first run of this test returned 204 on a live headless run. That contradicted the published contract in both directions: Fixed in ResultsRun
B is the assertion that matters — it proves the resume is zero-based and gap-free, not merely that a stream came back. I diffed the first chunk of the The slot claim is proven by A (200 with the correct run id, only possible if the claim landed); the release by J/K (null afterwards). Docs drift found — needs a follow-up docs PR
{"status":"error","missing_fields":["startIndex"],"error":"startIndex must be a non-negative integer"}
{"status":"error","error":"Forbidden"}Every 4xx on this endpoint uses that shape. The schema is pre-existing — it already backed 401/403/404 before this PR — but my new 400 references it too, so it is in scope to flag. The live response is ground truth here; the docs are wrong, and I'd rather say so than quietly ship a contract that misdescribes its own error body. Not fixing it in this PR since docs#286 is already merged — it wants its own PR, which I'll open unless you'd rather fold it elsewhere. Suite status after the fix commit
|
There was a problem hiding this comment.
3 issues found across 2 files (changes from recent commits).
Confidence score: 3/5
- In
lib/chat/runs/handleStartChatRun.ts,compareAndSetChatActiveStreamIdnow runs afterstart(), so if the CAS throws (DB/network) instead of returning{ ok:false }, the error path can revoke after launch and leave run state inconsistent for users—move the claim/guarding flow so failures are handled before or atomically with starting the workflow. - In
lib/chat/runs/handleStartChatRun.ts, writingactive_stream_idonly afterstart()diverges from the documented CAS placeholder-first pattern, which creates a race for very fast runs that can finish before ownership is claimed—restore the pending-claim-then-promote sequence to de-risk missed or conflicting stream ownership. handleStartChatRuninlib/chat/runs/handleStartChatRun.tsis now over the 100-line style limit, which raises maintenance risk around complex error/cleanup paths—extract the new claim/revocation logic into a focused helper to keep behavior easier to reason about and test.
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/chat/runs/__tests__/handleStartChatRun.test.ts">
<violation number="1" location="lib/chat/runs/__tests__/handleStartChatRun.test.ts:136">
P2: The new claim call is placed inside the main try after start() has already launched the workflow. If compareAndSetChatActiveStreamId throws (DB/network error) rather than returning {ok:false}, the catch block revokes the ephemeral key for a run that is already live and returns 500, which contradicts the 'best-effort, don't fail a started run' comment and can cause the caller to retry (duplicate run). The test only covers the claimed:true path, so this failure branch is unverified. Consider wrapping the claim in its own try/catch that logs and continues, and add a test for the throw case asserting the key is not revoked and the response is still 202.</violation>
</file>
<file name="lib/chat/runs/handleStartChatRun.ts">
<violation number="1" location="lib/chat/runs/handleStartChatRun.ts:73">
P2: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
The `handleStartChatRun` handler is now 114 lines, exceeding the 100-line limit prescribed by the codebase style rule. The newly added `compareAndSetChatActiveStreamId` claim block and its lengthy inline comment pushed the module past the threshold. Consider extracting the claim logic and error handling into a dedicated helper module to keep this handler focused and within the limit.</violation>
<violation number="2" location="lib/chat/runs/handleStartChatRun.ts:80">
P3: The new active_stream_id claim is written after `start()` returns, which differs from the CAS helper's documented pattern (claim a pending placeholder before start, then promote to the real run id). For a run that completes before the claim executes, the workflow's `clearChatActiveStream` release can happen first and the claim then overwrites the slot with an already-terminal run id that no later workflow will clear. The GET /stream route appears to self-heal this (returning 204 and clearing the stale id), so this mainly costs resumability for fast-completing runs and leaves a transient stale slot. Consider claiming the slot before `start()` with a placeholder and promoting to `run.runId` afterward, matching the interactive path, to close the race.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| it("claims chats.active_stream_id with the run id so the run is resumable", async () => { | ||
| await handleStartChatRun({} as never); | ||
|
|
||
| expect(compareAndSetChatActiveStreamId).toHaveBeenCalledWith("chat-1", null, "wrun_abc"); |
There was a problem hiding this comment.
P2: The new claim call is placed inside the main try after start() has already launched the workflow. If compareAndSetChatActiveStreamId throws (DB/network error) rather than returning {ok:false}, the catch block revokes the ephemeral key for a run that is already live and returns 500, which contradicts the 'best-effort, don't fail a started run' comment and can cause the caller to retry (duplicate run). The test only covers the claimed:true path, so this failure branch is unverified. Consider wrapping the claim in its own try/catch that logs and continues, and add a test for the throw case asserting the key is not revoked and the response is still 202.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chat/runs/__tests__/handleStartChatRun.test.ts, line 136:
<comment>The new claim call is placed inside the main try after start() has already launched the workflow. If compareAndSetChatActiveStreamId throws (DB/network error) rather than returning {ok:false}, the catch block revokes the ephemeral key for a run that is already live and returns 500, which contradicts the 'best-effort, don't fail a started run' comment and can cause the caller to retry (duplicate run). The test only covers the claimed:true path, so this failure branch is unverified. Consider wrapping the claim in its own try/catch that logs and continues, and add a test for the throw case asserting the key is not revoked and the response is still 202.</comment>
<file context>
@@ -121,4 +125,14 @@ describe("handleStartChatRun", () => {
+ it("claims chats.active_stream_id with the run id so the run is resumable", async () => {
+ await handleStartChatRun({} as never);
+
+ expect(compareAndSetChatActiveStreamId).toHaveBeenCalledWith("chat-1", null, "wrun_abc");
+ });
});
</file context>
| @@ -8,6 +8,7 @@ import { mintEphemeralAccountKey } from "@/lib/keys/mintEphemeralAccountKey"; | |||
| import { deleteApiKey } from "@/lib/supabase/account_api_keys/deleteApiKey"; | |||
There was a problem hiding this comment.
P2: Custom agent: Enforce Clear Code Style and Maintainability Practices
The handleStartChatRun handler is now 114 lines, exceeding the 100-line limit prescribed by the codebase style rule. The newly added compareAndSetChatActiveStreamId claim block and its lengthy inline comment pushed the module past the threshold. Consider extracting the claim logic and error handling into a dedicated helper module to keep this handler focused and within the limit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chat/runs/handleStartChatRun.ts, line 73:
<comment>The `handleStartChatRun` handler is now 114 lines, exceeding the 100-line limit prescribed by the codebase style rule. The newly added `compareAndSetChatActiveStreamId` claim block and its lengthy inline comment pushed the module past the threshold. Consider extracting the claim logic and error handling into a dedicated helper module to keep this handler focused and within the limit.</comment>
<file context>
@@ -69,6 +70,21 @@ export async function handleStartChatRun(request: NextRequest): Promise<Response
}),
]);
+ // Claim the chat's stream slot with this run so `GET /api/chat/{chatId}/stream`
+ // can resume it — that route keys on `active_stream_id`, so without this a
+ // headless run is unresumable and the documented "watch its output live"
</file context>
| // nothing contends for the slot; the workflow's `clearChatActiveStream` | ||
| // releases it on run end. Best-effort: a failed claim costs resumability, not | ||
| // the run, so don't fail a started run over it. | ||
| const claimed = await compareAndSetChatActiveStreamId(provisioned.chat.id, null, run.runId); |
There was a problem hiding this comment.
P3: The new active_stream_id claim is written after start() returns, which differs from the CAS helper's documented pattern (claim a pending placeholder before start, then promote to the real run id). For a run that completes before the claim executes, the workflow's clearChatActiveStream release can happen first and the claim then overwrites the slot with an already-terminal run id that no later workflow will clear. The GET /stream route appears to self-heal this (returning 204 and clearing the stale id), so this mainly costs resumability for fast-completing runs and leaves a transient stale slot. Consider claiming the slot before start() with a placeholder and promoting to run.runId afterward, matching the interactive path, to close the race.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chat/runs/handleStartChatRun.ts, line 80:
<comment>The new active_stream_id claim is written after `start()` returns, which differs from the CAS helper's documented pattern (claim a pending placeholder before start, then promote to the real run id). For a run that completes before the claim executes, the workflow's `clearChatActiveStream` release can happen first and the claim then overwrites the slot with an already-terminal run id that no later workflow will clear. The GET /stream route appears to self-heal this (returning 204 and clearing the stale id), so this mainly costs resumability for fast-completing runs and leaves a transient stale slot. Consider claiming the slot before `start()` with a placeholder and promoting to `run.runId` afterward, matching the interactive path, to close the race.</comment>
<file context>
@@ -69,6 +70,21 @@ export async function handleStartChatRun(request: NextRequest): Promise<Response
+ // nothing contends for the slot; the workflow's `clearChatActiveStream`
+ // releases it on run end. Best-effort: a failed claim costs resumability, not
+ // the run, so don't fail a started run over it.
+ const claimed = await compareAndSetChatActiveStreamId(provisioned.chat.id, null, run.runId);
+ if (!claimed.ok || !claimed.claimed) {
+ console.error(
</file context>
…me route
Two gaps found reviewing this route against upstream open-agents.
1. Admin override. validateChatOwnership called validateAuthContext with no
override options, so an org/admin key got a 403 on a chat it legitimately
administers — the same defect as DELETE /api/tasks (chat#1918). Now reads
`account_id` from the query string and passes it through.
Query rather than body: both /stream (GET) and /stop (POST) carry their id
in the path and parse no body, so a query param is the one channel that
works for both without consuming the request. validateAuthContext still
decides whether the caller may use the override, so this does not weaken
the check — it just stops discarding a legitimate one.
Because the validator is shared, this fixes POST /api/chat/{chatId}/stop at
the same time, which had the identical limitation before this PR.
2. x-workflow-stream-tail-index. Upstream returns readable.getTailIndex() so a
client knows which startIndex to send on its next reconnect; the SDK's
WorkflowChatTransport reads the same header to compute absolute chunk
positions. Without it a reconnect replays from chunk zero. getTailIndex()
is available on the WorkflowReadableStream in workflow@4.2.4.
Best-effort: if the runtime cannot report a tail index we still stream. A
replaying client beats no client.
Deliberately unchanged, having compared both against upstream:
- A failed getRun still returns 502 and keeps the slot. Upstream clears the
slot and returns 204 on any error; that would tell a client with a live run
to stop reconnecting, which is the silent truncation this route exists to
prevent. Ours mirrors reconcileExistingActiveStream.
- wrapWorkflowStreamWatcher stays instead of upstream's
createCancelableReadableStream: ours also reconciles orphaned tool-calls and
propagates cancel to the run.
Full api suite 4,329 pass.
Refs recoupable/chat#1923
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/chat/validateChatOwnership.ts (1)
44-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
validateChatOwnershipinto focused helpers.The function spans Lines 40–65 and handles query parsing, authentication, UUID validation, two database reads, ownership comparison, and response construction. Extract the chat/session loading and ownership check so this request boundary remains small and focused. Centralize the repeated
"Chat not found"response while refactoring.As per coding guidelines, functions longer than 20 lines must be flagged, and functions should remain small and focused.
🤖 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/chat/validateChatOwnership.ts` around lines 44 - 45, Refactor validateChatOwnership into focused helpers: keep query parsing, authentication, and final response handling at the request boundary, while extracting chat/session loading and ownership comparison into separate functions. Centralize the repeated “Chat not found” response in one shared path, preserve UUID validation and existing ownership behavior, and ensure each function remains under 20 lines.Source: Coding guidelines
lib/chat/runs/handleStartChatRun.ts (1)
73-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract active-stream claiming into a focused helper.
handleStartChatRunspans Lines 37-114 and now owns several lifecycle responsibilities. Move the CAS operation and its best-effort error boundary intolib/chat/claimChatActiveStream.ts, exportingclaimChatActiveStream.This keeps the run-start handler focused and makes claim failure behavior easier to test.
🤖 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/chat/runs/handleStartChatRun.ts` around lines 73 - 87, Extract the compareAndSetChatActiveStreamId call and its best-effort failure logging from handleStartChatRun into a new exported claimChatActiveStream helper in lib/chat/claimChatActiveStream.ts. Have the helper accept the chat ID and run ID, preserve the existing CAS arguments and console.error details, then invoke it from handleStartChatRun without allowing claim failure to fail the started run.Sources: Coding guidelines, Path instructions
🤖 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/chat/runs/handleStartChatRun.ts`:
- Around line 80-86: Handle rejections from compareAndSetChatActiveStreamId
within the claim block in handleStartChatRun, logging the failure and continuing
to the existing 202 response instead of allowing the outer catch to revoke
ephemeralKeyId or return 500. Preserve the existing handling for unsuccessful
claim results, and add a regression test confirming a rejected claim returns 202
without revoking the key.
In `@lib/chat/validateChatOwnership.ts`:
- Around line 44-45: Rename validateChatOwnership.ts and its exported function
to the required validate<EndpointName>Query.ts pattern, such as
validateChatOwnershipQuery.ts and validateChatOwnershipQuery. Update all stream
and stop callers and imports together, preserving the existing request
validation and ownership behavior.
- Around line 44-45: Validate the accountIdOverride in validateChatOwnership
with z.uuid() or the existing account-ID schema immediately after reading
account_id and before calling validateAuthContext. Reject empty or malformed
values at this boundary, while preserving the existing authentication flow for
valid overrides.
---
Nitpick comments:
In `@lib/chat/runs/handleStartChatRun.ts`:
- Around line 73-87: Extract the compareAndSetChatActiveStreamId call and its
best-effort failure logging from handleStartChatRun into a new exported
claimChatActiveStream helper in lib/chat/claimChatActiveStream.ts. Have the
helper accept the chat ID and run ID, preserve the existing CAS arguments and
console.error details, then invoke it from handleStartChatRun without allowing
claim failure to fail the started run.
In `@lib/chat/validateChatOwnership.ts`:
- Around line 44-45: Refactor validateChatOwnership into focused helpers: keep
query parsing, authentication, and final response handling at the request
boundary, while extracting chat/session loading and ownership comparison into
separate functions. Centralize the repeated “Chat not found” response in one
shared path, preserve UUID validation and existing ownership behavior, and
ensure each function remains under 20 lines.
🪄 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 Plus
Run ID: 0d9a6140-7179-4df5-b8e5-82eac7079558
⛔ Files ignored due to path filters (3)
lib/chat/__tests__/handleResumeChatStream.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/chat/__tests__/validateChatOwnership.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/chat/runs/__tests__/handleStartChatRun.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (3)
lib/chat/handleResumeChatStream.tslib/chat/runs/handleStartChatRun.tslib/chat/validateChatOwnership.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/chat/handleResumeChatStream.ts
| const claimed = await compareAndSetChatActiveStreamId(provisioned.chat.id, null, run.runId); | ||
| if (!claimed.ok || !claimed.claimed) { | ||
| console.error( | ||
| "[handleStartChatRun] could not claim active_stream_id; run is not resumable:", | ||
| { chatId: provisioned.chat.id, runId: run.runId }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep claim failures out of the started-run failure path.
If compareAndSetChatActiveStreamId rejects, the outer catch at Lines 100-112 revokes ephemeralKeyId and returns 500 after start already created run.runId. A caller retry can create a duplicate run, and the active workflow can lose its credential.
Catch claim exceptions inside this block. Log the failure and continue with the 202 response. The helper’s contract is in lib/chat/compareAndSetChatActiveStreamId.ts, Lines 34-49.
Proposed fix
- const claimed = await compareAndSetChatActiveStreamId(provisioned.chat.id, null, run.runId);
- if (!claimed.ok || !claimed.claimed) {
- console.error(
- "[handleStartChatRun] could not claim active_stream_id; run is not resumable:",
- { chatId: provisioned.chat.id, runId: run.runId },
- );
+ try {
+ const claimed = await compareAndSetChatActiveStreamId(
+ provisioned.chat.id,
+ null,
+ run.runId,
+ );
+ if (!claimed.ok || !claimed.claimed) {
+ console.error(
+ "[handleStartChatRun] could not claim active_stream_id; run is not resumable:",
+ { chatId: provisioned.chat.id, runId: run.runId },
+ );
+ }
+ } catch (claimError) {
+ console.error("[handleStartChatRun] failed to claim active_stream_id:", {
+ chatId: provisioned.chat.id,
+ runId: run.runId,
+ error: claimError,
+ });
}Add a regression test for a rejected claim. Verify that the handler still returns 202 and does not revoke the key.
📝 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 claimed = await compareAndSetChatActiveStreamId(provisioned.chat.id, null, run.runId); | |
| if (!claimed.ok || !claimed.claimed) { | |
| console.error( | |
| "[handleStartChatRun] could not claim active_stream_id; run is not resumable:", | |
| { chatId: provisioned.chat.id, runId: run.runId }, | |
| ); | |
| } | |
| try { | |
| const claimed = await compareAndSetChatActiveStreamId( | |
| provisioned.chat.id, | |
| null, | |
| run.runId, | |
| ); | |
| if (!claimed.ok || !claimed.claimed) { | |
| console.error( | |
| "[handleStartChatRun] could not claim active_stream_id; run is not resumable:", | |
| { chatId: provisioned.chat.id, runId: run.runId }, | |
| ); | |
| } | |
| } catch (claimError) { | |
| console.error("[handleStartChatRun] failed to claim active_stream_id:", { | |
| chatId: provisioned.chat.id, | |
| runId: run.runId, | |
| error: claimError, | |
| }); | |
| } |
🤖 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/chat/runs/handleStartChatRun.ts` around lines 80 - 86, Handle rejections
from compareAndSetChatActiveStreamId within the claim block in
handleStartChatRun, logging the failure and continuing to the existing 202
response instead of allowing the outer catch to revoke ephemeralKeyId or return
500. Preserve the existing handling for unsuccessful claim results, and add a
regression test confirming a rejected claim returns 202 without revoking the
key.
Source: Coding guidelines
| const accountIdOverride = new URL(request.url).searchParams.get("account_id") ?? undefined; | ||
| const auth = await validateAuthContext(request, { accountId: accountIdOverride }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the required validator filename pattern.
validateChatOwnership.ts matches its exported function, but it does not match the more-specific lib/**/validate*.ts rule. Rename both the file and export, for example to validateChatOwnershipQuery.ts and validateChatOwnershipQuery, or split request validation from ownership loading. Update the stream and stop callers together.
As per path instructions, validation files must use the validate<EndpointName>Body.ts or validate<EndpointName>Query.ts naming pattern.
🤖 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/chat/validateChatOwnership.ts` around lines 44 - 45, Rename
validateChatOwnership.ts and its exported function to the required
validate<EndpointName>Query.ts pattern, such as validateChatOwnershipQuery.ts
and validateChatOwnershipQuery. Update all stream and stop callers and imports
together, preserving the existing request validation and ownership behavior.
Source: Path instructions
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A50 -B10 '\bvalidateAuthContext\b' lib
rg -n -A30 -B10 'account_id|accountIdOverride' lib/chat/__tests__ lib/chatRepository: recoupable/api
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate target files =="
fd -a 'validateChatOwnership\.ts|validateAuthContext\.ts' lib | sed 's#^\./##'
echo "== target implementation =="
cat -n lib/chat/validateChatOwnership.ts
echo "== auth implementation outline and relevant sections =="
wc -l lib/auth/validateAuthContext.ts
sed -n '1,240p' lib/auth/validateAuthContext.ts | cat -nRepository: recoupable/api
Length of output: 8463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate validateAccountIdOverride =="
fd -a 'validateAccountIdOverride\.ts' lib | sed 's#^\./##'
echo "== validateAccountIdOverride implementation =="
cat -n lib/auth/validateAccountIdOverride.ts
echo "== account ID/Zod usages in auth =="
rg -n "accountId|account_id|validateAccountIdOverride|uuid|z\.string|z\.uuid" lib/auth lib/zod -g '*.ts'Repository: recoupable/api
Length of output: 8046
Validate the account_id query override with a Zod schema before authentication.
searchParams.get("account_id") can contain empty, malformed, or unauthorized values, and this helper routes it as the authorization boundary. Use z.uuid() or the existing account-ID schema before calling validateAuthContext; do not rely on downstream query parsing or auth logic to handle this boundary.
🤖 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/chat/validateChatOwnership.ts` around lines 44 - 45, Validate the
accountIdOverride in validateChatOwnership with z.uuid() or the existing
account-ID schema immediately after reading account_id and before calling
validateAuthContext. Reject empty or malformed values at this boundary, while
preserving the existing authentication flow for valid overrides.
Source: Path instructions
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Confidence score: 4/5
- In
lib/chat/validateChatOwnership.ts,account_idquery validation is inconsistent: an empty value like?account_id=is treated as the caller’s own account while other malformed values reach authorization and return 403, which can cause ambiguous auth behavior and harder-to-debug access outcomes — explicitly reject empty/malformedaccount_idup front with a uniform validation error path.
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/chat/validateChatOwnership.ts">
<violation number="1" location="lib/chat/validateChatOwnership.ts:44">
P3: An explicitly empty or malformed `account_id` is not rejected: `?account_id=` falls through as the caller's own account, while other malformed values reach authorization and return 403. Validate this query field as an optional UUID so supplied invalid overrides consistently return 400 instead of changing/obscuring request semantics.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| request: NextRequest, | ||
| chatId: string, | ||
| ): Promise<NextResponse | ValidatedChatOwnership> { | ||
| const accountIdOverride = new URL(request.url).searchParams.get("account_id") ?? undefined; |
There was a problem hiding this comment.
P3: An explicitly empty or malformed account_id is not rejected: ?account_id= falls through as the caller's own account, while other malformed values reach authorization and return 403. Validate this query field as an optional UUID so supplied invalid overrides consistently return 400 instead of changing/obscuring request semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chat/validateChatOwnership.ts, line 44:
<comment>An explicitly empty or malformed `account_id` is not rejected: `?account_id=` falls through as the caller's own account, while other malformed values reach authorization and return 403. Validate this query field as an optional UUID so supplied invalid overrides consistently return 400 instead of changing/obscuring request semantics.</comment>
<file context>
@@ -32,7 +41,8 @@ export async function validateChatOwnership(
chatId: string,
): Promise<NextResponse | ValidatedChatOwnership> {
- const auth = await validateAuthContext(request);
+ const accountIdOverride = new URL(request.url).searchParams.get("account_id") ?? undefined;
+ const auth = await validateAuthContext(request, { accountId: accountIdOverride });
if (auth instanceof NextResponse) return auth;
</file context>
| const accountIdOverride = new URL(request.url).searchParams.get("account_id") ?? undefined; | |
| const accountIdResult = z | |
| .string() | |
| .uuid("account_id must be a valid UUID") | |
| .optional() | |
| .safeParse(new URL(request.url).searchParams.get("account_id") ?? undefined); | |
| if (!accountIdResult.success) { | |
| const firstError = accountIdResult.error.issues[0]; | |
| return validationErrorResponse(firstError.message, firstError.path); | |
| } | |
| const accountIdOverride = accountIdResult.data; |
…isibility Three refinements after comparing against upstream open-agents. 1. Consume x-workflow-stream-tail-index. recoupable/api#809 now reports where the read it served ends; a custom transport fetch captures it and prepareReconnectToStreamRequest sends startIndex = tail + 1. Reconnects are now gap-free instead of replaying the turn from chunk zero. 2. Thresholds tightened: stall 20s -> 10s, cooldown 15s -> 8s (upstream's STREAM_RECOVERY_MIN_INTERVAL_MS), poll 5s -> 3s. Safe precisely because of (1) — an unnecessary reconnect now costs a request rather than re-rendering content the client already has. NOT upstream's STREAM_RECOVERY_STALL_MS = 4_000. That constant feeds a scheduler their shouldScheduleStallRecovery unconditionally disables (`void options; return false`), so it is not a live stall threshold to copy. It also would not survive our workload: a single legitimate tool call streams nothing for up to ~200s (measured on prod), so a 4s window would fire dozens of pointless reconnects per turn. 3. Visibility probe, upstream's only live recovery trigger. A backgrounded tab can have its connection killed silently and no amount of waiting produces a chunk to time out on, so a visibility check skips the silence window. The cooldown still applies, so a focus-flapping tab cannot spam reconnects. Kept our stall-based trigger rather than adopting upstream's posture wholesale: their live triggers are `status === "error"` and a visibility probe when `status === "ready"`, and our failure mode produces neither — the stream ends with a clean [DONE] and no error, on a visible tab. 366 chat tests pass; tsc delta 0 vs main. Refs #1923 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ised tail Preview testing of recoupable/api#809 caught this. The route reports x-workflow-stream-tail-index at the moment the read is OPENED, not when it ends: a live read that returned 22 chunks advertised a tail of 9. Resuming at tail + 1 would therefore have replayed 12 chunks the client had already rendered — the exact duplication startIndex exists to avoid. That matches the SDK contract on closer reading: the header is a base for computing absolute positions, and "subsequent retries always resume from the last received chunk". So count the chunks instead. The transport fetch tees the response body, counts SSE frames (excluding the [DONE] terminator), and tracks the absolute index as requestedStartIndex + framesSeen. Reconnect sends that + 1. Best-effort: a torn read just means the next reconnect resumes from the last index counted, which is still ahead of replaying from zero. Refs #1923 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-verified on
|
| Check | Result |
|---|---|
x-workflow-stream-tail-index present on a live resume |
✅ x-workflow-stream-tail-index: 9 alongside x-workflow-run-id |
account_id = own account |
✅ 204 — auth passes, route runs, nothing to resume |
account_id = another account, personal key |
✅ 403 Access denied to specified account_id |
That last row is the one that matters: the override is validated by validateAuthContext, not trusted blindly. Passing it through fixed the admin case without opening a hole — a personal key still cannot reach another account's chat by asserting an id.
Regression surface, re-run on this head
400 (startIndex=abc), 400 (startIndex=-1), 401 (no auth), 400 (malformed uuid), 404 (unknown chat), 403 (other account's key) — all unchanged from the bcd819bb run.
What this caught — a bug in the client, not this route
The header reported tail: 9 while that same read delivered 22 chunks. getTailIndex() is evaluated when the read is opened, so a read that stays open past it under-reports.
That is correct behaviour for the route — and it matches the SDK contract, where the header is a base for computing absolute positions and "subsequent retries always resume from the last received chunk". But it invalidated how chat#1924 was consuming it: sending startIndex = tail + 1 would have resumed at 10 when the client already had 22, replaying 12 rendered chunks — the exact duplication startIndex exists to prevent.
Fixed on the chat side (cb8cea2e): the transport now tees the response body, counts SSE frames excluding the [DONE] terminator, and reconnects at requestedStartIndex + framesSeen + 1. No change needed here — this route's contract is right as written.
Worth stating plainly: unit tests could not have caught that. It only shows up against a live stream where chunks keep arriving after the read opens.
Status
- Full api suite 4,329 passing;
tsc --noEmitzero errors in files this PR touches;eslintclean. - Every documented path re-exercised on the current head.
…d-turn (#1924) * fix(chat): reconnect a dropped response stream instead of freezing mid-turn A long turn's SSE stream can end before the run does. Reproduced on prod 2026-08-02: the connection closed at ~123s with a clean [DONE] and no finish chunk while the workflow ran on to completion. useChat saw a stream that ended without a terminal chunk, stopped rendering, and never marked the message complete — no error, no retry, composer looking idle. The user got 6 of 13 iterations and had to refresh to see the rest. The AI SDK already has the machinery: DefaultChatTransport.reconnectToStream defaults to GET {api}/{chatId}/stream, which is exactly the route recoupable/api#809 implements. We had neither the client wiring nor the endpoint. - resume: true on useChat — re-attach to an in-progress response on mount, so returning to a chat mid-turn keeps rendering. - useStreamRecovery — watches an in-flight turn for silence and calls resumeStream(). Also re-checks on visibilitychange, since a backgrounded tab is where drops are most likely and least likely to be noticed. - shouldRecoverStalledStream — the pure decision, unit-tested. - prepareReconnectToStreamRequest on the transport — the resume route is authenticated like every other endpoint, so without this the reconnect 401s and a dropped stream stays dropped. Silence-based rather than duration-based: a turn still streaming is healthy however long it runs, and a turn gone quiet is suspect even if it just started. A cooldown stops a permanently dead stream being retried every tick. Depends on recoupable/api#809 (the resume route) and docs#286 (contract). Refs #1923 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(chat): resume from the tail index, tighten thresholds, probe on visibility Three refinements after comparing against upstream open-agents. 1. Consume x-workflow-stream-tail-index. recoupable/api#809 now reports where the read it served ends; a custom transport fetch captures it and prepareReconnectToStreamRequest sends startIndex = tail + 1. Reconnects are now gap-free instead of replaying the turn from chunk zero. 2. Thresholds tightened: stall 20s -> 10s, cooldown 15s -> 8s (upstream's STREAM_RECOVERY_MIN_INTERVAL_MS), poll 5s -> 3s. Safe precisely because of (1) — an unnecessary reconnect now costs a request rather than re-rendering content the client already has. NOT upstream's STREAM_RECOVERY_STALL_MS = 4_000. That constant feeds a scheduler their shouldScheduleStallRecovery unconditionally disables (`void options; return false`), so it is not a live stall threshold to copy. It also would not survive our workload: a single legitimate tool call streams nothing for up to ~200s (measured on prod), so a 4s window would fire dozens of pointless reconnects per turn. 3. Visibility probe, upstream's only live recovery trigger. A backgrounded tab can have its connection killed silently and no amount of waiting produces a chunk to time out on, so a visibility check skips the silence window. The cooldown still applies, so a focus-flapping tab cannot spam reconnects. Kept our stall-based trigger rather than adopting upstream's posture wholesale: their live triggers are `status === "error"` and a visibility probe when `status === "ready"`, and our failure mode produces neither — the stream ends with a clean [DONE] and no error, on a visible tab. 366 chat tests pass; tsc delta 0 vs main. Refs #1923 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(chat): type the recovery ref instead of declaring an unused param Lint flagged the placeholder param in the useRef initializer. Typing the ref gives the same call signature without an unused binding. Refs #1923 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(chat): resume from the chunk we actually received, not the advertised tail Preview testing of recoupable/api#809 caught this. The route reports x-workflow-stream-tail-index at the moment the read is OPENED, not when it ends: a live read that returned 22 chunks advertised a tail of 9. Resuming at tail + 1 would therefore have replayed 12 chunks the client had already rendered — the exact duplication startIndex exists to avoid. That matches the SDK contract on closer reading: the header is a base for computing absolute positions, and "subsequent retries always resume from the last received chunk". So count the chunks instead. The transport fetch tees the response body, counts SSE frames (excluding the [DONE] terminator), and tracks the absolute index as requestedStartIndex + framesSeen. Reconnect sends that + 1. Best-effort: a torn read just means the next reconnect resumes from the last index counted, which is still ahead of replaying from zero. Refs #1923 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(chat): rebuild the reconnect URL instead of appending to the base Preview testing on the chat#1924 branch: every reconnect hit `/api/chat?startIndex=376` and got a 405. prepareReconnectToStreamRequest receives `api` as the BASE (`…/api/chat`), not the reconnect URL — the SDK only falls back to `${api}/${id}/stream` when the callback returns no `api` of its own. Returning one replaces the whole URL, so appending `?startIndex=N` to the base produced a GET against the POST-only chat endpoint. Rebuilds the path from `api` + the `id` the callback is handed. The rest of the chain was already working in that run: stall detection fired twice, and the chunk counter produced startIndex 376 then 409 off real deltas. Only the URL was wrong. Refs #1923 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(chat): reconnect to the api-minted chat id, not the useChat instance id Second preview run: the URL shape was right but every reconnect 404'd — /api/chat/ca264e2f.../stream while the page was on chat d40b5147... The `id` prepareReconnectToStreamRequest receives is the useChat INSTANCE id. For a new chat that is still the client placeholder; the api-minted id arrives later and lives in chatIdRef, which is exactly why that ref exists (useChat captures the transport at mount and never swaps it). So the reconnect was addressing a chat that does not exist. Uses chatIdRef.current — the same value the request body already sends as `chatId`. Refs #1923 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(chat): extract stream-position tracking, reset it on a fresh read Addresses both review comments on chat#1924. OCP — the counting fetch and the reconnect-URL construction were net new logic inline in useChatTransport. Extracted to their own lib files, so the hook is wiring again and both units are directly testable: - lib/chat/createChunkCountingFetch.ts — wraps fetch, counts SSE frames off a tee of the body, reports the absolute position. - lib/chat/buildStreamReconnectUrl.ts — pure URL builder. Stale index — the position ref was never reset, so a reconnect could send a startIndex belonging to a previous turn or a different chat and skip chunks the client never saw. Valid, and narrower than it first looks: the counter re-seeds from each request's own startIndex, so it self-corrects on the first frame of any new read. The exposed window is between issuing a request and its first frame — which, with a 10s stall threshold and a slow sandbox start, a reconnect can land in. Closed at both ends: - createChunkCountingFetch reports null the moment it issues a read that has no startIndex, i.e. one starting from chunk zero, before awaiting the response. That covers a new turn and a new chat's first POST. - useChatTransport clears the ref when chatId changes, since the transport is memoised for the lifetime of the hook. 9 new unit tests cover the counting, the seeding, both reset paths, and pass-through. 88 chat test files pass; tsc delta 0 vs main. Refs #1923 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Implements the resume route for chat#1923. Contract: docs#286.
Why
The endpoint was already documented and never implemented.
api-reference/chat/workflow-stream.mdx("Resume Chat Stream") and a full spec block have shipped since the workflow cutover, cross-referenced fromPOST /api/chat,POST /api/chat/runsandGET /api/chat/runs/{runId}— butapp/api/chat/[chatId]/contained onlystop/. Documented-but-missing drift.It matters now because a long turn's SSE stream can end before the run does. Reproduced on prod 2026-08-02 (
wrun_01KZ04KVHBVA405WDFVYCEADE8): the stream closed at ~123 s with a cleandata: [DONE]and nofinishchunk while the workflow ran on to completion. The client rendered 6 of 13 iterations and froze — no stop button, composer idle — while everything after was generated and persisted but never delivered. Our only recovery path,maybeResumeChatStream, runs insidePOST /api/chat, which is why a page refresh recovered and sitting still did not.What
app/api/chat/[chatId]/stream/route.tsGET+OPTIONS.maxDuration = 800to matchPOST /api/chat— a resumed stream lives as long as the turn it follows.lib/chat/handleResumeChatStream.tsx-workflow-run-idwhen live; 204 when nothing to resume (clearing a staleactive_stream_id); 502 when the status lookup throws.lib/chat/parseStreamStartIndex.tsinteger, minimum 0.lib/chat/validateChatOwnership.tsvalidateStopChatWorkflowRequestso/stopand/streamshare one auth + ownership rule.Reuses the existing
wrapWorkflowStreamWatcher, so a resumed stream gets the same tool-call reconciliation and cancel-propagation as the primary one.Two deliberate calls
Negative
startIndexis rejected (400) even though the SDK accepts it. The SDK reads negative values relative to the end of a live stream, which resolves to a different absolute position on every call — it cannot give a client a gap-free resume, and the docs warn about exactly this. The published contract isminimum: 0.A failed status read returns 502, not 204. Reporting "nothing to resume" on a transient workflow-api blip would tell a client with a live run to stop reconnecting — precisely the silent truncation this route exists to prevent. This mirrors
reconcileExistingActiveStream, which already prefers conflict over clearing a slot it cannot confidently read.Tests — RED before GREEN, per unit
parseStreamStartIndex(6): absent →undefined; valid0and42; 400 for non-numeric, negative, fractional, and present-but-empty.handleResumeChatStream(8): 204 with no active stream (and nogetRuncall); 204 + stale-id clear on a terminal run; 200 withx-workflow-run-idandtext/event-stream;startIndexforwarded togetReadable;undefinedforwarded when absent; 400 on malformedstartIndexwithout touching the run; validator responses (401/403/404) propagated unchanged; 502 rather than 204 when the status lookup throws.Both files confirmed failing first (module-not-found), then implemented to green.
tsc --noEmit: 203 errors, zero in any file this PR touches — identical to themainbaseline (203).eslintclean.Verification still owed
Preview verification against a live in-flight run is not done yet and is the gate here: start a long turn, reconnect mid-run with a
startIndex, and confirm the resumed stream continues without duplicating or skipping chunks, plus the 204 / 400 / 403 paths against real ids. Results will be posted as a comment.Merge order
docs#286 → this → chat client reconnect. The docs PR publishes the
startIndexparameter this implements.🤖 Generated with Claude Code
Summary by cubic
Adds
GET /api/chat/{chatId}/streamto resume an in-progress chat response and make headless runs watchable live. Also honors theaccount_idadmin override and returns a stream tail index for precise reconnects.New Features
startIndex(>= 0). Returns 200 SSE withx-workflow-run-idand best-effortx-workflow-stream-tail-index; 204 when nothing to resume (clears staleactive_stream_id); 502 on run-status read failures.chats.active_stream_idon start soGET /api/chat/{chatId}/streamcan watch their output live.maxDuration = 800; adds CORSOPTIONS.Bug Fixes
account_idquery override for org/admin keys in bothGET /api/chat/{chatId}/streamandPOST /api/chat/{chatId}/stop, via sharedvalidateChatOwnershipfor consistent auth and ownership checks.Written for commit c75ece1. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor