-
Notifications
You must be signed in to change notification settings - Fork 697
refactor(agent): split runtime lifecycle owners #1971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
ab71671
refactor(agent): thin runtime presenter
zerob13 4c5c20b
refactor(agent): dedupe turn preparation
zerob13 f0c8bc9
refactor(agent): split runtime lifecycle owners
zerob13 9fcba6e
chore(agent): prune tests and sdd
zerob13 a335093
test: prune low-value suite coverage
zerob13 8e89bbe
fix(agent): address runtime review feedback
zerob13 d74d9fc
fix(agent): handle interaction review feedback
zerob13 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
68 changes: 68 additions & 0 deletions
68
docs/architecture/agent-runtime-presenter-thinning/spec.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| # Agent Runtime Presenter Thinning — Spec | ||
|
|
||
| > Status: implemented | ||
| > Baseline: `dev@868241322`, 2026-07-14 | ||
|
|
||
| ## Problem | ||
|
|
||
| The layered runtime migration moved session-owned mutable state into | ||
| `DeepChatAgentInstance` and split direct ACP from the DeepChat backend, but | ||
| `src/main/presenter/agentRuntimePresenter/index.ts` is still an implementation owner rather than a | ||
| thin presenter boundary: | ||
|
|
||
| - 8,167 lines; | ||
| - 211 class methods; | ||
| - 29 class fields; | ||
| - prompt assembly, generation-setting normalization, permission review, tool-result adaptation, | ||
| interaction projection, compaction projection, and turn orchestration remain in one file. | ||
|
|
||
| The latest reliability work increased the file by 557 net lines. The backend split was real, but it | ||
| did not finish the presenter split. | ||
|
|
||
| ## Goal | ||
|
|
||
| Reduce `AgentRuntimePresenter` to the remaining turn/session façade and wiring by moving cohesive, | ||
| independently testable policies to their existing owners or focused modules. This goal must remove | ||
| responsibilities from the class, not move the entire class behind a new name. | ||
|
|
||
| ## Acceptance Criteria | ||
|
|
||
| - `agentRuntimePresenter/index.ts` is at most 3,200 lines. | ||
| - The class has at most 130 methods. | ||
| - Public presenter and session-application contracts remain unchanged. | ||
| - Extracted modules do not receive the presenter instance or a generic service locator. | ||
| - Generation settings, auto-approve review, system prompt/resources, and tool-result normalization | ||
| have focused tests that do not construct the full presenter graph. | ||
| - Existing provider round, Tape, compaction, pending input, permission, tool, Memory, and terminal | ||
| ordering remains unchanged. | ||
| - A repository check prevents the presenter from silently growing past the accepted ceiling. | ||
|
|
||
| ## Constraints | ||
|
|
||
| - No new runtime dependency. | ||
| - No IPC, event payload, database schema, renderer, or user-facing behavior change. | ||
| - Preserve cancellation and stale-instance checks at their current boundaries. | ||
| - Keep `DeepChatAgentInstance` as the owner of per-session mutable runtime state. | ||
| - Keep `ToolPresenter`, `SkillPresenter`, `McpPresenter`, Memory, message, and Tape ownership intact. | ||
|
|
||
| ## Non-goals | ||
|
|
||
| - Rewriting the provider/tool loop. | ||
| - Moving the 8,000-line implementation unchanged into a `TurnRunner` class. | ||
| - Introducing a DI container, plugin lifecycle, base presenter, mixin, or inheritance hierarchy. | ||
| - Forcing `index.ts` below 1,000 lines in one high-risk change. The remaining turn runner can be | ||
| extracted separately once these collaborators are stable. | ||
| - Fixing unrelated behavior found during extraction. | ||
|
|
||
| ## Outcome | ||
|
|
||
| - The initial policy-extraction checkpoint reduced `agentRuntimePresenter/index.ts` from 8,167 to | ||
| 4,905 lines and `AgentRuntimePresenter` from 211 to 135 methods. That checkpoint did not yet meet | ||
| the final 3,200-line / 130-method criteria. | ||
| - Generation policy, prompt/resource assembly, permission review, tool normalization, interaction | ||
| projection, session settings, tool resolution, deferred execution, ACP compatibility, compaction | ||
| projection, and provider permission settlement now have focused owners with explicit dependencies. | ||
| - Follow-up [runtime lifecycle ownership](../deepchat-runtime-lifecycle-owners/spec.md) moved the | ||
| initial/resume turn, provider/tool loop, and paused-interaction control flows behind three explicit | ||
| owners, reduced the presenter boundary to 2,604 lines / 122 methods, and adopted the 3,200-line | ||
| architecture guard, completing the acceptance criteria. | ||
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
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
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
73 changes: 73 additions & 0 deletions
73
docs/architecture/deepchat-runtime-lifecycle-owners/spec.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| # DeepChat Runtime Lifecycle Owners — Spec | ||
|
|
||
| > Status: implemented | ||
| > Baseline: `codex/agent-runtime-presenter-thinning@4c5c20b0c`, 2026-07-14 | ||
|
|
||
| ## Problem | ||
|
|
||
| `AgentRuntimePresenter` is still 4,874 lines because it owns four long-lived control flows: | ||
|
|
||
| - initial turn setup in `processMessage`; | ||
| - resumed turn setup in `resumeAssistantMessage`; | ||
| - provider/tool loop execution in `runStreamForMessage`; | ||
| - paused interaction settlement in `respondToolInteraction`. | ||
|
|
||
| These are not four independent state owners. Initial and resumed setup are two entries into one turn | ||
| lifecycle, while loop execution and interaction settlement have separate invariants. Keeping all four | ||
| flows in the presenter hides those three ownership boundaries. | ||
|
|
||
| ## Goal | ||
|
|
||
| Move the lifecycle implementations behind three explicit owners that do not retain session or run | ||
| state: | ||
|
|
||
| - `TurnCoordinator.start()` and `TurnCoordinator.resume()` own pre-stream turn preparation and | ||
| terminal settlement; | ||
| - `DeepChatLoopRunner.run()` owns provider/tool round execution and context-pressure recovery; | ||
| - `InteractionCoordinator.respond()` owns paused interaction reconciliation and resume decisions. | ||
|
|
||
| `AgentRuntimePresenter` remains the public façade and composition root. | ||
|
|
||
| ## Acceptance Criteria | ||
|
|
||
| - The presenter keeps its existing public API and thin compatibility wrappers used by current tests. | ||
| - No extracted owner receives an `AgentRuntimePresenter` instance or a general-purpose service | ||
| locator. | ||
| - Owner ports are explicit, owner-specific, and contain no mutable runtime state. | ||
| - Per-session and per-run mutable state remains in `DeepChatAgentInstance` and `LoopRun`. | ||
| - Initial/resume ordering, cancellation, stale-instance checks, persistence, hooks, Tape, Memory, | ||
| permissions, queue draining, and terminal outcomes remain unchanged. | ||
| - `agentRuntimePresenter/index.ts` is at most 3,200 lines. | ||
| - Total production TypeScript across the presenter and the three new owner files grows by no more | ||
| than 700 lines over the 4,874-line baseline. The allowed delta is the explicit owner-port | ||
| contracts and composition wiring, not duplicated control flow. | ||
| - The architecture guard adopts the new presenter ceiling. | ||
|
|
||
| ## Constraints | ||
|
|
||
| - No new runtime dependency, database schema, IPC contract, event payload, or user-visible change. | ||
| - No new shared mutable maps, generic dependency container, inheritance hierarchy, or framework. | ||
| - Preserve the existing private wrapper names where tests intentionally replace the loop or resume | ||
| boundary. | ||
| - Prefer existing stores, coordinators, and loop primitives over new abstractions. | ||
|
|
||
| ## Non-goals | ||
|
|
||
| - Rewriting the provider/tool algorithm. | ||
| - Merging initial and resumed context construction. | ||
| - Moving session state out of `DeepChatAgentInstance`. | ||
| - Splitting every helper into its own class or forcing the presenter below 1,000 lines. | ||
| - Syncing a GitHub issue. | ||
|
|
||
| ## Outcome | ||
|
|
||
| - `agentRuntimePresenter/index.ts`: 4,874 → 2,604 lines; class methods: 136 → 122. | ||
| - `TurnCoordinator` owns `start()`/`resume()` in 1,226 lines. | ||
| - `DeepChatLoopRunner` owns `run()`, context-pressure recovery, Tape manifests, request traces, and | ||
| rate-limit projection in 967 lines. | ||
| - `InteractionCoordinator` owns `respond()` and deferred permission/question/skill-draft settlement | ||
| in 728 lines. | ||
| - The four-file production total is 5,525 lines, a 651-line increase for explicit port contracts and | ||
| composition wiring, within the 700-line ceiling. | ||
| - The presenter guard is 3,200 lines. Focused runtime validation passes 605 tests with 19 skipped; | ||
| format, i18n, lint, architecture guards, and node/web type checks pass. |
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Outcome contradicts this spec's own acceptance criteria.
Acceptance criteria require ≤3,200 lines / ≤130 methods (Lines 30-31), but the Outcome reports 4,905 lines / 135 methods (Lines 59-60) while also claiming the guard already "rejects growth beyond 3,200 lines" (Line 64). A 4,905-line file cannot coexist with an already-enforced 3,200-line guard. Per the follow-up spec, the 3,200 ceiling appears to actually be adopted by the later
deepchat-runtime-lifecycle-ownerswork, not this one — Line 64 seems to prematurely claim that outcome here.Also applies to: 59-64
🤖 Prompt for AI Agents