Skip to content

feat(hooks): add before_response_emit hook for output policies [claude, human developer oversight]#39207

Open
zeroaltitude wants to merge 47 commits intoopenclaw:mainfrom
zeroaltitude:feat/hook-before-response-emit
Open

feat(hooks): add before_response_emit hook for output policies [claude, human developer oversight]#39207
zeroaltitude wants to merge 47 commits intoopenclaw:mainfrom
zeroaltitude:feat/hook-before-response-emit

Conversation

@zeroaltitude
Copy link
Copy Markdown

@zeroaltitude zeroaltitude commented Mar 7, 2026

TL;DR

Adds the before_response_emit sequential / 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.ts merge will be trivial (adjacent lines).

New hook (contract)

Hook Fires Can modify Can block Primary use
before_response_emit When final assistant text is selected, before delivery content, allContent yes Output policies / PII redaction / response gating

Follows the same sequential "merge results across handlers" pattern as before_tool_call / message_sending.

Merge semantics

Field Type Semantics
content string First-writer-wins, mutually exclusive with allContent
allContent string[] First-writer-wins, takes precedence over content
block boolean One-way latch (once true, stays true)
blockReason string First-writer-wins

Capabilities

Single-message modification

return { content: redact(event.content) };

Rewrites only the final assistant message text.

Multi-turn PII redaction (allContent)

return { allContent: event.allContent.map(redact) };

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.length from the original snapshot; plugins cannot inject new messages.

Blocking

return { block: true, blockReason: "policy violation" };

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.

Session messages:
  [prior history] [user prompt] [assistant msg 1] [tool call] [assistant msg 2]
  ^--- untouched   ^--- run boundary               ^--- allContent[0]            ^--- allContent[1] / content

Compaction safety

preRunMessageCount marks 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 text parts). Modified content is spliced into the correct content part index, preserving non-text parts.

Review guide (start here)

  1. API surface: src/plugins/types.tsPluginHookBeforeResponseEmitEvent / PluginHookBeforeResponseEmitResult
  2. Hook runner semantics: src/plugins/hooks.tsrunBeforeResponseEmit (sequential/FIFO merge, content/allContent mutual exclusion)
  3. Implementation: src/agents/pi-embedded-runner/run/hook-response-emit.tsrunResponseEmitHook + getRunScopedMessages helper
  4. Integration point: src/agents/pi-embedded-runner/run/attempt.ts — fires after sessionIdUsed assignment, before promptError check

Security / correctness notes

  • before_response_emit blocking clears/redacts content from all current-run assistant messages (not just the last) so blocked content does not persist into session history
  • Multi-part assistant messages are handled — modified content does not leave stale fragments
  • Run-scoped rewrites never corrupt previously-delivered session history, even after compaction
  • allContent enables full multi-turn PII redaction across tool-loop iterations
  • allContent length is bounded to the original assistantTexts.length — plugins cannot inject messages
  • Handler execution order is FIFO (registration order), consistent with registry expectations

Testing

Adds/extends unit & integration tests across:

  • Response emit handler: 26 tests (modification, blocking, session-history scrubbing, allContent, run scoping, compaction fallback, multi-part messages)
  • getRunScopedMessages: normal path, compaction path, defensive fallback
  • Hook runner merge semantics: 4 tests (first-writer-wins, allContent precedence, one-way latch, no-handler paths)

30 total tests, pnpm check clean.

AI-Assisted Development 🤖

This PR was developed with AI assistance (Claude/OpenClaw agent "Tank").

  • Degree of testing: Comprehensive — 30 unit tests across 2 test files. pnpm check clean.
  • Human oversight: All changes reviewed and directed by Eddie Abrams
  • Understanding confirmed: The contributor understands the run-scoped message handling, compaction-safe fallback logic, content-part manipulation, and hook merge semantics

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/agents/pi-embedded-runner/run/hook-stream-wrapper.ts Outdated
Comment thread src/agents/pi-embedded-runner/run/hook-response-emit.ts
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 7, 2026

Greptile Summary

This PR adds the before_response_emit sequential/mutating plugin hook, completing the outbound pipeline for OpenClaw's plugin system. It fires after the agent run completes and before the final response is delivered, allowing plugins to modify, redact, or block the response. The implementation is architecturally sound, with compaction-safe run-scoped rewrites, correct fail-closed error handling, and comprehensive test coverage (30 tests). All previously-flagged issues from earlier review rounds have been addressed.\n\nOne concrete bug remains before merging:\n\n- types.ts duplicate \"before_dispatch\" entries — The PR's diff was incorrectly applied: it adds | \"before_dispatch\"; and \"before_dispatch\", as new lines while the original entries were already in the context, resulting in duplicates in both the PluginHookName union type and the PLUGIN_HOOK_NAMES array. In the type union, the first | \"before_dispatch\"; (line 1425) terminates the type alias, leaving | \"before_dispatch\" on line 1426 as a dangling expression statement that TypeScript should reject. Both duplicates need a one-line deletion each.\n\nStrengths:\n- Fail-closed error handling (hook crash → response suppressed) at two levels: per-handler via catchErrors and infrastructure-level in attempt.ts\n- Compaction-safe boundary detection via tail-based scan\n- rewriteLastAssistantContent correctly skips tool-use-only messages\n- allContent shrink correctly handles toolResult orphan prevention\n- Streaming limitation clearly documented at both the integration point and the type definition\n- lastAssistant suppression via responseEmitBlocked prevents payloads.ts fallback from leaking blocked content

Confidence Score: 4/5

Nearly 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.

Important Files Changed

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)

  1. src/plugins/types.ts, line 1425-1426 (link)

    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:

    // 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:

  2. src/plugins/types.ts, line 1455-1456 (link)

    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.

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

Comment thread src/agents/pi-embedded-runner/run/hook-response-emit.ts
Comment thread src/agents/pi-embedded-runner/run/hook-stream-wrapper.ts Outdated
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/agents/pi-embedded-runner/run/attempt.ts Outdated
Comment thread src/agents/pi-embedded-runner/run/hook-response-emit.ts Outdated
@zeroaltitude
Copy link
Copy Markdown
Author

@greptile-apps Please rescore.

Comment thread src/agents/pi-embedded-runner/run/attempt.ts Outdated
Comment thread src/agents/pi-embedded-runner/run/attempt.ts Outdated
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/agents/pi-embedded-runner/run/attempt.ts Outdated
Comment thread src/agents/pi-embedded-runner/run/hook-response-emit.ts
@zeroaltitude
Copy link
Copy Markdown
Author

@greptile-apps Please rescore.

Comment thread src/agents/pi-embedded-runner/run/hook-response-emit.ts Outdated
Comment thread src/agents/pi-embedded-runner/run/hook-response-emit.ts Outdated
Comment thread src/agents/pi-embedded-runner/run/hook-response-emit.ts Outdated
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/agents/pi-embedded-runner/run/hook-response-emit.ts Outdated
Comment thread src/agents/pi-embedded-runner/run/attempt.ts Outdated
@zeroaltitude
Copy link
Copy Markdown
Author

@greptile-apps Please rescore.

Comment thread src/agents/pi-embedded-runner/run/attempt.ts Outdated
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/agents/pi-embedded-runner/run/attempt.ts Outdated
Comment thread src/agents/pi-embedded-runner/run/attempt.ts Outdated
@zeroaltitude
Copy link
Copy Markdown
Author

@greptile-apps Please rescore.

Comment thread src/plugins/hooks.ts Outdated
@zeroaltitude
Copy link
Copy Markdown
Author

@greptile-apps Please rescore.

Comment thread src/plugins/hooks.extended.test.ts Outdated
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/agents/pi-embedded-runner/run/attempt.ts Outdated
@zeroaltitude
Copy link
Copy Markdown
Author

@greptile-apps Please rescore.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/agents/pi-embedded-runner/run/attempt.ts Outdated
@zeroaltitude
Copy link
Copy Markdown
Author

@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.
zeroaltitude added a commit to zeroaltitude/openclaw that referenced this pull request Mar 22, 2026
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.
@zeroaltitude zeroaltitude force-pushed the feat/hook-before-response-emit branch from 777c313 to 25b1196 Compare March 22, 2026 06:00
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/agents/pi-embedded-runner/run/attempt.ts Outdated
Comment thread src/agents/pi-embedded-runner/run/hook-response-emit.ts
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.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/agents/pi-embedded-runner/run/attempt.ts Outdated
Comment thread src/agents/pi-embedded-runner/run/attempt.ts Outdated
@zeroaltitude zeroaltitude force-pushed the feat/hook-before-response-emit branch from 612ef4c to a16dd96 Compare March 26, 2026 06:42
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/plugins/types.ts Outdated
Comment on lines 1425 to 1426
| "before_dispatch";
| "before_dispatch";
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Badge 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 👍 / 👎.

@openclaw-barnacle openclaw-barnacle bot added the docs Improvements or additions to documentation label Mar 26, 2026
@openclaw-barnacle openclaw-barnacle bot removed the docs Improvements or additions to documentation label Apr 2, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/plugins/hooks.ts
Comment on lines +61 to +62
PluginHookBeforeResponseEmitEvent,
PluginHookBeforeResponseEmitResult,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +457 to +459
if (i < newContents.length) {
rewriteSingleAssistantMessage(assistantMsgs[i], newContents[i]);
} else {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +108 to +110
export async function applyBeforeResponseEmitHook(
params: ApplyBeforeResponseEmitParams,
): Promise<ApplyBeforeResponseEmitResult | undefined> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

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

Labels

agents Agent runtime and tooling size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants