fix(experts): resolve residual review findings from PR #13 #14

fischer · 2026-07-01T13:27:48+08:00

fischer commented

2026-07-01 13:27:48 +08:00

Summary

Addresses 4 actionable findings (1 P1 + 3 P2) from the ce-code-review of feat/ui-ue-enhancement (PR #13, merged to main at 8066e0b).

Residuals doc: docs/residual-review-findings/feat-ui-ue-enhancement.md

P1 — expert_step payload alignment (`_phase_executor.py`)

The thinking/tool_call/tool_result event payloads were missing the fields the frontend WsServerMessage contract requires (expert_name/expert_color/content/step). Now all expert_step broadcasts carry the full contract; tool_call/tool_result keep step_data for the raw payload.

P2 #1 — execute_stream CancellationToken registration (`config_driven.py`)

execute_stream() bypassed BaseAgent.execute() and never registered a CancellationToken, so cancel_task() could not cooperatively cancel a streaming task. Now registers the token and cleans up in finally.

P2 #2 — team_synthesis orphan milestone cleanup (`orchestrator.py`)

If synthesis streaming was interrupted (cancel/exception), no terminal team_synthesis event was emitted, leaving the frontend streaming milestone spinning forever. Now an inner try/except emits a terminal team_synthesis with status=cancelled|error before re-raising. Success path also carries the synthesis_id.

P2 #3 — synthesis_id dedup (`orchestrator.py` + `types.ts` + `chatStream.ts`)

Without an identifier the frontend could not precisely match a team_synthesis terminal event to its streaming milestone. Backend now injects a stable synthesis_id ({plan.id}:synthesis) into both team_synthesis_chunk and team_synthesis events; frontend uses it for exact milestone matching and treats error/cancelled status as terminal.

Test updates

Updated test_thinking_events_forwarded_as_expert_step to assert the new payload contract (expert_id/name/color/content/step).
Added test_tool_call_events_forwarded_as_expert_step covering tool_call/tool_result payload shape (content=tool_name summary + step_data=raw payload).

Test plan

ruff check src/agentkit/experts/_phase_executor.py src/agentkit/core/config_driven.py src/agentkit/experts/orchestrator.py → All checks passed
pytest tests/unit/experts/test_phase_executor_streaming.py -x -q → 14/14 passed
npm run typecheck → clean
npx vitest run → 126/127 passed (1 unrelated baseline failure in tauri-auth.test.ts)

Post-deploy monitoring

WebSocket expert_step event payload shape — verify frontend step indicators render with name/color/content correctly
team_synthesis / team_synthesis_chunk events — verify no orphan streaming milestones remain after cancelled/error synthesis
cancel_task() on streaming team tasks — verify cooperative cancellation now actually stops the streaming task

## Summary Addresses **4 actionable findings (1 P1 + 3 P2)** from the ce-code-review of feat/ui-ue-enhancement (PR #13, merged to main at 8066e0b). Residuals doc: `docs/residual-review-findings/feat-ui-ue-enhancement.md` ### P1 — expert_step payload alignment (`_phase_executor.py`) The thinking/tool_call/tool_result event payloads were missing the fields the frontend `WsServerMessage` contract requires (`expert_name`/`expert_color`/`content`/`step`). Now all `expert_step` broadcasts carry the full contract; tool_call/tool_result keep `step_data` for the raw payload. ### P2 #1 — execute_stream CancellationToken registration (`config_driven.py`) `execute_stream()` bypassed `BaseAgent.execute()` and never registered a `CancellationToken`, so `cancel_task()` could not cooperatively cancel a streaming task. Now registers the token and cleans up in `finally`. ### P2 #2 — team_synthesis orphan milestone cleanup (`orchestrator.py`) If synthesis streaming was interrupted (cancel/exception), no terminal `team_synthesis` event was emitted, leaving the frontend streaming milestone spinning forever. Now an inner `try/except` emits a terminal `team_synthesis` with `status=cancelled|error` before re-raising. Success path also carries the `synthesis_id`. ### P2 #3 — synthesis_id dedup (`orchestrator.py` + `types.ts` + `chatStream.ts`) Without an identifier the frontend could not precisely match a `team_synthesis` terminal event to its streaming milestone. Backend now injects a stable `synthesis_id` (`{plan.id}:synthesis`) into both `team_synthesis_chunk` and `team_synthesis` events; frontend uses it for exact milestone matching and treats `error`/`cancelled` status as terminal. ## Test updates - Updated `test_thinking_events_forwarded_as_expert_step` to assert the new payload contract (expert_id/name/color/content/step). - Added `test_tool_call_events_forwarded_as_expert_step` covering tool_call/tool_result payload shape (content=tool_name summary + step_data=raw payload). ## Test plan - `ruff check src/agentkit/experts/_phase_executor.py src/agentkit/core/config_driven.py src/agentkit/experts/orchestrator.py` → All checks passed - `pytest tests/unit/experts/test_phase_executor_streaming.py -x -q` → 14/14 passed - `npm run typecheck` → clean - `npx vitest run` → 126/127 passed (1 unrelated baseline failure in `tauri-auth.test.ts`) ## Post-deploy monitoring - WebSocket `expert_step` event payload shape — verify frontend step indicators render with name/color/content correctly - `team_synthesis` / `team_synthesis_chunk` events — verify no orphan streaming milestones remain after cancelled/error synthesis - `cancel_task()` on streaming team tasks — verify cooperative cancellation now actually stops the streaming task

fischer added 1 commit 2026-07-01 13:27:48 +08:00

Test / backend-test (pull_request) Has been cancelled Details

Test / frontend-unit (pull_request) Has been cancelled Details

Test / api-e2e (pull_request) Has been cancelled Details

Test / frontend-e2e (pull_request) Has been cancelled Details

47a437c5e3 fix(experts): resolve residual review findings from PR #13

Addresses 4 actionable findings (1 P1 + 3 P2) from ce-code-review of
feat/ui-ue-enhancement (PR #13), now merged to main (8066e0b).

P1 — expert_step payload alignment (_phase_executor.py)
  The thinking/tool_call/tool_result event payloads were missing the
  fields the frontend WsServerMessage contract requires
  (expert_name/expert_color/content/step). Frontend code consuming these
  events silently degraded. Now all expert_step broadcasts carry the
  full contract; tool_call/tool_result keep step_data for the raw payload.

P2 #1 — execute_stream CancellationToken registration (config_driven.py)
  execute_stream() bypassed BaseAgent.execute() and never registered a
  CancellationToken, so cancel_task() could not cooperatively cancel a
  streaming task. Now registers the token and cleans it up in finally.

P2 #2 — team_synthesis orphan milestone cleanup (orchestrator.py)
  If synthesis streaming was interrupted (cancel/exception), no terminal
  team_synthesis event was emitted, leaving the frontend streaming
  milestone spinning forever. Now an inner try/except emits a terminal
  team_synthesis with status=cancelled|error before re-raising, so the
  frontend can finalize the milestone. The success path also carries
  the synthesis_id.

P2 #3 — synthesis_id dedup (orchestrator.py + types.ts + chatStream.ts)
  Without an identifier, the frontend could not precisely match a
  team_synthesis terminal event to its streaming milestone (especially
  across retries/concurrent teams). The backend now injects a stable
  synthesis_id (`{plan.id}:synthesis`) into both team_synthesis_chunk
  and team_synthesis events; the frontend uses it for exact milestone
  matching and treats error/cancelled status as terminal.

Test updates
  - Updated test_thinking_events_forwarded_as_expert_step to assert the
    new payload contract (expert_id/name/color/content/step).
  - Added test_tool_call_events_forwarded_as_expert_step covering
    tool_call/tool_result payload shape (content=tool_name摘要 +
    step_data=原始 payload).

Verification
  - ruff check: clean
  - pytest tests/unit/experts/test_phase_executor_streaming.py: 14/14
  - npm run typecheck: clean
  - vitest: 126/127 (1 unrelated baseline failure in tauri-auth.test.ts)

Residuals doc: docs/residual-review-findings/feat-ui-ue-enhancement.md