feat(plugins): add transform_context hook for per-LLM-call message re…#57585
feat(plugins): add transform_context hook for per-LLM-call message re…#57585HaotianChen616 wants to merge 1 commit into
Conversation
…writing Expose Agent.transformContext as a plugin hook so plugins can inspect and modify the full messages array before every LLM call in the agentic loop. This enables context-level transformations (e.g. compressing historical tool results while preserving the most recent one) that per-message hooks like tool_result_persist cannot achieve. - Add PluginHookTransformContextEvent/Result types and handler signature - Add runTransformContext to HookRunner (sequential, lastDefined merge) - Extend installToolResultContextGuard with optional pluginTransform callback, invoked between the original transformContext and budget enforcement - Wire hookRunner.runTransformContext through attempt.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c0229f29a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Greptile SummaryThis PR adds a Key observations from the review:
Confidence Score: 5/5Safe to merge — all findings are P2 style/design suggestions with no functional breakage on the changed path. No P0 or P1 issues found. The feature is purely additive, budget enforcement is unchanged, and the fallback behavior is correct. Remaining comments concern documentation clarity, type-contract alignment, and non-composable handler semantics — all non-blocking. No files require special attention; minor follow-ups in src/plugins/hooks.ts (composability docs) and src/agents/pi-embedded-runner/run/attempt.ts (return-type alignment) are optional.
|
| Filename | Overview |
|---|---|
| src/agents/pi-embedded-runner/run/attempt.ts | Wires hookRunner.runTransformContext into installToolResultContextGuard via the new pluginTransform callback. The ?? messages fallback means the callback never returns undefined, making the guard's nil-check a no-op, but there is no functional breakage. |
| src/agents/pi-embedded-runner/tool-result-context-guard.ts | Adds the optional pluginTransform callback, invoked after originalTransformContext and before budget enforcement. Logic is correct: falls back to the original transformed value if the plugin returns nothing, and preserves all existing budget-enforcement behavior. |
| src/plugins/hooks.ts | Adds runTransformContext using runModifyingHook with 'last defined wins' semantics. All handlers receive the original event messages rather than the progressively transformed output, which is consistent with other hooks but may surprise plugin authors expecting composable transforms. |
| src/plugins/types.ts | Adds PluginHookTransformContextEvent and PluginHookTransformContextResult types using unknown[] for messages (consistent with other hooks), adds transform_context to the PluginHookName union and PLUGIN_HOOK_NAMES array, and registers the handler signature in PluginHookHandlerMap. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/plugins/types.ts
Line: 1991-2002
Comment:
**`unknown[]` in event/result loses type safety at the boundary**
`PluginHookTransformContextEvent.messages` and `PluginHookTransformContextResult.messages` are both `unknown[]`, while the actual runtime values are `AgentMessage[]`. The cast in `attempt.ts` (`result?.messages as AgentMessage[]`) is unchecked — a plugin returning structurally invalid objects in the array will only fail later, deep inside the budget-enforcement or LLM-call paths, with no actionable error message.
Other event types in this file (e.g. `PluginHookBeforeCompactionEvent`) also use `unknown[]` for messages, so this is consistent with the existing API surface. If that's intentional for plugin isolation, it's worth a brief comment here noting that callers should treat the returned array as unvalidated and handle failures accordingly.
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/hooks.ts
Line: 549-568
Comment:
**Multiple handlers receive original messages, not chained output**
The `runModifyingHook` loop passes the same `event` object to every handler without updating `event.messages` between calls. Combined with `lastDefined(acc?.messages, next.messages)` (last non-undefined value wins, with hooks sorted highest-priority-first), this means:
- Handler A (priority=100) receives original messages and returns `[compressed…]`
- Handler B (priority=50) receives the **same original messages** — not Handler A's output — and returns `[annotated…]`
- Final result: `[annotated…]` (Handler A's compression is silently discarded)
The docstring says "last handler that returns `{ messages }` wins," which is correct, but it doesn't make clear that handlers **cannot see or build on prior handlers' transformations**. This is a meaningful footgun for plugin authors who assume sequential transforms compose. Consider either:
1. Updating `event.messages` to the accumulated result before each handler call so transforms chain, or
2. Making the non-composable intent explicit in the JSDoc.
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/agents/pi-embedded-runner/run/attempt.ts
Line: 868
Comment:
**`pluginTransform` never returns `undefined`, making the guard's `if (pluginMessages)` check dead code**
The `?? messages` fallback ensures `pluginTransform` always resolves to an `AgentMessage[]`, never `undefined`. The `if (pluginMessages)` guard in `tool-result-context-guard.ts` is therefore always truthy when `pluginTransform` is set.
This is not a functional bug, but the `Promise<AgentMessage[] | undefined>` return type on `installToolResultContextGuard`'s `pluginTransform` parameter is misleading. Either return `undefined` explicitly when `result?.messages` is nullish, or tighten the parameter type to `(messages: AgentMessage[]) => Promise<AgentMessage[]>`.
```suggestion
return result?.messages != null ? (result.messages as AgentMessage[]) : undefined;
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(plugins): add transform_context hoo..." | Re-trigger Greptile
| export type PluginHookTransformContextEvent = { | ||
| /** The complete messages array about to be sent to the LLM. */ | ||
| messages: unknown[]; | ||
| }; | ||
|
|
||
| export type PluginHookTransformContextResult = { | ||
| /** | ||
| * If provided, replaces the messages array sent to the LLM. | ||
| * Return only the modified messages — the caller will use this in place of the original. | ||
| */ | ||
| messages?: unknown[]; | ||
| }; |
There was a problem hiding this comment.
unknown[] in event/result loses type safety at the boundary
PluginHookTransformContextEvent.messages and PluginHookTransformContextResult.messages are both unknown[], while the actual runtime values are AgentMessage[]. The cast in attempt.ts (result?.messages as AgentMessage[]) is unchecked — a plugin returning structurally invalid objects in the array will only fail later, deep inside the budget-enforcement or LLM-call paths, with no actionable error message.
Other event types in this file (e.g. PluginHookBeforeCompactionEvent) also use unknown[] for messages, so this is consistent with the existing API surface. If that's intentional for plugin isolation, it's worth a brief comment here noting that callers should treat the returned array as unvalidated and handle failures accordingly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/types.ts
Line: 1991-2002
Comment:
**`unknown[]` in event/result loses type safety at the boundary**
`PluginHookTransformContextEvent.messages` and `PluginHookTransformContextResult.messages` are both `unknown[]`, while the actual runtime values are `AgentMessage[]`. The cast in `attempt.ts` (`result?.messages as AgentMessage[]`) is unchecked — a plugin returning structurally invalid objects in the array will only fail later, deep inside the budget-enforcement or LLM-call paths, with no actionable error message.
Other event types in this file (e.g. `PluginHookBeforeCompactionEvent`) also use `unknown[]` for messages, so this is consistent with the existing API surface. If that's intentional for plugin isolation, it's worth a brief comment here noting that callers should treat the returned array as unvalidated and handle failures accordingly.
How can I resolve this? If you propose a fix, please make it concise.| /** | ||
| * Run transform_context hook. | ||
| * Allows plugins to modify the full messages array before each LLM call. | ||
| * Runs sequentially; the last handler that returns { messages } wins. | ||
| */ | ||
| async function runTransformContext( | ||
| event: PluginHookTransformContextEvent, | ||
| ctx: PluginHookAgentContext, | ||
| ): Promise<PluginHookTransformContextResult | undefined> { | ||
| return runModifyingHook<"transform_context", PluginHookTransformContextResult>( | ||
| "transform_context", | ||
| event, | ||
| ctx, | ||
| { | ||
| mergeResults: (acc, next) => ({ | ||
| messages: lastDefined(acc?.messages, next.messages), | ||
| }), | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The runModifyingHook loop passes the same event object to every handler without updating event.messages between calls. Combined with lastDefined(acc?.messages, next.messages) (last non-undefined value wins, with hooks sorted highest-priority-first), this means:
- Handler A (priority=100) receives original messages and returns
[compressed…] - Handler B (priority=50) receives the same original messages — not Handler A's output — and returns
[annotated…] - Final result:
[annotated…](Handler A's compression is silently discarded)
The docstring says "last handler that returns { messages } wins," which is correct, but it doesn't make clear that handlers cannot see or build on prior handlers' transformations. This is a meaningful footgun for plugin authors who assume sequential transforms compose. Consider either:
- Updating
event.messagesto the accumulated result before each handler call so transforms chain, or - Making the non-composable intent explicit in the JSDoc.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/hooks.ts
Line: 549-568
Comment:
**Multiple handlers receive original messages, not chained output**
The `runModifyingHook` loop passes the same `event` object to every handler without updating `event.messages` between calls. Combined with `lastDefined(acc?.messages, next.messages)` (last non-undefined value wins, with hooks sorted highest-priority-first), this means:
- Handler A (priority=100) receives original messages and returns `[compressed…]`
- Handler B (priority=50) receives the **same original messages** — not Handler A's output — and returns `[annotated…]`
- Final result: `[annotated…]` (Handler A's compression is silently discarded)
The docstring says "last handler that returns `{ messages }` wins," which is correct, but it doesn't make clear that handlers **cannot see or build on prior handlers' transformations**. This is a meaningful footgun for plugin authors who assume sequential transforms compose. Consider either:
1. Updating `event.messages` to the accumulated result before each handler call so transforms chain, or
2. Making the non-composable intent explicit in the JSDoc.
How can I resolve this? If you propose a fix, please make it concise.| workspaceDir: params.workspaceDir, | ||
| }, | ||
| ); | ||
| return (result?.messages as AgentMessage[]) ?? messages; |
There was a problem hiding this comment.
pluginTransform never returns undefined, making the guard's if (pluginMessages) check dead code
The ?? messages fallback ensures pluginTransform always resolves to an AgentMessage[], never undefined. The if (pluginMessages) guard in tool-result-context-guard.ts is therefore always truthy when pluginTransform is set.
This is not a functional bug, but the Promise<AgentMessage[] | undefined> return type on installToolResultContextGuard's pluginTransform parameter is misleading. Either return undefined explicitly when result?.messages is nullish, or tighten the parameter type to (messages: AgentMessage[]) => Promise<AgentMessage[]>.
| return (result?.messages as AgentMessage[]) ?? messages; | |
| return result?.messages != null ? (result.messages as AgentMessage[]) : undefined; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/run/attempt.ts
Line: 868
Comment:
**`pluginTransform` never returns `undefined`, making the guard's `if (pluginMessages)` check dead code**
The `?? messages` fallback ensures `pluginTransform` always resolves to an `AgentMessage[]`, never `undefined`. The `if (pluginMessages)` guard in `tool-result-context-guard.ts` is therefore always truthy when `pluginTransform` is set.
This is not a functional bug, but the `Promise<AgentMessage[] | undefined>` return type on `installToolResultContextGuard`'s `pluginTransform` parameter is misleading. Either return `undefined` explicitly when `result?.messages` is nullish, or tighten the parameter type to `(messages: AgentMessage[]) => Promise<AgentMessage[]>`.
```suggestion
return result?.messages != null ? (result.messages as AgentMessage[]) : undefined;
```
How can I resolve this? If you propose a fix, please make it concise.|
Closing this as implemented after Codex automated review. Current main already solves the PR's central request through the shipped ContextEngine plugin surface. A plugin can register a context engine, receive the full AgentMessage[] in assemble(), return a replacement ordered message array, and, when it owns compaction, run through Agent.transformContext during long tool loops. Tests cover repeated loop iterations and replacement arrays. The raw transform_context hook proposed here is no longer the best core shape, especially given the review feedback about prompt-mutation policy. Best possible solution: Keep the shipped ContextEngine plugin surface as the supported path for full-context compression and model-context rewriting. Plugin authors should register a context engine and select it with plugins.slots.contextEngine; separate llm_input/llm_output interception work can remain in its own PR if maintainers want prompt/response-level hooks. What I checked:
So I’m closing this as already implemented rather than keeping a duplicate issue open. Codex Review notes: model gpt-5.5, reasoning high; reviewed against 7e376e5aba32; fix evidence: release v2026.4.14, commit 7e376e5aba32. |
|
I appreciate the review, but I believe the closure conflates two separate concerns.What the bot's argument gets right:
Why
This is exactly the gap this PR addresses: context compression needs to inspect and potentially rewrite the message array before each LLM call, not just once at the start. I'd respectfully ask that this PR be reopened, or at minimum, that this gap be tracked as a follow-up issue so it doesn't get lost. |
Summary
before_prompt_build,llm_input,tool_result_persist) are either one-shot, observation-only, or per-message — none provide context-level access on every LLM turn. Andassembleis called per attempt but not per-llm, which is outside theactiveSession.prompt()loop.transform_contextplugin hook that wrapsAgent.transformContext, firing before every LLM call with the complete messages array. Plugins can return a modified messages array that replaces the original. Execution order: original transformContext → plugin hook → budget enforcement (truncation/compaction).transform_context, behavior is identical to before.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
N/A
Regression Test Plan (if applicable)
N/A
User-visible / Behavior Changes
None. This is a new plugin API surface — no behavior changes unless a plugin explicitly registers a
transform_contexthandler.Diagram (if applicable)
Security Impact (required)
No)No)No)No)No)Repro + Verification
Environment
Steps
transform_contexthandler{ messages: modified }from the handler replaces the messages sent to the LLMExpected
Actual
(To be verified)
Evidence
Human Verification (required)
transform_contexthook type definitions compile without errorsrunTransformContextrunner integrates into HookRunner exportinstallToolResultContextGuardaccepts and invokes the optionalpluginTransformcallbackattempt.tscorrectly wireshookRunner.runTransformContextinto the guardopenclaw-pluginregisters viaapi.on("transform_context", ...)transform_contexthandlers registered →pluginTransformisundefined, no-opundefined→ original messages pass through unchanged{ messages }→ modified messages replace originalReview Conversations
Compatibility / Migration
Yes)No)No)Risks and Mitigations
before_prompt_build(which can also inject arbitrary content).