Repository navigation
🤖 refactor: move TaskService's pure tool-output and message helpers into modules - #7665
Merged
Merged
Conversation
…nto their own modules taskToolOutputs.ts holds the task/workflow tool-output parsers and report receipts, taskMessagePredicates.ts the chat-message predicates. WorkspaceService now imports the six predicates it had copied instead of keeping duplicates.
ThomasK33
changed the title
This was referenced Oct 27, 2026
ThomasK33
marked this pull request as ready for review
October 27, 2026 08:00
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Contributor
🟢 e2e stories (Haiku): 14 passed22 agent steps · 7 replayed from cache · 15 model calls · 46.2k tokens (24% cached) All 14 tests in 9 files
e2e 0.20.0 · 1m 18s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.0k tokens (22% cached) All 3 tests in 3 files
e2e 0.20.0 · 18.8s · electron · run artifacts 🟢 e2e agent repros (Haiku): no tests selectedSpend: $0.00 of the $1.00 cap (proxy ledger) e2e 0.20.0 · 5.3s · 2 targets (web, phone) · run artifacts |
This was referenced Oct 27, 2026
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 27, 2026
…der#7666) ## Summary Moves TaskService's owed workflow terminal wakes (`pendingWorkflowRunAttention`) and the per-run settlement chain (`workflowRunSettlementByRun`) into a new `WorkflowRunAttention` class (`workflowRunAttention.ts`). `taskService.ts`: 22,382 → 22,224 lines. ## Background PR 2 to 5 of the TaskService split stack (coder#7665 is PR 1). Each layer moves one group of TaskService state and the methods that own it into a plain class that TaskService builds in its constructor. The rules are the same in every layer: public methods stay on TaskService as one-line facades, methods that tests spy on or patch stay reachable on TaskService and the new class calls them through call-time closures, and tests that cast a moved field change only the cast target. No behaviour changes. ## Implementation - Moved unchanged: `queueWorkflowRunAttention`, `markWorkflowRunTerminalAttentionSettled`, `settleWorkflowRunTerminalAttention`, `clearWorkflowRunDowngradeSettlement`. The two public ones keep TaskService facades (tools/task_await, tools/workflow_resume, WorkflowService and turnRequestBuilder call them). - The sweep, `noteWorkflowRunTerminalAttention` and `drainTerminalAttention` stay on TaskService. The drain uses three accessors: `queuedRunIds`, `dropOwner` and `dropRun`. - Deps are two closures, `sessionsDir` and `terminalAttentionStore`, so a test that swaps the store still works. - The class stays separate from `workspaceWorkflowRuns.ts`, which caches sidebar run activity for WorkspaceService. - Tests: the 7 queue seeds in `taskService.test.ts` and `queuedWorkflowRunAttention` in the shared harness now cast `workflowRunAttention.pendingWorkflowRunAttention`. The new `workflowRunAttention.test.ts` (14 tests) covers queue dedupe, settle write order, a failed marker write, a newer generation, an unreadable run, and that settlements of one run do not overlap. ## Validation - `bun test` on taskService, workspaceTurnManager, workflows, tools and the new file: all pass (on this layer and on the stack top, 4,898 tests). - `make static-check`, `make lint-eslint`, `make crap-check` (moved methods are CC 2 to 6), `make dry-check`: pass. - Mutation on the new class: 18 killed, 2 equivalent (`??` → `||` on a Set or Promise). ## Risks Low. The settlement chain and the settle write order move as one unit, with the same statements. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$53.33`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=53.33 -->
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 27, 2026
## Summary Moves peer sends that wait for a delegated turn (`parkedPeerSendsByTarget`, `parkedPeerSendFlushLocks`) and the turn registration epochs (`workspaceTurnRegistrationEpochs`) into a new `PeerSendParking` class (`peerSendParking.ts`). `taskService.ts` loses 99 lines in this layer. ## Background PR 2 to 5 of the TaskService split stack (coder#7665 is PR 1). Each layer moves one group of TaskService state and the methods that own it into a plain class that TaskService builds in its constructor. The rules are the same in every layer: public methods stay on TaskService as one-line facades, methods that tests spy on or patch stay reachable on TaskService and the new class calls them through call-time closures, and tests that cast a moved field change only the cast target. No behaviour changes. ## Implementation - Moved unchanged: `parkPeerSend`, `scheduleParkedPeerSendFlush`, `flushParkedPeerSends`, `onWorkspaceTurnRegistered` and the `ParkedPeerSend` type. - `flushParkedPeerSends` stays on TaskService as a private facade, because six test sites spy on it, mock it or bind it. The class's scheduled flush calls it back through `deps.flush`, so the spies see every flush. `sendTreeMessage` (also spied) is a closure too. - `onWorkspaceTurnRegistered` stays public on TaskService (TaskHost needs it). - Accessors for code that stays: `countParked` (the peer-message broker's queue cap), `hasParked` and `removeRetry` (sendTreeMessage), `registrationEpoch` (the coder#5271 retry gate), `dropParked` (markParentWorkspaceInterrupted). - `TreeMessageSpec` and `PeerDelegatedTurnWait` are now exported types from `taskService.ts`. The new file imports them as `import type`. - Tests: `taskService.peerLimitsRepro.test.ts` changes its cast target. The new `peerSendParking.test.ts` (11 tests) covers park order, holds while a registration or stop is live, the stop baseline, dropped retries, `dropParked` during a drain, and a send parked during a drain. ## Validation - Tests on this layer and the stack top: all pass. - `make static-check`, `make lint-eslint`: pass. `make crap-check`: `flushParkedPeerSends` is CC 11. The new test covers all 78 lines of the file, so its CI CRAP score is 11 (limit 15). - `make dry-check`: `onWorkspaceTurnRegistered` matches two existing per-key counters. A `dry-duplicate-ok:` marker says why they stay apart. - Mutation on the new class: 4 equivalent `??` → `||` survivors, marked. ## Risks Low. Park first, then schedule, and one flush at a time per target, as before. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$53.33`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=53.33 -->
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 27, 2026
## Summary Moves TaskService's report waiters, start waiters, foreground awaits, user-backgrounded marks, launch-failure marks and the completed-report cache (7 fields) into a new `TaskWaiters` class (`taskWaiters.ts`). `taskService.ts` loses 205 lines in this layer. ## Background PR 2 to 5 of the TaskService split stack (coder#7665 is PR 1). Each layer moves one group of TaskService state and the methods that own it into a plain class that TaskService builds in its constructor. The rules are the same in every layer: public methods stay on TaskService as one-line facades, methods that tests spy on or patch stay reachable on TaskService and the new class calls them through call-time closures, and tests that cast a moved field change only the cast target. No behaviour changes. ## Implementation - Moved: the foreground-await and backgrounding methods (all public ones keep facades), `armWaitersOfUnendedTask`, `startPendingStartWaiters`, `enforceCompletedReportCacheLimit`, the waiter and cache types, and `ForegroundWaitBackgroundedError`. `taskService.ts` re-exports the error, so its importers do not change. - `resolveWaiters` stays on TaskService (tests call it through casts). It still computes the ancestry, then calls `cacheCompletedReport` and `resolvePendingWaiters`, so the cache is still written before any waiter resolves. - `rejectWaiters` stays as the TaskService entry point (a test spies on it), and its body is in the class. - `waitForAgentReport`, `readPersistedReportForWait`, `inspectAttemptOutcome` and the termination, reawaken and timeout paths stay. They use accessors in the same order as before. - Two small add/remove helpers (`appendToListMap`, `removeFromListMap`) replace four near-copies that the DRY gate flagged. - Tests: five test files change only a cast target (`waiters.<field>`). The new `taskWaiters.test.ts` (13 tests) covers nested foreground awaits, the backgrounding order (mark, persist, reject), start-waiter order and cache eviction. ## Validation - Tests on this layer and the stack top: all pass. - `make static-check`, `make lint-eslint`, `make crap-check` (all moved methods CC 8 or less): pass. - `make dry-check`: the moved `ForegroundWaitBackgroundedError` constructor matches four existing error constructors. That is existing code reported again because it moved. - Mutation on the new class: 47 of 53 killed. The 6 survivors cannot change behaviour (for example `?? 0` → `|| 0`). ## Risks Low to medium: waiters order the report flow. The orders that matter (cache before drain, register before recheck, mark before start) are unchanged and have unit tests now. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$53.33`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=53.33 -->
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 27, 2026
…Notifier (coder#7669) ## Summary Moves terminal-attention scheduling and bookkeeping into a new `TerminalAttentionNotifier` class (`terminalAttentionNotifier.ts`): notify_on_terminal persists, the per-owner drain chains, idle waits, fresh sub-agent edges and the wake-send backoff (6 fields). `drainTerminalAttention` stays on TaskService. `taskService.ts` loses 178 lines in this layer. After the whole stack (coder#7665 to coder#7669) it has 21,742 lines, down from 22,800. ## Background PR 2 to 5 of the TaskService split stack (coder#7665 is PR 1). Each layer moves one group of TaskService state and the methods that own it into a plain class that TaskService builds in its constructor. The rules are the same in every layer: public methods stay on TaskService as one-line facades, methods that tests spy on or patch stay reachable on TaskService and the new class calls them through call-time closures, and tests that cast a moved field change only the cast target. No behaviour changes. ## Implementation - Moved: `markBackgroundWorkNotifyOnTerminal`, `scheduleNotifyOnTerminalPersist`, `persistNotifyOnTerminalPolicy`, `schedulePendingTerminalAttentionOwnerDrains`, `suppressTerminalAttention`, `enqueueTerminalAttention`, `scheduleTerminalAttentionDrain`, `scheduleTerminalAttentionDrainAfterIdle`, `backOffTerminalWakeSends`. - TaskService keeps facades for the public ones and for the ones tests spy on or cast-call (`enqueueTerminalAttention`, `schedulePendingTerminalAttentionOwnerDrains`). The facades return the inner promise, so they add no ticks. - The drain chain calls `deps.drain`, which goes through `this.drainTerminalAttention`, because the test harness replaces that method on the instance. - Code that stays uses accessors: `takeFreshTurns` (still synchronous, before the drain's first await), `isWakeSendBackedOff` (the drain's two backoff reads, same expire-on-read logic), `trackDrain` and `untrackDrain` (deferred descendant messages and the withdrawn-turn reconcile), `inFlightNotifyOnTerminalPersists` (handleStreamEnd). - `WORKFLOW_TERMINAL_ATTENTION_SWEEP_INTERVAL_MS` moves to the new file, because the backoff needs it. TaskService imports it back for the sweep timer. - `cutBusyOwnerTurnForSubagentAttention`, `resolveTerminalWakeCallerSendRestrictions`, `acknowledgeAgentReports` and the sweep stay. They mostly use other TaskService state. - Tests: the shared harness and `terminalAttentionIdleWait.test.ts` change only a cast target. The new `terminalAttentionNotifier.test.ts` (20 tests) covers per-owner drain order, the idle-wait slot release, fresh edges, backoff expiry and the persist paths. ## Validation - Stack top: `bun test` on taskService, the seven new test files, workspaceTurnManager, workflows, tools and workspaceService: 4,898 pass, 0 fail. Typecheck passes on every layer. - `make static-check`, `make lint-eslint`, `make dry-check`: pass. `make crap-check`: moved methods CC 7 or less. `drainTerminalAttention` drops from CC 106 to 94. - Mutation on the new class: 23 killed, 4 equivalent. ## Risks Medium. Terminal attention decides when parents wake. Every ordering point listed in the plan moves unchanged, and the drain body itself does not move. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$53.33`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=53.33 -->
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 27, 2026
…rvice (coder#7674) ## Summary Splits the five longest stop and attempt methods in `TaskService` into smaller private steps, in place. No behaviour change, no new file, no new class. This is PR 6 of the TaskService split (stack 1, coder#7665 to coder#7669, has merged). It prepares PRs 7 to 10, which move the stop latch and the attempt ledgers into their own classes. ## Background PRs 7 and 9 move these methods to new files. The CRAP gate scores a moved method as new, and its move relief allows at most CC 20 (CRAP 30 in CI). The maintainer chose to split all five methods above CC 15 first, so each later move fits with room to spare. | Method (CC before -> after) | New steps (CC) | |---|---| | `recheckWorkspaceStopRelease` 21 -> 7 | `isWorkspaceStopRecordSettled` (module fn) 5, `isStopReceiptDecisionPending` 5, `startStopSettlementReceipt` 4, `releaseWorkspaceStopRecord` 5 | | `createAdmittedSend` 19 -> 4 | `isAdmittedSendStale` 7, `streamEndBlocksNewTurn` 2, `recordAdmittedSendTurn` 5, `disposeSendBeforeAdmission` 5 | | `admitTaskWorkspaceTurn` 17 -> 10 | `admitSendToCurrentAttempt` 8 | | `evaluateAttemptLineage` 16 -> 9 | `ownedAttemptSettlementPhase` 3, `lineageProofFromReceipt` (module fn) 4 | | `readUnownedSettlementProof` 17 -> 10 | `unownedSettlementCandidate` (module fn) 7, `unownedSettlementReceiptGap` (module fn) 4 | ## Implementation - Each method splits along its early returns. The originals keep their names and signatures, because tests spy on several of them. - No await moved and none was added. The token callbacks in `createAdmittedSend` are now one-line arrows that call the new methods, and they still run synchronously. - `evaluateAttemptLineage` now reads the settlement entry through `ownedAttemptSettlementPhase` twice instead of re-reading a `let`. Without the closing wait there is no await between the two reads, so they return the same value. ## Validation - `make crap-check BASE=<stack 1 head>` reports 0 violations and 0 warnings, with and without coverage. Every new helper above CC 4 has at least 69 % line coverage from the existing tests. - The test run `bun test src/node/services/taskService* workspaceTurnManager workflows tools` passes: 3,579 tests, 0 failures. ## Risks Low. The change is a mechanical split of stop-release and send-admission code. A wrong split would show as a stuck stop latch or a refused send. The settlement, stop and admission test suites cover those paths. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$68.74`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=68.74 -->
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 27, 2026
## Summary Moves the stop latch out of `TaskService` into a new plain class, `TaskStopLatch` (`src/node/services/taskStopLatch.ts`). The move is move-only: no behaviour change, no Effect tag. `taskService.ts` drops from 21,830 to 21,470 lines. PR 7 of the TaskService split, on top of coder#7674. ## Background `TaskService` is ~21,000 lines. Stack 1 (coder#7665 to coder#7669) moved five state groups into owner classes. This stack moves the stop and attempt state: PR 7 the stop latch, PRs 8 and 9 the attempt ledger, PR 10 the stream-end decisions. ## Implementation - **Fields that moved:** `workspaceStopEpochs`, `workspaceStopsInProgress`, `queueRechecksOwedByStopLatch` and `workspaceStopRecords`. `workspaceUserStopEpochs` stays on TaskService, because it counts user Stops for `sendTreeMessage` and is not latch state. - **Methods that moved:** the stop cascade (`beginWorkspaceStop`, `runWorkspaceStopCleanup`, `markWorkspaceStopPersisted`), the release (`recheckWorkspaceStopRelease` and the four steps coder#7674 split out of it), the latch and epoch queries, `deferLaunchWhileStopInProgress`, `releaseRetainedStopLatches` and `waitForWorkspaceStopsToRelease`. - **Facades:** TaskService keeps one-line facades for every seam method (`bumpWorkspaceStopEpoch`, `getWorkspaceStopEpoch`, `latchWorkspaceStopsInProgress`, `isWorkspaceStopInProgress`, `releaseRetainedStopLatches`) and for the two methods tests spy on (`runWorkspaceStopCleanup`, `waitForWorkspaceStopsToRelease`). - **Coordinators stay:** `recordWorkspaceTurnSettled` and `recordWorkspaceTurnSuperseded` stay on TaskService. They write three owners in order (admitted sends, stream-end decisions, stop records). Their stop-record tails are now `releaseCapturedTurn` / `rebindCapturedTurn` calls. - **Deps:** the latch reaches attempt state through call-time TaskService closures (`TaskStopLatchDeps`), so test spies and patches on TaskService still see every call. Stop records and admitted sends move by reference. - **Exported types:** `OwnedTaskAttempt`, `AdmittedSend`, `StreamEndDecision` and `WorkspaceStopRecord` are now exported from `taskService.ts` for `import type`. The new `SettlementReceiptDecision` alias names the existing return type of `decideSettlementReceipt`. ## Validation - I checked the moved blocks against the originals with a whitespace-insensitive comparison, after the `this.x` -> `this.deps.x()` swaps. They match. Statement order is the same, and no await was added. - The new file `taskStopLatch.test.ts` has 23 tests on fake deps. They cover latch refcounting, deferred-launch rechecks, capture by reference, the release hold conditions, the receipt write before release, and the wait's listener cleanup and deadline. - Tests changed only their cast targets. - CRAP: `beginWorkspaceStop` and `runWorkspaceStopCleanup` are CC 13 and count as new functions. With coverage they score CRAP 13.0 and 13.5, under the CI limit of 15. ## Risks Medium-low. The stop latch gates reawakens, peer sends and queued launches. A wiring mistake would leave a stuck latch or release one early. The stop, reawakening, settlement-receipt and attempt-admission suites cover those paths and pass unchanged. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$68.74`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=68.74 --> Signed-off-by: Thomas Kosiewski <tk@coder.com>
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
PR 1 of the TaskService split stack. It moves TaskService's stateless helpers out of
taskService.tsinto two modules, without changing their bodies:taskToolOutputs.ts: parsers for task and workflow tool inputs and outputs, and completed-report receipts (collectCompletedTaskReportReceipts,parseTaskToolInputForRecovery,collectAgentReferencedWorkflowRunIdsFromParts, …).taskMessagePredicates.ts: chat-message predicates for supersession points and terminal workflow results.workspaceService.tshad byte-identical copies of six of the predicates. It now imports them fromtaskMessagePredicates.ts, and its copies are gone.taskService.tsgoes from 22,800 to 22,382 lines.Background
The plan for the whole stack (approved, all defaults kept): seven owner classes take 33 of TaskService's 42 Map, Set and lock fields, one group per PR. The order is pure helpers, WorkflowRunAttention, PeerSendParking, TaskWaiters, TerminalAttentionNotifier. TaskStopLatch, AttemptLedger and StreamEndLedger come later, after their own plan. Every move keeps the public TaskService methods as facades. Follow-up issue for two leftovers found while planning: #7664.
Implementation
withToolPolicystays intaskService.ts, because it is not a message predicate.taskToolOutputs.test.ts,taskMessagePredicates.test.ts) cover the parsers directly: receipt provenance forcode_execution, legacyvariantsrecovery bounds, and the workflow-run provenance rule.Validation
bun test src/node/services/taskService(1,270 tests),src/node/services/workspaceService,workflowsandtools(3,239 tests): all pass.make static-check,make lint-eslint: pass.make crap-check: pass. The 7 moved functions above CC 8 are warnings under net-extraction relief, because removing the duplicates lowers the total CC (7883 → 7854).make dry-check: pass. It flagged the six workspaceService copies before the dedupe.make mutate-check: 170 killed, 14 survived. The survivors are?./||mutants in private predicates that only run behind guards that already checked those fields, plus the 30-level recursion bound incollectCompletedTaskReportReceipts.Risks
Low. The function bodies are unchanged and the import sites changed only their paths. The workspaceService dedupe swaps byte-identical functions.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$53.33