refactor(agent): replace runtime root with harness facade - #2028
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe PR replaces ChangesDeepChat harness migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MainProcess
participant DeepChatAgentHarness
participant TurnCoordinator
participant DeepChatLoopRunner
participant MessageProjectionService
MainProcess->>DeepChatAgentHarness: create harness with dependencies
DeepChatAgentHarness->>TurnCoordinator: send or processMessage
TurnCoordinator->>DeepChatLoopRunner: run turn
DeepChatLoopRunner->>MessageProjectionService: refresh projection
MessageProjectionService-->>MainProcess: publish session update
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
resources/acp-registry/registry.json (1)
490-501: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAlign
fast-agent-acpwith the supported Python range before publishing.
dirac-cli@0.4.24is published with the expected npm bin, butfast-agent-acp==0.9.24requires Python>=3.12,<3.15, which will fail for users/runtime environments outside that range. Either bump the registry entries to versions compatible with the targetuvxruntime or drop the unsupported bumps.Also applies to: 537-547
🤖 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 `@resources/acp-registry/registry.json` around lines 490 - 501, Update the `fast-agent-acp` registry entries, including the additional entry referenced by the review, so their declared versions and `uvx` distributions are compatible with the supported Python runtime range. If no compatible release exists, remove the unsupported version bumps and restore the prior compatible versions; keep the `dirac-cli` entry unchanged unless required for alignment.Source: MCP tools
src/main/agent/deepchat/runtime/compactionRuntimeCoordinator.ts (1)
315-335: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEarly
assertCurrentinapply()skips message cleanup and swallows the original error on staleness.At Line 315 (catch path) and Line 325 (post-try path),
assertCurrent(sessionId, expectedInstance)runs beforedeleteMessage/updateCompactionMessageandmessageProjection.refresh. Neither of those calls depends on instance identity — they only needsessionId/compactionMessageId. If the instance goes stale mid-flight (session evicted/rehydrated concurrently), this:
- In the catch branch: throws a
StaleDeepChatAgentInstanceErrorthat replaces the original compaction failure (throw erroris never reached), and leaves the "compacting" placeholder message undeleted.- In the success branch: skips finalizing the compaction message to
'compacted'(or deleting it on failure) and skips the projection refresh, even thoughcompactionService.applyCompactionalready succeeded and persisted its summary state — leaving an orphaned placeholder message and a stale UI projection.
emit()(Line 373-378) already callsthis.assertCurrent(...)internally before mutating state, so these two extra guards are redundant for enforcing staleness and only cause harm by gating unrelated bookkeeping.🛠️ Proposed fix: let cleanup run unconditionally, rely on `emit()`'s own currency check
} catch (error) { - this.assertCurrent(sessionId, expectedInstance) this.deps.messageStore.deleteMessage(compactionMessageId) this.deps.messageProjection.refresh(sessionId, compactionMessageId) this.emit(sessionId, this.fromSummary(intent.previousState), expectedInstance) if (isAbortError(error) || options?.signal?.aborted) { throwIfAbortRequested(options?.signal) } throw error } - this.assertCurrent(sessionId, expectedInstance) if (result.succeeded) {🤖 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/agent/deepchat/runtime/compactionRuntimeCoordinator.ts` around lines 315 - 335, Remove the pre-cleanup assertCurrent calls in apply() so message cleanup and projection refresh always run using sessionId and compactionMessageId, including stale-instance cases. In the catch path, perform deleteMessage and refresh before preserving the original error and abort handling; in the success path, finalize or delete the compaction message and refresh before emitting. Continue relying on emit() for the instance-currentness check.src/main/agent/deepchat/runtime/sessionSettingsCoordinator.ts (1)
68-117: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winTOCTOU:
setModel/setAgentContext/updateGenerationSettingscapturestatebefore anawait, then mutate/read it as if still current.
setModel(Lines 75, 103-106) capturesstate = this.state(sessionId)before thegetEffectiveGenerationSettings/sanitizeGenerationSettingsawaits, then fetches a possibly-different freshinstanceafterward, but mutates the stalestateobject (state.providerId = nextProviderId) instead of the fresh instance's state. If the session is evicted/rehydrated during those awaits (e.g. concurrentSessionLifecycleCoordinator.cleanup/destroyor agent reassignment), this silently fails to update the current instance's provider/model even though the DB write succeeds — runtime/DB state diverges.setAgentContexthas the same stale-capture pattern forstate?.status(Line 160).updateGenerationSettingsshares the pattern but is lower risk since its final mutation targets a freshly-fetched instance.This file's own
getEffectiveGenerationSettings/resolveProjectDir(and sibling coordinators likeTranscriptMutationCoordinator,PendingInputPump.drain) get this right by capturing a single instance reference and callingassertCurrent/scope.assertCurrent()right before any post-await mutation — that pattern should be applied here too.🔒 Proposed fix for setModel (same pattern applies to setAgentContext)
async setModel(sessionId: string, providerId: string, modelId: string): Promise<void> { const nextProviderId = providerId?.trim() const nextModelId = modelId?.trim() if (!nextProviderId || !nextModelId) { throw new Error('Session model update requires providerId and modelId.') } - const state = this.state(sessionId) + const instance = this.instance(sessionId) + const state = instance.getRuntimeState() const dbSession = this.deps.sessionStore.get(sessionId) if (!state && !dbSession) { throw new Error(`Session ${sessionId} not found`) } ... this.deps.sessionStore.updateSessionConfiguration( sessionId, nextProviderId, nextModelId, buildPersistedGenerationSettingsReplacement(sanitized) ) - const instance = this.instance(sessionId) + this.assertCurrent(sessionId, instance) if (state) { state.providerId = nextProviderId state.modelId = nextModelId } else { instance.setRuntimeState({ status: 'idle', providerId: nextProviderId, modelId: nextModelId, permissionMode }) } instance.setGenerationSettings(sanitized) this.invalidateToolProfile(sessionId) }Also applies to: 119-178, 240-270
🤖 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/agent/deepchat/runtime/sessionSettingsCoordinator.ts` around lines 68 - 117, Fix the TOCTOU handling in setModel, setAgentContext, and updateGenerationSettings by capturing the session instance once before awaits, asserting it remains current immediately before each post-await read or mutation, and using that same instance’s runtime state rather than a stale state snapshot. Apply the existing getEffectiveGenerationSettings/resolveProjectDir assertCurrent pattern so eviction, rehydration, or reassignment cannot leave runtime state divergent from persisted state.
🧹 Nitpick comments (1)
src/main/agent/deepchat/runtime/turnCoordinator.ts (1)
1320-1336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefault
expectedInstanceparameter silently rehydrates rather than requiring an explicit instance.
rollbackPendingInputTurn's default (this.ports.registry.getOrHydrateScope(...).instance) is unused today since both call sites instart()pass the already-heldinstanceexplicitly. Since the whole point ofexpectedInstanceis staleness fencing viaassertCurrentInstance, a silent default that always re-hydrates the current instance could mask a future caller's intent to validate a specific instance. Consider making the parameter required to keep the staleness contract explicit.🤖 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/agent/deepchat/runtime/turnCoordinator.ts` around lines 1320 - 1336, Make the expectedInstance parameter required in rollbackPendingInputTurn instead of defaulting it through getOrHydrateScope. Preserve the existing assertCurrentInstance staleness fencing and ensure every call site, including those in start(), passes the explicitly held instance.
🤖 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/architecture/deepchat-agent-harness-boundaries/tasks.md`:
- Around line 81-82: Complete the validation and handoff checklist in the
documented harness specification: run focused and full validation, review the
complete diff, and commit each reviewed stage without pushing. Mark the slice as
ready only after these tasks are complete; otherwise explicitly leave it marked
as not ready.
In `@resources/model-db/providers.json`:
- Around line 124163-124166: Update the input modalities for the Grok Imagine
Video 1.5 entry to use the provider-documented combination, likely text and
image, instead of image and pdf. Verify the exact supported modalities against
the provider docs and keep the video output declaration unchanged.
- Around line 170913-170914: Reconcile the catalog dates in
resources/model-db/providers.json at lines 170913-170914 and 178417-178418 so
each entry’s last_updated is not earlier than its release_date, correcting the
transposed values while preserving the intended dates.
- Around line 262546-262599: Update the type field for the
anthropic/claude-opus-5 and anthropic/claude-opus-5-fast entries to "chat",
preserving all other model metadata unchanged.
In `@scripts/agent-cleanup-guard.mjs`:
- Around line 387-428: Update findDeepChatHarnessBarrelViolations to reject
export assignments, including export default someIdentifier, unless the exported
identifier is in DEEPCHAT_HARNESS_PUBLIC_EXPORTS. Also include exported
EnumDeclaration and ModuleDeclaration names in the existing flag validation,
using TypeScript’s ExportAssignment, EnumDeclaration, and ModuleDeclaration node
handling while preserving current named re-export behavior.
In `@src/main/agent/deepchat/harness/createDeepChatAgentHarness.ts`:
- Around line 277-288: Replace the temporary async sessionState adapter in the
PendingInputPump construction with explicit per-session synchronization or
ownership tracking shared by steering and settlement-triggered draining. Ensure
steer-merging claims the in-flight turn before a competing drain can adopt it,
independent of microtask timing, and remove the workaround comment and extra
async boundary.
---
Outside diff comments:
In `@resources/acp-registry/registry.json`:
- Around line 490-501: Update the `fast-agent-acp` registry entries, including
the additional entry referenced by the review, so their declared versions and
`uvx` distributions are compatible with the supported Python runtime range. If
no compatible release exists, remove the unsupported version bumps and restore
the prior compatible versions; keep the `dirac-cli` entry unchanged unless
required for alignment.
In `@src/main/agent/deepchat/runtime/compactionRuntimeCoordinator.ts`:
- Around line 315-335: Remove the pre-cleanup assertCurrent calls in apply() so
message cleanup and projection refresh always run using sessionId and
compactionMessageId, including stale-instance cases. In the catch path, perform
deleteMessage and refresh before preserving the original error and abort
handling; in the success path, finalize or delete the compaction message and
refresh before emitting. Continue relying on emit() for the instance-currentness
check.
In `@src/main/agent/deepchat/runtime/sessionSettingsCoordinator.ts`:
- Around line 68-117: Fix the TOCTOU handling in setModel, setAgentContext, and
updateGenerationSettings by capturing the session instance once before awaits,
asserting it remains current immediately before each post-await read or
mutation, and using that same instance’s runtime state rather than a stale state
snapshot. Apply the existing getEffectiveGenerationSettings/resolveProjectDir
assertCurrent pattern so eviction, rehydration, or reassignment cannot leave
runtime state divergent from persisted state.
---
Nitpick comments:
In `@src/main/agent/deepchat/runtime/turnCoordinator.ts`:
- Around line 1320-1336: Make the expectedInstance parameter required in
rollbackPendingInputTurn instead of defaulting it through getOrHydrateScope.
Preserve the existing assertCurrentInstance staleness fencing and ensure every
call site, including those in start(), passes the explicitly held instance.
🪄 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 Plus
Run ID: 8c8d85f3-59c1-4f00-9c52-0d7df5bca7c9
📒 Files selected for processing (70)
docs/architecture/baselines/agent-system-layered-runtime-baseline.jsondocs/architecture/baselines/dependency-report.mddocs/architecture/baselines/main-kernel-boundary-baseline.mddocs/architecture/baselines/main-kernel-bridge-register.mddocs/architecture/baselines/main-kernel-migration-scoreboard.jsondocs/architecture/baselines/main-kernel-migration-scoreboard.mddocs/architecture/baselines/zero-inbound-candidates.mddocs/architecture/deepchat-agent-harness-boundaries/plan.mddocs/architecture/deepchat-agent-harness-boundaries/spec.mddocs/architecture/deepchat-agent-harness-boundaries/tasks.mddocs/architecture/tape-layering/spec.mdresources/acp-registry/registry.jsonresources/model-db/providers.jsonscripts/agent-cleanup-guard.mjsscripts/generate-architecture-baseline.mjssrc/main/agent/deepchat/harness/createDeepChatAgentHarness.tssrc/main/agent/deepchat/harness/deepChatAgentHarness.tssrc/main/agent/deepchat/harness/index.tssrc/main/agent/deepchat/harness/pendingInputWakeupBinding.tssrc/main/agent/deepchat/harness/runtimeServices.tssrc/main/agent/deepchat/instance/deepChatAgentInstance.tssrc/main/agent/deepchat/instance/deepChatAgentRuntime.tssrc/main/agent/deepchat/memory/memoryRuntimeCoordinator.tssrc/main/agent/deepchat/runtime/compactionRuntimeCoordinator.tssrc/main/agent/deepchat/runtime/deepChatLoopRunner.tssrc/main/agent/deepchat/runtime/deepChatRuntimeCoordinator.tssrc/main/agent/deepchat/runtime/deferredToolExecutor.tssrc/main/agent/deepchat/runtime/interactionCoordinator.tssrc/main/agent/deepchat/runtime/messageProjectionService.tssrc/main/agent/deepchat/runtime/pendingInputAdmissionCoordinator.tssrc/main/agent/deepchat/runtime/pendingInputPump.tssrc/main/agent/deepchat/runtime/promptAssemblyService.tssrc/main/agent/deepchat/runtime/providerPermissionCoordinator.tssrc/main/agent/deepchat/runtime/runLifecycleCoordinator.tssrc/main/agent/deepchat/runtime/runtimeHookSink.tssrc/main/agent/deepchat/runtime/sessionIdentityService.tssrc/main/agent/deepchat/runtime/sessionLifecycleCoordinator.tssrc/main/agent/deepchat/runtime/sessionSettingsCoordinator.tssrc/main/agent/deepchat/runtime/sessionStateResolver.tssrc/main/agent/deepchat/runtime/streamRequestId.tssrc/main/agent/deepchat/runtime/toolResolver.tssrc/main/agent/deepchat/runtime/toolRuntimeBindings.tssrc/main/agent/deepchat/runtime/transcriptMutationCoordinator.tssrc/main/agent/deepchat/runtime/turnCoordinator.tssrc/main/agent/deepchat/runtime/turnResumeContract.tssrc/main/agent/manager/deepChatAgentBackend.tssrc/main/app/composition.tssrc/main/tape/ports/capabilities.tstest/main/agent/deepchat/harness/deepChatAgentHarness.test.tstest/main/agent/deepchat/instance/deepChatAgentRuntime.test.tstest/main/agent/deepchat/memory/memoryRuntimeCoordinator.test.tstest/main/agent/deepchat/runtime/compactionRuntimeCoordinator.test.tstest/main/agent/deepchat/runtime/messageProjectionService.test.tstest/main/agent/deepchat/runtime/pendingInputAdmissionCoordinator.test.tstest/main/agent/deepchat/runtime/pendingInputPump.test.tstest/main/agent/deepchat/runtime/promptAssemblyService.test.tstest/main/agent/deepchat/runtime/runLifecycleCoordinator.test.tstest/main/agent/deepchat/runtime/runtimeHookSink.test.tstest/main/agent/deepchat/runtime/sessionIdentityService.test.tstest/main/agent/deepchat/runtime/sessionLifecycleCoordinator.test.tstest/main/agent/deepchat/runtime/sessionStateResolver.test.tstest/main/agent/deepchat/runtime/sessionStatusPublisher.test.tstest/main/agent/deepchat/runtime/toolResolver.test.tstest/main/agent/deepchat/runtime/toolRuntimeBindings.test.tstest/main/agent/deepchat/runtime/transcriptMutationCoordinator.test.tstest/main/agent/manager/deepChatAgentBackend.test.tstest/main/agent/manager/deepChatAgentBackendFixture.tstest/main/scripts/agentCleanupGuard.test.tstest/main/session/runtimeIntegration.test.tstest/main/session/session.integration.test.ts
💤 Files with no reviewable changes (1)
- src/main/agent/deepchat/runtime/deepChatRuntimeCoordinator.ts
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
Replaces
DeepChatRuntimeCoordinatorwith a thinDeepChatAgentHarnessfacade over aone-directional owner graph.
The TypeScript runtime refactor preserves existing behavior. The required build-preflight resource
refresh is isolated in its own commit and is the only behavior-affecting exception.
This is the fifth slice of the
deepchat-agent-harness-boundariesarchitecture goal, following thetyped tool execution contract and coordinator ownership slices.
Why
The previous slice extracted lifecycle, admission, pump, and compaction owners, but the root remained
both composition root and implementation site. Its constructor held 72 anonymous callbacks pointing
back into private root methods:
root creates owner -> owner closes over root method -> root method reaches another owner
As a result, owners were not independently constructible, runtime ports carried root helpers instead
of collaborators,
DeepChatAgentInstancecalled back into orchestration through a delegate, and the1185-line root was approaching its 1300-line guard ceiling.
What changed
pendingInputWakeupBinding)DeepChatLoopRunnerPortsTurnCoordinatorPorts/InteractionCoordinatorPortsmessage projection, prompt assembly, and tool runtime bindings.
DeepChatLoopTapePort.DeepChatAgentInstanceDelegate, the registry hydrator, andDeepChatAgentRuntime.dispose().remains in a dependency cycle.
bypassing the facade or constructing a second runtime owner graph.
surface, protected implementation ownership, and the 350-line facade ceiling.
tests.
rollback.
Summary by CodeRabbit