Skip to content

Commit bdda9f4

Browse files
icecrasher321claude
andcommitted
fix(copilot): refuse an in-band tool call whose egress catalog is unavailable
Production shows 88 of these in fourteen days, every one from this route and every one caused by a workspace id that no longer exists reaching the in-band lane. The handler ran anyway, which is the worst pair of outcomes available: the side effect happened, and because the projection can vouch for nothing without a catalog, the caller got a bare success or an opaque sentinel naming neither the cause nor whether anything had changed. It is also where the reported "cannot tell whether the mutation occurred" came from — of the tools affected, read and grep dominate, and the runs were bursts inside single sessions. Refuse before dispatch instead. Nothing runs, so there is nothing to be uncertain about, and the caller is told which workspace and why. A missing workspace also reported itself as an access denial, which sent every deleted-workspace call down a permissions path nobody could reproduce. `checkWorkspaceAccess` already distinguishes the two, so say which one it was. The refusal log now carries the user and workspace it refused; without them the only way to find the cause was to join by timestamp. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 9cf9a89 commit bdda9f4

3 files changed

Lines changed: 48 additions & 8 deletions

File tree

apps/sim/app/api/copilot/tools/execute/route.test.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -95,12 +95,22 @@ describe('POST /api/copilot/tools/execute (in-band)', () => {
9595
expect(body.error).toBe('File not found: files/a.md')
9696
})
9797

98-
it('withholds results when no egress registry can be built', async () => {
99-
mockPrepareEnvironmentContext.mockRejectedValue(new Error('env unavailable'))
98+
/**
99+
* Running the tool without a catalog used to produce the worst pair of outcomes available:
100+
* the side effect happened and the caller got a bare `{success: true}` naming neither the
101+
* cause nor whether anything had changed.
102+
*/
103+
it('refuses the call, without running the tool, when no egress registry can be built', async () => {
104+
mockPrepareEnvironmentContext.mockRejectedValue(new Error('Workspace ws-gone does not exist'))
100105
mockHandler.mockResolvedValue({ success: true, output: { content: 'sensitive' } })
106+
101107
const res = await POST(makeRequest({ ...BASE_BODY, messageId: 'msg-no-registry' }) as never)
102108
const body = await res.json()
103-
expect(body).toEqual({ success: true })
109+
110+
expect(mockHandler).not.toHaveBeenCalled()
111+
expect(body.success).toBe(false)
112+
expect(body.error).toContain('Workspace ws-gone does not exist')
113+
expect(body.output).toEqual({ resultWithheld: true, effect: 'not_attempted' })
104114
})
105115

106116
it('reuses one turn registry across calls that share a messageId', async () => {

apps/sim/app/api/copilot/tools/execute/route.ts

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { type NextRequest, NextResponse } from 'next/server'
44
import { copilotToolExecuteInternalBodySchema } from '@/lib/api/contracts/copilot'
55
import { validationErrorResponse } from '@/lib/api/server'
66
import { prepareCopilotEnvironmentContext } from '@/lib/copilot/environment-context'
7+
import { MothershipStreamV1ToolOutcome } from '@/lib/copilot/generated/mothership-stream-v1'
78
import { TraceAttr } from '@/lib/copilot/generated/trace-attributes-v1'
89
import { TraceSpan } from '@/lib/copilot/generated/trace-spans-v1'
910
import { checkInternalApiKey } from '@/lib/copilot/request/http'
@@ -17,6 +18,7 @@ import { handleResourceSideEffects } from '@/lib/copilot/request/tools/resources
1718
import type { ToolCallResult } from '@/lib/copilot/request/types'
1819
import { ensureHandlersRegistered } from '@/lib/copilot/tool-executor'
1920
import { executeTool } from '@/lib/copilot/tool-executor/executor'
21+
import { TOOL_EFFECT_PHASE } from '@/lib/copilot/tool-executor/types'
2022
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
2123
import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry'
2224

@@ -116,16 +118,35 @@ export const POST = withRouteHandler((request: NextRequest) =>
116118
[TraceAttr.UserId]: userId,
117119
})
118120

119-
let toolRegistry: ResolvedSecretTraceRegistry | undefined
120-
let turnRegistry: ResolvedSecretTraceRegistry | undefined
121+
let toolRegistry: ResolvedSecretTraceRegistry
122+
let turnRegistry: ResolvedSecretTraceRegistry
121123
try {
122124
turnRegistry = await getTurnEgressRegistry(userId, workspaceId, messageId)
123125
toolRegistry = turnRegistry.forkForInputPaths([])
124126
} catch (err) {
125-
logger.error('In-band egress registry unavailable; results will be withheld', {
127+
/**
128+
* Without a catalog the projection can vouch for nothing, so every result this call
129+
* could produce would be withheld. Running the tool anyway was the worst of both
130+
* outcomes: the side effect happened and the caller got an opaque sentinel that named
131+
* neither the cause nor whether anything had changed. Refusing before dispatch is
132+
* both truthful and the only answer that leaves nothing behind.
133+
*
134+
* The cause is almost always the workspace itself — a deleted or inaccessible id
135+
* reaching this lane — which is actionable, so it is reported rather than swallowed.
136+
*/
137+
const reason = getErrorMessage(err)
138+
logger.error('In-band egress registry unavailable; refusing the call', {
126139
toolName,
127140
toolCallId,
128-
error: getErrorMessage(err),
141+
userId,
142+
workspaceId,
143+
error: reason,
144+
})
145+
rootSpan.setAttributes({ [TraceAttr.ToolOutcome]: MothershipStreamV1ToolOutcome.error })
146+
return NextResponse.json({
147+
success: false,
148+
error: `${toolName} was not run: ${reason}`,
149+
output: { resultWithheld: true, effect: TOOL_EFFECT_PHASE.notAttempted },
129150
})
130151
}
131152

@@ -149,7 +170,7 @@ export const POST = withRouteHandler((request: NextRequest) =>
149170
})
150171
const projection = inspectToolResultForCopilot(result, toolRegistry, toolName)
151172
const projected = projection.result
152-
if (projection.safe && toolRegistry?.isComplete() && turnRegistry) {
173+
if (projection.safe && toolRegistry.isComplete()) {
153174
turnRegistry.mergeToolCallRegistry(toolRegistry)
154175
}
155176
if (!projected.success) {

apps/sim/lib/environment/utils.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,15 @@ export async function getPersonalAndWorkspaceEnv(
151151
let workspaceCanAdmin = false
152152
if (workspaceId) {
153153
const access = options?.workspaceAccess ?? (await checkWorkspaceAccess(workspaceId, userId))
154+
/**
155+
* A workspace that no longer exists and one the caller may not read are different facts
156+
* and take different corrections — stop using the id versus ask for access. Collapsing
157+
* them sent every deleted-workspace call down the access-denied path, where it read as a
158+
* permissions problem nobody could reproduce.
159+
*/
160+
if (!access.exists) {
161+
throw new Error(`Workspace ${workspaceId} does not exist`)
162+
}
154163
if (!access.hasAccess) {
155164
throw new Error(`Access denied to workspace ${workspaceId}`)
156165
}

0 commit comments

Comments
 (0)