🤖 refactor: extract the agent peer-messaging broker from TaskService - #4010
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This comment has been minimized.
This comment has been minimized.
ibetitsmike
force-pushed
the
mike/arch-peer-message-broker
branch
from
August 30, 2026 16:38
daa28db to
dc16171
Compare
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This comment has been minimized.
This comment has been minimized.
ibetitsmike
force-pushed
the
mike/arch-peer-message-broker
branch
from
August 30, 2026 21:10
dc16171 to
686fa3c
Compare
ibetitsmike
force-pushed
the
mike/arch-peer-message-broker
branch
from
August 30, 2026 21:33
686fa3c to
9c236c2
Compare
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Aug 31, 2026
…eline (coder#4024) ## Summary Unifies the four agent-tree message dispatch paths in `taskService.ts` (descendant guidance, peer/ancestor `task_send_message`, and the parent/sibling RLM family-message routes) behind one relation-parameterized `sendTreeMessage` pipeline. The four public methods keep their exact signatures, result/error shapes, and delivery semantics; they are now thin wrappers that build a relation spec and enter the shared pipeline. ## Background Card 3 of the 2026-08-31 architecture review. PR coder#4010 centralized peer-messaging admission invariants (throttles, budgets, envelope prep) in `agentPeerMessageBroker.ts`, but the four dispatch paths still each reimplemented the same ritual: validate topology, admission refusals, envelope/payload construction, budget reservation, delivery lock, dispatch, refund accounting. The same char-cap, budget, envelope, and lock logic appeared up to four times with only the relationship check differing (~1,150 lines). ## Implementation - One private `sendTreeMessage(spec)` pipeline owns the shared phase order (normalize/cap, topology, refusals, envelope, budget, lock, dispatch, refund accounting), parameterized by a relation-discriminated spec. Overloads keep each public route's result type exact. - The trusted descendant machinery (queued-prompt splice, reactivation, live guidance reservation) stays its own dispatcher under the pipeline; it has no cap/budget/envelope by design, and the sibling family route still dispatches through it with the shared parent as authorizing ancestor. - Parent and sibling family routes collapse into one `sendFamilyTreeMessage` core; their envelope/trigger construction moves into `AgentPeerMessageBroker.prepareFamilyMessage` next to `preparePeerMessage`, so one module owns every envelope format. Payload/trigger strings are byte-identical to before. - Budget reservation/refund mechanics are unified (`reserveTreeMessageBudget` with `markPersisted`/`refundIfUnpersisted`) while each route keeps its exact refund policy (refund only when nothing persisted; queued-then-cleared sends keep the charge). - Peer admission machinery (sender/target liveness, stop-epoch latches, `admissionStale` probe, dedupe keys, correlation metadata, wake accounting) is preserved unchanged inside the peer leg. - Duplicated per-route ritual tests (cap, budget refund, workflow refusal, envelope framing) are consolidated into relation-parameterized tests asserting through the public methods; unique-mechanics tests (reactivation, guidance reservation cleanup, stop races, nuclear-family topology) remain separate. Net: -28 LOC across production and tests (+457/-485). ## Validation - `make static-check` green; `make typecheck` green. - `bun test` on `taskService.test.ts`, `agentPeerMessageBroker.test.ts`, `tools/task_list.test.ts`, `tests/ipc/tasks/persistentSubagentCompaction.test.ts`: fail set identical to a clean-base run of the same suite (two pre-existing host-specific fails, no regressions). - Remote dogfood UAT exercised all four relations end to end on this exact head (descendant steering, inactive-child reactivation, sibling envelope + relationship tags, ancestor turn-end default, 16384-char peer cap refusal, best-of candidate refusal). One High finding surfaced: child-to-root sends that queue behind a busy root turn (`queued`/`turn-end`) are never delivered after the turn ends. A controlled re-run of the identical scenario against base main `0672c8f47` reproduced it exactly, so it is a pre-existing defect in the queued-wake drain path, not introduced by this refactor; it deserves a separate fix. ## Risks Intricate-logic refactor in the messaging hot path. Highest-severity regression classes would be: lost or duplicated peer wakes (budget/refund drift), envelope framing changes (security boundary), and descendant reactivation semantics. Mitigations: phase order and refund horizons preserved verbatim per route, envelope strings byte-identical, admission probes untouched, and the relation-parameterized tests assert through the public API with per-route error shapes. --- _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh` • Cost: `$27.13`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh costs=27.13 -->
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.
Summary
Extracts the peer/family agent-messaging protocol's admission invariants out of
TaskServiceinto a newAgentPeerMessageBroker: sliding-window rate limits (per sender-target pair and per target), duplicate suppression, queued-peer-message capacity, consecutive-peer-wake caps, per-pair/per-target session budgets with idempotent refunds, per-target delivery locks, title capping, and peer envelope/trigger composition.TaskServicekeeps tree topology, stop-epoch/overlay orchestration, and dispatch, and delegates every admission decision directly (no pass-through wrappers).Background
Refactor #4 from the 2026-08-29 architecture review (evidence at main @ f04e0f8): taskService.ts inlined the whole protocol across two regions (~5654-6736, ~9612-10097), and every protocol-invariant test had to spin up the 35-method
WorkspaceHostmock harness. Protocol invariants now live in one module testable with a fake one-method host and a mock clock.Behavior-preserving by construction: check ordering, refusal codes, reason strings,
retryAfterMsmath, and budget charge arithmetic are byte-identical to the pre-refactor code.Net LOC delta
agentPeerMessageBroker.ts+taskService.ts)agentPeerMessageBroker.test.ts+taskService.test.ts)Harness-heavy tests that only re-asserted pure invariants through the full TaskService stack were deleted and replaced by table-driven broker tests; one thin delegation test per route (peer rate-limit refusal,
task_message_parentand sibling budget exhaustion) plus every test covering behavior the broker cannot see (refund-on-dispatch-failure, charge-at-admission ordering, stop races, envelope-row persistence, delivery serialization) were kept. The only irreducible addition is the broker's structural boilerplate (imports, class/constructor, the local one-method host interface), which is what makes the invariants independently testable.The broker declares its own minimal host interface (
countQueuedAgentPeerMessages) instead of importing fromtaskWorkspaceSeam.ts, keeping this diff merge-independent from the parallel WorkspaceHost role-interface split.Validation
ae9bf3e93— live Xum UI multi-agent tree across descendant/sibling/ancestor relations, an independent 22-assertion edge-case probe (rate/dedupe/wake/budget boundaries at exact ceilings, refund idempotence), and a source-level cross-check of every refusal string and retry computation against f04e0f8. The one commit since (daa28db4f) is cosmetic (test-cast type reuse), revalidated locally.bun test src/node/services/agentPeerMessageBroker.test.ts: 12/12.bun test src/node/services/taskService.test.ts: 514 pass; the 2-3 sporadic failures (terminal recovery ...x2,higher-ancestor waiters ...) reproduce identically on pristineorigin/mainin this environment (config.json atomic-write chown/rename ENOENT under /tmp) and are unrelated to this diff.make static-check: green.Risks
Low. The moved logic is byte-identical and the delegation points are 1:1 at existing callsites; the riskiest surface (admission/charge ordering under concurrent sends) is covered by kept integration tests and the broker's check-order test (queued-count host query is only reached after in-memory checks pass).
Generated with
xum• Model:anthropic:claude-fable-5• Thinking:xhigh• Cost:$37.96Stack
Layer 3/10 of the architecture refactor stack (net -5,101 LOC overall). This PR's diff is only this layer, against
mike/arch-git-patch-engine.