Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Dispatch state and Task lifecycle src/core/task/dispatchState.ts, src/core/task/Task.ts, src/core/task/__tests__/Task.spec.ts, docs/architecture/task-lifecycle-model.md, scripts/check-tool-turn-continuation.ts |
Adds IDLE, SERIAL, and PARALLEL states, a pending-work type, and transition validation. Task uses dispatch state and pending work for readiness and request resets. Lifecycle tests and mappings use the new fields. |
Assistant-message presentation
| Layer / File(s) | Summary |
|---|---|
Presenter dispatch and re-entrancy src/core/assistant-message/presentAssistantMessage.ts, src/core/assistant-message/__tests__/presentAssistantMessage.spec.ts, src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts |
The presenter queues serial work while dispatch is active, drains pending work, and restores IDLE in finally. Tests cover re-entrant calls, state transitions, queued updates, aborts, and failures. |
Task mocks and supporting tests src/core/assistant-message/__tests__/presentAssistantMessage-auto-deny.spec.ts, src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts, src/core/assistant-message/__tests__/presentAssistantMessage-tool-repetition.spec.ts, src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts, src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts, src/core/task/__tests__/ToolExecutionContext.spec.ts |
Test fixtures replace the former presenter-lock flags with dispatchState and pendingWork. Related assertions and resets use the new state. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Refactor
Merge Risk: 
52986
-
The dispatch-state refactor has no established merge-blocking issue; it is ready for normal merge checks.
Pre-merge checks: 
-
Check name Status Explanation Description check The description includes the linked issues, implementation details, reviewer notes, test procedure, checklist, and documentation impact. It omits the Get in Touch section, but the required technical c… Title check The title is concise, specific, and accurately describes the main refactor from dispatch lock booleans to DispatchState. Linked Issues check #374requirements are implemented.Tasknow usesdispatchStateand nullablependingWork; the former boolean fields are removed.DispatchState.PARALLELis defined but not assigned by the serial…Out of Scope Changes check The changes stay within #374. The production changes implement the dispatch state model and serial re-entrancy flow. The test changes cover that flow and update affected task fixtures. The architectur…Regression Evidence PASS. The changed dispatch behavior has focused unit coverage. presentAssistantMessage.spec.tscovers SERIAL and PARALLEL queueing, one pending drain, rapid re-entry, presenter failure cleanup, and …Security Boundaries No changed path matches the security failure conditions. The runtime changes only replace the internal presenter lock with DispatchStateandpendingWork; the only queued value is the fixed `{ mode…Persistence Integrity No changed persistence path exists. The PR changes only in-memory Task.dispatchState/pendingWork, the presenter control flow, tests, and documentation. The diff adds no storage or database writes,…Lifecycle Resource Cleanup No changed lifecycle path meets the failure condition. presentAssistantMessagesetsSERIAL, clearspendingWorkat each pass, drains at most one pending continuation per pass, stops draining when…Full details: Description check
-
Explanation
The description includes the linked issues, implementation details, reviewer notes, test procedure, checklist, and documentation impact. It omits the Get in Touch section, but the required technical content is otherwise complete.
Full details: Linked Issues check
-
Explanation
#374requirements are implemented.Tasknow usesdispatchStateand nullablependingWork; the former boolean fields are removed.DispatchState.PARALLELis defined but not assigned by the serial presenter.transitionDispatchaccepts only the four required transitions and throws for other transitions.presentAssistantMessagequeues serial work while dispatch is active, drains one pending pass, and restoresIDLEinfinally. The added and updated tests cover re-entrancy, bounded draining, error reset, and transition validation. The test updates only rename the removed state fields and preserve the existing behavior checks.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within
#374. The production changes implement the dispatch state model and serial re-entrancy flow. The test changes cover that flow and update affected task fixtures. The architecture document and model-check comments document the renamed state fields. No unrelated behavior or feature change is shown.
Full details: Regression Evidence
-
Explanation
PASS. The changed dispatch behavior has focused unit coverage.
presentAssistantMessage.spec.tscovers SERIAL and PARALLEL queueing, one pending drain, rapid re-entry, presenter failure cleanup, and every valid and invalidtransitionDispatchtransition.presentAssistantMessage-custom-tool.spec.tscovers handoff draining, abort stopping, consecutive blocks, partial updates, later-block errors, and dispatch errors.Task.spec.tscovers IDLE, SERIAL, and PARALLEL readiness states plus real presenter completion, failure, and abort paths. The documentation and comment updates introduce no executable behavior, and the PR has no durable UI change requiring a Playwright snapshot.
Full details: Security Boundaries
-
Explanation
No changed path matches the security failure conditions. The runtime changes only replace the internal presenter lock with
DispatchStateandpendingWork; the only queued value is the fixed{ mode: "serial" }, and state transitions use internal enum values. Tool validation and approval checks remain in place, with no changed secret or PII handling and no new execution of unvalidated input.
Full details: Persistence Integrity
-
Explanation
No changed persistence path exists. The PR changes only in-memory
Task.dispatchState/pendingWork, the presenter control flow, tests, and documentation. The diff adds no storage or database writes, serialization, rollback, or persistence defaults. Therefore no persistence loss or corruption condition is introduced.
Full details: Lifecycle Resource Cleanup
-
Explanation
No changed lifecycle path meets the failure condition.
presentAssistantMessagesetsSERIAL, clearspendingWorkat each pass, drains at most one pending continuation per pass, stops draining whenabortis true, and always restoresIDLEinfinally(src/core/assistant-message/presentAssistantMessage.ts:106-124, 127-135). The request reset also clears both fields before a new turn (src/core/task/Task.ts:4118-4142). The refactor preserves the prior boolean guard and cleanup behavior;PARALLELis defined but no production path sets it.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Review statusThanks for contributing. This comment tracks the review sequence and the next action. Current step: Awaiting fresh human maintainer or CODEOWNER approval. Automated review is complete for the latest commit but does not replace human approval. Review-state labels are managed by this workflow; do not edit them manually. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
coderabbit-review-active
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
github-actions
Bot
added
awaiting-maintainer
Related GitHub Issue
Closes: #374
Refs: #325, #359
Description
This PR replaces the two dispatch booleans on
Taskwith a state field. It does not change behavior.A boolean cannot show whether a serial or a parallel dispatch holds the lock. The parallel path in Story 4.2b needs that information. This PR adds the state machine first, so the serial path stays correct when 4.2b lands.
Changes:
Task.dispatchState(IDLE,SERIAL,PARALLEL) replacespresentAssistantMessageLocked.Task.pendingWork({ mode } | null) replacespresentAssistantMessageHasPendingUpdates.src/core/task/dispatchState.tsholds the enum andtransitionDispatch. The helper throws on any transition exceptIDLEtoSERIAL,IDLEtoPARALLEL,SERIALtoIDLE, andPARALLELtoIDLE.presentAssistantMessagequeuespendingWorkwhen the state is notIDLE. The existing drain loop in thetryblock readspendingWork.PARALLELis defined but never set. It is reserved for Story 4.2b.Points for reviewers:
finallyblock and the per-request reset inTask.tsassignIDLEdirectly. They do not calltransitionDispatch. A call from a reset would throwIDLEtoIDLEand hide the original error.tryblock, as before. It does not move tofinally.transitionDispatchis only called forIDLEtoSERIAL. A release that only the owning pass can do belongs in Story 4.2b.PARALLELyet. Story 4.2b must add that.Test Procedure
pnpm --dir src run check-types.pnpm --dir src exec vitest run core/assistant-message core/task/__tests__/Task.spec.ts core/task/__tests__/ToolExecutionContext.spec.ts. All 357 tests pass.pnpm tool-turn:model-check. It passes.The new spec
presentAssistantMessage.spec.tscovers:SERIALorPARALLELqueuespendingWorkand does not run.IDLEwhen the presenter throws.transitionDispatch.Pre-Submission Checklist
Documentation Updates
The lifecycle model doc and the model script comment now name the new fields.
Additional Notes
Follow-up for Story 4.2b: release the state only from the pass that owns it, and drain work queued during
PARALLEL.🤖 Generated with Claude Code