Skip to content

Commit 4d2ba50

Browse files
icecrasher321claude
andcommitted
docs(copilot): state that a named run may resolve to nothing
Both reviewers read an id on a preflight failure as a defect. It is the one place this contract is deliberately coarse, so say so where each of them was looking rather than leave it to be rediscovered. `attempted` already means "zero or one executions exist under this id" — the id is a correlation key, not a promise that a row exists. A caller resolves it, finds nothing, and retries, which is the right outcome at the cost of one lookup. Buying that lookup back means an executor-side dispatch marker: a callback on every block of every execution in the product, which this branch just reverted for that reason. It would also gain nothing, since all four preflight throws are invariant violations — no workspace id, no billing attribution, no principal, attribution mismatch — and a retry fails identically. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 79401fd commit 4d2ba50

2 files changed

Lines changed: 19 additions & 1 deletion

File tree

apps/sim/lib/copilot/tool-executor/types.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,14 @@ export interface ToolExecutionContext {
5656
export const TOOL_EFFECT_PHASE = {
5757
/** Rejected before anything could happen. Correcting the call and retrying is safe. */
5858
notAttempted: 'not_attempted',
59-
/** Dispatched; zero or one effects may exist. Resolve by id before retrying. */
59+
/**
60+
* Dispatched; zero or one effects may exist. Resolve by id before retrying.
61+
*
62+
* Zero is a legitimate outcome here, not a defect: the id is a correlation key, not a
63+
* promise that a row exists. Narrowing this to "a run definitely exists" would take
64+
* per-block instrumentation across every execution in the product to spare one caller a
65+
* lookup that answers the question definitively either way.
66+
*/
6067
attempted: 'attempted',
6168
/** The effect ran to completion, whatever its outcome. Never retry blind. */
6269
performed: 'performed',

apps/sim/lib/workflows/application/run-workflow-from-copilot.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,17 @@ describe('Copilot workflow run application commands', () => {
334334
expect(readAttemptedExecutionId(await failWith())).toBe('child-execution-1')
335335
})
336336

337+
/**
338+
* Deliberate, and the one place this contract is deliberately coarse: `executeWorkflow`
339+
* validates its own arguments before creating anything, and those failures still name
340+
* the run. `attempted` means "zero or one executions exist under this id, resolve it",
341+
* so the caller resolves, finds nothing, and retries — correct, at the cost of a lookup.
342+
*
343+
* Paying to avoid that lookup means an executor-side dispatch marker, which is a
344+
* callback on every block of every execution in the product. It would also buy nothing:
345+
* all four preflight throws are invariant violations — no workspace id, no billing
346+
* attribution, no principal, attribution mismatch — so a retry fails identically.
347+
*/
337348
it('names the run for a failure inside the executor call, whatever its cause', async () => {
338349
mocks.executeWorkflow.mockRejectedValueOnce(
339350
new Error('Billing attribution is required for workspace execution')

0 commit comments

Comments
 (0)