test(#754): pin the missing no-holder-fallback regression + comment cleanup - #819
Merged
Conversation
…r resolver must never reintroduce #333 Phase 3 (#726) already delivered the full seam this issue describes: RoleHolderSource / RoleHolderRegistry (harness-channel-sdk/src/roleHolderSource.ts), the conductor-local wiring (src/conductor/roleHolderResolver.ts), and index.ts building the registry and threading it into both the executor (resolveRoleHolders) and the await worker (holdersOnly). RunExecutor already refuses quorum='all' completion on a partial holder list and already withholds the no-holder fallback on a partial-empty lookup (runExecutor.ts resolveAwait / openHumanAwait). What was still missing was the issue's second mandatory regression test: - quorum='all' partial/unavailable non-completion already had a test (conductorQuorumAndTimeout.test.ts:127, from #726). - 'role has no holder -> fallback' firing ONLY on a resolved-empty lookup, never on unavailable, had no test at all — openHumanAwait's fail-closed branch was unexercised. Added that missing pair via ConductorRunExecutor.startRun with a minimal in-memory workflow/run/await-store harness (same pattern as the existing quorum tests): - a RESOLVED empty holder list takes the fallback path (records the step 'failed' with actor.noHolder=true) and never opens an await. - an UNAVAILABLE (partial) holder lookup does NOT take the fallback -- it parks (opens an await, calls runStore.park) exactly as if a real holder existed but had not answered yet. Mutation-verified: reverting either fail-closed guard in runExecutor.ts (the partial branch in openHumanAwait, or the !holdersPartial conjunct in the quorum='all' completeness check) turns the corresponding test red; both were confirmed against a clean tsc rebuild before and after. Also updated the roleStore.ts:22 comment (issue requirement #5) -- it still described the external-resolver seam as future work after #726 had already filled it.
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
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.
Summary
Closes #754. The scope was almost entirely already delivered by #333 Phase 3 (PR #726) — this PR
finds and fills the one gap: the second mandatory regression test the issue calls out, plus a
stale comment.
What #726 already shipped (verified, not re-designed)
middleware/packages/harness-channel-sdk/src/roleHolderSource.ts—RoleHolderSource,RoleHolderRegistry,AggregateHolderLookupwith theresolved | unavailable+partialdiscipline the issue requires (never a bare
string[]).middleware/src/conductor/roleHolderResolver.ts— wiresConductorRoleStorein as the'conductor-local'source,buildRoleHolderRegistry(), andholdersOnly()for thecall sites that may legitimately degrade (operator inbox, reminder nudger).
middleware/src/conductor/index.ts— already builds the registry(
buildRoleHolderRegistry(roleStore, deps.roleHolderSources ?? [])) and threads it into bothRunExecutor(resolveRoleHolders) and the await worker (holdersOnly(...)).middleware/src/conductor/runExecutor.ts:resolveAwait'squorum='all'completeness check already refuses to close on a partialholder list, even if every currently-known holder responded.
openHumanAwait's "role has no holder → take the fallback" already only fires on aresolved-empty lookup; a partial-empty lookup parks instead (the run reaches the same
fallback later via its deadline, but only after the real holders had a chance to answer).
I verified this by reading
roleHolderResolver.ts/runExecutor.tsend to end before touchinganything — the "check whether wiring is the entire remaining scope" the issue asks for. It is.
What was missing
Of the issue's two mandatory regression tests:
quorum='all'non-completion onpartial/unavailable— already existed(
conductorQuorumAndTimeout.test.ts:127, from feat(#333): role sources + pluggable role→holder resolution (phases 2 and 3) #726), with a same-inputspartial:falsecontrol right below it.
resolved-empty, never onunavailable— had notest.
openHumanAwait's fail-closed branch was implemented but unexercised.Added the missing pair in
middleware/test/conductorQuorumAndTimeout.test.ts(new describeblock, same minimal-fake-harness pattern as the existing quorum tests, driven through the public
ConductorRunExecutor.startRun):'failed'withactor: { kind: 'human', noHolder: true }, opens zero awaits.await, calls
runStore.park) exactly as if a real, not-yet-answered holder existed.Also updated the
roleStore.ts:22comment (issue requirement #5): it still described theexternal-resolver seam as future work after #726 had already filled it.
Blast radius
middleware/src/conductor/roleStore.ts— comment-only change (no behavior change).middleware/test/conductorQuorumAndTimeout.test.ts— additive; no existing test modified.credentials/namespaces,src/index.ts.Test + mutation evidence
npx tsx --test test/conductorQuorumAndTimeout.test.ts— 9/9 pass (4 suites).npx tsx --test test/conductor*.ts(full conductor suite) — 229/229 pass (58 suites).npx tsx --test test/roleHolderSource.test.ts test/roleSource.test.ts test/conductorCancelAndStrictApproval.test.ts test/conductorReminders.test.ts test/conductorRunExecutorNotifyRunEnded.test.ts— 59/59 pass.npm run build— clean.npm run typecheck:test— ratchet unchanged (371 known errors, baseline 371, no regressions).runExecutor.tsone at a time against aclean rebuild —
partialbranch inopenHumanAwait→ new test chore(deps,ci): Bump docker/metadata-action from 5 to 6 #2 (unavailable → no fallback)goes red.
!holdersPartialconjunct from thequorum='all'completeness check → thepre-existing feat(#333): role sources + pluggable role→holder resolution (phases 2 and 3) #726 test goes red.
Restored both, rebuilt, reran — clean diff, 9/9 green again.
Open questions
None — the scope really was "wiring is already the entire remaining work," confirmed before
writing anything new, per the issue's own instruction.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.