From 5858102603165745c52b170398d745363e468914 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Sat, 5 Sep 2026 20:30:19 +0200 Subject: [PATCH] fix(sdk): report the missing done(), not the step the author awaited MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #183. A body that awaited its only step and forgot `done()` was refused as `unawaited_step`, naming the step it had awaited: unawaited_step: flow "missing-completion" returned with unawaited steps: run-1 (f.run) The step was awaited. The defect was the missing completion, and there is a code for exactly that. Cause, found by probing rather than by reading: `isHandled` decides whether an operation was consumed by asking whether the COMPLETION depends on it, and returns false outright when there is no completion: isHandled(operation) { const completion = this.completionAsyncId; ... if (completion === undefined || ...) return false; With no `done()` there is no completion async id to trace from, so every operation is unhandled by construction. `verifyAuthoredOperations` ran before the completion check, computed an answer its own precondition did not support, and reported the symptom as the cause. The completion check now runs first, stopping operations and closing the lifecycle the way the body-failure path already does. A body that both forgets `done()` AND leaves a step unawaited now reports the missing completion. That is the honest order rather than a convenience: the unawaited-step verdict is not computable until there is a completion to compute it against, and once the author adds `done()` the verification runs normally and catches it. `requires an explicit completion after journal-backed steps` goes back to asserting `missing_completion`, the code it asserted before #140 dropped it. #184 had to weaken it to the refusal's class precisely because of this bug. Evidence. Commands runnable from the repository root; output below is verbatim, nothing removed: $ shasum -a 256 sdk/src/authored-flow-executor.ts b7eed1ca1197fd38dd01cfd4199be5883031fd7b26d1fc6f9a61dc8fb100a211 sdk/src/authored-flow-executor.ts $ (cd sdk && ./node_modules/.bin/vitest run tests/authored-flow.test.ts -t "requires an explicit completion") RUN v2.1.9 /Users/khaliqgant/AgentWorkforce/flows-183/sdk ✓ tests/authored-flow.test.ts (23 tests | 22 skipped) 5ms Test Files 1 passed (1) Tests 1 passed | 22 skipped (23) Start at 20:38:36 Duration 398ms (transform 92ms, setup 0ms, collect 167ms, tests 5ms, environment 0ms, prepare 38ms) # MUTATION: verification moved back BEFORE the completion check (the #183 order) $ shasum -a 256 sdk/src/authored-flow-executor.ts a999fbdcf998330718fd91fa9ff3711a3c6ea3d68cbbf585b5f041e649747178 sdk/src/authored-flow-executor.ts $ (cd sdk && npm --userconfig /private/tmp/claude-501/-Users-khaliqgant-Projects-AgentWorkforce-chief/c228933d-4f94-4d83-9a9a-daf3c83b94f1/scratchpad/empty-npmrc run build >/dev/null 2>&1; ./node_modules/.bin/vitest run tests/authored-flow.test.ts -t "requires an explicit completion") RUN v2.1.9 /Users/khaliqgant/AgentWorkforce/flows-183/sdk ❯ tests/authored-flow.test.ts (23 tests | 1 failed | 22 skipped) 9ms × authored flow journal executor > requires an explicit completion after journal-backed steps 7ms → expected AuthoredFlowExecutionError: unawaited_ste… { …(3) } to match object { code: 'missing_completion' } (3 matching properties omitted from actual) ⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯ FAIL tests/authored-flow.test.ts > authored flow journal executor > requires an explicit completion after journal-backed steps AssertionError: expected AuthoredFlowExecutionError: unawaited_ste… { …(3) } to match object { code: 'missing_completion' } (3 matching properties omitted from actual) - Expected + Received - Object { - "code": "missing_completion", + AuthoredFlowExecutionError { + "code": "unawaited_step", } ❯ tests/authored-flow.test.ts:477:7 475| // `done()` they forgot (#183). Fixed by checking the completion… 476| // so this can pin the code again. 477| await expect(executeAuthoredFlow(handle, client)).rejects.toMatc… | ^ 478| code: 'missing_completion', 479| }); ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯ Test Files 1 failed (1) Tests 1 failed | 22 skipped (23) Start at 20:38:38 Duration 323ms (transform 80ms, setup 0ms, collect 125ms, tests 9ms, environment 0ms, prepare 34ms) # RESTORED (rebuilt from main and re-patched, not un-edited) $ shasum -a 256 sdk/src/authored-flow-executor.ts b7eed1ca1197fd38dd01cfd4199be5883031fd7b26d1fc6f9a61dc8fb100a211 sdk/src/authored-flow-executor.ts Full SDK suite at this head: 33 test files total -- 32 passed, 1 skipped -- and 665 tests total: 662 passed, 3 skipped. Typecheck clean. An earlier draft of this message called an abbreviated snippet "output complete" and wrote "32 files" for a total of 33. Both were rejected by review, fairly. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- sdk/src/authored-flow-executor.ts | 32 +++++++++++++++++++++++++------ sdk/tests/authored-flow.test.ts | 21 +++++++++----------- 2 files changed, 35 insertions(+), 18 deletions(-) diff --git a/sdk/src/authored-flow-executor.ts b/sdk/src/authored-flow-executor.ts index b8582d5f7..bdc8b11df 100644 --- a/sdk/src/authored-flow-executor.ts +++ b/sdk/src/authored-flow-executor.ts @@ -197,16 +197,36 @@ export async function executeAuthoredFlow( } throw bodyFailure; } - try { - await verifyAuthoredOperations(definition.name, authoredSteps, lifecycle); - } finally { - lifecycle.close(); - } + // The completion requirement is checked BEFORE operation verification, + // because without a completion the verification cannot answer its own + // question. `AuthoredFlowLifecycle.isHandled` decides whether an operation + // was consumed by asking whether the COMPLETION depends on it; with no + // completion there is no async id to trace from, so it returns false for + // every operation. Verifying first therefore reported correctly-awaited + // steps as `unawaited_step`, naming the step the author had awaited and + // saying nothing about the `done()` they forgot (#183). + // + // A body that both forgets `done()` and leaves a step unawaited now reports + // the missing completion. That is the honest order: the unawaited-step + // verdict is not computable until there is a completion to compute it + // against, and once the author adds `done()` the verification runs normally + // and will catch it. if (requestedCompletion === undefined) { - throw new AuthoredFlowExecutionError( + const missingCompletion = new AuthoredFlowExecutionError( 'missing_completion', `flow "${definition.name}" returned without done()`, ); + try { + await stopAuthoredOperations(authoredSteps, missingCompletion); + } finally { + lifecycle.close(); + } + throw missingCompletion; + } + try { + await verifyAuthoredOperations(definition.name, authoredSteps, lifecycle); + } finally { + lifecycle.close(); } await lowerDeterministic(`complete-${nextStep}`, ':'); diff --git a/sdk/tests/authored-flow.test.ts b/sdk/tests/authored-flow.test.ts index b2bcc9edd..985ee93a6 100644 --- a/sdk/tests/authored-flow.test.ts +++ b/sdk/tests/authored-flow.test.ts @@ -465,20 +465,17 @@ describe('authored flow journal executor', () => { }); const client = await connectedClient('authored-flow-missing-completion-test'); try { - // The PROPERTY is what this pins: a body that runs a journal-backed step - // and never calls done() must be refused, not silently completed. + // Back to the code the original test asserted before #140 dropped it. // - // The original version of this test (dropped in #140, restored here) - // asserted `missing_completion`. On current main the refusal arrives as - // `unawaited_step` instead, because `verifyAuthoredOperations` runs - // before the `requestedCompletion === undefined` check in - // `authored-flow-executor.ts` and throws first. The body here DOES await - // its step, so "returned with unawaited steps" is a misleading label for - // it -- see #183. Asserting either code specifically would either fail on - // main or bake in a message that looks wrong, so this asserts the - // refusal and its class instead. + // Between the restoration and now this had to assert only the refusal's + // class, because the executor answered `unawaited_step` -- it verified + // operations before checking for a completion, and `isHandled` returns + // false for every operation when there is no completion to trace from. + // It named the step the author HAD awaited and said nothing about the + // `done()` they forgot (#183). Fixed by checking the completion first, + // so this can pin the code again. await expect(executeAuthoredFlow(handle, client)).rejects.toMatchObject({ - name: 'AuthoredFlowExecutionError', + code: 'missing_completion', }); } finally { client.close();