fix(agent-loop): /goal replace + request_idle turn-end + intercept emit invariant#182
Merged
Merged
Conversation
…cept emit invariant
User-reported symptoms (macmini-03-64 session 080c24cf):
- "/goal <new>" 在已有 active goal 时静默失败 (TUI 假阳性 "Goal set: ...",
实际 goal.json mtime 不变)
- request_idle 后 TUI 显示 "Stale (turn ended after 2m11s)" 误判
- request_idle 后又发起一次 empty end_turn LLM call (与 CCProxy SSE preflight
bug 共谋触发 502 风暴)
Root causes & fixes:
1. /goal replace semantics
- session.create() 在 active goal 时返回 AlreadyExists (LLM tool 路径正确语义)
- 但 TUI ControlCommand::GoalCreate 复用 create()，user-initiated 路径同样
被 reject + 仅 warn log，不反馈给客户端
- 新增 GoalRuntimeSession::create_or_replace() (复用 create_internal 通过
replace_existing: bool)
- goal_control.rs 改 ControlCommand::GoalCreate 走 create_or_replace
- LLM tool create_goal 仍走 create() 保持 "Fails if exists" 契约
2. intercept handler emit ToolResult contract
- 4 个 intercept handler 中 3 个漏 emit ToolResult (request_idle / EnterPlanMode
/ ExitPlanMode), view-state invocation 卡 Running 直到 turn-end reconcile
才被强标 Stale
- 新增 complete_intercepted_tool() helper 收敛 "必须 emit + 返回 ContentBlock"
契约到单点 enforcement
- 4 个 handler 全部走 helper, 历史 false-emit bug 不可再现
3. request_idle 后跳过下次 LLM
- runner 新增 tool_signaled_turn_end: bool (pub(super), 由 signal/take method
封装保证 single-writer)
- handle_request_idle.signal_turn_end_after_tools() 通知 turn machine
- turn_exec::ToolResultsWritten 用 take_turn_end_signal() 消费, 决定 Complete
vs ReadyToCall
- run_loop turn 边界显式 take 防错误路径泄漏到下一 turn (invariant 守门)
E2e 测试覆盖 (7 new tests in 3 files):
- e2e_goal_replace_test.rs: replace 语义 + LLM 路径不变守门
- e2e_request_idle_terminates_turn_test.rs: emit + 不发后续 LLM + 信号不跨 turn
- e2e_intercept_emit_test.rs: EnterPlanMode emit invariant
…est boilerplate trim Code review pass on ad27631. Net -140 lines, no behavior change. * tool-name SSOT: extract `NAME` / `ENTER_PLAN_NAME` / `EXIT_PLAN_NAME` pub consts in the tool crates; intercept dispatcher + 3 handlers reference them instead of each redefining `const TOOL_NAME: &str = "..."` locally. * `complete_intercepted_tool` takes `content: impl Into<String>` instead of `&str`: callers can pass `String` / `&str` / `format!()` and the helper performs exactly one clone (event) + one move (block) — was two clones via `tool_result_block`. * e2e tests use `events::collect_until_idle` + a small local find-helper instead of hand-rolled `loop { timeout_at(deadline, rx.recv()).await ... }` pattern. Shrinks `e2e_request_idle_terminates_turn_test.rs` from 199 to 119 lines and `e2e_intercept_emit_test.rs` from 128 to 81 lines without losing assertions. * trim verbose module `//!` docs on the 3 new test files (test names + commit message already convey the intent; per CLAUDE.md "test names ARE the documentation"). * trim `// reason:` blocks that mixed why with what — keep the why (invariant, non-obvious decision), drop the what (visible from method names like `signal_turn_end_after_tools` / `take_turn_end_signal`).
…ol-result helpers Cleanup of two structural smells flagged in arch review on ad27631: * `tool_signaled_turn_end` migrates from `AgentLoopRunner` to `TurnContext`. Previously a runner field required defensive `take()` at every turn boundary to prevent leak on error paths. Now it lives on the turn-scoped struct and is dropped with the turn — invariant by lifetime, no defensive code. Signal flows through return values: handler -> `intercept_special_tools` -> `ToolExecStats.turn_end_signal` -> `execute_tool_phase` sets it on `turn_ctx` -> `turn_exec::ToolResultsWritten` consumes it. * Unify tool-result emit + block: new `emit_and_block(id, name, content, is_error, metadata) -> Result<ContentBlock>` collapses the `emit_tool_error / emit_tool_cancelled + tool_result_block` two-step pattern that appeared in 5 spots (tools_check_one, tools_check, tools_resolve). Delete obsolete `tools_check_emit.rs` (39 lines). `complete_intercepted_tool` becomes a thin wrapper that adds (idx, turn_end_signal). `tools_resolve.rs` previously emitted "Permission denied" generic to view-state but the specific tool-name version to the LLM — unified to specific so TUI now sees the full message too. Net -14 lines, behavior preserved (88/88 tests pass).
…passthrough Second simplify pass on top of 8079c0a. Apply review findings: * turn-end signal 5-hop → 1-hop: pass `&mut TurnContext` directly into the intercept chain (`execute_tools` → `intercept_special_tools` → handlers). `handle_request_idle` calls `turn_ctx.signal_turn_end_after_tools()` directly; the bool no longer threads through `(Intercepted, Remaining, bool)` tuple, `ToolExecStats.turn_end_signal`, `execute_tool_phase` bubble, or `complete_intercepted_tool`'s 7th param. Net: 3 fields/params deleted, layers of plumbing removed. * `emit_all_interrupted` migrates the last hand-rolled `emit + tool_result_block` pair onto `emit_and_block`. Now every "tool result writeback" goes through the single helper. * AskUser stringly-typed match arm: add `pub const NAME: &str = "AskUser"` in `loopal-tool-ask-user`; intercept dispatcher imports + uses it, matching the pattern already established for the other 3 intercept tools. * Cleanup: `tools_inject.rs` module summary (CLAUDE.md HARD RULE), verbose `// reason:` blocks where the code is self-evident, test module docs shrunk to one line each, `find_tool_result_full` promoted from a local test helper into shared `loopal_test_support::assertions`. Net -74 lines across 26 files. 88/88 tests pass.
Third simplify pass. Net -56 lines across 9 files. 88/88 pass.
* Delete `complete_intercepted_tool` wrapper. 15 callsites inline to
`Ok((idx, self.emit_and_block(...).await?))`. One less indirection;
callers' intent (emit + tuple-wrap idx) is now visible at the call site.
* Delete single-use type aliases `Intercepted` / `Remaining` in
`tools_intercept.rs`. Tuple shape inlined into the function signature.
* Trim `emit_and_block` doc from 4-line history to 2-line why (single
point of writeback). Trim `tool_signaled_turn_end` field doc to one
line ("唯一 setter").
* `find_tool_result` now expressed as
`find_tool_result_full(events, name).map(|(_, c)| c)`. 8 lines → 3.
* `"main"` magic string in `e2e_request_idle_terminates_turn_test.rs`
replaced by `loopal_session::ROOT_AGENT` const.
…orts 7 test files had stale imports after the prior batch substitution: - 5 files imported `make_cancel` but switched all callsites to `make_turn_ctx` - 2 files imported `make_turn_ctx` but never used it (still on `make_cancel`) CI lint failed with `unused-imports` (clippy `-D warnings`).
Apply rustfmt across files touched by the simplify passes: - handler files (handle_request_idle, tools_plan, tools_plan_exit, tools_ask_user, tools, tools_inject) — `Ok((idx, ...))` patterns reformatted onto multiple lines per rustfmt rules - test files (5 agent_loop tests + e2e_intercept_emit_test) — same - suite.rs — `mod` declarations alphabetized within E2E section
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
修复用户在 macmini-03-64 session
080c24cf上报告的 3 个关联问题，并通过 4 轮 simplify 把生产代码瘦身。<new>在已有 active goal 上静默失败 → user-initiated path 走新的create_or_replace；LLM tool path 仍 fail-on-exists 保持 description 契约emit_and_blockhelper 把 contract 收敛到单点 enforcementTurnContext::tool_signaled_turn_endflag 让 turn 在ToolResultsWritten后直接 CompleteKey changes
loopal-runtime/goal/session.rscreate_or_replace+ 共享create_internal(replace_existing: bool)loopal-runtime/agent_loop/tools_inject.rsemit_and_blockSSOT — 5 处emit + tool_result_block模式合并到单点；删complete_intercepted_toolwrapperloopal-runtime/agent_loop/turn_context.rstool_signaled_turn_endfield + signal/take method (turn-scoped)loopal-runtime/agent_loop/tools_intercept.rs&mut TurnContext直接喂给 handler (1-hop signal)loopal-runtime/agent_loop/handle_request_idle.rs+tools_plan{,_exit}.rs+tools_ask_user.rsemit_tool_error/cancelled(dead 39 行)tools/agent/{ask-user,idle,plan-mode}/src/lib.rspub const NAME / ENTER_PLAN_NAME / EXIT_PLAN_NAMESSOTloopal-test-support/assertions.rsfind_tool_result_full共享，find_tool_result改为 thin wrapperloopal-tui/tests/suite/e2e_*(3 new)e2e_goal_replace_test(3) +e2e_request_idle_terminates_turn_test(3) +e2e_intercept_emit_test(2) = 8 个新回归用例5 个 commit；vs main 净 +547/-301（含 ~250 行新 e2e 测试），生产代码净瘦身。
Test plan
bazel test //...88/88 全过（本地）~/.local/bin/loopal, 0.4.0-47-gba13450e-dirty-dev)080c24cf，验证 (1) /goal 替换、(2) TUI request_idle 显示 "Done" 非 "Stale"、(3) request_idle 后无多余 LLM 调用