Skip to content

Commit 2a701a7

Browse files
committed
fix(plan): preserve fork scope and benchmark redaction boundaries
1 parent 53b1f06 commit 2a701a7

10 files changed

Lines changed: 142 additions & 51 deletions

File tree

‎apps/sim/app/api/mothership/chats/[chatId]/fork/route.test.ts‎

Lines changed: 36 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { copilotChats, member } from '@sim/db/schema'
1+
import { copilotChats, member, workspace } from '@sim/db/schema'
22
import { createSessionPrincipal } from '@sim/testing/factories/principal.factory'
33
import { createRouteContext } from '@sim/testing/helpers/http'
44
import { authBanMock } from '@sim/testing/mocks/auth-ban.mock'
@@ -145,17 +145,26 @@ function createRequest(chatId: string, body?: unknown) {
145145
})
146146
}
147147

148+
function mockWorkspaceForkRows(
149+
chat: Omit<typeof parentRow, 'resources'> & { resources: unknown[] } = parentRow
150+
) {
151+
resetDbChainMock()
152+
queueTableRows(copilotChats, [chat])
153+
queueTableRows(copilotChats, [chat])
154+
queueTableRows(workspace, [{ organizationId: null, archivedAt: null }])
155+
queueTableRows(copilotChats, [{ id: chat.id, memorySpaceId: null }])
156+
dbChainMockFns.returning.mockResolvedValue([{ id: 'row-id', workspaceId: 'ws-1' }])
157+
}
158+
148159
describe('POST /api/mothership/chats/[chatId]/fork', () => {
149160
beforeEach(() => {
150-
resetDbChainMock()
161+
mockWorkspaceForkRows()
151162
setEnv({ COPILOT_API_KEY: undefined })
152163
copilotHttpMockFns.mockAuthenticateCopilotRequestSessionOnly.mockResolvedValue({
153164
userId: 'user-1',
154165
isAuthenticated: true,
155166
principal: createSessionPrincipal(),
156167
})
157-
dbChainMockFns.limit.mockResolvedValue([parentRow])
158-
dbChainMockFns.returning.mockResolvedValue([{ id: 'row-id', workspaceId: 'ws-1' }])
159168
mockListForkableChatFiles.mockResolvedValue([])
160169
mockLoadCopilotChatMessages.mockResolvedValue(threeMessages)
161170
mockPlanChatFileCopies.mockReturnValue({
@@ -180,12 +189,13 @@ describe('POST /api/mothership/chats/[chatId]/fork', () => {
180189
})
181190

182191
it('forks organization history under current membership without inventing workspace files', async () => {
183-
dbChainMockFns.limit.mockReset()
192+
resetDbChainMock()
184193
const chat = { ...parentRow, workspaceId: null, organizationId: 'org-1', resources: [] }
185194
queueTableRows(copilotChats, [chat])
186195
queueTableRows(copilotChats, [chat])
187196
queueTableRows(member, [{ role: 'member' }])
188197
queueTableRows(copilotChats, [{ id: chat.id }])
198+
dbChainMockFns.returning.mockResolvedValue([{ id: 'row-id', workspaceId: null }])
189199
const attachments = [
190200
{
191201
id: 'upload-1',
@@ -220,7 +230,7 @@ describe('POST /api/mothership/chats/[chatId]/fork', () => {
220230
// The fork shares its attachment keys with the source, and cleanup deletes an unreferenced
221231
// key only after deleting the source row: publishing without the source would leave the
222232
// fork pointing at bytes that cleanup is about to delete.
223-
dbChainMockFns.limit.mockReset()
233+
resetDbChainMock()
224234
const chat = { ...parentRow, workspaceId: null, organizationId: 'org-1', resources: [] }
225235
queueTableRows(copilotChats, [chat])
226236
queueTableRows(copilotChats, [chat])
@@ -239,7 +249,7 @@ describe('POST /api/mothership/chats/[chatId]/fork', () => {
239249
it.each(['membership', 'capability'] as const)(
240250
'denies organization forks after %s revocation before copying',
241251
async (revocation) => {
242-
dbChainMockFns.limit.mockReset()
252+
resetDbChainMock()
243253
const chat = { ...parentRow, workspaceId: null, organizationId: 'org-1' }
244254
queueTableRows(copilotChats, [chat])
245255
queueTableRows(copilotChats, [chat])
@@ -254,7 +264,7 @@ describe('POST /api/mothership/chats/[chatId]/fork', () => {
254264
)
255265

256266
it('404s when the chat belongs to another user', async () => {
257-
dbChainMockFns.limit.mockResolvedValue([{ ...parentRow, userId: 'someone-else' }])
267+
mockWorkspaceForkRows({ ...parentRow, userId: 'someone-else' })
258268
const res = await POST(createRequest('chat-1'), createRouteContext({ chatId: 'chat-1' }))
259269
expect(res.status).toBe(404)
260270
expect(dbChainMockFns.transaction).not.toHaveBeenCalled()
@@ -447,19 +457,17 @@ describe('POST /api/mothership/chats/[chatId]/fork', () => {
447457

448458
it('drops a Sources tab whose response is past the cut', async () => {
449459
const kept = { type: 'sources', id: 'cited-sources', title: 'Sources' }
450-
dbChainMockFns.limit.mockResolvedValue([
451-
{ ...parentRow, resources: [{ ...kept, sources: { messageId: 'msg-3' } }] },
452-
])
460+
mockWorkspaceForkRows({
461+
...parentRow,
462+
resources: [{ ...kept, sources: { messageId: 'msg-3' } }],
463+
})
453464
await POST(createRequest('chat-1'), createRouteContext({ chatId: 'chat-1' }))
454465
expect(dbChainMockFns.values).toHaveBeenCalledWith(expect.objectContaining({ resources: [] }))
455466

456-
dbChainMockFns.values.mockClear()
457-
dbChainMockFns.limit.mockResolvedValue([
458-
{
459-
...parentRow,
460-
resources: [{ ...kept, sources: { messageId: 'live-id', requestId: 'req-2' } }],
461-
},
462-
])
467+
mockWorkspaceForkRows({
468+
...parentRow,
469+
resources: [{ ...kept, sources: { messageId: 'live-id', requestId: 'req-2' } }],
470+
})
463471
mockLoadCopilotChatMessages.mockResolvedValue([
464472
threeMessages[0],
465473
{ ...threeMessages[1], requestId: 'req-2' },
@@ -479,18 +487,16 @@ describe('POST /api/mothership/chats/[chatId]/fork', () => {
479487
// copies the kept upload AND the pre-cut apple; only the post-cut banana
480488
// stays behind, so only its resource is dropped — not left pointing at
481489
// the source chat.
482-
dbChainMockFns.limit.mockResolvedValue([
483-
{
484-
...parentRow,
485-
resources: [
486-
{ type: 'file', id: OLD_FILE_ID, title: 'cat.png' },
487-
{ type: 'file', id: 'wf_apple', title: 'apple.png' },
488-
{ type: 'file', id: 'wf_banana', title: 'banana.png' },
489-
{ type: 'file', id: 'wf_shared', title: 'shared.pdf' },
490-
{ type: 'workflow', id: 'wflow-1', title: 'My flow' },
491-
],
492-
},
493-
])
490+
mockWorkspaceForkRows({
491+
...parentRow,
492+
resources: [
493+
{ type: 'file', id: OLD_FILE_ID, title: 'cat.png' },
494+
{ type: 'file', id: 'wf_apple', title: 'apple.png' },
495+
{ type: 'file', id: 'wf_banana', title: 'banana.png' },
496+
{ type: 'file', id: 'wf_shared', title: 'shared.pdf' },
497+
{ type: 'workflow', id: 'wflow-1', title: 'My flow' },
498+
],
499+
})
494500
// Every chat-owned file of the source chat in the single read; messageId
495501
// drives the in-memory cut.
496502
mockListForkableChatFiles.mockResolvedValue([

‎apps/sim/app/api/mothership/chats/route.test.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,16 @@ vi.mock('@/lib/mothership/chat-status', () => mothershipChatStatusMock)
3131

3232
vi.mock('@/lib/posthog/server', () => posthogServerMock)
3333

34-
import { GET } from '@/app/api/mothership/chats/route'
34+
import { OrchestrationError } from '@/lib/core/orchestration/types'
35+
import { GET, POST } from '@/app/api/mothership/chats/route'
3536

3637
function createRequest(workspaceId: string) {
3738
return createMockRequest({
3839
url: `http://localhost:3000/api/mothership/chats?workspaceId=${workspaceId}`,
3940
})
4041
}
4142

42-
describe('GET /api/mothership/chats', () => {
43+
describe('/api/mothership/chats', () => {
4344
beforeEach(() => {
4445
resetDbChainMock()
4546
workspaceContextMockFns.mockResolveActiveWorkspaceApplicationContext.mockImplementation(
@@ -121,6 +122,18 @@ describe('GET /api/mothership/chats', () => {
121122
])
122123
})
123124

125+
it.each(['forbidden', 'not_found'] as const)(
126+
'identifies the workspace in a POST %s refusal',
127+
async (code) => {
128+
workspaceContextMockFns.mockResolveActiveWorkspaceApplicationContext.mockRejectedValueOnce(
129+
new OrchestrationError(code, 'Workspace unavailable')
130+
)
131+
const response = await POST(createMockRequest('POST', { workspaceId: 'ws-denied' }))
132+
expect(response.status).toBe(403)
133+
expect(await response.json()).toEqual({ error: 'Workspace access denied' })
134+
}
135+
)
136+
124137
afterAll(() => {
125138
resetDbChainMock()
126139
})

‎apps/sim/app/api/mothership/chats/route.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
7575
* Creates an empty mothership chat and returns its ID.
7676
*/
7777
export const POST = withRouteHandler(async (request: NextRequest) => {
78+
let isWorkspaceRequest = false
7879
try {
7980
const { userId, isAuthenticated, principal } = await authenticateCopilotRequestSessionOnly()
8081
if (!isAuthenticated || !userId) {
@@ -84,6 +85,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
8485
const validation = await parseRequest(createMothershipChatContract, request, {})
8586
if (!validation.success) return validation.response
8687
const { workspaceId, organizationId, mode } = validation.data.body
88+
isWorkspaceRequest = Boolean(workspaceId)
8789

8890
if (organizationId) {
8991
if (!principal) return createUnauthorizedResponse()
@@ -110,11 +112,13 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
110112
return NextResponse.json({ success: true, id: chat.id })
111113
} catch (error) {
112114
const code = asOrchestrationError(error)?.code
115+
if (
116+
isWorkspaceAccessDeniedError(error) ||
117+
(isWorkspaceRequest && (code === 'not_found' || code === 'forbidden'))
118+
)
119+
return createForbiddenResponse('Workspace access denied')
113120
if (code === 'not_found' || code === 'forbidden')
114121
return createForbiddenResponse('Organization access denied')
115-
if (isWorkspaceAccessDeniedError(error)) {
116-
return createForbiddenResponse('Workspace access denied')
117-
}
118122
logger.error('Error creating mothership chat:', error)
119123
return createInternalServerErrorResponse('Failed to create chat')
120124
}

‎apps/sim/lib/benchmarks/artifacts.test.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,26 @@ describe('benchmark reference integrity', () => {
5050
expect(() => validateBenchmarkRedaction(artifacts)).not.toThrow()
5151
})
5252

53+
it('rejects a repeated gold answer that survives outside the masks', () => {
54+
expect(() =>
55+
validateBenchmarkRedaction({
56+
...artifacts,
57+
redactedSpec:
58+
'[[BLANK:owner]] owns follow-up until Engineering accepts. Support monitors it.',
59+
})
60+
).toThrow()
61+
})
62+
63+
it('does not mistake a blank identifier for a surviving gold answer', () => {
64+
expect(() =>
65+
validateBenchmarkRedaction({
66+
referenceSpec: 'owner approves the request.',
67+
redactedSpec: '[[BLANK:owner]] approves the request.',
68+
blanks: [{ id: 'owner', answer: 'owner' }],
69+
})
70+
).not.toThrow()
71+
})
72+
5373
it('rejects a redaction that quietly changes an unmasked requirement', () => {
5474
expect(() =>
5575
validateBenchmarkRedaction({ ...artifacts, redactedSpec: '[[BLANK:owner]] owns everything.' })

‎apps/sim/lib/benchmarks/artifacts.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,15 @@ export function validateBenchmarkRedaction(
5959
'Every blank must appear as [[BLANK:id]], and replacing the masks with their answers must restore the reference exactly'
6060
)
6161
}
62+
const visiblePassages = artifacts.redactedSpec.split(/\[\[BLANK:[A-Za-z0-9_-]+\]\]/)
63+
if (
64+
[...answers.values()].some((answer) => visiblePassages.some((text) => text.includes(answer)))
65+
) {
66+
throw new OrchestrationError(
67+
'validation',
68+
'Every occurrence of a selected answer must be masked'
69+
)
70+
}
6271
}
6372

6473
/** Editing an upstream artifact atomically discards results derived from its previous value. */

‎apps/sim/lib/billing/core/usage-log.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,7 @@ describe('ledger aggregates', () => {
456456

457457
beforeEach(() => {
458458
installSharedDbMocks()
459+
vi.spyOn(Date, 'now').mockReturnValue(billingPeriod.start.getTime())
459460
})
460461

461462
for (const aggregate of aggregates) {

‎apps/sim/lib/billing/webhooks/stripe-sync-convergence.integration.ts‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -402,7 +402,7 @@ function startParkedTransaction(
402402
}
403403
throw new Error('No transaction ever waited on the parked one')
404404
}
405-
return { done, release, untilBlocking }
405+
return { ready: holderPid, done, release, untilBlocking }
406406
}
407407

408408
describe('cancel_at_period_end sync', () => {
@@ -1031,6 +1031,7 @@ describe('Team activation', () => {
10311031
reason: 'admin-cancel-at-period-end',
10321032
})
10331033
})
1034+
await cancelling.ready
10341035
const activating = testDatabase.transaction((tx) =>
10351036
ensureTeamOrganizationForAcceptance({
10361037
billingOwnerUserId: owner.id,
@@ -1039,8 +1040,11 @@ describe('Team activation', () => {
10391040
workspaceIdsToAttach: [],
10401041
})
10411042
)
1042-
await cancelling.untilBlocking()
1043-
cancelling.release()
1043+
try {
1044+
await cancelling.untilBlocking()
1045+
} finally {
1046+
cancelling.release()
1047+
}
10441048
await cancelling.done
10451049
await expect(activating).resolves.toMatchObject({ success: true })
10461050
expect((await storedSubscription(subscriptionId)).cancelAtPeriodEnd).toBe(false)
@@ -1082,9 +1086,13 @@ describe('operator retry', () => {
10821086
})
10831087
}
10841088
)
1089+
await writing.ready
10851090
const requeuing = requeueFromAdminApi(pauseSync)
1086-
await writing.untilBlocking()
1087-
writing.release()
1091+
try {
1092+
await writing.untilBlocking()
1093+
} finally {
1094+
writing.release()
1095+
}
10881096

10891097
await expect(Promise.all([writing.done, requeuing])).resolves.toBeDefined()
10901098
await deliverUnrelatedUpdate(pro.stripeSubscriptionId)
@@ -1123,14 +1131,18 @@ describe('operator retry', () => {
11231131
})
11241132
}
11251133
)
1134+
await writing.ready
11261135
const retrying = requestDashboardSubscriptionCancellation({
11271136
organizationId: org.organizationId,
11281137
operationId,
11291138
timing: 'period_end',
11301139
actor,
11311140
})
1132-
await writing.untilBlocking()
1133-
writing.release()
1141+
try {
1142+
await writing.untilBlocking()
1143+
} finally {
1144+
writing.release()
1145+
}
11341146

11351147
await expect(Promise.all([writing.done, retrying])).resolves.toBeDefined()
11361148
expect((await storedSubscription(org.subscriptionId)).cancelAtPeriodEnd).toBe(true)

‎apps/sim/lib/mothership/chat/application/fork.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,14 @@ export const forkChat = defineAuthorizedChatUseCase({
157157

158158
/** Publish only after both the file bytes and the worker conversation are prepared. */
159159
await db.transaction(async (tx) => {
160-
if (parent.workspaceId) await lockActiveWorkspace(tx, parent.workspaceId)
160+
if (parent.workspaceId) {
161+
const currentWorkspace = await lockActiveWorkspace(tx, parent.workspaceId)
162+
if (currentWorkspace.organizationId !== context.workspaceOrganizationId)
163+
throw new OrchestrationError(
164+
'conflict',
165+
'Workspace organization changed. Refresh and retry.'
166+
)
167+
}
161168
/**
162169
* The fork can share keys with its source (organization attachments, files whose copy
163170
* failed), and chat cleanup deletes a shared key once no remaining chat references it,

0 commit comments

Comments
 (0)