Repository navigation
🤖 refactor: split the five longest stop and attempt methods in TaskService - #7674
Merged
Merged
Conversation
…rvice Split recheckWorkspaceStopRelease (CC 21 -> 7), createAdmittedSend (19 -> 4), admitTaskWorkspaceTurn (17 -> 10), evaluateAttemptLineage (16 -> 9) and readUnownedSettlementProof (17 -> 10) along their early returns into named private methods and module-level pure functions in taskService.ts, so later stack PRs can move them into TaskStopLatch and AttemptLedger under move relief. Every new helper is CC 8 or less. No behavior change: statement and side-effect order are kept, no await or microtask boundary is added, and the original entry points keep their names and signatures.
Member
Author
|
@codex review |
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. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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.3k tokens (24% cached) All 14 tests in 9 files
e2e 0.20.0 · 1m 11s · 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 · 17.9s · electron · run artifacts 🟢 e2e agent repros (Haiku): no tests selectedSpend: $0.00 of the $1.00 cap (proxy ledger) e2e 0.20.0 · 5.4s · 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
## 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>
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 27, 2026
## Summary Moves send admission out of `TaskService` into the `AttemptLedger` class that coder#7677 added. The move is move-only. `taskService.ts` drops from 20,972 to 20,307 lines. PR 9 of the TaskService split, on top of coder#7677. ## Implementation - **Fields that moved, by reference:** `admittedSendsByTaskId` and `reawakeningsInFlight`. The new accessor `sendsOf(taskId)` returns the live set (or a shared empty set), never a copy. `beginReawakening` / `endReawakening` replace the four direct writes, and the end is still a conditional delete. - **Methods that moved:** - Admission: `admitTaskWorkspaceTurn` (with the `admitSendToCurrentAttempt` step from coder#7674), `createAdmittedSend` and its four token steps, `resolveEnqueuedSendDispatch` and `dischargeAdmittedSend`. - Queries: `attemptAdmissionOpen`, `hasPendingAdmissions`, `hasLiveReawakening` and `resolveStreamAttemptAtEvent`. - Lineage: `evaluateAttemptLineage`, `readUnownedSettlementProof` and their module helpers. - Retirement: `claimRetiredAttempt` and `tombstoneUnpublishedReservation`. - **Facades:** TaskService keeps one-line facades for `admitTaskWorkspaceTurn` (seam), `evaluateAttemptLineage`, `resolveStreamAttemptAtEvent`, `claimRetiredAttempt` and `tombstoneUnpublishedReservation`. Tests spy on these or call them through casts. - **Stays on TaskService:** - `captureReawakeningStopFence` and `reawakeningOvertakenByStop` touch only stop epochs and the config, not ledger state. - The coordinators `recordWorkspaceTurnSettled` and `recordWorkspaceTurnSuperseded` now read the sends through `sendsOf`. - **Cross-class calls:** `TaskStopLatch`'s `admittedSends` closure now returns `this.attemptLedger.sendsOf(taskId)`, still through TaskService. The ledger reaches stop state and stream-end decisions through TaskService closures. - **Import cycle:** `ATTEMPT_CLOSURE_SETTLE_WAIT_MS` now lives in `attemptLedger.ts`, and `taskService.ts` re-exports it. Without that, the ledger would import a value from `taskService.ts` and create a runtime import cycle. ## Validation - Statement order, the sync/async shape, the "add to the set last" order in `createAdmittedSend`, and the re-reads after awaits are unchanged. - `attemptLedger.test.ts` adds tests on fake deps. They cover: - send identity and ownership - every refusal leaving no obligation behind - disposal order - first admission wins, with the extra `stopStream` and the recheck - staleness and the dequeue gate - reawakening begin and end - the lineage re-read after the closing wait - the row-changed and row-unreadable refusals in `readUnownedSettlementProof` - each outcome of the claim and the tombstone - Three TaskService test files changed only their cast targets. - CRAP: every moved function scores CRAP 15 or less with coverage. The highest is `claimRetiredAttempt` at CC 15 with 100 % line coverage, and move relief also applies because its facade drops from 15 to 1. ## Risks Medium. Send admission decides whether a message may start a turn under a task's current attempt. A wiring mistake would refuse a valid send or let a stale send through. The attempt-admission, stale-attempt-effects and report-decision suites pass, and they changed only their cast targets. --- _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>
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 27, 2026
## Summary Moves the stream-end decisions out of `TaskService` into a new plain class, `StreamEndLedger` (`src/node/services/streamEndLedger.ts`). The move is move-only. `taskService.ts` drops from 20,307 to 20,023 lines. PR 10, the last PR of the TaskService stop-and-attempt split, on top of coder#7678. Across this stack (coder#7674, coder#7675, coder#7677, coder#7678 and this PR), `taskService.ts` goes from 21,742 to 20,023 lines. The stop and attempt state now lives in three owner classes: `TaskStopLatch`, `AttemptLedger` and `StreamEndLedger`. ## Implementation - **Fields that moved:** `streamEndDecisionsByTaskId` (readonly, because tests read it) and `deferredTaskStreamEnds`. The inline value type of `deferredTaskStreamEnds` is now the named interface `TaskStreamEndDeferral`, with the same fields and comments. - **Methods that moved, with names and statement order kept:** `runStreamEndHandler`, `deferChildStreamEnd`, `registerStreamEndDecision`, `findStreamEndDecision`, `findPendingStreamEndDecision`, `resolveStreamEndDecision`, `pruneStreamEndDecisions`, `reconcileDeferredTaskStreamEnd` and `resolveDecisionOfReportedAttempt`. - **Coordinators stay:** `recordWorkspaceTurnSettled` and `recordWorkspaceTurnSuperseded` stay on TaskService. Their decision loops become one `settleTurn` / `rebindTurn` call each, at the same position, so the order across the three owners does not change. - **Same-tick order:** the stream-end listener in the constructor keeps its one-tick order (snapshot, origin, register, run). - **Stays on TaskService:** `handleStreamEnd`, `inspectAttemptOutcome` and `reawakenInterruptedTask`. A later plan covers these long methods. - **Cross-class calls:** the `AttemptLedger` and `TaskStopLatch` closures `findStreamEndDecision`, `pruneStreamEndDecisions` and `discardStreamEndDecisions` now point at `streamEndLedger` through TaskService. The three classes never call each other directly. - **Facades:** none. No test spies on, patches or casts a moved method. ## Validation - The new file `streamEndLedger.test.ts` has 21 tests on fake deps. They cover: - decision identity - find order - the order of resolve, prune, stop recheck and drain - the prune rules - settle and rebind of turns - handler outcomes (a return gives nonreport, a throw gives indeterminate) - the deferral listener - each branch of `reconcileDeferredTaskStreamEnd` - Three TaskService test files changed only their cast targets. Prettier re-wrapped some of the longer lines. - CRAP: every moved function scores CRAP 13 or less with coverage. The highest is `reconcileDeferredTaskStreamEnd` at CC 13 with 100 % line coverage. ## Risks Medium-low. The stream-end decision says whether a child's last stream ended in a report. A wiring mistake would hold or drop a report, or hold a stop latch. The report-decision, settlement-receipt and child-goal suites pass, and they changed only their cast targets. --- _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
Splits the five longest stop and attempt methods in
TaskServiceinto smaller private steps, in place. No behaviour change, no new file, no new class. This is PR 6 of the TaskService split (stack 1, #7665 to #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.
recheckWorkspaceStopRelease21 -> 7isWorkspaceStopRecordSettled(module fn) 5,isStopReceiptDecisionPending5,startStopSettlementReceipt4,releaseWorkspaceStopRecord5createAdmittedSend19 -> 4isAdmittedSendStale7,streamEndBlocksNewTurn2,recordAdmittedSendTurn5,disposeSendBeforeAdmission5admitTaskWorkspaceTurn17 -> 10admitSendToCurrentAttempt8evaluateAttemptLineage16 -> 9ownedAttemptSettlementPhase3,lineageProofFromReceipt(module fn) 4readUnownedSettlementProof17 -> 10unownedSettlementCandidate(module fn) 7,unownedSettlementReceiptGap(module fn) 4Implementation
createAdmittedSendare now one-line arrows that call the new methods, and they still run synchronously.evaluateAttemptLineagenow reads the settlement entry throughownedAttemptSettlementPhasetwice instead of re-reading alet. 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.bun test src/node/services/taskService* workspaceTurnManager workflows toolspasses: 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