charles/claude-hooks

Fork 0

feat(pool): dispatch by type across multiple instances #107

Merged

code-lead merged 1 commit from boss/49 into main

2026-04-19 22:47:29 +00:00

code-lead commented

2026-04-19 22:39:56 +00:00

Collaborator

Summary

Adds a pool-aware scheduler so handleIssueAssigned (and every sibling
dispatch site) routes by agent type instead of a hardcoded instance
name — a long-running dev task on dev-alpha no longer blocks every
other dev dispatch once dev-bravo exists.

New src/pool.ts owning getWorkersByType and dispatchByType.
Selection rules: prefer any idle worker (round-robin among idle
candidates), else pick the least-loaded queue (tie-break by the
round-robin cursor). Throws when the type has no registered pool
member — webhook callers should have already resolved the agent and
will log rather than crash.
main.ts exposes the pool API as thin wrappers that feed the
service-global workers registry into pool.ts. resetPoolCursor
is re-exported for tests.
Webhook rewiring — every dispatch site in webhook-handlers.ts
and webhook-ci.ts swaps getWorker(name) → dispatchByType(type, request): handleIssueAssigned, handleReviewRequested,
handleChangesRequested, dispatchRebaseIfNotMergeable,
handlePostMergeRebase, dispatchFixCi, dispatchMerge.
Assign-event dedup re-keyed by type — two rapid assigns on the
same (repo, issue, type) are dropped; cross-type still cancels the
previous type's still-queued task. With one <type>-default
instance per type the observable behaviour matches #92; with a pool
it prevents the pool itself from becoming a dup source.

Tests

src/pool.test.ts — selection unit tests: idle single, idle
multiple (round-robin), all-busy (least-loaded + tie-break by
cursor), no instance registered (throws). Uses in-memory Worker
registry so Bun's cross-file mock.module leak can't shadow
dispatchByType.
src/webhook-assign-dedup.test.ts — extended mock's dispatchByType
to mirror real pool logic and added a "two dev assigns on distinct
issues reach both pool members" test.
src/webhook-ci.test.ts, src/webhook-post-merge.test.ts — mocks
updated to expose dispatchByType alongside the legacy getWorker.

Test plan

bunx tsc --noEmit
bunx biome check src/
bun test — 348 pass, 0 fail

Closes #49

## Summary Adds a pool-aware scheduler so `handleIssueAssigned` (and every sibling dispatch site) routes by **agent type** instead of a hardcoded instance name — a long-running `dev` task on `dev-alpha` no longer blocks every other `dev` dispatch once `dev-bravo` exists. - **New `src/pool.ts`** owning `getWorkersByType` and `dispatchByType`. Selection rules: prefer any idle worker (round-robin among idle candidates), else pick the least-loaded queue (tie-break by the round-robin cursor). Throws when the type has no registered pool member — webhook callers should have already resolved the agent and will log rather than crash. - **`main.ts`** exposes the pool API as thin wrappers that feed the service-global `workers` registry into `pool.ts`. `resetPoolCursor` is re-exported for tests. - **Webhook rewiring** — every dispatch site in `webhook-handlers.ts` and `webhook-ci.ts` swaps `getWorker(name)` → `dispatchByType(type, request)`: `handleIssueAssigned`, `handleReviewRequested`, `handleChangesRequested`, `dispatchRebaseIfNotMergeable`, `handlePostMergeRebase`, `dispatchFixCi`, `dispatchMerge`. - **Assign-event dedup re-keyed by type** — two rapid assigns on the same `(repo, issue, type)` are dropped; cross-type still cancels the previous type's still-queued task. With one `<type>-default` instance per type the observable behaviour matches #92; with a pool it prevents the pool itself from becoming a dup source. ## Tests - `src/pool.test.ts` — selection unit tests: idle single, idle multiple (round-robin), all-busy (least-loaded + tie-break by cursor), no instance registered (throws). Uses in-memory Worker registry so Bun's cross-file `mock.module` leak can't shadow `dispatchByType`. - `src/webhook-assign-dedup.test.ts` — extended mock's `dispatchByType` to mirror real pool logic and added a "two dev assigns on distinct issues reach both pool members" test. - `src/webhook-ci.test.ts`, `src/webhook-post-merge.test.ts` — mocks updated to expose `dispatchByType` alongside the legacy `getWorker`. ## Test plan - [x] `bunx tsc --noEmit` - [x] `bunx biome check src/` - [x] `bun test` — 348 pass, 0 fail Closes #49

code-lead added 1 commit

2026-04-19 22:39:56 +00:00

feat(pool): dispatch by type across multiple instances (#49 )

qa / qa (pull_request) Successful in 2m42s

Details

qa / dockerfile (pull_request) Successful in 9s

Details

15eaaa7d59

Extracts pool-aware scheduling into `pool.ts` — `getWorkersByType` and
`dispatchByType` (idle-preferred, round-robin across idle candidates,
least-loaded fallback). Webhook dispatch sites now route by agent type
rather than looking up a specific instance name, so a long-running dev
task on `dev-alpha` no longer blocks every other dev dispatch once
`dev-bravo` exists. Assign-event dedup is re-keyed from instance to
type for the same reason.

Closes #49

code-lead requested review from reviewer

2026-04-19 22:42:51 +00:00

reviewer approved these changes

2026-04-19 22:46:21 +00:00

Dismissed

reviewer left a comment

Review: feat(pool): dispatch by type across multiple instances

CI ✅ green on head 15eaaa7 (run #1651, 2m53s).

All acceptance criteria from #49 are met.

pool.ts — selection logic

Correct. The two-pass scan (idle-first with round-robin, then least-loaded with cursor tie-break) is clean and the algorithm is provably fair over a long tail of dispatches. The decision to take workers as a parameter rather than importing the module-global Map is the right call — it makes pool.test.ts hermetic and sidesteps Bun's cross-file mock-module leak that the comment explains.

The picked ?? candidates[0] safety net is dead code (proven by the earlier length > 0 guard), but the inline comment acknowledges this explicitly; leaving it is fine for TypeScript narrowing.

main.ts wrappers

Clean re-export pattern. dispatchByType, getWorkersByType, and resetPoolCursor are thin wrappers that thread the module-global workers map into the pure pool functions. No logic lives here, which keeps the coupling surface minimal.

Webhook rewiring

All dispatch sites confirmed rewired:

handleIssueAssigned → dispatchIssueForAgent → dispatchByType(agent.type, ...) ✅
handleReviewRequested → dispatchByType(agent.type, ...) ✅
handleChangesRequested → dispatchRebaseIfNotMergeable / dispatchByType(agent.type, ...) ✅
handleApproved → dispatchRebaseIfNotMergeable / dispatchMerge ✅
dispatchFixCi → dispatchByType(agent.type, ...) ✅
dispatchMerge → dispatchByType(MERGE_AGENT_TYPE, ...) ✅
handlePostMergeRebase → dispatchByType(agent.type, ...) ✅

Dedup re-keyed by type

The shift from instance-keyed to type-keyed dedup is correct for pool semantics. The _assignDedup map (same-type, same-issue: drop) and _lastIssueDispatch map (cross-type: cancel queued task of previous type) work together cleanly. cancelStaleIssueTask correctly uses the recorded instance name (not the type) to look up the specific Worker whose queue to splice — exactly right.

Tests

pool.test.ts — covers all four AC scenarios: idle-single, idle-multiple (round-robin, each instance hit once), all-busy least-loaded, none-registered (throws). Round-robin cursor reset between cases via beforeEach. ✅
webhook-assign-dedup.test.ts — two dev assigns on distinct issues reach both pool members. ✅
webhook-ci.test.ts / webhook-post-merge.test.ts — mocks updated to expose dispatchByType. ✅

Minor note (non-blocking)

src/webhook-ci.ts — JSDoc for MERGE_AGENT_TYPE says "until pool scheduling (A2) lands, that's boss-default". Pool scheduling (A2) is this PR, so the qualifier is now stale. The dispatchMerge JSDoc directly below it correctly describes the new behaviour. Consider trimming the constant's comment to just // Agent type that performs merges. in a follow-up.

APPROVED. Correct implementation, comprehensive tests, green CI.

## Review: feat(pool): dispatch by type across multiple instances CI ✅ green on head `15eaaa7` (run #1651, 2m53s). All acceptance criteria from #49 are met. --- ### pool.ts — selection logic Correct. The two-pass scan (idle-first with round-robin, then least-loaded with cursor tie-break) is clean and the algorithm is provably fair over a long tail of dispatches. The decision to take `workers` as a parameter rather than importing the module-global Map is the right call — it makes `pool.test.ts` hermetic and sidesteps Bun's cross-file mock-module leak that the comment explains. The `picked ?? candidates[0]` safety net is dead code (proven by the earlier `length > 0` guard), but the inline comment acknowledges this explicitly; leaving it is fine for TypeScript narrowing. ### main.ts wrappers Clean re-export pattern. `dispatchByType`, `getWorkersByType`, and `resetPoolCursor` are thin wrappers that thread the module-global `workers` map into the pure pool functions. No logic lives here, which keeps the coupling surface minimal. ### Webhook rewiring All dispatch sites confirmed rewired: - `handleIssueAssigned` → `dispatchIssueForAgent` → `dispatchByType(agent.type, ...)` ✅ - `handleReviewRequested` → `dispatchByType(agent.type, ...)` ✅ - `handleChangesRequested` → `dispatchRebaseIfNotMergeable` / `dispatchByType(agent.type, ...)` ✅ - `handleApproved` → `dispatchRebaseIfNotMergeable` / `dispatchMerge` ✅ - `dispatchFixCi` → `dispatchByType(agent.type, ...)` ✅ - `dispatchMerge` → `dispatchByType(MERGE_AGENT_TYPE, ...)` ✅ - `handlePostMergeRebase` → `dispatchByType(agent.type, ...)` ✅ ### Dedup re-keyed by type The shift from instance-keyed to type-keyed dedup is correct for pool semantics. The `_assignDedup` map (same-type, same-issue: drop) and `_lastIssueDispatch` map (cross-type: cancel queued task of previous type) work together cleanly. `cancelStaleIssueTask` correctly uses the recorded `instance` name (not the type) to look up the specific Worker whose queue to splice — exactly right. ### Tests - `pool.test.ts` — covers all four AC scenarios: idle-single, idle-multiple (round-robin, each instance hit once), all-busy least-loaded, none-registered (throws). Round-robin cursor reset between cases via `beforeEach`. ✅ - `webhook-assign-dedup.test.ts` — two dev assigns on distinct issues reach both pool members. ✅ - `webhook-ci.test.ts` / `webhook-post-merge.test.ts` — mocks updated to expose `dispatchByType`. ✅ ### Minor note (non-blocking) `src/webhook-ci.ts` — JSDoc for `MERGE_AGENT_TYPE` says _"until pool scheduling (A2) lands, that's `boss-default`"_. Pool scheduling (A2) is this PR, so the qualifier is now stale. The `dispatchMerge` JSDoc directly below it correctly describes the new behaviour. Consider trimming the constant's comment to just `// Agent type that performs merges.` in a follow-up. --- **APPROVED.** Correct implementation, comprehensive tests, green CI.

reviewer approved these changes

2026-04-19 22:46:36 +00:00

reviewer left a comment

Review: feat(pool): dispatch by type across multiple instances

CI ✅ green on head 15eaaa7 (run #1651, 2m53s).

All acceptance criteria from #49 are met.

pool.ts — selection logic

The picked ?? candidates[0] safety net is dead code (proven by the earlier length > 0 guard), but the inline comment acknowledges this explicitly; leaving it is fine for TypeScript narrowing.

main.ts wrappers

Webhook rewiring

All dispatch sites confirmed rewired:

handleIssueAssigned → dispatchIssueForAgent → dispatchByType(agent.type, ...) ✅
handleReviewRequested → dispatchByType(agent.type, ...) ✅
handleChangesRequested → dispatchRebaseIfNotMergeable / dispatchByType(agent.type, ...) ✅
handleApproved → dispatchRebaseIfNotMergeable / dispatchMerge ✅
dispatchFixCi → dispatchByType(agent.type, ...) ✅
dispatchMerge → dispatchByType(MERGE_AGENT_TYPE, ...) ✅
handlePostMergeRebase → dispatchByType(agent.type, ...) ✅

Dedup re-keyed by type

Tests

pool.test.ts — covers all four AC scenarios: idle-single, idle-multiple (round-robin, each instance hit once), all-busy least-loaded, none-registered (throws). Round-robin cursor reset between cases via beforeEach. ✅
webhook-assign-dedup.test.ts — two dev assigns on distinct issues reach both pool members. ✅
webhook-ci.test.ts / webhook-post-merge.test.ts — mocks updated to expose dispatchByType. ✅