Skip to content

Commit e42d58a

Browse files
committed
fix(memory): read-lock note authorization, redact legacy tool calls, skip keyless cleanup
Project note checks took the Project lock exclusively while already holding it shared, so overlapping workspace checks could deadlock; they now read under the shared Project lock and the permission-group read lock, as Project Files does. Exports also redact remember calls stored in the legacy top-level toolCalls list. A Sim without COPILOT_API_KEY never reached a worker, so it queues no memory cleanup it could not deliver.
1 parent a2e090f commit e42d58a

6 files changed

Lines changed: 115 additions & 11 deletions

File tree

‎apps/sim/lib/data-drains/sources/copilot-chats.ts‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,12 +144,26 @@ async function* pages(input: SourcePageInput): AsyncIterable<CopilotChatRow[]> {
144144
}
145145
}
146146

147+
/** A message written before content blocks keeps its tool calls in a top-level `toolCalls` list. */
148+
function projectLegacyToolCall(call: unknown): unknown {
149+
if (!isRecordLike(call) || call.name !== 'remember') return call
150+
return {
151+
name: 'remember',
152+
...(typeof call.id === 'string' ? { id: call.id } : {}),
153+
...(typeof call.status === 'string' ? { status: call.status } : {}),
154+
}
155+
}
156+
147157
/** Tool payloads can contain private notes even when their mutation failed or targeted a shared note. */
148158
function projectExportMessage(message: unknown): unknown {
149-
if (!isRecordLike(message) || !Array.isArray(message.contentBlocks)) return message
159+
if (!isRecordLike(message)) return message
160+
const projected = Array.isArray(message.toolCalls)
161+
? { ...message, toolCalls: message.toolCalls.map(projectLegacyToolCall) }
162+
: message
163+
if (!Array.isArray(projected.contentBlocks)) return projected
150164
return {
151-
...message,
152-
contentBlocks: message.contentBlocks.map((block: unknown) => {
165+
...projected,
166+
contentBlocks: projected.contentBlocks.map((block: unknown) => {
153167
if (!isRecordLike(block) || !isRecordLike(block.toolCall)) return block
154168
if (block.toolCall.name !== 'remember') {
155169
if (!isRecordLike(block.toolCall.result) || !('output' in block.toolCall.result))

‎apps/sim/lib/data-drains/sources/coverage.integration.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,22 @@ describe('Drain source coverage and payload safety in PostgreSQL', () => {
118118
const content = {
119119
role: 'assistant',
120120
content: 'Visible reply',
121+
toolCalls: [
122+
{
123+
id: 'legacy',
124+
name: 'remember',
125+
status: 'success',
126+
params: { content: 'hidden-memory-legacy-args' },
127+
result: 'hidden-memory-legacy-result',
128+
error: 'hidden-memory-legacy-error',
129+
},
130+
{
131+
id: 'legacy-read',
132+
name: 'read',
133+
status: 'success',
134+
params: { path: 'visible-legacy-path' },
135+
},
136+
],
121137
contentBlocks: [
122138
{ type: 'text', content: 'Visible reply' },
123139
{
@@ -176,6 +192,7 @@ describe('Drain source coverage and payload safety in PostgreSQL', () => {
176192
expect(wire).not.toContain('hidden-memory')
177193
expect(wire).toContain('Visible reply')
178194
expect(wire).toContain('visible-path')
195+
expect(wire).toContain('visible-legacy-path')
179196
const [stored] = await runtime.db
180197
.select({ content: runtime.schema.copilotMessages.content })
181198
.from(runtime.schema.copilotMessages)

‎apps/sim/lib/mothership/memories/application/resolve-access.integration.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,19 @@ it('lets a read-only end user mutate private notes through either chat and keeps
242242
}
243243
})
244244

245+
it('resolves overlapping workspace checks that share one Project without deadlocking', async () => {
246+
const results = await Promise.all(
247+
Array.from({ length: 6 }, (_, index) =>
248+
resolveMemoryNotesAccess.execute({
249+
principal: principal(index % 2 === 1),
250+
input: { target: { scope: 'workspace', workspaceId }, action: 'read' },
251+
})
252+
)
253+
)
254+
for (const result of results)
255+
expect(result.target).toMatchObject({ scope: 'workspace', currentProject: { id: projectId } })
256+
})
257+
245258
it('does not turn write in one environment into shared Project write', async () => {
246259
await db
247260
.update(permissions)

‎apps/sim/lib/mothership/memories/application/resolve-access.ts‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import type {
2121
} from '@/lib/mothership/generated/memory-notes-access'
2222
import { assertWorkspaceCapability } from '@/lib/permission-groups/capability-assertions'
2323
import { PermissionGroupCapabilityError } from '@/lib/permission-groups/capability-error'
24-
import { acquirePermissionGroupOrgLock } from '@/lib/permission-groups/locks'
24+
import { acquirePermissionGroupOrgReadLock } from '@/lib/permission-groups/locks'
2525
import { loadProjectAccess } from '@/lib/projects/application/authorization'
2626
import {
2727
requireCurrentCopilotProjectInvocation,
@@ -91,15 +91,20 @@ async function projectAccess(
9191
projectId: string,
9292
scope: { organizationId: string | null; workspaceId?: string }
9393
): Promise<Extract<MemoryNotesAccessResult['target'], { scope: 'project' }>> {
94-
const access = await loadProjectAccess(tx, principal.subjectUserId, {
95-
projectId,
96-
...(scope.organizationId ? { organizationId: scope.organizationId } : {}),
97-
...(scope.workspaceId ? { workspaceId: scope.workspaceId } : {}),
98-
})
94+
const access = await loadProjectAccess(
95+
tx,
96+
principal.subjectUserId,
97+
{
98+
projectId,
99+
...(scope.organizationId ? { organizationId: scope.organizationId } : {}),
100+
...(scope.workspaceId ? { workspaceId: scope.workspaceId } : {}),
101+
},
102+
'shared'
103+
)
99104
if (scope.organizationId !== access.record.organizationId)
100105
throw new OrchestrationError('not_found', 'Project not found in this conversation')
101106
if (access.record.organizationId)
102-
await acquirePermissionGroupOrgLock(tx, access.record.organizationId)
107+
await acquirePermissionGroupOrgReadLock(tx, access.record.organizationId)
103108
await requireCurrentCopilotProjectInvocation(
104109
tx,
105110
{
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { mockEnv, mockEnqueue } = vi.hoisted(() => ({
7+
mockEnv: { COPILOT_API_KEY: undefined as string | undefined } as Record<
8+
string,
9+
string | undefined
10+
>,
11+
mockEnqueue: vi.fn(),
12+
}))
13+
14+
vi.mock('@/lib/core/config/env', () => ({ env: mockEnv }))
15+
vi.mock('@/lib/core/outbox/service', () => ({ enqueueOutboxEvent: mockEnqueue }))
16+
vi.mock('@/lib/mothership/constants', () => ({ SIM_AGENT_API_URL: 'https://agent.example.test' }))
17+
vi.mock('@/lib/mothership/request/go/fetch', () => ({ fetchGo: vi.fn() }))
18+
vi.mock('@/lib/mothership/request/headers', () => ({ mothershipRequestHeaders: () => ({}) }))
19+
vi.mock('@/lib/mothership/server/agent-url', () => ({ getMothershipSourceEnvHeaders: () => ({}) }))
20+
21+
import { enqueueMemoryCleanup } from '@/lib/mothership/memories/cleanup-outbox'
22+
23+
const executor = { insert: vi.fn() }
24+
const targets = [
25+
{ kind: 'private_owner' as const, userId: 'user-1' },
26+
{ kind: 'workspace' as const, workspaceId: '00000000-0000-4000-8000-000000000001' },
27+
]
28+
29+
describe('enqueueMemoryCleanup', () => {
30+
beforeEach(() => {
31+
vi.clearAllMocks()
32+
mockEnv.COPILOT_API_KEY = undefined
33+
})
34+
35+
it('queues nothing when this Sim has no worker credential', async () => {
36+
await enqueueMemoryCleanup(executor, targets)
37+
expect(mockEnqueue).not.toHaveBeenCalled()
38+
})
39+
40+
it('queues one credential-bound event per destination and target', async () => {
41+
mockEnv.COPILOT_API_KEY = 'configured-key'
42+
await enqueueMemoryCleanup(executor, targets)
43+
expect(mockEnqueue).toHaveBeenCalledTimes(targets.length)
44+
for (const [, , payload] of mockEnqueue.mock.calls)
45+
expect(payload).toMatchObject({
46+
baseUrl: 'https://agent.example.test',
47+
credentialFingerprint: expect.any(String),
48+
})
49+
})
50+
})

‎apps/sim/lib/mothership/memories/cleanup-outbox.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,17 @@ function credentialFingerprint(): string | null {
3535
return env.COPILOT_API_KEY ? createHash('sha256').update(env.COPILOT_API_KEY).digest('hex') : null
3636
}
3737

38-
/** Captures configured routes before deletion; notes can outlive a user's environment selection. */
38+
/**
39+
* Captures configured routes before deletion; notes can outlive a user's environment selection.
40+
* Without `COPILOT_API_KEY` this Sim never reached a worker, so it holds no notes to erase and
41+
* queues nothing rather than an event no delivery could authenticate.
42+
*/
3943
export async function enqueueMemoryCleanup(
4044
executor: Pick<typeof db, 'insert'>,
4145
targets: MemoryCleanupInput['target'][]
4246
): Promise<void> {
4347
const fingerprint = credentialFingerprint()
48+
if (!fingerprint) return
4449
const sourceHeaders = getMothershipSourceEnvHeaders()
4550
for (const baseUrl of destinations()) {
4651
for (const target of targets) {

0 commit comments

Comments
 (0)