Skip to content

Commit 9262f65

Browse files
sweetmantechclaude
andcommitted
refactor(chat-workflow): address PR review — SRP/DRY cleanup
Two review fixes per PR feedback: 1. SRP/DRY — drop the local errorResponse helper from handleChatWorkflowStream.ts; use the shared lib/networking/errorResponse and lib/zod/validationErrorResponse helpers instead. 2. SRP — move auth + body parsing out of handleChatWorkflowStream.ts into the validator. Rename validateChatWorkflowBody → validateChatWorkflow so it accepts a full NextRequest (like the existing validateChatRequest) and returns an auth-augmented body (accountId/orgId/authToken). The handler now opens with a single `validateChatWorkflow(request)` call. Tests reshaped to match new seams: - Validator test mocks validateAuthContext only - Handler test mocks validateChatWorkflow (the new seam) - Old "400 invalid JSON" + "400 missing chatId" handler tests collapsed into a single "validator short-circuit passes through" test — both are now the validator's responsibility, not the handler's 22/22 new tests green; full suite 2900/2900 pass; lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 89920a4 commit 9262f65

9 files changed

Lines changed: 243 additions & 216 deletions

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,3 +40,4 @@ yarn-error.log*
4040
# typescript
4141
*.tsbuildinfo
4242
next-env.d.ts
43+
.env*.local

‎lib/chat/__tests__/handleChatWorkflowStream.test.ts‎

Lines changed: 26 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,13 @@ import { describe, it, expect, vi, beforeEach } from "vitest";
22
import{NextRequest,NextResponse}from"next/server";
33

44
import{handleChatWorkflowStream}from"@/lib/chat/handleChatWorkflowStream";
5-
6-
import{validateAuthContext}from"@/lib/auth/validateAuthContext";
5+
import{validateChatWorkflow}from"@/lib/chat/validateChatWorkflow";
76
import{selectSessions}from"@/lib/supabase/sessions/selectSessions";
87
import{selectChats}from"@/lib/supabase/chats/selectChats";
98
import{isSandboxActive}from"@/lib/sandbox/isSandboxActive";
109

11-
vi.mock("@/lib/auth/validateAuthContext",()=>({
12-
validateAuthContext: vi.fn(),
10+
vi.mock("@/lib/chat/validateChatWorkflow",()=>({
11+
validateChatWorkflow: vi.fn(),
1312
}));
1413
vi.mock("@/lib/supabase/sessions/selectSessions",()=>({
1514
selectSessions: vi.fn(),
@@ -29,26 +28,27 @@ const OTHER_ACCOUNT_ID = "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb";
2928
constSESSION_ID="22222222-2222-2222-2222-222222222222";
3029
constCHAT_ID="11111111-1111-1111-1111-111111111111";
3130

32-
constvalidBody={
33-
messages: [{id: "m-1",role: "user",parts: [{type: "text",text: "hi"}]}],
34-
chatId: CHAT_ID,
35-
sessionId: SESSION_ID,
36-
};
37-
38-
functionmakeRequest(body: unknown=validBody): NextRequest{
31+
functionmakeRequest(): NextRequest{
3932
returnnewNextRequest("http://localhost/api/chat/workflow",{
4033
method: "POST",
4134
headers: {"x-api-key": "test-key","content-type": "application/json"},
42-
body: typeofbody==="string" ? body : JSON.stringify(body),
35+
body: JSON.stringify({messages: [],chatId: CHAT_ID,sessionId: SESSION_ID}),
4336
});
4437
}
4538

46-
functionmockOwnedSessionWithActiveSandbox(){
47-
vi.mocked(validateAuthContext).mockResolvedValue({
48-
accountId: ACCOUNT_ID,
39+
functionmockValidatedRequest(overrides: Partial<{accountId: string}>={}){
40+
vi.mocked(validateChatWorkflow).mockResolvedValue({
41+
messages: [],
42+
chatId: CHAT_ID,
43+
sessionId: SESSION_ID,
44+
accountId: overrides.accountId??ACCOUNT_ID,
4945
orgId: null,
5046
authToken: "test-key",
5147
});
48+
}
49+
50+
functionmockOwnedSessionWithActiveSandbox(){
51+
mockValidatedRequest();
5252
vi.mocked(selectSessions).mockResolvedValue([
5353
{id: SESSION_ID,account_id: ACCOUNT_ID,sandbox_state: {ready: true}}asnever,
5454
]);
@@ -61,54 +61,30 @@ describe("handleChatWorkflowStream (stub)", () => {
6161
vi.clearAllMocks();
6262
});
6363

64-
describe("auth",()=>{
65-
it("returns 401 short-circuit from validateAuthContext",async()=>{
64+
describe("validation short-circuits",()=>{
65+
it("returns the validator's short-circuit response unchanged (e.g. 401)",async()=>{
6666
constauthError=NextResponse.json(
6767
{status: "error",error: "Unauthorized"},
6868
{status: 401},
6969
);
70-
vi.mocked(validateAuthContext).mockResolvedValue(authError);
70+
vi.mocked(validateChatWorkflow).mockResolvedValue(authError);
7171
constres=awaithandleChatWorkflowStream(makeRequest());
7272
expect(res.status).toBe(401);
7373
});
74-
});
7574

76-
describe("body validation",()=>{
77-
it("returns 400 on invalid JSON body",async()=>{
78-
vi.mocked(validateAuthContext).mockResolvedValue({
79-
accountId: ACCOUNT_ID,
80-
orgId: null,
81-
authToken: "k",
82-
});
83-
constreq=newNextRequest("http://localhost/api/chat/workflow",{
84-
method: "POST",
85-
headers: {"x-api-key": "test-key","content-type": "application/json"},
86-
body: "{not-json",
87-
});
88-
constres=awaithandleChatWorkflowStream(req);
89-
expect(res.status).toBe(400);
90-
});
91-
92-
it("returns 400 when chatId is missing",async()=>{
93-
vi.mocked(validateAuthContext).mockResolvedValue({
94-
accountId: ACCOUNT_ID,
95-
orgId: null,
96-
authToken: "k",
97-
});
98-
const{chatId: _omit, ...rest}=validBody;
99-
constres=awaithandleChatWorkflowStream(makeRequest(rest));
75+
it("returns the validator's 400 unchanged (e.g. invalid body)",async()=>{
76+
constbadBody=NextResponse.json(
77+
{status: "error",error: "Invalid JSON body"},
78+
{status: 400},
79+
);
80+
vi.mocked(validateChatWorkflow).mockResolvedValue(badBody);
81+
constres=awaithandleChatWorkflowStream(makeRequest());
10082
expect(res.status).toBe(400);
10183
});
10284
});
10385

10486
describe("session / chat ownership",()=>{
105-
beforeEach(()=>{
106-
vi.mocked(validateAuthContext).mockResolvedValue({
107-
accountId: ACCOUNT_ID,
108-
orgId: null,
109-
authToken: "k",
110-
});
111-
});
87+
beforeEach(()=>mockValidatedRequest());
11288

11389
it("returns 404 when the session does not exist",async()=>{
11490
vi.mocked(selectSessions).mockResolvedValue([]);
Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
import{describe,it,expect,vi,beforeEach}from"vitest";
2+
import{NextRequest,NextResponse}from"next/server";
3+
4+
import{validateChatWorkflow}from"@/lib/chat/validateChatWorkflow";
5+
import{validateAuthContext}from"@/lib/auth/validateAuthContext";
6+
7+
vi.mock("@/lib/auth/validateAuthContext",()=>({
8+
validateAuthContext: vi.fn(),
9+
}));
10+
11+
constACCOUNT_ID="aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa";
12+
constCHAT_ID="11111111-1111-1111-1111-111111111111";
13+
constSESSION_ID="22222222-2222-2222-2222-222222222222";
14+
15+
constvalidBody={
16+
messages: [{id: "m-1",role: "user",parts: [{type: "text",text: "hi"}]}],
17+
chatId: CHAT_ID,
18+
sessionId: SESSION_ID,
19+
};
20+
21+
functionmakeRequest(body: unknown=validBody): NextRequest{
22+
returnnewNextRequest("http://localhost/api/chat/workflow",{
23+
method: "POST",
24+
headers: {"x-api-key": "k","content-type": "application/json"},
25+
body: typeofbody==="string" ? body : JSON.stringify(body),
26+
});
27+
}
28+
29+
functionmockAuthOk(){
30+
vi.mocked(validateAuthContext).mockResolvedValue({
31+
accountId: ACCOUNT_ID,
32+
orgId: null,
33+
authToken: "k",
34+
});
35+
}
36+
37+
describe("validateChatWorkflow",()=>{
38+
beforeEach(()=>vi.clearAllMocks());
39+
40+
describe("valid input",()=>{
41+
beforeEach(()=>mockAuthOk());
42+
43+
it("returns the validated body augmented with accountId / orgId / authToken",async()=>{
44+
constresult=awaitvalidateChatWorkflow(makeRequest());
45+
expect(result).not.toBeInstanceOf(NextResponse);
46+
if(resultinstanceofNextResponse)return;
47+
expect(result.chatId).toBe(CHAT_ID);
48+
expect(result.sessionId).toBe(SESSION_ID);
49+
expect(result.messages).toEqual(validBody.messages);
50+
expect(result.accountId).toBe(ACCOUNT_ID);
51+
expect(result.orgId).toBe(null);
52+
expect(result.authToken).toBe("k");
53+
});
54+
55+
it("accepts an optional context.contextLimit integer",async()=>{
56+
constresult=awaitvalidateChatWorkflow(
57+
makeRequest({ ...validBody,context: {contextLimit: 50}}),
58+
);
59+
expect(result).not.toBeInstanceOf(NextResponse);
60+
if(resultinstanceofNextResponse)return;
61+
expect(result.context?.contextLimit).toBe(50);
62+
});
63+
64+
it("accepts an empty messages array",async()=>{
65+
constresult=awaitvalidateChatWorkflow(makeRequest({ ...validBody,messages: []}));
66+
expect(result).not.toBeInstanceOf(NextResponse);
67+
});
68+
});
69+
70+
describe("invalid body",()=>{
71+
it("returns 400 when JSON is malformed",async()=>{
72+
constreq=newNextRequest("http://localhost/api/chat/workflow",{
73+
method: "POST",
74+
headers: {"x-api-key": "k","content-type": "application/json"},
75+
body: "{not-json",
76+
});
77+
constresult=awaitvalidateChatWorkflow(req);
78+
expect(result).toBeInstanceOf(NextResponse);
79+
if(!(resultinstanceofNextResponse))return;
80+
expect(result.status).toBe(400);
81+
});
82+
83+
it("returns 400 when chatId is missing",async()=>{
84+
const{chatId: _omit, ...rest}=validBody;
85+
constresult=awaitvalidateChatWorkflow(makeRequest(rest));
86+
expect(result).toBeInstanceOf(NextResponse);
87+
if(!(resultinstanceofNextResponse))return;
88+
expect(result.status).toBe(400);
89+
});
90+
91+
it("returns 400 when sessionId is missing",async()=>{
92+
const{sessionId: _omit, ...rest}=validBody;
93+
constresult=awaitvalidateChatWorkflow(makeRequest(rest));
94+
expect(result).toBeInstanceOf(NextResponse);
95+
if(!(resultinstanceofNextResponse))return;
96+
expect(result.status).toBe(400);
97+
});
98+
99+
it("returns 400 when messages is not an array",async()=>{
100+
constresult=awaitvalidateChatWorkflow(makeRequest({ ...validBody,messages: "nope"}));
101+
expect(result).toBeInstanceOf(NextResponse);
102+
if(!(resultinstanceofNextResponse))return;
103+
expect(result.status).toBe(400);
104+
});
105+
106+
it("returns 400 when chatId is empty string",async()=>{
107+
constresult=awaitvalidateChatWorkflow(makeRequest({ ...validBody,chatId: ""}));
108+
expect(result).toBeInstanceOf(NextResponse);
109+
if(!(resultinstanceofNextResponse))return;
110+
expect(result.status).toBe(400);
111+
});
112+
113+
it("returns 400 when context.contextLimit is not an integer",async()=>{
114+
constresult=awaitvalidateChatWorkflow(
115+
makeRequest({ ...validBody,context: {contextLimit: "fifty"}}),
116+
);
117+
expect(result).toBeInstanceOf(NextResponse);
118+
if(!(resultinstanceofNextResponse))return;
119+
expect(result.status).toBe(400);
120+
});
121+
122+
it("does not call validateAuthContext when body validation fails",async()=>{
123+
const{chatId: _omit, ...rest}=validBody;
124+
awaitvalidateChatWorkflow(makeRequest(rest));
125+
expect(validateAuthContext).not.toHaveBeenCalled();
126+
});
127+
});
128+
129+
describe("auth",()=>{
130+
it("returns the auth short-circuit response when validateAuthContext rejects",async()=>{
131+
constauthError=NextResponse.json(
132+
{status: "error",error: "Unauthorized"},
133+
{status: 401},
134+
);
135+
vi.mocked(validateAuthContext).mockResolvedValue(authError);
136+
constresult=awaitvalidateChatWorkflow(makeRequest());
137+
expect(result).toBeInstanceOf(NextResponse);
138+
if(!(resultinstanceofNextResponse))return;
139+
expect(result.status).toBe(401);
140+
});
141+
});
142+
});

‎lib/chat/__tests__/validateChatWorkflowBody.test.ts‎

Lines changed: 0 additions & 104 deletions
This file was deleted.

0 commit comments

Comments
 (0)