Skip to content

Commit 5482a55

Browse files
icecrasher321icecrasher321
committed
fix(plan): revalidate access and retain benchmark state
1 parent 657c8de commit 5482a55

11 files changed

Lines changed: 257 additions & 35 deletions

File tree

‎apps/sim/app/o/[organizationId]/benchmark/benchmark.test.tsx‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
/** @vitest-environment jsdom */
2+
import { act } from 'react'
3+
import { flushMicrotasks } from '@sim/testing/helpers/async'
4+
import { jsonResponse } from '@sim/testing/helpers/http'
5+
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
6+
import { NuqsTestingAdapter } from 'nuqs/adapters/testing'
7+
import { createRoot } from 'react-dom/client'
8+
import { expect, it, vi } from 'vitest'
9+
import { Benchmark } from '@/app/o/[organizationId]/benchmark/benchmark'
10+
11+
vi.mock('@/app/o/[organizationId]/benchmark/components', () => ({
12+
BenchmarkDetail: () => null,
13+
CreateBenchmark: () => null,
14+
}))
15+
16+
it('preserves cached benchmarks through a failed next page and retries without losing the first page', async () => {
17+
vi.useFakeTimers()
18+
vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true)
19+
const client = new QueryClient({ defaultOptions: { queries: { retry: false } } })
20+
const container = document.createElement('div')
21+
document.body.append(container)
22+
const root = createRoot(container)
23+
const record = (id: string) => ({
24+
id,
25+
name: id,
26+
organizationId: 'org',
27+
userId: 'owner',
28+
runAsUserId: 'owner',
29+
sourceWorkspaceId: 'workspace',
30+
version: 1,
31+
runningStage: null,
32+
leaseExpiresAt: null,
33+
attemptId: null,
34+
plannerChatId: null,
35+
error: null,
36+
createdAt: '2026-01-01T00:00:00.000Z',
37+
updatedAt: '2026-01-01T00:00:00.000Z',
38+
})
39+
const fetcher = vi
40+
.fn()
41+
.mockResolvedValueOnce(
42+
jsonResponse({ benchmarks: [record('First benchmark')], nextCursor: 'next' })
43+
)
44+
.mockResolvedValueOnce(jsonResponse({ error: 'Temporary failure' }, 503))
45+
.mockResolvedValueOnce(
46+
jsonResponse({ benchmarks: [record('Older benchmark')], nextCursor: null })
47+
)
48+
vi.stubGlobal('fetch', fetcher)
49+
const settle = async (action: () => void) => {
50+
await act(async () => action())
51+
await act(async () => {
52+
await flushMicrotasks()
53+
await vi.advanceTimersByTimeAsync(1)
54+
})
55+
}
56+
const openButtons = () =>
57+
[...container.querySelectorAll<HTMLButtonElement>('button')].filter(
58+
(button) => button.textContent === 'Open'
59+
)
60+
const olderButton = () =>
61+
[...container.querySelectorAll<HTMLButtonElement>('button')].find((button) =>
62+
/older benchmarks/i.test(button.textContent ?? '')
63+
)
64+
try {
65+
await settle(() =>
66+
root.render(
67+
<QueryClientProvider client={client}>
68+
<NuqsTestingAdapter hasMemory>
69+
<Benchmark organizationId='org' runAsUserId='owner' canPlan />
70+
</NuqsTestingAdapter>
71+
</QueryClientProvider>
72+
)
73+
)
74+
expect(openButtons()).toHaveLength(1)
75+
await settle(() => olderButton()?.click())
76+
expect(container.querySelector('[role="alert"]')).not.toBeNull()
77+
expect(openButtons()).toHaveLength(1)
78+
expect(olderButton()?.disabled).toBe(false)
79+
await settle(() => olderButton()?.click())
80+
expect(openButtons()).toHaveLength(2)
81+
expect(container.querySelector('[role="alert"]')).toBeNull()
82+
expect(olderButton()).toBeUndefined()
83+
} finally {
84+
await act(async () => root.unmount())
85+
client.clear()
86+
container.remove()
87+
vi.useRealTimers()
88+
}
89+
})

‎apps/sim/app/o/[organizationId]/benchmark/benchmark.tsx‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,15 +65,16 @@ export function Benchmark({ organizationId, canPlan, runAsUserId }: BenchmarkPro
6565
/>
6666
) : (
6767
<>
68-
{benchmarks.error ? (
68+
{benchmarks.error && (
6969
<p role='alert' className='text-[var(--text-error)] text-small'>
7070
{benchmarks.error.message}
7171
</p>
72-
) : benchmarks.isLoading ? (
72+
)}
73+
{benchmarks.isLoading ? (
7374
<p role='status' className='text-[var(--text-muted)] text-small'>
7475
Loading benchmarks…
7576
</p>
76-
) : records.length === 0 ? (
77+
) : !benchmarks.data ? null : records.length === 0 ? (
7778
<div className='rounded-lg border border-[var(--border)] p-5'>
7879
<p className='text-[var(--text-primary)] text-small'>No benchmarks yet</p>
7980
<p className='mt-1 text-[var(--text-muted)] text-small'>
@@ -109,7 +110,11 @@ export function Benchmark({ organizationId, canPlan, runAsUserId }: BenchmarkPro
109110
disabled={benchmarks.isFetchingNextPage}
110111
onClick={() => benchmarks.fetchNextPage()}
111112
>
112-
{benchmarks.isFetchingNextPage ? 'Loading…' : 'Load older benchmarks'}
113+
{benchmarks.isFetchingNextPage
114+
? 'Loading…'
115+
: benchmarks.isFetchNextPageError
116+
? 'Retry older benchmarks'
117+
: 'Load older benchmarks'}
113118
</Chip>
114119
</div>
115120
)}

‎apps/sim/app/o/[organizationId]/benchmark/components/benchmark-score-chart.tsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@ export function BenchmarkScoreChart({
3737
>
3838
<div className='min-w-0'>
3939
<Chip
40-
variant={run.id === selectedId ? 'primary' : undefined}
40+
active={run.id === selectedId}
41+
aria-pressed={run.id === selectedId}
4142
onClick={() => onSelect(run.id)}
4243
>
4344
{benchmarkModelLabel(run.modelRuns.plan?.config)}

‎apps/sim/app/workspace/[workspaceId]/settings/components/desktop/computer-use.test.tsx‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import type {
66
ComputerUseStatus,
77
} from '@sim/desktop-bridge'
88
import { toast } from '@sim/emcn'
9+
import { createDeferred } from '@sim/testing/helpers/deferred'
910
import { libDesktopMock, libDesktopMockFns } from '@sim/testing/mocks/lib-desktop.mock'
1011
import { createRoot, type Root } from 'react-dom/client'
1112
import { afterEach, beforeEach, expect, it, vi } from 'vitest'
@@ -74,8 +75,6 @@ function renderSettings() {
7475
dashboards: false,
7576
'table-row-ttl': false,
7677
'mothership-model-selector': false,
77-
'mothership-plan-mode': false,
78-
'mothership-desktop-background-executor': false,
7978
'mothership-computer-use': enabled,
8079
}}
8180
>
@@ -119,3 +118,21 @@ it('keeps settings dormant while rollout is off, including focus and activity ch
119118
function appRow(bundleId: string) {
120119
return container.querySelector<HTMLElement>(`[title="${bundleId}"]`)?.parentElement
121120
}
121+
122+
it('does not restore a revoked app when an earlier activity refresh settles late', async () => {
123+
await act(async () => renderSettings())
124+
const stale = createDeferred<ComputerUseAppPermission[]>()
125+
native.listAppPermissions.mockImplementationOnce(() => stale.promise)
126+
await act(async () => {
127+
activity = { toolCallId: 'in-flight', scopeId: 'chat', action: 'click', startedAt: Date.now() }
128+
for (const listener of listeners) listener(activity)
129+
})
130+
const revoke = appRow('com.example.First')?.querySelector<HTMLButtonElement>('button')
131+
expect(revoke).toBeTruthy()
132+
await act(async () => revoke?.click())
133+
expect(appRow('com.example.First')).toBeUndefined()
134+
expect(appRow('com.example.Second')).toBeDefined()
135+
await act(async () => stale.resolve(initialPermissions))
136+
expect(appRow('com.example.First')).toBeUndefined()
137+
expect(appRow('com.example.Second')).toBeDefined()
138+
})

‎apps/sim/app/workspace/[workspaceId]/settings/components/desktop/computer-use.tsx‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
'use client'
22

3-
import { useEffect, useState } from 'react'
3+
import { useCallback, useEffect, useRef, useState } from 'react'
44
import type { ComputerUseAppPermission } from '@sim/desktop-bridge'
55
import { Chip, ChipSwitch, Label, toast } from '@sim/emcn'
66
import { ComputerUseActivity } from '@/components/computer-use/activity'
@@ -19,18 +19,29 @@ export function ComputerUseSettings() {
1919
}
2020

2121
function ComputerUseSettingsControls() {
22+
const requestVersion = useRef(0)
2223
const bridge = getDesktopBridge()?.computerUse
2324
const enabled = useFeatureFlag('mothership-computer-use')
2425
const { status, setStatus, refresh, error } = useComputerUseStatus()
2526
const [apps, setApps] = useState<ComputerUseAppPermission[]>([])
2627
const [pending, setPending] = useState(false)
28+
const refreshApps = useCallback(async () => {
29+
if (!bridge) return
30+
const version = ++requestVersion.current
31+
try {
32+
const approved = await bridge.listAppPermissions()
33+
if (version === requestVersion.current) setApps(approved)
34+
} catch {
35+
if (version === requestVersion.current) toast.error('Could not load approved apps')
36+
}
37+
}, [bridge])
2738
useEffect(() => {
28-
if (!bridge || !enabled) return
29-
void bridge
30-
.listAppPermissions()
31-
.then(setApps)
32-
.catch(() => toast.error('Could not load approved apps'))
33-
}, [bridge, enabled, status?.activeAction])
39+
if (!enabled) return
40+
void refreshApps()
41+
return () => {
42+
requestVersion.current += 1
43+
}
44+
}, [enabled, refreshApps, status?.activeAction])
3445
if (!bridge || !enabled || status?.supported === false) return null
3546
const update = async (action: () => Promise<void>) => {
3647
setPending(true)
@@ -105,8 +116,9 @@ function ComputerUseSettingsControls() {
105116
disabled={pending}
106117
onClick={() =>
107118
void update(async () => {
119+
requestVersion.current += 1
108120
await bridge.revokeApp(app.bundleId)
109-
setApps(await bridge.listAppPermissions())
121+
await refreshApps()
110122
})
111123
}
112124
>

‎apps/sim/lib/benchmarks/application/prepare-plan.ts‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,18 @@ import {
1212
} from '@/lib/mothership/chat/organization-chats'
1313
import { isPlanModeEnabled } from '@/lib/mothership/feature-flags'
1414

15+
/** Checks the selected user's current Plan permission without creating another conversation. */
16+
export async function authorizeBenchmarkPlanTarget(
17+
principal: Principal,
18+
input: { organizationId: string; runAsUserId: string }
19+
) {
20+
const target = await authorizeBenchmarkTarget(principal, input)
21+
await requireOrganizationBuildPermission(target)
22+
if (!(await isPlanModeEnabled(target.userId)))
23+
throw new OrchestrationError('not_found', 'Plan mode is unavailable')
24+
return target
25+
}
26+
1527
/** The target owns the isolated agent context; the operator owns the benchmark and report. */
1628
export const prepareBenchmarkPlan = defineAuthorizedBenchmarkUseCase({
1729
operation: benchmarkOperations.preparePlan,
@@ -23,13 +35,10 @@ export const prepareBenchmarkPlan = defineAuthorizedBenchmarkUseCase({
2335
input: { organizationId: string; benchmarkId: string }
2436
}) {
2537
const benchmark = await requireBenchmarkCaseAccess(principal, input)
26-
const target = await authorizeBenchmarkTarget(principal, {
38+
const target = await authorizeBenchmarkPlanTarget(principal, {
2739
organizationId: benchmark.organizationId,
2840
runAsUserId: benchmark.runAsUserId ?? benchmark.userId,
2941
})
30-
await requireOrganizationBuildPermission(target)
31-
if (!(await isPlanModeEnabled(target.userId)))
32-
throw new OrchestrationError('not_found', 'Plan mode is unavailable')
3342
const chat = await createOrganizationChatRecord(target, 'plan', {
3443
id: benchmark.id,
3544
operatorUserId: benchmark.userId,

‎apps/sim/lib/benchmarks/application/run-stage.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { z } from 'zod'
55
import { defineAuthorizedBenchmarkUseCase } from '@/lib/benchmarks/application/access'
66
import { requireBenchmarkCaseAccess } from '@/lib/benchmarks/application/cases'
77
import { benchmarkOperations } from '@/lib/benchmarks/application/operations'
8+
import { authorizeBenchmarkPlanTarget } from '@/lib/benchmarks/application/prepare-plan'
89
import {
910
BENCHMARK_LEASE_MS,
1011
withBenchmarkStageLease,
@@ -255,6 +256,11 @@ export const runBenchmarkStage = defineAuthorizedBenchmarkUseCase({
255256
}
256257
request?.signal?.throwIfAborted()
257258
await requireBenchmarkCaseAccess(principal, input)
259+
if (input.stage === 'plan')
260+
await authorizeBenchmarkPlanTarget(principal, {
261+
organizationId: claimed.organizationId,
262+
runAsUserId: claimed.runAsUserId ?? claimed.userId,
263+
})
258264
return {
259265
benchmark: await completeBenchmarkStage({
260266
...attempt,

‎apps/sim/lib/benchmarks/repository.integration.ts‎

Lines changed: 71 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ const database = await vi.hoisted(async () => {
1717
process.env.MOTHERSHIP_BENCHMARK_ENABLED = 'true'
1818
const { createServer } = await import('node:http')
1919
const requests: Record<string, unknown>[] = []
20-
const responses: { text: string; onRequest?: () => void }[] = []
20+
const responses: { text: string; onRequest?: () => void | Promise<void> }[] = []
2121
const worker = createServer(async (request, response) => {
2222
let body = ''
2323
for await (const chunk of request) body += chunk
@@ -28,7 +28,7 @@ const database = await vi.hoisted(async () => {
2828
const payload = JSON.parse(body) as Record<string, unknown>
2929
requests.push(payload)
3030
const scripted = responses.shift()
31-
scripted?.onRequest?.()
31+
await scripted?.onRequest?.()
3232
const frames = [
3333
{ type: 'text', payload: { channel: 'assistant', text: scripted?.text ?? '{"ok":true}' } },
3434
{ type: 'complete', payload: { status: 'complete' } },
@@ -87,6 +87,7 @@ import {
8787
import { prepareBenchmarkExecution } from '@/lib/benchmarks/application/prepare-execution'
8888
import { prepareBenchmarkPlan } from '@/lib/benchmarks/application/prepare-plan'
8989
import { runBenchmarkComparison } from '@/lib/benchmarks/application/run-comparison'
90+
import { runBenchmarkStage } from '@/lib/benchmarks/application/run-stage'
9091
import {
9192
getBenchmarkRun,
9293
listBenchmarkRuns,
@@ -113,6 +114,11 @@ import {
113114
} from '@/lib/benchmarks/repository'
114115
import { type BenchmarkArtifacts, emptyBenchmarkArtifacts } from '@/lib/benchmarks/types'
115116
import { executeBenchmarkJson } from '@/lib/benchmarks/worker'
117+
import {
118+
getAccessibleCopilotChatAuth,
119+
getAccessibleCopilotChatForCancellation,
120+
getAccessibleCopilotChatWithMessages,
121+
} from '@/lib/mothership/chat/lifecycle'
116122
import { listOrganizationChats } from '@/lib/mothership/chat/organization-chats'
117123

118124
describe('private benchmark persistence and attempt fencing', () => {
@@ -180,6 +186,7 @@ describe('private benchmark persistence and attempt fencing', () => {
180186
'user',
181187
'settings',
182188
'copilot_chats',
189+
'copilot_messages',
183190
'copilot_runs',
184191
'copilot_request_stops',
185192
'copilot_organization_request_stops',
@@ -330,6 +337,68 @@ describe('private benchmark persistence and attempt fencing', () => {
330337
}
331338
}
332339

340+
it.each(['organization', 'workspace'] as const)(
341+
'hides persisted %s Plan chats after eligibility revocation while preserving Stop',
342+
async (surface) => {
343+
await connection`UPDATE member SET role = 'owner' WHERE id = 'owner-member'`
344+
const chat = await prepareBenchmarkPlan.execute({
345+
principal,
346+
input: { organizationId: 'org', benchmarkId: 'benchmark' },
347+
})
348+
if (surface === 'workspace')
349+
await connection`UPDATE copilot_chats SET organization_id = NULL, workspace_id = 'workspace' WHERE id = ${chat.chatId}`
350+
const options = { principal }
351+
expect(await getAccessibleCopilotChatAuth(chat.chatId, 'owner', options)).toMatchObject({
352+
mode: 'plan',
353+
})
354+
expect(
355+
await getAccessibleCopilotChatWithMessages(chat.chatId, 'owner', options)
356+
).toMatchObject({ mode: 'plan', messages: [] })
357+
await connection`UPDATE settings SET super_user_mode_enabled = false WHERE user_id = 'owner'`
358+
expect(await getAccessibleCopilotChatAuth(chat.chatId, 'owner', options)).toBeNull()
359+
expect(await getAccessibleCopilotChatWithMessages(chat.chatId, 'owner', options)).toBeNull()
360+
expect(
361+
await getAccessibleCopilotChatForCancellation(chat.chatId, 'owner', options)
362+
).toMatchObject({ mode: 'plan' })
363+
}
364+
)
365+
366+
it('refuses to publish a completed Plan after its selected target loses eligibility', async () => {
367+
await connection`UPDATE member SET role = 'owner' WHERE id = 'owner-member'`
368+
await connection`UPDATE "user" SET role = 'admin' WHERE id = 'target'`
369+
await connection`INSERT INTO settings (id, user_id, super_user_mode_enabled) VALUES ('target', 'target', true) ON CONFLICT (user_id) DO UPDATE SET super_user_mode_enabled = true`
370+
const current = await updateBenchmarkRecord({
371+
...scope,
372+
version: 1,
373+
name: 'Target revocation',
374+
artifacts: { ...graded, generatedSpec: null, reconstruction: null, grade: null },
375+
})
376+
await connection`UPDATE mothership_benchmarks SET run_as_user_id = 'target' WHERE id = 'benchmark'`
377+
database.responses.push({
378+
text: 'Unpublished plan',
379+
onRequest: async () => {
380+
await connection`UPDATE settings SET super_user_mode_enabled = false WHERE user_id = 'target'`
381+
},
382+
})
383+
await expect(
384+
runBenchmarkStage.execute({
385+
principal,
386+
input: {
387+
organizationId: 'org',
388+
benchmarkId: 'benchmark',
389+
version: current.version,
390+
stage: 'plan',
391+
},
392+
})
393+
).rejects.toMatchObject({ code: 'not_found', message: 'Plan mode is unavailable' })
394+
expect(database.requests).toHaveLength(1)
395+
expect(await getBenchmarkRecord(scope)).toMatchObject({
396+
runningStage: null,
397+
artifacts: { generatedSpec: null },
398+
})
399+
expect(await connection`SELECT id FROM copilot_chats`).toHaveLength(1)
400+
})
401+
333402
it('executes selected planners in fresh conversations with a fixed evaluator and saves comparable model snapshots', async () => {
334403
const input = await prepareComparison()
335404
await runBenchmarkComparison.execute({ principal, input })

0 commit comments

Comments
 (0)