Skip to content

feat: add PostTurn hook event for turn-level post-processing#3266

Closed
zhangxy-zju wants to merge 8 commits intomainfrom
feat/post-turn-hook
Closed

feat: add PostTurn hook event for turn-level post-processing#3266
zhangxy-zju wants to merge 8 commits intomainfrom
feat/post-turn-hook

Conversation

@zhangxy-zju
Copy link
Copy Markdown
Collaborator

@zhangxy-zju zhangxy-zju commented Apr 14, 2026

Summary

Add a new PostTurn hook event that fires at every model turn boundary (tool_call or end of response). Hook scripts receive the turn's accumulated thoughts, messages, and tool calls, and can optionally inject a message into the ACP stream with custom metadata.

Closes #3265

Changes (10 files, +319 lines)

Core hook framework (packages/core):

  • types.ts: New PostTurn enum, PostTurnInput, PostTurnOutput, PostTurnHookOutput class with getAcpMessage() / getAcpMeta()
  • hookEventHandler.ts: firePostTurnEvent()
  • hookSystem.ts: Public firePostTurnEvent()
  • hookAggregator.ts: PostTurn fire-and-forget handling (preserve outputs, ignore errors, first non-empty acpMessage wins)
  • toolHookTriggers.ts: firePostTurnHook() via MessageBus
  • config.ts: PostTurn case in MessageBus hook dispatcher

ACP integration (packages/cli):

  • Session.ts: Accumulate turn content during streaming, fire-and-forget PostTurn hook at every turn boundary, pass through acpMessage + acpMeta to ACP stream
  • settingsSchema.ts: PostTurn in hooks schema
  • hooks/constants.ts: PostTurn descriptions and exit codes

Hook Protocol

Input (stdin JSON):

{
  "turn_index": 3,
  "thoughts": ["..."],
  "messages": ["..."],
  "tool_calls": [{ "name": "read_file", "args": { "file_path": "..." } }]
}

Output (stdout JSON):

{
  "decision": "allow",
  "hookSpecificOutput": {
    "hookEventName": "PostTurn",
    "acpMessage": "Message to inject into ACP stream",
    "acpMeta": { "custom": "metadata passed through as _meta" }
  }
}

Configuration

{
  "hooks": {
    "PostTurn": [{
      "hooks": [{
        "type": "command",
        "command": "python3 .qwen/hooks/my_script.py",
        "timeout": 30000
      }]
    }]
  }
}

Design Decisions

  • Async fire-and-forget: Does not block agent execution
  • ACP-only: Fires in Session path, not in non-interactive CLI mode
  • Framework-agnostic metadata: acpMeta defined by hook scripts, framework passes through as-is
  • Single hook limit: Multiple PostTurn hooks → first non-empty acpMessage wins
  • Safe degradation: Hook failures silently ignored, original messages unaffected
  • Backward compatible: Old qwen-code versions ignore unknown PostTurn hook config with zero side effects

Test plan

  • TypeScript compilation passes
  • ESLint passes
  • Hook script unit test (stdin/stdout JSON protocol)
  • End-to-end ACP eval with PostTurn hook (in progress)

New PostTurn hook event fires at each model turn boundary (tool_call
or end of response). Hook receives accumulated thoughts + messages,
and can return an acpMessage to inject into the ACP stream with
_meta.rewritten: true.

Core changes:
- types.ts: PostTurn enum, PostTurnInput, PostTurnHookOutput with
  getAcpMessage() method
- hookEventHandler.ts: firePostTurnEvent()
- hookSystem.ts: public firePostTurnEvent()
- hookAggregator.ts: PostTurn fire-and-forget (preserve outputs,
  ignore errors)
- toolHookTriggers.ts: firePostTurnHook() via MessageBus

ACP integration:
- Session.ts: accumulate turn content during streaming, fire-and-forget
  PostTurn hook at turn boundaries, inject acpMessage via sendUpdate
- previous_rewrites → previous_hook_outputs (generic)
- rewriteHistory → hookOutputHistory (generic)
- _meta.rewritten → _meta.postTurnHook (framework-level marker)
…ough

- Add acpMeta field to PostTurn hook output (hookSpecificOutput.acpMeta)
- PostTurnHookOutput.getAcpMeta() extracts it
- Session.ts passes acpMeta as-is to ACP message _meta
- Remove hardcoded _meta.postTurnHook — hook scripts decide metadata
PostTurnInput should only carry current turn data, not track hook
output history. Scripts that need context across turns should manage
their own state (e.g. temp files). This keeps the hook interface
generic and avoids coupling ACP message injection semantics into
the core hook protocol.
Replace boolean has_tool_calls with tool_calls array containing
name and args for each function call in the turn. This gives hook
scripts full visibility into what tools were invoked, enabling
richer processing (e.g. filtering by tool type, logging tool usage).
P1: Fire PostTurn hook at every turn boundary, not just when
    thoughts/messages are present. Pure tool_call turns now trigger.
P1: Fix protocol field inconsistency — docs updated from
    has_tool_calls to tool_calls array (matching implementation).
P2: Add PostTurn to settings schema and UI hook constants
    (description, exit codes, detail text).
P2: Multi-hook aggregation: first non-empty acpMessage wins,
    explicitly documented in hookAggregator.
@github-actions
Copy link
Copy Markdown
Contributor

📋 Review Summary

This PR introduces a new PostTurn hook event that fires at every model turn boundary (after tool calls or end of response), allowing hook scripts to inject messages into the ACP stream with custom metadata. The implementation is well-architected with proper fire-and-forget semantics, backward compatibility, and consistent integration with the existing hook framework. Overall, this is a solid addition that extends the hook system's capabilities without disrupting existing functionality.

🔍 General Feedback

  • Strong architectural consistency: The PostTurn hook follows the same patterns as existing hooks (PreToolUse, PostToolUse, etc.), making it easy to understand and maintain.
  • Fire-and-forget design: The async, non-blocking approach with Promise.allSettled is appropriate for this use case and prevents hook failures from affecting agent execution.
  • ACP-focused integration: The hook only fires in the Session path (not non-interactive CLI mode), which aligns with its purpose of injecting messages into the ACP stream.
  • Clean separation of concerns: Core hook logic is in packages/core, while ACP-specific integration is in packages/cli.
  • Comprehensive type safety: New types (PostTurnInput, PostTurnOutput, PostTurnHookOutput) are well-defined with proper validation.

🎯 Specific Feedback

🟡 High

  1. File: packages/cli/src/acp-integration/session/Session.ts:343-347 - The accumulation logic for turnThoughts and turnMessages may have a bug. The condition if (part.thought) checks if the boolean flag is set, but then pushes part.text to turnThoughts. However, the else branch pushes to turnMessages for all non-thought parts, which could include non-text content. Consider adding explicit type checking:

    if (part.thought && part.text) {
      turnThoughts.push(part.text);
    } else if (part.text) {
      turnMessages.push(part.text);
    }
  2. File: packages/core/src/hooks/hookAggregator.ts:58-83 - The PostTurn handling uses firstAcpOutput logic where "first non-empty acpMessage wins". However, the iteration order of results is not guaranteed to be deterministic if hooks run in parallel. If hook execution order matters, this should be documented or the implementation should preserve sequential execution order when the sequential flag is set.

🟢 Medium

  1. File: packages/core/src/hooks/types.ts:385 - The getAcpMessage() method has a validation check msg.trim().length < 5 which silently returns undefined for messages shorter than 5 characters. This magic number should be either:

    • Documented with a comment explaining why 5 characters is the threshold
    • Made configurable
    • Changed to a more sensible minimum (e.g., 1 character)
    if (!msg || typeof msg !== 'string' || msg.trim().length < 5) {
      return undefined; // Why 5 characters?
    }
  2. File: packages/cli/src/acp-integration/session/Session.ts:416-447 - The PostTurn hook firing creates a new async function that captures several variables. While this works, the pattern of pushing promises to pendingPostTurnHooks and awaiting them all at the end could be extracted into a helper method for better testability and readability.

  3. File: packages/core/src/config/config.ts:959-972 - The PostTurn case in the MessageBus hook dispatcher uses inline type assertions for the input. Consider creating a type guard or validation function to ensure the input shape matches PostTurnInput before passing to firePostTurnEvent.

🔵 Low

  1. File: packages/core/src/hooks/types.ts:49 - The HookEventName.PostTurn enum value should have a JSDoc comment consistent with other enum values (e.g., StopFailure, PreCompact).

  2. File: packages/cli/src/ui/components/hooks/constants.ts:97-105 - The exit code descriptions for PostTurn could be more specific about what "Other" means in practice. Consider: { code: 'Other', description: t('hook failed or returned no acpMessage, stderr shown to user') }

  3. File: packages/core/src/core/toolHookTriggers.ts:507 - The firePostTurnHook function has a catch block that returns {} on error. While this is the intended fire-and-forget behavior, adding a debug log call (like other hook triggers have) would help with troubleshooting:

    debugLogger.warn(
      `PostTurn hook error: ${error instanceof Error ? error.message : String(error)}`,
    );

    (Note: I see this is actually present at line 553-556 in the diff - this is already implemented correctly)

  4. File: packages/core/src/index.ts:276-281 - The export comment says "Export hook triggers for notification and PostTurn hooks" but only exports fireNotificationHook, firePermissionRequestHook, and firePostTurnHook. Consider updating the comment to be more accurate or adding any missing exports.

✅ Highlights

  • Excellent backward compatibility design: Old qwen-code versions will ignore unknown PostTurn hook config with zero side effects, as noted in the PR description.
  • Robust error handling: Hook failures are silently ignored, ensuring the original agent flow is never disrupted.
  • Well-documented hook protocol: The stdin/stdout JSON protocol is clearly defined in the PR body with examples.
  • Comprehensive test coverage: The PR includes unit tests for the hook script JSON protocol.
  • Smart ACP message injection: The acpMeta field allows hook scripts to attach arbitrary metadata that's passed through as _meta in the ACP stream, enabling rich extensibility.

@tanzhenxin
Copy link
Copy Markdown
Collaborator

Hey @zhangxy-zju Thanks for the PR!

Quick note — the existing hook framework already has PostToolUse (fires after each tool call) and Stop (fires at end of response), which together cover the same trigger points as PostTurn. The new parts here are the turn-level content aggregation and ACP message injection, but it's worth discussing whether those should be a new event or extensions to the existing ones.

Assigning @DennisYu07 to review since he's been leading the hook system work (#2827, #3248) and can best assess how this fits the architecture.

@DennisYu07
Copy link
Copy Markdown
Collaborator

Hey @zhangxy-zju Thanks for the PR!

Quick note — the existing hook framework already has PostToolUse (fires after each tool call) and Stop (fires at end of response), which together cover the same trigger points as PostTurn. The new parts here are the turn-level content aggregation and ACP message injection, but it's worth discussing whether those should be a new event or extensions to the existing ones.

Assigning @DennisYu07 to review since he's been leading the hook system work (#2827, #3248) and can best assess how this fits the architecture.

Agree with this, personally I think it is a little redundant.

zhangxy-zju added a commit that referenced this pull request Apr 15, 2026
Explain the feature purpose (business-oriented output customization),
mark it as a temporary solution, and reference the hook-based
alternative (#3266) for future discussion.
yiliang114 pushed a commit that referenced this pull request Apr 15, 2026
…3191)

* feat(acp): LLM-based message rewrite middleware

Add MessageRewriteMiddleware that intercepts ACP messages and appends
LLM-rewritten versions with _meta.rewritten=true at turn boundaries.

Original messages pass through unmodified. At the end of each turn
(before tool calls or at response end), accumulated thought/message
chunks are sent to LLM for rewriting into business-friendly text.

- TurnBuffer: accumulates chunks per turn
- LlmRewriter: calls LLM with configurable prompt
- MessageRewriteMiddleware: orchestrates intercept → buffer → rewrite → emit
- BaseEmitter.sendUpdate: routes through middleware when configured
- Session: initializes middleware from settings.messageRewrite config

Enable via settings.json:
{
  "messageRewrite": {
    "enabled": true,
    "target": "both",
    "prompt": "custom system prompt for rewriter"
  }
}

Rewritten messages carry _meta.rewritten=true for frontend to
prioritize display. Original messages remain for debugging.

* fix: TypeScript 编译错误修复 + 优化默认改写 prompt(参考竞品风格)

* fix: 从 user/workspace originalSettings 读取 messageRewrite 配置(绕过 schema 校验)

* feat: 非交互 CLI 模式也支持 message rewrite(eval 可用)

* fix: 禁用 rewriter LLM 的 thinking,过滤 thought 部分只取纯文本输出

* fix: cron 路径补齐 message rewrite flush + 代码质量优化

- Session.ts cron 路径添加 messageRewriter.flushTurn() 调用
- nonInteractiveCli.ts cron 路径添加 turnBuffer 累积 + flush + rewrite
- 提取 loadRewriteConfig() 共享函数,消除两处重复配置读取
- 主路径和 cron 路径添加 turnBuffer.markToolCall()
- rewrite 调用添加 30s 超时保护(AbortSignal.timeout)
- 修复 import 语句被 const 声明分割的问题

* feat: rewrite 支持 async/sync 模式(默认 async,不增加执行时间)

* feat: rewrite prompt 通用化 + 上下文连贯 + promptFile + async 修复

- 默认 prompt 改为通用英文版(适配任意 coding agent,不绑定数据分析场景)
- 支持 promptFile 配置项,从文件加载自定义 prompt(优先于 inline prompt)
- 上下文连贯性:lastOutput 记录上一轮改写结果,拼接到下一轮输入,
  避免连续 turn 间信息重复
- 修复 CLI 非交互模式 async rewrite 丢失:void doRewrite() 改为
  pendingRewrites 数组 + emitResult 前 Promise.allSettled
- 增加 debug logging:REWRITE INPUT/OUTPUT 完整内容 + prev_output 长度

* refactor: remove sync rewrite mode, always use async (non-blocking) rewrite

- Remove `async` field from MessageRewriteConfig
- MessageRewriteMiddleware.flushTurn() always fires in background
- nonInteractiveCli.ts main & cron paths always push to pendingRewrites
- No user-facing latency from rewrite calls

* fix: address review feedback — trust check, timeout, history replay

1. loadRewriteConfig: skip workspace settings when !isTrusted, preventing
   untrusted repos from enabling rewriter with a custom prompt
2. MessageRewriteMiddleware.flushTurn: always enforce 30s timeout internally,
   even when caller provides no AbortSignal (interactive path)
3. Install rewriter AFTER history replay completes (Session.installRewriter),
   so historical messages are never rewritten on session load

* fix: address second round review — target filter, timeout, rewrite queue

1. nonInteractiveCli: apply rewriteConfig.target filter to accumulation
   (main path and cron path), matching MessageRewriteMiddleware behavior
2. nonInteractiveCli: add 30s AbortSignal.timeout to rewrite calls in
   both main and cron paths
3. MessageRewriteMiddleware: replace single pendingRewrite slot with
   pendingRewrites array + Promise.allSettled, ensuring all rewrites
   complete before session exits

* test: add unit tests for TurnBuffer, loadRewriteConfig, MessageRewriteMiddleware

- TurnBuffer: flush, reset, isEmpty, markToolCall, whitespace filtering (12 tests)
- loadRewriteConfig: isTrusted gating, workspace/user precedence (5 tests)
- MessageRewriteMiddleware: target filtering, tool_call boundary flush,
  pendingRewrites queue, rewrite metadata (9 tests)

* fix: config.test.ts use unknown cast for LoadedSettings stub (fix tsc --build)

* fix: filter LLM literal "empty string" responses in rewriter output

LLM sometimes outputs "(空字符串)" or similar text instead of actual
empty string when instructed to "return empty string". Add regex patterns
to catch common variants and treat them as null (skip rewrite output).

* revert: remove LLM empty-string pattern defense, rely on prompt fix instead

* fix: prevent async rewrite from corrupting adapter state + honor config.model

1. nonInteractiveCli: rewrite promises now return data only, adapter
   emission happens synchronously via emitSettledRewrites() at safe
   boundaries (before next turn starts, before cron next turn, before
   final result). Prevents concurrent startAssistantMessage corruption.
2. LlmRewriter: use rewriteConfig.model when set, fallback to
   config.getModel(). Previously model field was defined but ignored.

* docs: add messageRewrite configuration guide to settings.md

* Revert "docs: add messageRewrite configuration guide to settings.md"

This reverts commit ecd57e2.

* feat: add contextTurns config for rewrite history context

Allow configuring how many previous rewrite outputs are included as
context when rewriting a new turn:
- contextTurns: 1 (default) = last rewrite only
- contextTurns: 0 = no context
- contextTurns: N = last N rewrites
- contextTurns: "all" = all previous rewrites

* refactor: rename target 'both' to 'all' + add LlmRewriter unit tests

- Rename target value 'both' → 'all' for future extensibility (e.g. 'tool')
- Add LlmRewriter tests: contextTurns (0/1/N/all), model override, filtering
- Total: 35 tests across 4 test files

* refactor: remove message rewrite from non-interactive CLI mode

Non-interactive mode (qwen -p "..." --output-format json) consumers are
scripts/programs that don't need user-friendly rewrites. Additionally,
the JSON output adapter doesn't support _meta fields, so rewritten text
was silently mixed into normal assistant messages without any marker.

Rewrite middleware is now ACP-only (Session path).

* revert: restore package-lock.json and nonInteractiveCli.ts to main state

* docs: add README for message rewrite middleware

Explain the feature purpose (business-oriented output customization),
mark it as a temporary solution, and reference the hook-based
alternative (#3266) for future discussion.

* docs: move temporary-solution notice to top of README

* docs: simplify temporary-solution notice in rewrite README
@zhangxy-zju
Copy link
Copy Markdown
Collaborator Author

Closing for now — turn-level post-processing is currently supported via the ACP message rewrite middleware (#3191) as a temporary solution. We can revisit the hook-based approach when the middleware no longer meets our needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: add PostTurn hook event for turn-level post-processing

3 participants