test(sdk): restore the four authored-flow cases dropped in #140 (#166) - #184
Merged
Conversation
#166. #140 shipped with committed conflict markers in this file, and resolving them meant taking main's copy whole -- which protected main's merged coverage but dropped four cases the branch had added, plus the `connectedClient` and `commandsSince` helpers they used. The issue recorded them as "not recoverable from the conflict hunks alone", and that was right: `6384600` IS the conflicted commit, so each incoming side is a fragment whose closing braces live in the shared trailing context after the `>>>>>>>` marker. Reconstructed by taking each incoming hunk plus that trailing context, then checking the result is brace- and paren-balanced with no markers left. Restored: * passes direct input into the journal-backed authored body * the `it.each` operator table -- 7 rows evaluating journal output through truthiness, negation, loose/strict equality, ternary, logical and, logical or * preserves separately awaited sibling ordering before the join * requires an explicit completion after journal-backed steps The loopback needed `outputFor(command)` back as well; main hardcoded `stdout_tail` for a single command, so none of these cases could observe a value. One restored case could not be restored verbatim. It asserted `missing_completion`, and on current main the refusal arrives as `unawaited_step` instead -- `verifyAuthoredOperations` runs before the completion check and throws first, even though the body DOES await its step. Asserting the old code would fail; asserting the new one would bake in a label that reads wrong. It asserts the refusal's class instead, with the reason in a comment, and the mislabelling is filed as #183. That exposed a real hole: disabling the completion check entirely (`if (false && requestedCompletion === undefined)`) left every test in this file GREEN, because the verification refusal always wins first. So `missing_completion` had no coverage at all for that shape. Added `refuses a body that completes nothing at all` -- a body with no operations to verify -- which reaches it and fails under that mutation: × authored flow journal executor > refuses a body that completes nothing at all Tests 1 failed | 22 passed (23) sha256 of authored-flow-executor.ts: 58b3edbf -> 5e35ffc5 -> restored 58b3edbf. authored-flow.test.ts: 23 passed. Full SDK suite: 32 files, 662 passed, 3 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe authored-flow tests add shared client and command helpers. They cover direct inputs, JavaScript branching, sibling-step ordering, emitted journal output, and missing completion errors. ChangesAuthored flow tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Essentials by visiting https://app.coderabbit.ai/settings/billing. Comment |
kjgbot
pushed a commit
that referenced
this pull request
Sep 5, 2026
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1
kjgbot
pushed a commit
that referenced
this pull request
Sep 5, 2026
…nd and filed as #185 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1
kjgbot
pushed a commit
that referenced
this pull request
Sep 5, 2026
#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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1
kjgbot
added a commit
that referenced
this pull request
Sep 5, 2026
…187) Fixes #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) ``` `isHandled` decides whether an operation was consumed by asking whether the **completion** depends on it, and returns false outright when `completionAsyncId` is undefined. With no `done()` there is nothing to trace from, so every operation is unhandled by construction — `verifyAuthoredOperations` ran first, 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 reports the missing completion: the unawaited 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. This also closes a loop from #184, which had to weaken `requires an explicit completion after journal-backed steps` to assert only the refusal's class because the code was wrong. It asserts `missing_completion` again, exactly as it did before #140 dropped it. Evidence at the merged head 5858102: - signoff: local 3-lens preswarm, maintainability / history / structure all REVIEW_PASSED - CI: run 33985621271 success on 5858102 - mutation: moving verification back before the completion check fails exactly that test with `expected AuthoredFlowExecutionError: unawaited_ste… to match object { code: 'missing_completion' }`. Verbatim transcript with runnable commands is in the commit message; sha256 `b7eed1ca` before, restored to `b7eed1ca` (rebuilt from main and re-patched rather than un-edited). - full SDK suite: 33 test files total — 32 passed, 1 skipped — and 665 tests total: 662 passed, 3 skipped; typecheck clean
This was referenced Sep 5, 2026
kjgbot
added a commit
that referenced
this pull request
Sep 5, 2026
Nothing verified main. `cloud-runtime-artifact.yml` triggered only on `pull_request` and `workflow_dispatch`, and `review-swarm.yml` only on `pull_request` — so every PR is checked at its own head and never as merged, and since we squash-merge onto a main that has moved since that CI ran, the composed result went unverified. Not theoretical: twelve PRs merged on 2026-09-05 across the exactly-once claim path (#171, #182), resume adoption (#177, #186), the authored-flow executor (#184, #187) and the CLI run loop (#180) — each green on its own branch, the composed tree never run until dispatched by hand: ``` run 33987924703 workflow_dispatch main completed/success ed917bf ``` Main is fine. But nobody knew that, and finding out required knowing to ask. Without this, a bad compose surfaces as an unrelated PR going red — the most expensive way to find it, since the author debugs their own change first. Tonight already produced three failures on PRs that belonged to something else (#179, #185, the #174 chain), and each cost a tick to attribute. Paths are deliberately not filtered on the push trigger: on a PR the question is "does this change affect the runtime", on main it is "is the tree good", and a docs-only merge can land on a tree someone else broke. Evidence at the merged head d1cb32a: - signoff: local 3-lens preswarm, maintainability / history / structure all REVIEW_PASSED - CI: run 33988314276 success on d1cb32a, verified by headSha and event=pull_request - YAML parsed and asserted: triggers are `pull_request`, `push`, `workflow_dispatch`; push is branch-scoped to main; the pull_request path filter is unchanged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #166.
#140 shipped with committed conflict markers in
sdk/tests/authored-flow.test.ts. Resolving them meant taking main's copy whole — which protected main's merged coverage, but dropped four cases the branch had added plus the helpers they used.Reconstruction
The issue recorded them as "not recoverable from the conflict hunks alone", and that was right:
6384600is the conflicted commit, so each incoming side is a fragment whose closing braces live in the shared trailing context after the>>>>>>>marker. Each was rebuilt from its incoming hunk plus that trailing context, then checked brace- and paren-balanced with zero markers remaining.Restored:
it.eachoperator table — 7 rows evaluating journal output through truthiness, negation, loose/strict equality, ternary, logical and, logical orThe loopback also needed
outputFor(command)back; main hardcodedstdout_tailfor one command, so none of these cases could observe a value.One case could not be restored verbatim — and finding out why exposed a hole
It asserted
missing_completion. On current main the refusal arrives asunawaited_step, becauseverifyAuthoredOperationsruns before the completion check and throws first — even though the body does await its step. Filed as #183.Asserting the old code would fail; asserting the new one would bake in a label that reads wrong. So it asserts the refusal's class, with the reason in a comment.
Then the real problem showed up. Disabling the completion check entirely (
if (false && requestedCompletion === undefined)) left every test in this file green — the verification refusal always wins first, somissing_completionhad no coverage for that shape at all.Added
refuses a body that completes nothing at all(a body with no operations to verify), which reaches it and fails under that mutation:sha256 of
authored-flow-executor.ts:58b3edbf→5e35ffc5→ restored58b3edbf.Evidence
authored-flow.test.ts: 23 passedLenses and CI not yet run at this head — will follow.
🤖 Generated with Claude Code
https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR