Repository navigation
feat(run-state): add the turn sub-machine (#1800) - #2127
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the main change: adding the turn sub-machine to run-state. |
| Description check | |
The description includes the linked issue, implementation details, design decisions, test procedure, checklist, documentation impact, and scope notes. It is sufficiently complete for review. |
| Linked Issues check | |
Issue #1800 requires the five-field turn state, an unguarded turnStarted reset, independence from stream.tag, unit coverage for transitions, post-stream tool writes, and abort-before-first-chunk b… |
| Out of Scope Changes check | |
The changed files are limited to the run-state implementation, its unit tests, and the target architecture document. These changes directly implement and document issue #1800. No unrelated behavior or… |
| Regression Evidence | |
PASS. The pull request changes only the run-state kernel and architecture documentation; it has no UI change. Focused nextRunState tests cover all seven turn events, initial defaults and freezing, p… |
| Security Boundaries | |
The changed paths only add boolean turn fields, typed event variants, immutable reducer transitions, tests, and architecture documentation. nextRunState performs no I/O, secret or PII handling, inpu… |
| Persistence Integrity | |
No changed persistence path exists. The PR changes the pure in-memory nextRunState transition and its frozen initialRunState, plus tests and architecture documentation. It adds turn fields by obje… |
| Lifecycle Resource Cleanup | |
PASS. The changed implementation only adds immutable turn data and pure nextRunState transitions in src/core/task/run-state/runState.ts (lines 62-106). It creates no listener, watcher, provider, t… |
Full details: Linked Issues check
-
Explanation
Issue
#1800requires the five-field turn state, an unguardedturnStartedreset, independence fromstream.tag, unit coverage for transitions, post-stream tool writes, and abort-before-first-chunk behavior.runState.tsaddsTurn, initializes all five fields, handles all seven turn events, preserveswaitingForFirstChunkduringturnStarted, and does not gate turn events by phase or stream.runState.spec.tscovers the required transitions and scenarios. The implementation does not route writes or flip reads.
Full details: Out of Scope Changes check
-
Explanation
The changed files are limited to the run-state implementation, its unit tests, and the target architecture document. These changes directly implement and document issue
#1800. No unrelated behavior or follow-up write/read routing is added.
Full details: Regression Evidence
-
Explanation
PASS. The pull request changes only the run-state kernel and architecture documentation; it has no UI change. Focused
nextRunStatetests cover all seven turn events, initial defaults and freezing, per-field set/clear behavior, preservation of the other fields,turnStartedreset semantics including retainedwaitingForFirstChunk, stream isolation, acceptance in uninitialized/completed/aborted phases, post-stream tool writes, and abort-before-first-chunk behavior (src/core/task/run-state/__tests__/runState.spec.ts:88-191, 524-543). No changed error, negative, or unset branch lacks plausible focused coverage.
Full details: Security Boundaries
-
Explanation
The changed paths only add boolean turn fields, typed event variants, immutable reducer transitions, tests, and architecture documentation.
nextRunStateperforms no I/O, secret or PII handling, input execution, or approval/allowlist decisions. No concrete security-boundary failure was introduced.
Full details: Persistence Integrity
-
Explanation
No changed persistence path exists. The PR changes the pure in-memory
nextRunStatetransition and its frozeninitialRunState, plus tests and architecture documentation. It adds turn fields by object-copy transitions and does not add storage writes, serialization, async operations, rollback handling, or persisted-status consumers. The architecture document explicitly keepsRunStateseparate from persisted status.
Full details: Lifecycle Resource Cleanup
-
Explanation
PASS. The changed implementation only adds immutable turn data and pure
nextRunStatetransitions insrc/core/task/run-state/runState.ts(lines 62-106). It creates no listener, watcher, provider, timer, task, subscription, or cleanup operation. Turn events only return copied state and do not start work or alter cancellation/disposal latches. The changed event names have no production callers outside this reducer; tests and documentation are the other changed files. No changed lifecycle path can leak a resource or duplicate work after cancellation, disposal, or restart.
✨ 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: The required review sequence passed. Remaining merge requirements apply. 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
github-actions
Bot
added
the
community-approved
github-actions
Bot
removed
awaiting-maintainer
Related GitHub Issue
Closes: #1800
Description
This PR adds the turn sub-machine to the run-state kernel (RSK-09). Nothing is wired. #1804 and #1805 route the writes and flip the reads.
A turn is one request iteration of the loop. The new
turnregion holds the five within-turn fields:TaskfieldwaitingForFirstChunkisWaitingForFirstChunkfirstChunkAwaitStarted,firstChunkAwaitEndedtoolRejecteddidRejectTooltoolRejectedtoolUseddidAlreadyUseTooltoolUsedtoolFaileddidToolFailInCurrentTurntoolFailedstreamReadCompletedidCompleteReadingStreamstreamReadCompletedDesign choices:
turnStartedmaps to the per-request reset block. It clears four fields and keepswaitingForFirstChunk, because the legacy block does not clearisWaitingForFirstChunk. If the chunk loop breaks on the first chunk, the flag stays true. The kernel keeps this behavior, so the later read flip does not change graceful-abort timing.stream.tag.The design doc now lists the
Turntype, the seven events, a transition row for each, and the name mapping.Reviewers must check the
turnStartedchoice that keepswaitingForFirstChunk.Test Procedure
pnpm --dir src exec vitest run core/task/run-state core/task/__tests__/Task.run-shadow.spec.ts.pnpm --dir src run check-types.pnpm --dir src exec eslint --max-warnings=0 core/task/run-state.The new tests cover:
turnStartedresets four fields and keepswaitingForFirstChunkPre-Submission Checklist
Documentation Updates
docs/architecture/task-lifecycle-target-architecture.mdis updated in this PR.Additional Notes
Exit criteria from the issue: the turn sub-machine compiles and is tested. Nothing is wired.
🤖 Generated with Claude Code