refactor(session): extract application coordinators#1961
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR extracts session lifecycle, turn, assignment, and projection behavior into session-application coordinators and typed ports. ChangesSession application migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SessionService
participant ChatService
participant SessionApplication
participant AgentManager
participant TypedBackend
SessionService->>SessionApplication: create or restore session
ChatService->>SessionApplication: send, steer, or cancel turn
SessionApplication->>AgentManager: resolve agent kind and session handle
AgentManager->>TypedBackend: execute lifecycle or turn operation
TypedBackend-->>SessionApplication: runtime state or operation result
SessionApplication-->>SessionService: projected session response
SessionApplication-->>ChatService: turn result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/main/presenter/sessionApplication/agentAssignmentCoordinator.test.ts (1)
244-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePreflight-count coupling is brittle but likely intentional for this characterization suite.
The induced-failure trigger (
resolveTransferTarget.mock.calls.length > 2) pins the exact number of preflight calls rather than asserting on arguments. Fine for pinning current ordering behavior, but any future preflight-call-count change (even a behavior-preserving one) would break this test for an unrelated reason.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/presenter/sessionApplication/agentAssignmentCoordinator.test.ts` around lines 244 - 272, Update the failure trigger in the preflight test around moveAgentSessions to depend on the resolveTransferTarget call arguments or relevant session/project identity instead of mock.calls.length. Preserve the intended behavior: one batch entry should fail during preflight before setSessionAgentContext or updateAgentId is invoked.src/main/presenter/sessionApplication/lifecycleCoordinator.ts (1)
484-489: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRoute warnings through
@shared/loggerinstead ofconsole.warn. These coordinators emit info logs via the app logger but drop warning-level diagnostics ontoconsole.warn, so failures bypass the app's log sinks/formatting and are easily lost in production.
src/main/presenter/sessionApplication/lifecycleCoordinator.ts#L484-L489: replace theconsole.warncleanup-failure logs (also at L270, L383, L424) withlogger.warn.src/main/presenter/sessionApplication/agentAssignmentCoordinator.ts#L426-L433: import@shared/loggerand replace theconsole.warncalls (also at L463, L473, L561, L570) withlogger.warn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/sessionApplication/lifecycleCoordinator.ts` around lines 484 - 489, Replace all specified console.warn calls in lifecycleCoordinator.ts (lines 270, 383, 424, and 484-489) with the shared logger.warn while preserving their messages and error arguments. In agentAssignmentCoordinator.ts (lines 426-433, 463, 473, 561, and 570), import `@shared/logger` and route each corresponding warning through logger.warn instead of console.warn.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/FLOWS.md`:
- Around line 157-158: Rename the new Mermaid node id in the section 5 flowchart
from Compat to a unique identifier, updating its incoming and outgoing edges
while preserving the existing label and connections. Leave the pre-existing
Compat node used by the Provider acp branch unchanged.
In `@src/main/presenter/sessionApplication/lifecycleCoordinator.ts`:
- Around line 66-78: Update the session-created log in createSession to remove
the user-derived title from its message, while retaining sessionId for
correlation. Leave title generation and session creation unchanged.
---
Nitpick comments:
In `@src/main/presenter/sessionApplication/lifecycleCoordinator.ts`:
- Around line 484-489: Replace all specified console.warn calls in
lifecycleCoordinator.ts (lines 270, 383, 424, and 484-489) with the shared
logger.warn while preserving their messages and error arguments. In
agentAssignmentCoordinator.ts (lines 426-433, 463, 473, 561, and 570), import
`@shared/logger` and route each corresponding warning through logger.warn instead
of console.warn.
In `@test/main/presenter/sessionApplication/agentAssignmentCoordinator.test.ts`:
- Around line 244-272: Update the failure trigger in the preflight test around
moveAgentSessions to depend on the resolveTransferTarget call arguments or
relevant session/project identity instead of mock.calls.length. Preserve the
intended behavior: one batch entry should fail during preflight before
setSessionAgentContext or updateAgentId is invoked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64193fe8-71be-4024-8eb8-39322a0f8f5f
📒 Files selected for processing (58)
docs/ARCHITECTURE.mddocs/FLOWS.mddocs/README.mddocs/architecture/agent-system-layered-runtime/README.mddocs/architecture/agent-system.mddocs/architecture/baselines/agent-system-layered-runtime-baseline.jsondocs/architecture/baselines/dependency-report.mddocs/architecture/baselines/main-kernel-boundary-baseline.mddocs/architecture/baselines/main-kernel-migration-scoreboard.jsondocs/architecture/baselines/main-kernel-migration-scoreboard.mddocs/architecture/session-application-coordinators/plan.mddocs/architecture/session-application-coordinators/spec.mddocs/architecture/session-application-coordinators/tasks.mddocs/architecture/session-boundary-cleanup/spec.mddocs/architecture/session-management.mddocs/guides/code-navigation.mddocs/guides/getting-started.mdscripts/architecture-guard.mjssrc/main/agent/shared/agentSessionNormalization.tssrc/main/presenter/agentSessionPresenter/index.tssrc/main/presenter/cronJobs/index.tssrc/main/presenter/cronJobs/runSessionStarter.tssrc/main/presenter/index.tssrc/main/presenter/lifecyclePresenter/hooks/after-start/cronJobsStartHook.tssrc/main/presenter/remoteControlPresenter/index.tssrc/main/presenter/remoteControlPresenter/interface.tssrc/main/presenter/remoteControlPresenter/services/remoteConversationRunner.tssrc/main/presenter/sessionApplication/agentAssignmentCoordinator.tssrc/main/presenter/sessionApplication/agentAssignmentPolicy.tssrc/main/presenter/sessionApplication/lifecycleCoordinator.tssrc/main/presenter/sessionApplication/lifecycleDeletionTransaction.tssrc/main/presenter/sessionApplication/ports.tssrc/main/presenter/sessionApplication/projectionCoordinator.tssrc/main/presenter/sessionApplication/turnCoordinator.tssrc/main/routes/chat/chatService.tssrc/main/routes/hotPathPorts.tssrc/main/routes/index.tssrc/main/routes/sessions/sessionService.tstest/main/agent/manager/directAcpAgentBackend.test.tstest/main/presenter/agentSessionPresenter/agentSessionPresenter.test.tstest/main/presenter/agentSessionPresenter/assignmentCoordinatorFixture.tstest/main/presenter/agentSessionPresenter/integration.test.tstest/main/presenter/agentSessionPresenter/projectionCoordinatorFixture.tstest/main/presenter/cronJobRunSessionStarter.test.tstest/main/presenter/cronJobs.test.tstest/main/presenter/lifecyclePresenter/hooks/after-start/cronJobsStartHook.test.tstest/main/presenter/remoteControlPresenter/remoteControlPresenter.test.tstest/main/presenter/remoteControlPresenter/remoteConversationRunner.test.tstest/main/presenter/sessionApplication/agentAssignmentCoordinator.test.tstest/main/presenter/sessionApplication/agentAssignmentPolicy.test.tstest/main/presenter/sessionApplication/lifecycleCoordinator.test.tstest/main/presenter/sessionApplication/lifecycleDeletionTransaction.test.tstest/main/presenter/sessionApplication/projectionCoordinator.test.tstest/main/presenter/sessionApplication/turnCoordinator.test.tstest/main/routes/chatService.test.tstest/main/routes/dispatcher.test.tstest/main/routes/sessionService.test.tstest/main/scripts/architectureGuard.test.ts
This was referenced Jul 14, 2026
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
1 participant
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
Why
AgentSessionPresenter combined unrelated application transactions, policies, projection state, and consumer wiring behind one broad contract. This made ownership unclear and let typed routes, Remote, and Cron depend on substantially more behavior than they used.
The new coordinators make those ownership boundaries explicit while preserving existing route, event, persistence, DeepChat, direct-ACP, Remote, and Cron behavior.
Integration
dev@135779210through merge commit1122b2406Validation
pnpm run formatpnpm run i18n— 0 missing, 0 invalidpnpm run lintpnpm run typecheck— node and webpnpm run test:main— 3980 passed, 198 skippedpnpm run test:renderer— 1257 passedpnpm run lint:architecturegit diff --checkNotes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation