Repository navigation
🤖 refactor: move task waiter state into TaskWaiters - #7668
Conversation
Move-only. TaskWaiters (taskWaiters.ts) owns the report waiters, start waiters, unpersisted launch-failure marks, foreground-await counts, backgroundable foreground waiters, user-backgrounded marks and the bounded completed-report cache, plus the methods that only touch them. ForegroundWaitBackgroundedError moves with it and taskService.ts re-exports it. TaskService keeps its public and interface methods as one-line facades, keeps resolveWaiters (ancestry, then cache, then drain) and rejectWaiters (spied on in tests) as entry points, and reads the cache and waiter lists through accessors. Deps are call-time closures (scheduleNotifyOnTerminalPersist, workspaceService.hasQueuedMessages). Test casts now point at `waiters.<field>`.
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 |
|
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". |
🛡️ 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. |
🟢 e2e stories (Haiku): 14 passed22 agent steps · 7 replayed from cache · 15 model calls · 46.3k tokens (28% cached) All 14 tests in 9 files
e2e 0.20.0 · 1m 13s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.0k tokens (32% cached) All 3 tests in 3 files
e2e 0.20.0 · 20.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.3s · 2 targets (web, phone) · run artifacts |
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
TaskWaitersclass (taskWaiters.ts).taskService.tsloses 205 lines in this layer.Background
PR 2 to 5 of the TaskService split stack (#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
armWaitersOfUnendedTask,startPendingStartWaiters,enforceCompletedReportCacheLimit, the waiter and cache types, andForegroundWaitBackgroundedError.taskService.tsre-exports the error, so its importers do not change.resolveWaitersstays on TaskService (tests call it through casts). It still computes the ancestry, then callscacheCompletedReportandresolvePendingWaiters, so the cache is still written before any waiter resolves.rejectWaitersstays as the TaskService entry point (a test spies on it), and its body is in the class.waitForAgentReport,readPersistedReportForWait,inspectAttemptOutcomeand the termination, reawaken and timeout paths stay. They use accessors in the same order as before.appendToListMap,removeFromListMap) replace four near-copies that the DRY gate flagged.waiters.<field>). The newtaskWaiters.test.ts(13 tests) covers nested foreground awaits, the backgrounding order (mark, persist, reject), start-waiter order and cache eviction.Validation
make static-check,make lint-eslint,make crap-check(all moved methods CC 8 or less): pass.make dry-check: the movedForegroundWaitBackgroundedErrorconstructor matches four existing error constructors. That is existing code reported again because it moved.?? 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