将 LLM 执行移出会话 Actor Turn#2298
Merged
eanzhao merged 10 commits intoJun 19, 2026
Merged
Conversation
eanzhao
added
crnd:lifecycle:managed
Contributor
Author
📊 当前状态 — review-gate(不需要人介入)
下一步自动会做: 三路 reviewer 完成后按固定 truth table 决定 merge、fix 或等待明确审批。 何时需要人介入:
🤖 controller status banner ⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 Tests review：需要补齐两个入口的 off-actor 分支测试TL;DR
测试覆盖结论
具体缺口：
建议补齐的测试：
本轮没有发现新增 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: tests
head_sha: b4b87edffbb98a026bba7e0f1546a7a2e7243410
verdict: reject
---
## Verdict
Reject: the PR adds feature-gated off-actor executor branches to Chat Completions and Messages, but tests only cover the equivalent Responses branch.
## Evidence
- `src/platform/Aevatar.GAgentService.Application/Responses/ChatCompletionsCommandFacade.cs:442` and `:623` add the `OffActorLlmRunExecutorEnabled` branch that calls `ILlmRunExecutor.StartAsync` and then starts `ExecuteAsync` without waiting, but `test/Aevatar.GAgentService.Tests/Application/ChatCompletionsCommandFacadeTests.cs:757` still cannot inject `OffActorLlmRunExecutorEnabled` or an `ILlmRunExecutor`, and the file has no flag-on/flag-off assertions for this new behavior.
- `src/platform/Aevatar.GAgentService.Application/Responses/MessagesCommandFacade.cs:133` and `:608` add the same off-actor executor branch, but `test/Aevatar.GAgentService.Tests/Application/MessagesCommandFacadeTests.cs:305` still cannot inject `OffActorLlmRunExecutorEnabled` or an `ILlmRunExecutor`, and the file has no matching behavior test for Messages.
- `test/Aevatar.GAgentService.Tests/Application/ResponsesCommandFacadeTests.cs:870` and `:901` do cover `/v1/responses` flag-off legacy dispatch and flag-on non-blocking executor start, so the coverage gap is not the executor itself; it is the two other ingress facades that gained the same net-new branch.
- Diff scan found no added `[Skip]`, manual traits, `Task.Delay`, `Thread.Sleep`, `WaitUntilAsync`, bump-only assertions, or polling allowlist changes.
## What would change your verdict (only if comment or reject)
Add behavior tests in `ChatCompletionsCommandFacadeTests` and `MessagesCommandFacadeTests` equivalent to the existing Responses tests: with `OffActorLlmRunExecutorEnabled = false`, assert legacy `LlmRunRequested` dispatch and no executor start; with it enabled plus a blocking fake executor, assert `StartAsync` is called, `ExecuteAsync` starts without blocking the client-facing result, and the legacy dispatch port is not used for the run command.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:tests:reject⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 Quality review: 可合并但建议收紧 executor 启动语义TL;DR
质量意见
我没有发现新死代码、无授权的抽象扩张或无关 drive-by cleanup。主要可读性问题是两点：
建议把 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: quality
head_sha: b4b87edffbb98a026bba7e0f1546a7a2e7243410
verdict: comment
---
## Verdict
Comment: the PR is focused and reachable, but the off-actor executor API and facade wiring would be easier to review if the two-phase admission/execution contract were named and factored more explicitly.
## Evidence
- `src/platform/Aevatar.GAgentService.Abstractions/Responses/LlmRunCoreContracts.cs:20` exposes `ILlmRunExecutor.StartAsync`, but `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:17` only creates a `DispatchAdmission`; actual provider execution starts only when callers separately invoke `ExecuteAsync`. The name reads as "the run has started" while the method actually means "admit/synthesize the run start receipt", which makes the public contract less self-documenting.
- `src/platform/Aevatar.GAgentService.Application/Responses/ResponsesCommandFacade.cs:979`, `src/platform/Aevatar.GAgentService.Application/Responses/MessagesCommandFacade.cs:608`, and `src/platform/Aevatar.GAgentService.Application/Responses/ChatCompletionsCommandFacade.cs:623` repeat the same off-actor helper shape: build executor request, call `StartAsync`, fire-and-forget `ExecuteAsync`, return admission. That is three near-identical copies of the new two-phase launch protocol; extracting a shared helper or renaming the executor API would reduce the chance that the facades drift.
## What would change your verdict (only if comment or reject)
Rename `StartAsync` to an admission-specific verb such as `AdmitAsync` / `CreateAdmissionAsync`, or wrap the two-step launch in a small shared helper so each facade calls a single clearly named operation for "start off-actor execution and return admission".
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:quality:comment⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 架构审查结论：reject，off-actor ACK 语义不诚实TL;DR
架构阻塞点是 actor 之间投递命令后的接收凭据，只能表示“已经到达 dispatch/inbox 边界”。现在 在本地构造 envelope 后直接调用 ，没有调用 （actor 之间发命令的标准通道），所以这个 ACK 没有真实投递事实支撑。 、、 在 时把这个 synthetic admission 交给 observation kickoff；这会让上层认为 run 已 accepted，但 session actor 还没有 actor-owned active run。随后 executor 发送 chunk/tool/terminal record commands 时， 要求 已存在；当前 diff 没有新增 command/handler，只有 schema，因此 flag-on 路径可能先记录后启动，或者直接进入“no active LLM run”的错误分支。 阻塞依据来自 CLAUDE.md：同步 ACK 只能承诺真实达到的阶段；稳定业务事实必须由唯一 actor 拥有，committed event 必须进入可观察主链。这里的 run-start 是稳定控制流事实，不能只存在于本地 admission envelope。 修复方向：把 run-start 改为真实 actor-owned command/fact。可以新增 typed /等价命令，经 dispatch 到 并持久化 后返回真实 admission；或者继续用现有 作为 admission 命令，只把长 LLM loop 移出 actor turn。无论选哪条路，返回的 必须来自真实 dispatch 边界，且 chunk/tool/terminal record commands 只能在 actor active run 存在之后发送。 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: architect
head_sha: b4b87edffbb98a026bba7e0f1546a7a2e7243410
verdict: reject
---
## Verdict
reject - the off-actor feature-flag path returns a synthetic accepted dispatch receipt for `LlmRunStartedEvent` without actually dispatching or committing the run-start fact, so ACK semantics and actor-owned state are no longer honest.
## Evidence
- `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:29`-`45` builds an `EventEnvelope` carrying `LlmRunStartedEvent` and returns `DispatchAdmissionFactory.Create(...)` locally, but never calls `IActorDispatchPort.DispatchAsync`; the only real dispatch in this executor is later record-command dispatch at `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:131`-`142`. CLAUDE.md clause: `ACK 诚实：同步返回只承诺已达到阶段（默认 accepted + stable command id）；committed/read-model observed 等强保证须通过独立契约或异步观察获取。`
- `src/platform/Aevatar.GAgentService.Application/Responses/ResponsesCommandFacade.cs:983`-`986`, `src/platform/Aevatar.GAgentService.Application/Responses/ChatCompletionsCommandFacade.cs:627`-`630`, and `src/platform/Aevatar.GAgentService.Application/Responses/MessagesCommandFacade.cs:612`-`615` use that synthetic admission as the `ObserveAsync` kickoff result when `OffActorLlmRunExecutorEnabled` is true. That tells callers/observers an envelope was accepted even though no actor inbox boundary accepted the start event. CLAUDE.md clause: `ACK 诚实：同步返回只承诺已达到阶段（默认 accepted + stable command id）；committed/read-model observed 等强保证须通过独立契约或异步观察获取。`
- `src/platform/Aevatar.GAgentService.Abstractions/Protos/llm_sessions.proto:235`-`240` defines `LlmRunStartedEvent`, but the same diff has no `RecordLlmRunStarted` command and `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:331`-`350` starts directly with chunk-record handlers. Those handlers call `TryPrepareRunRecord`, which throws when `State.ActiveRun` is missing at `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:924`-`926`; therefore the flag-on facade path has no actor-owned run-start fact before executor record commands arrive. CLAUDE.md clauses: `单一权威拥有者：每个稳定业务事实有唯一 actor 拥有；committed event store + actor state 是唯一真相，readmodel 只是查询副本。` and `committed domain event 必须可观察：write-side 完成 committed event 后必须送入 projection 主链；禁止只落 event store 而不进入可观察流。`
## What would change your verdict
Make run-start a real actor-owned command/fact in the off-actor path before returning the admission: dispatch a typed `RecordLlmRunStarted`/equivalent command through `IActorDispatchPort` and persist `LlmRunStartedEvent` in `LlmSessionGAgent`, or keep using the existing `LlmRunRequested` actor command for admission and move only the long LLM loop out of the turn. The returned `DispatchAdmission` must come from a real dispatch boundary, and subsequent chunk/tool/terminal record commands must only run after the session actor has an active run.
⟦AI:AUTO-LOOP⟧⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 架构审查结论更正：reject，off-actor ACK 语义不诚实TL;DR
架构阻塞点
阻塞依据来自 CLAUDE.md：同步 ACK 只能承诺真实达到的阶段；稳定业务事实必须由唯一 actor 拥有，committed event 必须进入可观察主链。这里的 run-start 是稳定控制流事实，不能只存在于本地 admission envelope。 修复方向：把 run-start 改为真实 actor-owned command/fact。可以新增 typed 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: architect
head_sha: b4b87edffbb98a026bba7e0f1546a7a2e7243410
verdict: reject
---
## Verdict
reject - the off-actor feature-flag path returns a synthetic accepted dispatch receipt for `LlmRunStartedEvent` without actually dispatching or committing the run-start fact, so ACK semantics and actor-owned state are no longer honest.
## Evidence
- `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:29`-`45` builds an `EventEnvelope` carrying `LlmRunStartedEvent` and returns `DispatchAdmissionFactory.Create(...)` locally, but never calls `IActorDispatchPort.DispatchAsync`; the only real dispatch in this executor is later record-command dispatch at `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:131`-`142`. CLAUDE.md clause: `ACK 诚实：同步返回只承诺已达到阶段（默认 accepted + stable command id）；committed/read-model observed 等强保证须通过独立契约或异步观察获取。`
- `src/platform/Aevatar.GAgentService.Application/Responses/ResponsesCommandFacade.cs:983`-`986`, `src/platform/Aevatar.GAgentService.Application/Responses/ChatCompletionsCommandFacade.cs:627`-`630`, and `src/platform/Aevatar.GAgentService.Application/Responses/MessagesCommandFacade.cs:612`-`615` use that synthetic admission as the `ObserveAsync` kickoff result when `OffActorLlmRunExecutorEnabled` is true. That tells callers/observers an envelope was accepted even though no actor inbox boundary accepted the start event. CLAUDE.md clause: `ACK 诚实：同步返回只承诺已达到阶段（默认 accepted + stable command id）；committed/read-model observed 等强保证须通过独立契约或异步观察获取。`
- `src/platform/Aevatar.GAgentService.Abstractions/Protos/llm_sessions.proto:235`-`240` defines `LlmRunStartedEvent`, but the same diff has no `RecordLlmRunStarted` command and `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:331`-`350` starts directly with chunk-record handlers. Those handlers call `TryPrepareRunRecord`, which throws when `State.ActiveRun` is missing at `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:924`-`926`; therefore the flag-on facade path has no actor-owned run-start fact before executor record commands arrive. CLAUDE.md clauses: `单一权威拥有者：每个稳定业务事实有唯一 actor 拥有；committed event store + actor state 是唯一真相，readmodel 只是查询副本。` and `committed domain event 必须可观察：write-side 完成 committed event 后必须送入 projection 主链；禁止只落 event store 而不进入可观察流。`
## What would change your verdict
Make run-start a real actor-owned command/fact in the off-actor path before returning the admission: dispatch a typed `RecordLlmRunStarted`/equivalent command through `IActorDispatchPort` and persist `LlmRunStartedEvent` in `LlmSessionGAgent`, or keep using the existing `LlmRunRequested` actor command for admission and move only the long LLM loop out of the turn. The returned `DispatchAdmission` must come from a real dispatch boundary, and subsequent chunk/tool/terminal record commands must only run after the session actor has an active run.
⟦AI:AUTO-LOOP⟧⟦AI:AUTO-LOOP⟧ |
Contributor
Author
📊 当前状态 — fixing(不需要人介入)
下一步自动会做: fix codex 会修复 blocking reject；完成后控制器提交并推送，再重新派三路 reviewer。 何时需要人介入:
🤖 controller status banner ⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 架构审查结论：reject，off-actor start ACK 不诚实TL;DR
架构阻断点
建议修复方向： 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: architect
head_sha: b4b87edffbb98a026bba7e0f1546a7a2e7243410
verdict: reject
---
## Verdict
Reject: the new off-actor executor path fabricates an accepted dispatch receipt without actually admitting the run-start message to the actor runtime, so the actor-owned run fact and timeout setup are bypassed.
## Evidence
- `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:17`: `StartAsync` builds a `LlmRunStartedEvent` envelope and returns `DispatchAdmissionFactory.Create(...)` at line 45, but never calls `IActorDispatchPort.DispatchAsync`; that means the receipt claims accepted-for-dispatch without the envelope entering the actor inbox. This violates CLAUDE.md Command / Envelope / Dispatch: `ACK 诚实：同步返回只承诺已达到阶段（默认 accepted + stable command id）；committed/read-model observed 等强保证须通过独立契约或异步观察获取。` It also violates CLAUDE.md Command / Envelope / Dispatch: `传输载体可替换：上层依赖投递契约（IActorDispatchPort），不依赖具体载体；链路可从直投切换为异步传输而不污染应用语义。`
- `src/platform/Aevatar.GAgentService.Application/Responses/ResponsesCommandFacade.cs:979`, `src/platform/Aevatar.GAgentService.Application/Responses/ChatCompletionsCommandFacade.cs:623`, and `src/platform/Aevatar.GAgentService.Application/Responses/MessagesCommandFacade.cs:608`: each `StartOffActorRunAsync` calls `llmRunExecutor.StartAsync(...)`, then fire-and-forgets `ExecuteAsync(...)` directly, bypassing the old `DispatchRunAsync` path that sent `LlmRunRequested` through `IActorDispatchPort`. The actor-owned start fact is only persisted inside `LlmSessionGAgent.HandleLlmRunRequestedAsync` at `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:305`, where it also schedules the run timeout at line 312; the new off-actor path no longer reaches that event handler before executor output records are dispatched. This violates CLAUDE.md Actor 设计 / 生命周期 / 执行模型: `回调只发信号：Task.Run/Timer/线程池回调不直接读写运行态或推进业务；只发布内部触发事件。` The off-turn execution itself is fine as a concept, but the first state transition must still be an actor message/fact rather than a fabricated local admission.
- `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:911`: run record handlers require an existing `State.ActiveRun`; if the off-actor executor dispatches `RecordLlmStreamChunkObserved` / `RecordLlmRunCompleted` after the fabricated start admission, `TryPrepareRunRecord` throws `"has no active LLM run"` at lines 924-926. The resulting executor failure record follows the same path and is rejected for the same reason. This violates CLAUDE.md 权威状态 / ReadModel / Projection: `单一权威拥有者：每个稳定业务事实有唯一 actor 拥有；committed event store + actor state 是唯一真相，readmodel 只是查询副本。` The PR introduces a path where the executor assumes the run has started, but the authoritative actor state has not committed that fact.
## What would change your verdict
Route the off-actor start through the actor-owned command/event boundary before returning admission. Concretely, `StartAsync` should either dispatch the same `LlmRunRequested` / start command through `IActorDispatchPort` and return that real admission, or introduce a typed actor command that opens the run and schedules timeout, then have the executor begin only after that admission. Do not return `DispatchAdmissionFactory.Create(...)` for an envelope that was not actually admitted to the runtime.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:architect:reject⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 PR 2298 round 1 reject 修复已完成TL;DR
修复摘要本轮只把
验证
📎 完整 codex 原始输出(存档备查)# Fix report for PR 2298 round 1
## Applied
- (A) src/platform/Aevatar.GAgentService.Abstractions/Protos/llm_sessions.proto:242: added typed `RecordLlmRunStarted` command carrying the original `LlmRunRequested` payload so off-actor admission has a real actor command contract (addresses reviewer:architect's evidence #1/#3).
- (A) src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:17: changed `StartAsync` to dispatch `RecordLlmRunStarted` through `IActorDispatchPort` and return the real `DispatchAdmission`, removing the synthetic local admission (addresses reviewer:architect's evidence #1/#2).
- (A) src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:303: added `HandleRecordRunStartedAsync` so the session actor persists `LlmRunStartedEvent`, schedules timeout, and only then starts `ExecuteAsync` (addresses reviewer:architect's evidence #3).
- (A) src/platform/Aevatar.GAgentService.Application/Responses/ResponsesCommandFacade.cs:979, src/platform/Aevatar.GAgentService.Application/Responses/ChatCompletionsCommandFacade.cs:623, src/platform/Aevatar.GAgentService.Application/Responses/MessagesCommandFacade.cs:610: kept off-actor facades at admission only; provider execution is no longer started directly from the facades (addresses reviewer:architect's evidence #2).
- (A) test/Aevatar.GAgentService.Tests/Application/LlmRunExecutorTests.cs:17 and test/Aevatar.GAgentService.Tests/Core/LlmSessionGAgentTests.cs:800: updated behavior tests to assert real start-command dispatch and actor-owned start persistence before executor execution (addresses reviewer:architect's evidence #1/#3).
- (B) test/Aevatar.GAgentService.Tests/Application/ChatCompletionsCommandFacadeTests.cs:415: scope-extend reason: reviewer:tests rejected missing coverage in this ingress file, which is outside the original PR file list but in the same logical off-actor ingress refactor; added flag-off legacy dispatch and flag-on executor admission tests (addresses reviewer:tests's evidence #1).
- (B) test/Aevatar.GAgentService.Tests/Application/MessagesCommandFacadeTests.cs:98: scope-extend reason: reviewer:tests rejected missing coverage in this ingress file, which is outside the original PR file list but in the same logical off-actor ingress refactor; added flag-off legacy dispatch and flag-on executor admission tests (addresses reviewer:tests's evidence #2).
## Rejected as false positive
- None.
## Blocked (cannot fix this round)
- None.
## Build status
- build: pass (`dotnet build aevatar.slnx --nologo`)
- tests: pass (`dotnet test test/Aevatar.GAgentService.Tests/Aevatar.GAgentService.Tests.csproj --nologo`; `bash tools/ci/test_stability_guards.sh`)
## Recommendation for next round
- Expect merge: both reject demands were addressed. reviewer:quality's comment about naming/factoring was read as advisory under the fixed reject=0 gate and was not treated as a blocking demand.
⟦AI:AUTO-LOOP⟧
FIX_DONE:2298:round-1:applied-7:rejected-0:blocked-0⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 PR2298 quality review: approveTL;DR
质量结论
关键检查点:
文件依据:
📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: quality
head_sha: c1f1273406d1be0fa68644b5faa4695bde47bec1
verdict: approve
---
## Verdict
Approve: the off-actor LLM run changes are readable, scoped to the run-start/executor path, and do not introduce quality-blocking dead code, naming drift, or avoidable complexity.
## Evidence
- `src/platform/Aevatar.GAgentService.Abstractions/Responses/LlmRunCoreContracts.cs:11` names the new request around the business action (`LlmRunExecutorRequest`) and both new `ILlmRunExecutor` methods are called from production paths: ingress facades call `StartAsync`, while `LlmSessionGAgent` calls `ExecuteAsync`.
- `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:17` keeps the admission step separate from `ExecuteAsync` at line 53, which makes the off-turn lifecycle easy to follow without a new manager/helper-style abstraction.
- `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:276` and `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:303` share run-start persistence through `TryCommitRunStartedAsync` at line 353 instead of duplicating the active-run/timeout checks.
- `src/platform/Aevatar.GAgentService.Application/Responses/ChatCompletionsCommandFacade.cs:623`, `src/platform/Aevatar.GAgentService.Application/Responses/MessagesCommandFacade.cs:610`, and `src/platform/Aevatar.GAgentService.Application/Responses/ResponsesCommandFacade.cs:979` add small off-actor start helpers. The shape repeats across three facades, but each helper delegates to that facade's existing plan-specific `BuildRunRequested` mapping, so extraction is optional rather than a must-fix DRY issue.
- `HOST_REFACTOR_COMMENT_POLICY` is unset and normalizes to `none`; the diff adds no new `// Refactor`, `// Old pattern`, or `// New principle` source comments.
- `git diff --check origin/feature/integrate...origin/refactor/2026-06-19_issue-2274-off-actor-llm-run-executor` reported no whitespace errors, and a scan of added lines found no debug leftovers or obvious dead-code markers.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:quality:approve⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 Tests review: comment，覆盖足够但测试命名需校正TL;DR
测试结论本轮检查了 PR diff、实际测试文件、 结论是非阻塞 comment：覆盖面基本匹配本次变更，新增的 off-actor LLM 执行入口、三类 ingress facade 的开关路径、 需要修的是测试命名质量： 建议改成类似 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: tests
head_sha: c1f1273406d1be0fa68644b5faa4695bde47bec1
verdict: comment
---
## Verdict
Test coverage and stability are adequate for the diff, but several new executor test names should be corrected because they describe `StartAsync` while exercising `ExecuteAsync`.
## Evidence
- `test/Aevatar.GAgentService.Tests/Application/LlmRunExecutorTests.cs:64` `StartAsync_ShouldDispatchForwardedToolCallRecordCommandsInOrder` calls `executor.ExecuteAsync(...)` at line 102 and asserts forwarded-tool record commands. Since `StartAsync` now only dispatches `RecordLlmRunStarted`, the test name no longer describes the behavior under test.
- `test/Aevatar.GAgentService.Tests/Application/LlmRunExecutorTests.cs:135` `StartAsync_ShouldDispatchLocalToolResultRecordCommand` calls `executor.ExecuteAsync(...)` at line 174 and asserts local tool execution records. This should be named for `ExecuteAsync` or the off-turn execution loop.
- `test/Aevatar.GAgentService.Tests/Application/LlmRunExecutorTests.cs:191`, `:219`, and `:246` similarly name provider failure, cancellation, and sink-dispatch failure scenarios as `StartAsync_*`, but each test starts at `executor.ExecuteAsync(...)` and asserts terminal record commands emitted by the execution loop.
- Positive coverage observed: `test/Aevatar.GAgentService.Tests/Application/LlmRunExecutorTests.cs:18` covers `StartAsync` admission and `RecordLlmRunStarted`, then separately runs `ExecuteAsync`; facade tests cover `OffActorLlmRunExecutorEnabled` true/false for Chat Completions, Messages, and Responses; `test/Aevatar.GAgentService.Tests/Core/LlmSessionGAgentTests.cs:801` covers `RecordLlmRunStarted` being persisted before executor continuation.
- Stability check: `bash tools/ci/test_stability_guards.sh` passed. No new `[Skip]`, manual category, `Task.Delay`, `Thread.Sleep`, or `WaitUntilAsync` additions were found in the PR diff.
## What would change your verdict
Rename the affected `LlmRunExecutorTests` methods so the behavior name matches `ExecuteAsync` / off-turn record dispatch, for example `ExecuteAsync_ShouldDispatchForwardedToolCallRecordCommandsInOrder`.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:tests:comment⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 架构审查：reject，Core actor 仍在启动 Application executorTL;DR
架构阻塞点
阻塞依据是 建议修复把 让 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: architect
head_sha: c1f1273406d1be0fa68644b5faa4695bde47bec1
verdict: reject
---
## Verdict
reject - the PR still exposes the off-actor LLM executor as an Abstractions contract and the new start-record path kicks Application execution from inside the session actor.
## Evidence
- `src/platform/Aevatar.GAgentService.Abstractions/Responses/LlmRunCoreContracts.cs:1` and `:18`: the PR adds `Aevatar.Foundation.Abstractions.DispatchAdmission` to the shared `ILlmRunExecutor` contract, keeping an Application orchestration/executor surface in `Aevatar.GAgentService.Abstractions`. This contradicts the accepted issue-2274 design in `.refactor-loop/runs/phase9-issue2274-r4-judge.md:13`, which says `ILlmRunExecutor` / `LlmRunExecutorRequest` belong in `src/platform/Aevatar.GAgentService.Application/Responses/ILlmRunExecutor.cs` and must not be placed in `Abstractions/Responses/LlmRunCoreContracts.cs`. Clause cite: `CLAUDE.md` says "边界清晰：协议适配、业务编排、状态管理分属不同层；禁止跨层偷渡语义。" and "严格分层：`Domain / Application / Infrastructure / Host`；`API` 仅做宿主与组合，不承载业务编排。"
- `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:304`: the new `HandleRecordRunStartedAsync` path records start, then at `:322` resolves `ILlmRunExecutor` via `Services.GetRequiredService<ILlmRunExecutor>()` and at `:329` fire-and-forgets `ExecuteAsync` from the actor handler. That means the Core actor no longer only records the typed `RecordLlmRunStarted` fact; it also owns the kickoff of Application provider execution. Clause cite: `CLAUDE.md` says "不用 `Service Locator` 隐藏依赖——依赖一律构造注入、显式可见。" and "边界清晰：协议适配、业务编排、状态管理分属不同层；禁止跨层偷渡语义。"
## What would change your verdict
Move `ILlmRunExecutor` / `LlmRunExecutorRequest` out of `Aevatar.GAgentService.Abstractions` into `src/platform/Aevatar.GAgentService.Application/Responses/ILlmRunExecutor.cs` under namespace `Aevatar.GAgentService.Application.Responses`.
Make `RecordLlmRunStarted` handling in `LlmSessionGAgent` actor-owned fact recording only: persist `LlmRunStartedEvent`, schedule timeout or record start failure, and do not resolve or execute `ILlmRunExecutor` from Core. Start `ExecuteAsync` from an Application-owned continuation/event-consumer path after the actor-owned start fact is durably recorded, and keep subsequent executor output flowing back as typed `RecordLlm*` commands through `IActorDispatchPort`.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:architect:reject⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 Quality review: approveTL;DR
质量复核结论
三个 facade 中的 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: quality
head_sha: c1f1273406d1be0fa68644b5faa4695bde47bec1
verdict: approve
---
## Verdict
Approve: the PR keeps the off-actor LLM execution change readable and focused, with reachable new surface and no quality-axis blocker.
## Evidence
- `src/platform/Aevatar.GAgentService.Abstractions/Responses/LlmRunCoreContracts.cs:11` names `LlmRunExecutorRequest` around the concrete LLM run execution boundary; this is domain-specific and avoids generic helper/manager terminology.
- `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:17` and `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:53` split admission (`StartAsync`) from execution (`ExecuteAsync`) cleanly; both public methods are called by facades/actor code and covered by tests, so the new public surface is not dead code.
- `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:353` extracts the repeated start-commit logic into `TryCommitRunStartedAsync`; the two event handlers stay short and the method is below the preferred complexity threshold.
- `src/platform/Aevatar.GAgentService.Application/Responses/ChatCompletionsCommandFacade.cs:623`, `src/platform/Aevatar.GAgentService.Application/Responses/MessagesCommandFacade.cs:610`, and `src/platform/Aevatar.GAgentService.Application/Responses/ResponsesCommandFacade.cs:979` add the same narrow off-actor adapter pattern per endpoint; each endpoint already owns slightly different `LlmRunRequested` construction, so this is not a harmful abstraction gap.
- `src/platform/Aevatar.GAgentService.Application/Responses/ResponsesIngressOptions.cs:34` adds only a focused feature switch. `${HOST_REFACTOR_COMMENT_POLICY}` is empty and normalizes to `none`; the diff does not add new `Refactor`/`Old pattern`/`New principle` source comments, so self-doc handling is compliant for this review.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:quality:approve⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 架构复审通过：off-actor admission 已回到真实 actor dispatchTL;DR
架构结论
📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: architect
head_sha: c1f1273406d1be0fa68644b5faa4695bde47bec1
verdict: approve
---
## Verdict
approve - no architectural concerns; the current head routes off-actor LLM run admission through a real actor command dispatch and keeps the session actor as the authoritative run fact owner.
## Evidence
- `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:17` returns `DispatchAdmission` from `IActorDispatchPort.DispatchAsync` at line 50 after packing typed `RecordLlmRunStarted`; this satisfies CLAUDE.md: `ACK 诚实：同步返回只承诺已达到阶段（默认 accepted + stable command id）；committed/read-model observed 等强保证须通过独立契约或异步观察获取。`
- `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:303` handles `RecordLlmRunStarted`, and `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:353` persists `LlmRunStartedEvent` plus schedules timeout before calling `ExecuteAsync`; this satisfies CLAUDE.md: `单一权威拥有者：每个稳定业务事实有唯一 actor 拥有；committed event store + actor state 是唯一真相，readmodel 只是查询副本。`
- `src/platform/Aevatar.GAgentService.Abstractions/Protos/llm_sessions.proto:242` adds a typed `RecordLlmRunStarted` command rather than a generic bag; this satisfies CLAUDE.md: `核心语义强类型：影响业务语义、控制流、稳定读取且仓库内可控的数据，必须建模为 proto field / typed option / typed sub-message，禁止塞入通用 bag。`
- `src/platform/Aevatar.GAgentService.Application/Responses/ResponsesCommandFacade.cs:979`, `src/platform/Aevatar.GAgentService.Application/Responses/ChatCompletionsCommandFacade.cs:623`, and `src/platform/Aevatar.GAgentService.Application/Responses/MessagesCommandFacade.cs:610` keep ingress facades at the admission boundary by delegating to `ILlmRunExecutor.StartAsync`; the long `ILlmRunCore` execution is started by the session actor after the start fact is committed, matching CLAUDE.md: `业务推进内聚：工作流推进（成功/失败/分支/重试）在 Actor 事件处理流程内完成，保证顺序性与可重放性。`
- Added-line scans found no new `ChatAsync`, `GetAwaiter().GetResult()`, `TypeUrl.Contains`, hardcoded `5000`/`5050`, technical `*WriteActor`/`*ReadActor` split, external repo dependency, host production fact under `.refactor-loop/`, or new Old/New refactor-history source comments. With empty host policy variables, `HOST_REFACTOR_COMMENT_POLICY` normalizes to `none` and absence is compliant.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:architect:approve⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 tests review：需要补 streaming off-actor 覆盖TL;DR
测试覆盖缺口
补两个行为测试即可改变 verdict： 本轮还检查了 touched tests 的新增内容，未发现新增 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: tests
head_sha: c1f1273406d1be0fa68644b5faa4695bde47bec1
verdict: reject
---
## Verdict
Reject: the new off-actor LLM executor branch is covered for non-streaming facades, but two public streaming paths added by the diff have no matching behavior coverage.
## Evidence
- `src/platform/Aevatar.GAgentService.Application/Responses/MessagesCommandFacade.cs:133` adds the streaming `StreamAsync` branch that chooses `StartOffActorRunAsync` when `OffActorLlmRunExecutorEnabled` is true; `test/Aevatar.GAgentService.Tests/Application/MessagesCommandFacadeTests.cs:217` only covers the default streaming path and asserts the legacy `LlmRunRequested` dispatch, while the new off-actor streaming branch has no test asserting executor `StartAsync` admission and no legacy dispatch.
- `src/platform/Aevatar.GAgentService.Application/Responses/ResponsesCommandFacade.cs:221` adds the same off-actor selection inside `ResponsesCommandFacade.StreamAsync`; `test/Aevatar.GAgentService.Tests/Application/ResponsesCommandFacadeTests.cs:490` covers streaming through the default dispatch path, and the added off-actor tests at `test/Aevatar.GAgentService.Tests/Application/ResponsesCommandFacadeTests.cs:871` and `test/Aevatar.GAgentService.Tests/Application/ResponsesCommandFacadeTests.cs:902` cover only non-streaming `CreateAsync`. There is no behavior test for streaming with `OffActorLlmRunExecutorEnabled = true`, including the no-legacy-dispatch outcome.
- Diff scan found no added `Task.Delay`, `Thread.Sleep`, `[Skip]`, `[Trait("Category","Manual")]`, or `Assert.True(true)` in touched tests.
## What would change your verdict (only if comment or reject)
Add behavior tests for `MessagesCommandFacade.StreamAsync` and `ResponsesCommandFacade.StreamAsync` with `ResponsesIngressOptions.OffActorLlmRunExecutorEnabled = true` and an `ILlmRunExecutor` test double. Each test should assert the stream returns observed deltas/completion, `StartAsync` is called once with the expected `LlmRunExecutorRequest`, the returned admission is used, and `IActorDispatchPort.DispatchAsync` is not called for the legacy `LlmRunRequested` path.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:tests:reject⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 Quality review 结论: approveTL;DR
质量复审证据本轮只看命名、可读性、复杂度、死代码、过度/不足抽象、无关改动和 refactor self-doc comment policy。
📎 完整 codex 原始输出(存档备查)pr: 2298
|
Contributor
Author
🤖 tests review: reject，streaming off-actor 分支缺测试TL;DR
测试覆盖结论本次问题只看测试覆盖，不评价生产架构。 具体缺口：
我没有发现新增 需要补的测试很具体：给 Chat Completions、Messages、Responses 各补一个 flag-on 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: tests
head_sha: c1f1273406d1be0fa68644b5faa4695bde47bec1
verdict: reject
---
## Verdict
Reject because the PR adds off-actor LLM execution branches to streaming entry points, but the new tests only cover the flag on non-streaming create paths.
## Evidence
- `src/platform/Aevatar.GAgentService.Application/Responses/MessagesCommandFacade.cs:128` adds a flag-aware `StreamAsync` dispatch branch, but `test/Aevatar.GAgentService.Tests/Application/MessagesCommandFacadeTests.cs:217` still builds the stream facade without `OffActorLlmRunExecutorEnabled` or `ILlmRunExecutor`; the added flag-on/off tests at `MessagesCommandFacadeTests.cs:97` and `MessagesCommandFacadeTests.cs:121` only exercise `CreateAsync`.
- `src/platform/Aevatar.GAgentService.Application/Responses/ResponsesCommandFacade.cs:214` adds the same off-actor branch to `StreamAsync`, including the existing incoming-tool-result continuation after admission, but `test/Aevatar.GAgentService.Tests/Application/ResponsesCommandFacadeTests.cs:490` covers only legacy stream dispatch. The new off-actor tests at `ResponsesCommandFacadeTests.cs:870` and `ResponsesCommandFacadeTests.cs:901` cover only non-streaming `CreateAsync`.
- `src/platform/Aevatar.GAgentService.Application/Responses/ChatCompletionsCommandFacade.cs:433` makes stream observation choose `StartOffActorRunAsync` when the flag is enabled, but `test/Aevatar.GAgentService.Tests/Application/ChatCompletionsCommandFacadeTests.cs:495` still exercises streaming with default options; the new flag tests at `ChatCompletionsCommandFacadeTests.cs:414` and `ChatCompletionsCommandFacadeTests.cs:442` cover `CreateAsync` only.
- Negative checks were clean: no added `[Skip]`, manual traits, or sleep/delay pacing were found in the changed tests; `bash tools/ci/test_stability_guards.sh` passed, and the focused `Aevatar.GAgentService.Tests` filter for the touched suites passed 120 tests.
## What would change your verdict (only if comment or reject)
Add flag-on `StreamAsync` behavior tests for Chat Completions, Messages, and Responses that assert the stream path admits via `ILlmRunExecutor.StartAsync`, does not dispatch legacy `LlmRunRequested` through `IActorDispatchPort`, still returns observed deltas/completion, and for Responses preserves incoming forwarded-tool-result resolution around the off-actor admission.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:tests:reject⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 架构复审结论：approveTL;DR
架构证据
本轮额外检查：新增行未发现 Old/New refactor-history 源码注释、外部仓库新增依赖、 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: architect
head_sha: c1f1273406d1be0fa68644b5faa4695bde47bec1
verdict: approve
---
## Verdict
approve: no architecture-compliance regressions found in the PR diff.
## Evidence
- No blocking issues flagged. `src/platform/Aevatar.GAgentService.Abstractions/Protos/llm_sessions.proto:242` adds `RecordLlmRunStarted` as a typed protobuf command for the run-start transition, matching CLAUDE.md: "核心语义强类型：影响业务语义、控制流、稳定读取且仓库内可控的数据，必须建模为 `proto field / typed option / typed sub-message`，禁止塞入通用 bag。"
- `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:17` now returns the real `DispatchAdmission` from `IActorDispatchPort` for the start command, matching CLAUDE.md: "ACK 诚实：同步返回只承诺已达到阶段（默认 `accepted + stable command id`）；`committed`/`read-model observed` 等强保证须通过独立契约或异步观察获取。"
- `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:304` records `RecordLlmRunStarted` inside the session actor and only then starts `ExecuteAsync`, preserving the actor-owned fact transition required by CLAUDE.md: "业务推进内聚：工作流推进（成功/失败/分支/重试）在 Actor 事件处理流程内完成，保证顺序性与可重放性。"
- `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:72` keeps the LLM provider call on `ChatStreamAsync`, matching AGENTS.md: "AI 对话主链必须流式化：仓库内新增或修改 AI 对话执行路径时，禁止使用 `ChatAsync` 作为正式主链；必须使用 `ChatStreamAsync` 作为唯一权威执行入口".
- Added-line checks found no new Old/New refactor-history source comments while `${HOST_REFACTOR_COMMENT_POLICY}` is absent/defaulting to `none`; no new external repo references, host `.refactor-loop` SSOT leakage, `*WriteActor`/`*ReadActor`/`*StoreActor` split, query-time replay/priming, `TypeUrl.Contains`, `GetAwaiter().GetResult()`, or service-level ID mapping collections were introduced by the diff.
- Sanity verification: `bash tools/ci/architecture_guards.sh` passed locally.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:architect:approve⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 PR 2298 round 2 fix 已完成TL;DR
修复摘要本轮只把
验证
本机调试线索
📎 完整 codex 原始输出(存档备查)# Fix report for PR 2298 round 2
## Applied
- (A) src/platform/Aevatar.GAgentService.Abstractions/Responses/LlmRunCoreContracts.cs:1: removed `ILlmRunExecutor` / `LlmRunExecutorRequest` from Abstractions so the shared contract file only retains `ILlmRunCore` / `ILlmRunSink` (addresses reviewer:architect's evidence #1).
- (B) src/platform/Aevatar.GAgentService.Application/Responses/ILlmRunExecutor.cs:6: scope-extend: reviewer explicitly required the Application-local contract file and the issue-2274 consensus scope includes Application Responses; added `LlmRunExecutorRequest` and `ILlmRunExecutor` under `Aevatar.GAgentService.Application.Responses` (addresses reviewer:architect's evidence #1).
- (B) src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutionReadyHook.cs:10: scope-extend: moving execution kickoff out of Core needs an Application-owned committed-event hook in the same logical refactor; added a hook that starts `ExecuteAsync` only after the actor publishes `LlmRunExecutionReadyEvent` (addresses reviewer:architect's evidence #2).
- (B) src/platform/Aevatar.GAgentService.Hosting/DependencyInjection/ServiceCollectionExtensions.cs:87: scope-extend: the new Application hook must be registered for runtime behavior; wired `LlmRunExecutionReadyHook` as an `ICommittedStatePublicationHook` (addresses reviewer:architect's evidence #2).
- (A) src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:288: `HandleLlmRunRequestedAsync` / `HandleRecordRunStartedAsync` now only commit actor-owned run-start facts and scheduling; Core no longer resolves `ILlmRunExecutor` or fire-and-forgets execution from an actor handler (addresses reviewer:architect's evidence #2).
- (A) src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:354: added typed `LlmRunExecutionReadyEvent` publication after `LlmRunStartedEvent` and successful timeout scheduling, so execution starts after the actor-owned start setup is durable; subsequent executor output still returns through typed `RecordLlm*` commands via `IActorDispatchPort` (addresses reviewer:architect's evidence #2).
- (A) test/Aevatar.GAgentService.Tests/Application/MessagesCommandFacadeTests.cs:251: added off-actor `MessagesCommandFacade.StreamAsync` coverage asserting returned stream output, executor `StartAsync` admission, expected `LlmRunExecutorRequest`, no legacy `IActorDispatchPort.DispatchAsync` call, and no immediate `ExecuteAsync` (addresses reviewer:tests's evidence #1).
- (A) test/Aevatar.GAgentService.Tests/Application/ResponsesCommandFacadeTests.cs:525: added matching off-actor `ResponsesCommandFacade.StreamAsync` coverage with the same admission/request/no-legacy-dispatch assertions (addresses reviewer:tests's evidence #2).
## Rejected as false positive
- None.
## Blocked (cannot fix this round)
- None.
## Build status
- build: pass (`dotnet build aevatar.slnx --nologo`, warnings only).
- tests: pass (`dotnet test test/Aevatar.GAgentService.Tests/Aevatar.GAgentService.Tests.csproj --filter "FullyQualifiedName~LlmRunExecutorTests|FullyQualifiedName~MessagesCommandFacadeTests|FullyQualifiedName~ResponsesCommandFacadeTests|FullyQualifiedName~LlmSessionGAgentTests" --nologo`: 107 passed, 0 failed, 0 skipped).
- guard: pass (`bash tools/ci/test_stability_guards.sh`).
- diff check: pass (`git diff --check`).
- review-thread completion: not seeded (`/Users/chronoai/Code/aevatar/.refactor-loop/state/review-thread-completion/pr2298.json` absent).
- refactor self-doc: not applicable (`HOST_REFACTOR_COMMENT_POLICY=none`); no new `Refactor (...)`, `Old pattern`, or `New principle` source comments were added for this round.
## Recommendation for next round
- expect merge/approve: both reject demands were addressed, no false positives or blockers remain.
⟦AI:AUTO-LOOP⟧
FIX_DONE:2298:round-2:applied-8:rejected-0:blocked-0⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 Quality review r5：approve，无可读性阻塞TL;DR
质量复审结论
本轮没有发现需要阻塞合并的质量问题：
📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: quality
head_sha: 5cfec218e2b44c2cbd375dae02671ad642db13df
verdict: approve
---
## Verdict
Approve: the current diff is focused on moving LLM run execution out of the session actor turn, with readable names, reachable new surface, and no quality blocker.
## Evidence
- `src/platform/Aevatar.GAgentService.Application/Responses/ILlmRunExecutor.cs:6`: `LlmRunExecutorRequest` names the boundary around the concrete session actor, response, run, command, and origin platform rather than a generic transport bag.
- `src/platform/Aevatar.GAgentService.Application/Responses/ILlmRunExecutor.cs:13`: `ILlmRunExecutor` is not new dead public surface; it is registered in `ServiceCollectionExtensions.cs:86`, called by the three ingress facades through `StartAsync`, called by `LlmRunExecutionReadyHook.cs:36` through `ExecuteAsync`, and covered by production/test doubles.
- `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:17` and `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:53`: the admission and execution methods are short and keep the two phases explicit without adding nested control flow or oversized methods.
- `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutionReadyHook.cs:25`: the hook has a single responsibility, filters the committed ready event first, and delegates request construction to small private helpers rather than embedding the unpacking logic in `BeforePublishAsync`.
- `src/platform/Aevatar.GAgentService.Application/Responses/ChatCompletionsCommandFacade.cs:623`, `src/platform/Aevatar.GAgentService.Application/Responses/MessagesCommandFacade.cs:610`, and `src/platform/Aevatar.GAgentService.Application/Responses/ResponsesCommandFacade.cs:979`: the three off-actor adapters are near-parallel but small and typed to different plan models, so extraction would add indirection without reducing meaningful complexity.
- `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:332`: `TryCommitRunStartedAsync` keeps the two run-start handlers readable and below the review size/branch threshold.
- `HOST_REFACTOR_COMMENT_POLICY` is absent in the review environment, which normalizes to `none`; the added-line scan found no new `Refactor` / `Old pattern` / `New principle` source comments and no hardcoded host placeholder text such as `该项目` or `该项目AI`.
## What would change your verdict (only if comment or reject)
N/A.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:quality:approve⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 测试审查结论: commentTL;DR
测试质量证据
剩余可改进点是测试命名： 我也检查了稳定性规则：没有新增 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: tests
head_sha: 5cfec218e2b44c2cbd375dae02671ad642db13df
verdict: comment
---
## Verdict
Test coverage and stability are adequate for the diff, but several executor test names still describe `StartAsync` while exercising `ExecuteAsync`.
## Evidence
- `test/Aevatar.GAgentService.Tests/Application/LlmRunExecutorTests.cs:118` `StartAsync_ShouldDispatchForwardedToolCallRecordCommandsInOrder` calls `executor.ExecuteAsync(...)` and asserts off-turn forwarded-tool record dispatch; the behavior is covered, but the method name points at the wrong public method.
- `test/Aevatar.GAgentService.Tests/Application/LlmRunExecutorTests.cs:189` `StartAsync_ShouldDispatchLocalToolResultRecordCommand` also exercises `ExecuteAsync(...)` and asserts local tool result record dispatch, not `StartAsync` admission.
- `test/Aevatar.GAgentService.Tests/Application/LlmRunExecutorTests.cs:245`, `:273`, and `:300` name provider failure, cancellation, and sink-dispatch failure scenarios as `StartAsync_*`, but each test drives `ExecuteAsync(...)` and verifies terminal record commands from the execution loop.
- Positive coverage observed: `test/Aevatar.GAgentService.Tests/Application/LlmRunExecutorTests.cs:18` covers `StartAsync` admission and `RecordLlmRunStarted`; `test/Aevatar.GAgentService.Tests/Application/ChatCompletionsCommandFacadeTests.cs:414`, `test/Aevatar.GAgentService.Tests/Application/MessagesCommandFacadeTests.cs:97`, and `test/Aevatar.GAgentService.Tests/Application/ResponsesCommandFacadeTests.cs:916` cover flag-off legacy dispatch; their flag-on/create and flag-on/stream tests cover executor admission with no legacy run dispatch.
- `test/Aevatar.GAgentService.Tests/Core/LlmSessionGAgentTests.cs:801` covers `RecordLlmRunStarted` persistence without direct executor execution, and `test/Aevatar.GAgentService.Tests/Projection/LlmSessionsProtoSurfaceRegressionTests.cs:37` covers the added proto surface.
- Negative checks were clean: no added `[Skip]`, manual category, `Task.Delay`, `Thread.Sleep`, `WaitUntilAsync`, or polling allowlist change was found. `bash tools/ci/test_stability_guards.sh` passed, and focused `dotnet test test/Aevatar.GAgentService.Tests/Aevatar.GAgentService.Tests.csproj --nologo --filter "FullyQualifiedName~LlmRunExecutorTests|FullyQualifiedName~LlmSessionGAgentTests|FullyQualifiedName~ChatCompletionsCommandFacadeTests|FullyQualifiedName~MessagesCommandFacadeTests|FullyQualifiedName~ResponsesCommandFacadeTests|FullyQualifiedName~LlmSessionsProtoSurfaceRegressionTests"` passed 120 tests.
## What would change your verdict (only if comment or reject)
Rename the affected `LlmRunExecutorTests` methods so the behavior names match `ExecuteAsync` / off-turn record dispatch, for example `ExecuteAsync_ShouldDispatchForwardedToolCallRecordCommandsInOrder`.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:tests:comment⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 架构 Review：reject，off-turn 执行事实源不够持久TL;DR
架构阻塞点
对应的项目规则是：
需要改到什么程度建议把完整、强类型的 run execution request 持久化为 actor-owned fact，可以放在 committed run-start event 或 actor state 中，再让 📎 完整 codex 原始输出(存档备查)pr: 2298
|
Contributor
Author
🤖 架构复审通过TL;DR
复审证据本轮重点看了
📎 完整 codex 原始输出(存档备查)pr: 2298
|
Contributor
Author
🤖 架构审查：reject，阻塞点是 actor 内隐藏调度依赖TL;DR
架构阻塞点
这违反了 需要的修正很具体：不要在 actor 内做隐藏可选服务查找。如果 runtime 无法对 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: architect
head_sha: c209e75ec79a3bc112ef75a8ae7ac5a1eff330e7
verdict: reject
---
## Verdict
Reject: the new off-session execution path still hides its scheduler dependency behind a Service Locator inside `LlmSessionGAgent`, and the missing-service branch can leave a run started/ready without dispatching execution.
## Evidence
- `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:385`: `Services.GetService(typeof(ILlmRunExecutionScheduler))` locates the scheduler from the actor service bag, and `return` on absence silently skips execution after `LlmRunStartedEvent`/`LlmRunExecutionReadyEvent`. This violates `CLAUDE.md` Design pattern constraints: `不用 Service Locator 隐藏依赖——依赖一律构造注入、显式可见。` It also undermines the same PR's actor-owned run fact chain because the committed "ready" fact is not followed by a guaranteed dispatch or terminal failure when the scheduler is unavailable.
## What would change your verdict
Make the LLM run execution scheduler an explicit dependency/continuation instead of an optional service lookup from `LlmSessionGAgent`. If the actor runtime cannot constructor-inject this dependency, move execution scheduling to an explicit committed-event/continuation component or fail the run with a typed terminal fact when scheduling is unavailable; do not leave the actor with a hidden optional dependency and silent no-op.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:architect:reject⟦AI:AUTO-LOOP⟧ |
eanzhao
added
crnd:phase:fixing
Contributor
Author
🤖 PR 2298 round 6 fix-codex 已处理架构 rejectTL;DR
修复内容
本轮修改为:
验证结果:
quality reviewer 的 facade test helper 清理建议是 📎 完整 codex 原始输出(存档备查)# Fix report for PR 2298 round 6
## Applied
- (A) src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:26: replaced the hidden `Services.GetService(typeof(ILlmRunExecutionScheduler))` lookup with an explicit constructor-injected `ILlmRunExecutionScheduler` field, so `LlmSessionGAgent` cannot silently skip off-actor execution scheduling after committing `LlmRunStartedEvent` / `LlmRunExecutionReadyEvent` (addresses reviewer:architect's evidence #1; cited `CLAUDE.md` design-pattern clause: "不用 Service Locator 隐藏依赖——依赖一律构造注入、显式可见。").
- (A) src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:389: dispatches the transient execution command through the injected scheduler and retains the existing typed terminal failure event path when scheduling throws (addresses reviewer:architect's evidence #1).
- (A) test/Aevatar.GAgentService.Tests/TestSupport/GAgentServiceTestKit.cs:141: updated LlmSession test construction to supply the explicit scheduler dependency through the test kit, preserving existing inline run execution behavior without reintroducing a production service locator.
- (A) test/Aevatar.GAgentService.Tests/TestSupport/GAgentServiceTestKit.cs:161: made the inline test `ILlmRunCore` registration explicit for `LlmSessionGAgent` tests; tests that do not execute an LLM run no longer eagerly require `ILLMProviderFactory`, while tests that execute a run still fail clearly unless they register `ILLMProviderFactory` or `ILlmRunCore`.
- (A) test/Aevatar.GAgentService.Tests/Core/LlmSessionGAgentTests.cs:1898: updated the helper factories to delegate `LlmSessionGAgent` construction to `GAgentServiceTestKit`, keeping test construction aligned with the new explicit dependency.
## Rejected as false positive
- None.
## Blocked (cannot fix this round)
- None.
## Non-blocking reviewer comments
- reviewer:quality commented that facade-only `BlockingLlmRunExecutor` helpers carry unreachable `ExecuteAsync` / `ExecuteStarted` members. This was not a blocking demand because the quality verdict was `comment`, not `reject`, so it was not treated as mandatory for this fix round.
- refactor self-doc: not applicable (HOST_REFACTOR_COMMENT_POLICY=none). No new `Refactor (...)`, `Old pattern`, `New principle`, or iter/cluster source comments were added.
- review-thread completion: not applicable; `/Users/chronoai/Code/aevatar/.refactor-loop/state/review-thread-completion/pr2298.json` does not exist.
## Build status
- build: pass (`dotnet build aevatar.slnx --nologo`)
- tests: pass (`dotnet test test/Aevatar.GAgentService.Tests/Aevatar.GAgentService.Tests.csproj --nologo`; `dotnet test test/Aevatar.GAgentService.Integration.Tests/Aevatar.GAgentService.Integration.Tests.csproj --nologo --filter FullyQualifiedName~GAgentServiceHostingServiceCollectionExtensionsTests`; `bash tools/ci/test_stability_guards.sh`)
- tests: n=skipped for full solution test suite, per prompt instruction to pick changed test projects and not run the full solution test suite.
## Recommendation for next round
- expect unanimous; the only reject demand was addressed, with no false positives and no blocked items.
⟦AI:AUTO-LOOP⟧
FIX_DONE:2298:round-6:applied-5:rejected-0:blocked-0⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 PR 2298 round 6 fix 已收敛到可合并状态TL;DR
修复摘要
验证结果
本机调试线索
📎 完整 codex 原始输出(存档备查)Fix report for PR 2298 round 6Demand list
Applied
Rejected as false positive
Blocked (cannot fix this round)
Non-blocking comments read
Review-thread completion
Build status
Recommendation for next round
⟦AI:AUTO-LOOP⟧ ⟦AI:AUTO-LOOP⟧ |
eanzhao
added
crnd:phase:reviewing
Contributor
Author
📊 当前状态 — review-gate(不需要人介入)
下一步自动会做: 三位 reviewer 基于新 head 重新评审；若 何时需要人介入:
🤖 controller status banner ⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 测试评审结论：reject，缺少一个失败分支行为测试TL;DR
测试覆盖缺口
当前匹配测试只覆盖了成功路径： 需要新增的测试建议放在同一组附近：注册 session，注入一个 已确认的非问题：没有新增 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: tests
head_sha: adb5916eafbe86d566eee189594088b0d3572000
verdict: reject
---
## Verdict
Reject: the PR has good coverage for the happy off-actor path, but one net-new production failure branch that persists business state is untested.
## Evidence
- `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:382` adds `TryDispatchTransientExecutionCommandAsync`, whose catch branch persists `LlmRunFailed` with `FailureCode = "execution_dispatch_failed"` and `RecordId = $"{runId}:execution-dispatch-failed"`. I opened the matching tests and found only the success coverage in `test/Aevatar.GAgentService.Tests/Core/LlmSessionGAgentTests.cs:803` (`HandleRecordRunStartedAsync_ShouldPersistReadyFact_AndDispatchTransientExecutionCommand`); no test makes `ILlmRunExecutionScheduler.ScheduleAsync` throw and asserts the failure event/state.
- `test/Aevatar.GAgentService.Tests/Application/LlmRunExecutorTests.cs:65`, `:96`, and `:122` cover scheduler dispatch, execution actor mapping, and target provisioning with concrete payload/route/actor-id assertions, so the gap is not those new components.
- Static diff checks found no added `[Skip]`, `[Trait("Category","Manual")]`, `Task.Delay`, `Thread.Sleep`, or `WaitUntilAsync`; `bash tools/ci/test_stability_guards.sh` passed locally.
## What would change your verdict (only if comment or reject)
Add a behavior test near `HandleRecordRunStartedAsync_ShouldPersistReadyFact_AndDispatchTransientExecutionCommand` that registers a session, injects an `ILlmRunExecutionScheduler` test double whose `ScheduleAsync` throws, sends `RecordLlmRunStarted`, and asserts the actor persisted `LlmRunFailed` with `execution_dispatch_failed`, the expected record id/sequence, and failed session/run state.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:tests:reject⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 架构审查 r14：approveTL;DR
架构结论本轮只看架构合规，不覆盖测试质量和性能。 关键点:
📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: architect
head_sha: adb5916eafbe86d566eee189594088b0d3572000
verdict: approve
---
## Verdict
approve: no architecture compliance regressions found in the off-actor LLM run execution diff.
## Evidence
- `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:340` keeps run ownership in the session actor, persists `LlmRunStartedEvent`, then schedules execution through a typed scheduler; this supports CLAUDE.md "Actor 即业务实体：一个 actor = 一个业务实体（数据与方法同住）" and "读写分离：`Command -> Event`，`Query -> ReadModel`；异步完成通过事件通知，不在会话内拼装流程。"
- `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutionScheduler.cs:24` and `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutionScheduler.cs:47` use `ILlmRunExecutionTargetProvisioner` plus `IActorDispatchPort` to route `ExecuteLlmRunRequested`; this matches CLAUDE.md "Runtime 与 Dispatch 分责：`Runtime` 负责 lifecycle/topology/lookup，`Dispatch Port` 负责投递；禁止揉成全能接口。"
- `src/platform/Aevatar.GAgentService.Core/GAgents/LlmRunExecutionGAgent.cs:22` models the long-running LLM execution as a dedicated run-scoped actor handler, and `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:72` streams through `ILlmRunCore.RunAsync` while dispatching typed `RecordLlm*` commands back to the session actor; this aligns with AGENTS.md "默认优先 `run/session/task-scoped actor`" and "AI 对话主链必须流式化...必须使用 `ChatStreamAsync` 作为唯一权威执行入口".
- `src/platform/Aevatar.GAgentService.Abstractions/Protos/llm_sessions.proto:196` adds typed protobuf messages (`RecordLlmRunStarted`, `ExecuteLlmRunRequested`) for the new control flow, satisfying CLAUDE.md "核心语义强类型：影响业务语义、控制流、稳定读取且仓库内可控的数据，必须建模为 `proto field / typed option / typed sub-message`".
- Host-specific checks: `HOST_REFACTOR_COMMENT_POLICY` is empty, so it normalizes to `none`; the PR diff adds no new `Refactor` / `Old pattern` / `New principle` source comments. `HOST_ARCHITECTURE_GREP_CHECKS` and `HOST_PROTO_POLICY` are empty, so no additional host-specific checks apply.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:architect:approve⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 Quality review: commentTL;DR
质量发现
相关位置:
建议二选一: 删除 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: quality
head_sha: adb5916eafbe86d566eee189594088b0d3572000
verdict: comment
---
## Verdict
Comment: the refactor is readable and focused overall, but it leaves an unused origin-platform value on the new executor start contract.
## Evidence
- `src/platform/Aevatar.GAgentService.Application/Responses/ILlmRunExecutor.cs:7` adds `LlmRunExecutorRequest.OriginPlatform`, and all three facades populate it (`src/platform/Aevatar.GAgentService.Application/Responses/ChatCompletionsCommandFacade.cs:639`, `src/platform/Aevatar.GAgentService.Application/Responses/MessagesCommandFacade.cs:625`, `src/platform/Aevatar.GAgentService.Application/Responses/ResponsesCommandFacade.cs:995`), but the production `LlmRunExecutor.StartAsync` path at `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:17` never reads that field before dispatching `RecordLlmRunStarted`. The later execution request is built from `LlmSessionGAgent.ResolveExecutionOriginPlatform()` at `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:420`, so the new public parameter is dead surface and obscures which origin value is authoritative.
## What would change your verdict (only if comment or reject)
Either remove `OriginPlatform` from `LlmRunExecutorRequest` and the three facade builders, or carry it through the typed start command so it is the value used when constructing `ExecuteLlmRunRequested`.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:quality:comment⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 Quality review: commentTL;DR
质量发现
建议把这类 facade-test fake 抽到共享测试支持代码,只保留 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: quality
head_sha: adb5916eafbe86d566eee189594088b0d3572000
verdict: comment
---
## Verdict
Comment: the production refactor is readable and focused, but the new facade tests copy the same fake executor three times and include unreachable execution hooks.
## Evidence
- `test/Aevatar.GAgentService.Tests/Application/ChatCompletionsCommandFacadeTests.cs:1037`: `BlockingLlmRunExecutor` implements only `ILlmRunExecutor`, whose public surface is `StartAsync`; the added `ExecuteAsync` method at `:1066` is not reachable through the interface and only supports assertions against dead test helper state.
- `test/Aevatar.GAgentService.Tests/Application/MessagesCommandFacadeTests.cs:596`: the same `BlockingLlmRunExecutor` shape is copied here, including the unreachable `ExecuteAsync` at `:625`, so this crosses the checklist threshold for near-identical inline duplication.
- `test/Aevatar.GAgentService.Tests/Application/ResponsesCommandFacadeTests.cs:1252`: a third local `BlockingLlmRunExecutor` repeats the same fake-start logic, while `ExecuteAsync` at `:1287` and `ReleaseExecute` at `:1296` are not callable via `ILlmRunExecutor` in these facade tests.
## What would change your verdict
Extract a shared facade-test fake for `ILlmRunExecutor` (or place it in the existing test support area) and remove the unreachable `ExecuteAsync` / execute gate members from the copies unless the fake intentionally implements the execution-service contract used by the code under test.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:quality:comment⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 tests reviewer 结论：approveTL;DR
测试审查要点我检查了实际 diff 和对应测试文件，没有只靠实现摘要判断。新增测试不是简单 bump 行数：它们断言了 admission、envelope payload、actor id、correlation id、record id、proto surface、DI 注册和终态状态等可观察业务结果。 关键覆盖点如下：
未发现新增 验证结果： 📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: tests
head_sha: adb5916eafbe86d566eee189594088b0d3572000
verdict: approve
---
## Verdict
Test coverage and test quality are adequate for the off-actor LLM run executor diff.
## Evidence
- `test/Aevatar.GAgentService.Tests/Application/LlmRunExecutorTests.cs:19` covers `StartAsync` returning the start admission before `ExecuteAsync` emits stream/completion record commands, with concrete envelope, actor id, correlation id, record id, and payload assertions.
- `test/Aevatar.GAgentService.Tests/Application/LlmRunExecutorTests.cs:65`, `:96`, and `:122` cover the new scheduler, execution GAgent mapping, and deterministic execution-target provisioner behavior.
- `test/Aevatar.GAgentService.Tests/Core/LlmSessionGAgentTests.cs:803`, `:913`, and `:957` cover the session actor's new run-start/ready event path, timeout-before-execution ordering, and scheduler-failure terminal outcome.
- `test/Aevatar.GAgentService.Tests/Application/ChatCompletionsCommandFacadeTests.cs:414`, `test/Aevatar.GAgentService.Tests/Application/MessagesCommandFacadeTests.cs:97`, `:251`, and `test/Aevatar.GAgentService.Tests/Application/ResponsesCommandFacadeTests.cs:414` cover both legacy dispatch and off-actor executor-admission paths across Chat Completions, Messages, and Responses.
- `test/Aevatar.GAgentService.Tests/Projection/LlmSessionsProtoSurfaceRegressionTests.cs:37` covers the new proto surface and explicitly asserts that `LlmRunExecutionReadyEvent` does not carry the execution request payload.
- `test/Aevatar.GAgentService.Integration.Tests/GAgentServiceHostingServiceCollectionExtensionsTests.cs:87` covers DI registration for `LlmRunExecutionScheduler` and `ILlmRunExecutionTargetProvisioner`.
- No newly added `[Skip]`, manual-category test, `Task.Delay`, `Thread.Sleep`, or `WaitUntilAsync` was found in the PR test diff; `bash tools/ci/test_stability_guards.sh` passed.
- Verification run: `dotnet test test/Aevatar.GAgentService.Tests/Aevatar.GAgentService.Tests.csproj --nologo --no-restore` passed with 896 tests, 0 skipped; `dotnet test test/Aevatar.GAgentService.Integration.Tests/Aevatar.GAgentService.Integration.Tests.csproj --nologo --no-restore --filter FullyQualifiedName~GAgentServiceHostingServiceCollectionExtensionsTests` passed with 22 tests, 0 skipped.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:tests:approve⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 架构审查 r15：approveTL;DR
架构结论本轮只看架构合规，不覆盖测试质量和性能。 关键检查结果：
📎 完整 codex 原始输出(存档备查)---
pr: 2298
role: architect
head_sha: adb5916eafbe86d566eee189594088b0d3572000
verdict: approve
---
## Verdict
approve: no architecture compliance concerns remain in the current off-actor LLM run execution diff.
## Evidence
- `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:340` keeps run-start facts owned by the session actor, and `src/platform/Aevatar.GAgentService.Core/GAgents/LlmSessionGAgent.cs:372` commits only a token-free `LlmRunExecutionReadyEvent`; this matches CLAUDE.md `Actor 即业务实体：一个 actor = 一个业务实体（数据与方法同住）` and `核心语义强类型：影响业务语义、控制流、稳定读取且仓库内可控的数据，必须建模为 proto field / typed option / typed sub-message，禁止塞入通用 bag`.
- `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutionScheduler.cs:24` resolves the execution target through `ILlmRunExecutionTargetProvisioner`, while `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutionScheduler.cs:47` sends the typed `ExecuteLlmRunRequested` through `IActorDispatchPort`; this follows CLAUDE.md `Runtime 与 Dispatch 分责：Runtime 负责 lifecycle/topology/lookup，Dispatch Port 负责投递；禁止揉成全能接口`.
- `src/platform/Aevatar.GAgentService.Core/GAgents/LlmRunExecutionGAgent.cs:22` models the long LLM execution as a run-scoped actor command handler, and `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:72` executes the LLM core while `src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs:147` records each resulting fact back through dispatch; this aligns with AGENTS.md `默认优先 run/session/task-scoped actor` and avoids the prior callback-started execution shape.
- `src/platform/Aevatar.GAgentService.Abstractions/Protos/llm_sessions.proto:242` through `src/platform/Aevatar.GAgentService.Abstractions/Protos/llm_sessions.proto:259` adds typed protobuf contracts for ready/start/execute handoff without adding a generic metadata bag or committed bearer-token event payload.
- Host-policy checks: `HOST_REFACTOR_COMMENT_POLICY` is empty and normalizes to `none`; I found no newly added `Refactor` / `Old pattern` / `New principle` source comments in the PR diff. The diff also adds no `.refactor-loop/host.env` production facts, no external repo references, no `*WriteActor` / `*ReadActor` / `*Store` split, and no new `Task.Run`, `ChatAsync`, `GetAwaiter().GetResult()`, `TypeUrl.Contains(...)`, or service-level ID map in the changed production path.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2298:architect:approve⟦AI:AUTO-LOOP⟧ |
Contributor
Author
📊 当前状态 — review-gate consensus reached评审 r15/r13 已完成：architect=approve, tests=approve, quality=comment。PR head 🤖 controller status banner ⟦AI:AUTO-LOOP⟧ |
eanzhao
deleted the
refactor/2026-06-19_issue-2274-off-actor-llm-run-executor
branch
eanzhao
added
crnd:phase:merged
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adb5916eaf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+19
to
+20
| var actorId = BuildActorId(request.SessionActorId, request.RunId); | ||
| _ = await _runtime.CreateByKindAsync(LlmRunExecutionGAgent.Kind, actorId, ct).ConfigureAwait(false); |
There was a problem hiding this comment.
When this runs for every LLM run, it creates a unique execution actor keyed by session+run, but neither LlmRunExecutionGAgent nor the executor path destroys that actor after completed/failed/cancelled terminal handling. In long-lived deployments this leaves one idle persisted actor identity per response, so high-volume chat traffic can grow the runtime actor catalog without bound; please either reuse a bounded execution target or call the runtime destroy/purge path once the run reaches a terminal state.
Useful? React with 👍 / 👎.
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
2 participants
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
将 LLM run 执行从
LlmSessionGAgentactor turn 中移出，保留 actor 作为事实记录者。新的LlmRunExecutor在 actor turn 外驱动ILlmRunCore，通过 typedRecordLlm*commands 把 run start、chunk batch、tool、terminal、failure、cancel facts 写回 session actor。Scope
src/platform/Aevatar.GAgentService.Application/Responses/LlmRunExecutor.cs，并移除旧的DefaultLlmRunExecutor/ILlmRunExecutorsurface。LlmSessionGAgent、LlmRunCore、protobuf run recording contract 和 DI wiring。Verification
dotnet test test/Aevatar.GAgentService.Tests/Aevatar.GAgentService.Tests.csproj --filter FullyQualifiedName~ResponsesCommandFacadeTests --nologo：32 passed。dotnet test test/Aevatar.GAgentService.Tests/Aevatar.GAgentService.Tests.csproj --filter FullyQualifiedName~LlmRunExecutorTests --nologo：6 passed。dotnet test test/Aevatar.GAgentService.Tests/Aevatar.GAgentService.Tests.csproj --nologo：919 passed。bash tools/ci/test_stability_guards.sh：passed。bash tools/ci/architecture_guards.sh：passed。dotnet build aevatar.slnx --nologo：passed，warnings only。git diff --check：passed。Notes
wakeup-planread-only probe is currently blocked by Python 3.9 compatibility (zip(..., strict=True)); this PR path used the already completed worker output plus explicit controller verification instead.⟦AI:AUTO-LOOP⟧