feat(hooks): add before_response_emit hook for output policies [claude, human developer oversight]#39207
feat(hooks): add before_response_emit hook for output policies [claude, human developer oversight]#39207zeroaltitude wants to merge 47 commits intoopenclaw:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22a884db65
ℹ️ 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".
Greptile SummaryThis PR adds the Confidence Score: 4/5Nearly merge-ready — one localized diff-application mistake in types.ts needs a one-line fix in two places before this is safe to ship. All prior review concerns have been thoroughly addressed. The implementation quality is high, tests are comprehensive, and the architecture is correct. The single remaining issue (duplicate before_dispatch in types.ts) is a self-contained typo from an incorrect diff application — trivial to fix but must be done as it likely causes a TypeScript syntax error. src/plugins/types.ts — lines 1425–1426 (type union) and 1455–1456 (PLUGIN_HOOK_NAMES array): remove the spurious duplicate before_dispatch entries.
|
| Filename | Overview |
|---|---|
| src/plugins/types.ts | Adds well-documented BeforeResponseEmit types and handler map entry, but contains duplicate before_dispatch entries in both the PluginHookName union and PLUGIN_HOOK_NAMES array due to an incorrectly applied diff. |
| src/agents/pi-embedded-runner/run/hook-response-emit.ts | Core hook implementation. All previously-flagged issues addressed: text-bearing assistant selection, findLast() throughout, content/allContent invariant holds. rewriteAllAssistantContent backward-scan for toolResult preservation is well-tested. |
| src/agents/pi-embedded-runner/run/attempt.ts | Integration point is correct: runs regardless of promptError, fail-closed on error, lastAssistant suppressed via responseEmitBlocked flag, messagesSnapshot refreshed after hook. |
| src/plugins/hooks.ts | runBeforeResponseEmit correctly implements first-writer-wins merge for content/allContent mutual exclusion and one-way latch for block. |
| src/agents/pi-embedded-runner/run/hook-response-emit.test.ts | Comprehensive 26-test suite covering all major paths including regression cases for previously-identified bugs. |
| src/plugins/hooks.extended.test.ts | 6 new tests covering first-writer-wins, allContent precedence, one-way block latch, no-handler path, and the cross-priority mutual exclusion case. |
Comments Outside Diff (2)
-
src/plugins/types.ts, line 1425-1426 (link)Duplicate
"before_dispatch"— incorrectly applied diffThe PR's diff accidentally adds an extra
| "before_dispatch";at line 1425 while the original entry at line 1426 remains in the context. This results in two problems:-
The
PluginHookNametype alias is terminated at line 1425 by the first semicolon. The second| "before_dispatch";on line 1426 is then a dangling expression statement at module scope —|is a binary operator with no left operand, which TypeScript should report as a parse error. -
In
PLUGIN_HOOK_NAMES,"before_dispatch"appears at both line 1455 and 1456:
// current state of PLUGIN_HOOK_NAMES "before_response_emit", "before_dispatch", // ← added by PR (should not have been) "before_dispatch", // ← original entry
The intended diff was to add only
| "before_response_emit"(and"before_response_emit"in the array) directly before the existing| "before_dispatch"— the closing entry should not be duplicated. The fix is to remove the spurious addition: -
-
src/plugins/types.ts, line 1455-1456 (link)Duplicate
"before_dispatch"inPLUGIN_HOOK_NAMESarraySame root cause as the type-union duplicate above.
"before_dispatch"appears twice in theas constarray — at index 26 and 27. WhilepluginHookNameSet(line 1464) usesnew Set(PLUGIN_HOOK_NAMES)which deduplicates, the array itself is exported and could be iterated directly by callers, causingbefore_dispatchhooks to be enumerated twice.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/plugins/types.ts
Line: 1425-1426
Comment:
**Duplicate `"before_dispatch"` — incorrectly applied diff**
The PR's diff accidentally adds an extra `| "before_dispatch";` at line 1425 while the original entry at line 1426 remains in the context. This results in two problems:
1. The `PluginHookName` type alias is terminated at line 1425 by the first semicolon. The second `| "before_dispatch";` on line 1426 is then a dangling expression statement at module scope — `|` is a binary operator with no left operand, which TypeScript should report as a parse error.
2. In `PLUGIN_HOOK_NAMES`, `"before_dispatch"` appears at both line 1455 and 1456:
```typescript
// current state of PLUGIN_HOOK_NAMES
"before_response_emit",
"before_dispatch", // ← added by PR (should not have been)
"before_dispatch", // ← original entry
```
The intended diff was to add only `| "before_response_emit"` (and `"before_response_emit"` in the array) directly before the existing `| "before_dispatch"` — the closing entry should not be duplicated. The fix is to remove the spurious addition:
```suggestion
| "before_response_emit"
| "before_dispatch";
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/plugins/types.ts
Line: 1455-1456
Comment:
**Duplicate `"before_dispatch"` in `PLUGIN_HOOK_NAMES` array**
Same root cause as the type-union duplicate above. `"before_dispatch"` appears twice in the `as const` array — at index 26 and 27. While `pluginHookNameSet` (line 1464) uses `new Set(PLUGIN_HOOK_NAMES)` which deduplicates, the array itself is exported and could be iterated directly by callers, causing `before_dispatch` hooks to be enumerated twice.
```suggestion
"before_response_emit",
"before_dispatch",
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (29): Last reviewed commit: "Merge origin/main into feat/hook-before-..." | Re-trigger Greptile
22a884d to
a142d2b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a142d2bb05
ℹ️ 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".
|
@greptile-apps Please rescore. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdca778bc3
ℹ️ 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".
|
@greptile-apps Please rescore. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0589d335a1
ℹ️ 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".
|
@greptile-apps Please rescore. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bb4e85d9d
ℹ️ 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".
|
@greptile-apps Please rescore. |
|
@greptile-apps Please rescore. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bcd0817e3
ℹ️ 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".
|
@greptile-apps Please rescore. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eff3a51829
ℹ️ 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".
|
@greptile-apps Please rescore. |
…in session history
A plugin returning content: '' left { role: 'assistant', content: '' } in
activeSession.messages, which causes Anthropic API 400 errors on subsequent
turns (empty assistant content not allowed). Now treated as a block: clears
assistant messages entirely and logs a warning guiding plugins to use
block: true instead.
Also fixes misleading warning log in rewriteAllAssistantContent — now
direction-aware (surplus removed vs extras ignored).
…s.length) and scopeCount fallback - types.ts: allContent is bounded to text-bearing assistant turns in the current run. In block-reply mode, assistantTexts accumulates per-chunk entries; the actual bound is the consolidated session turn count. - hook-response-emit.ts: explain that the || assistantTexts.length fallback is a best-effort hint for the compacted mid-turn case where getRunScopedMessages returns [] regardless.
When emitResult.content === '', the coercion to block was using runMessages (text-bearing assistant scope only), leaving tool-call-only assistant messages and their toolResults in session history. These may contain sensitive tool arguments/results. Now uses getRunScopedMessagesForBlock (same as explicit block: true), which includes tool-call-only assistants and their corresponding toolResult messages.
Address greptile review on openclaw#39207 — the undocumented firing condition was a concern. Text-only responses do not trigger this hook; plugins should use before_response_emit for output inspection.
777c313 to
25b1196
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25b11963a4
ℹ️ 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".
Adds runId to the response emit hook event for cross-hook correlation. Plugin authors can now correlate before_response_emit with context_assembled, loop_iteration, and LLM call hooks within the same agent run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 612ef4c170
ℹ️ 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".
612ef4c to
a16dd96
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ee981bce1
ℹ️ 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".
| | "before_dispatch"; | ||
| | "before_dispatch"; |
There was a problem hiding this comment.
Remove stray PluginHookName union arm after terminator
PluginHookName is terminated on the first "before_dispatch";, but the next line adds another top-level | "before_dispatch";, which is invalid TypeScript syntax. This makes src/plugins/types.ts fail to parse, blocking type-check/build/test flows that import plugin types.
Useful? React with 👍 / 👎.
…ndentation, handler map entry)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e645a0473
ℹ️ 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".
| PluginHookBeforeResponseEmitEvent, | ||
| PluginHookBeforeResponseEmitResult, |
There was a problem hiding this comment.
Export before_response_emit types from hooks.ts
createHookRunner now supports before_response_emit, and this file imports PluginHookBeforeResponseEmitEvent/PluginHookBeforeResponseEmitResult, but the export type { ... } re-export block does not include them. Consumers that import hook contracts from src/plugins/hooks.ts (the module explicitly labeled for consumer type re-exports) cannot reference the new hook event/result types without reaching into types.ts, which is an API inconsistency and a compile-time break compared to other hook contracts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4086bc534
ℹ️ 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".
| if (i < newContents.length) { | ||
| rewriteSingleAssistantMessage(assistantMsgs[i], newContents[i]); | ||
| } else { |
There was a problem hiding this comment.
Treat empty allContent entries as removals or blocks
The allContent rewrite path writes each replacement verbatim, including "", so a plugin that redacts a turn to an empty string leaves empty assistant messages in activeSession.messages. This bypasses the explicit content === "" guard above and can create malformed transcript entries (assistant with empty text) that break later provider calls while persisting ghost turns in history. This is reachable whenever a policy plugin returns allContent with one or more empty elements; the rewrite should reject empties and either remove those turns or coerce to the same block/fail-closed behavior used for empty content.
Useful? React with 👍 / 👎.
Upstream moved hook types to hook-types.ts. Added before_response_emit types to new location.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dbde31353
ℹ️ 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".
| export async function applyBeforeResponseEmitHook( | ||
| params: ApplyBeforeResponseEmitParams, | ||
| ): Promise<ApplyBeforeResponseEmitResult | undefined> { |
There was a problem hiding this comment.
Wire before_response_emit into the embedded run path
This commit adds applyBeforeResponseEmitHook and the runBeforeResponseEmit runner API, but there is no non-test call site that invokes either one in the actual pi-embedded run flow. In production runs, plugins can register before_response_emit handlers yet those handlers never execute, so output redaction/block policies are silently bypassed for all responses.
Useful? React with 👍 / 👎.
TL;DR
Adds the
before_response_emitsequential / mutating plugin hook — fires before the final assistant response is delivered to the user. Plugins can modify, redact (single-message or multi-turn), or block the response entirely.Split from #33916. Independent of #39206 — both can merge in any order. Each adds its own hook name to the type union and PLUGIN_HOOK_NAMES array;
types.tsmerge will be trivial (adjacent lines).New hook (contract)
before_response_emitFollows the same sequential "merge results across handlers" pattern as
before_tool_call/message_sending.Merge semantics
contentstringallContentallContentstring[]contentblockbooleanblockReasonstringCapabilities
Single-message modification
Rewrites only the final assistant message text.
Multi-turn PII redaction (
allContent)Rewrites all assistant messages from the current run — not just the last one. Enables full multi-turn PII scanning across tool-loop iterations. Bounded to
assistantTexts.lengthfrom the original snapshot; plugins cannot inject new messages.Blocking
Clears all current-run assistant content parts from session history — blocked content does not persist.
Architecture: run-scoped rewrites
The hook only modifies messages from the current run. Prior session history (messages before the user prompt that triggered this run) is untouched.
Compaction safety
preRunMessageCountmarks the boundary between prior history and current-run messages. If auto-compaction invalidates this count during the run (messages get compacted mid-turn), the handler falls back to a tail-based scan that finds assistant messages from the end of the snapshot — never corrupting previously-delivered history.Multi-part assistant messages
When blocking, the handler clears content from all content parts of multi-part assistant messages (not just
textparts). Modified content is spliced into the correct content part index, preserving non-text parts.Review guide (start here)
src/plugins/types.ts—PluginHookBeforeResponseEmitEvent/PluginHookBeforeResponseEmitResultsrc/plugins/hooks.ts—runBeforeResponseEmit(sequential/FIFO merge, content/allContent mutual exclusion)src/agents/pi-embedded-runner/run/hook-response-emit.ts—runResponseEmitHook+getRunScopedMessageshelpersrc/agents/pi-embedded-runner/run/attempt.ts— fires aftersessionIdUsedassignment, beforepromptErrorcheckSecurity / correctness notes
before_response_emitblocking clears/redacts content from all current-run assistant messages (not just the last) so blocked content does not persist into session historyallContentenables full multi-turn PII redaction across tool-loop iterationsallContentlength is bounded to the originalassistantTexts.length— plugins cannot inject messagesTesting
Adds/extends unit & integration tests across:
allContent, run scoping, compaction fallback, multi-part messages)getRunScopedMessages: normal path, compaction path, defensive fallbackallContentprecedence, one-way latch, no-handler paths)30 total tests,
pnpm checkclean.AI-Assisted Development 🤖
This PR was developed with AI assistance (Claude/OpenClaw agent "Tank").
pnpm checkclean.