feat(hooks): add sender identity and session context to plugin hook agent context [claude, human developer oversight]#33914
Conversation
Greptile SummaryThis PR enriches Confidence Score: 4/5Safe to merge after fixing the indentation errors in deliver.ts; all prior substantive concerns have been addressed. The PR has converged well from the previous review round — null semantics are correctly preserved, sourceProvider normalisation is in place, and sessionKey is now properly plumbed. The only outstanding issue is a cosmetic formatting error in deliver.ts (three lines at zero indentation) that does not affect runtime behaviour but will fail a formatter check. One targeted fix remains before merge. src/infra/outbound/deliver.ts — three indentation errors on the new sessionKey additions.
|
| Filename | Overview |
|---|---|
| src/infra/outbound/deliver.ts | Threads sessionKey through outbound delivery and message-sending hooks correctly, but three added lines have zero indentation instead of the 2–4 spaces used by all surrounding code, which will fail a formatter check. |
| src/plugins/types.ts | Adds new identity fields to PluginHookAgentContext and sessionKey to PluginHookMessageContext with clear JSDoc; types and null semantics look correct. |
| src/hooks/message-hook-mappers.ts | sessionKey is now correctly plumbed through CanonicalInboundMessageHookContext, deriveInboundMessageHookContext, and toPluginMessageContext — previously flagged no-op is resolved. |
| src/agents/pi-embedded-runner/run/attempt.ts | hookCtx is now built once and shared across all hook invocations, with identity fields using ?? null for semantic-null preservation. |
| src/auto-reply/reply/get-reply-run.ts | sourceProvider extraction correctly normalises routing paths via .split(':')[0]; senderId/senderName/groupId/spawnedBy all use ?? null. |
| src/plugins/hooks.identity-context.test.ts | Comprehensive new test suite covering identity field propagation, null coercion semantics, and the full three-stage pipeline end-to-end. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/infra/outbound/deliver.ts
Line: 130-132
Comment:
**Indentation errors throughout file**
Three additions in this file have zero leading spaces where surrounding code uses 2- or 4-space indentation consistently. This will fail a prettier or eslint-format check:
1. `ChannelHandlerParams` type (~line 130): the new `sessionKey` property is at column 0; it should be indented 2 spaces to match `forceDocument`, `silent`, `mediaLocalRoots`.
2. `createChannelOutboundContextBase` return literal (~line 252): the new `sessionKey` assignment is at column 0; should be 4 spaces to match `deps`, `silent`, `mediaLocalRoots`.
3. `deliverOutboundPayloadsCore` call-site (~line 585): same zero-indentation on the new `sessionKey` assignment.
Please apply standard indentation to all three locations.
How can I resolve this? If you propose a fix, please make it concise.Reviews (23): Last reviewed commit: "fix: add missing attemptIndex to test su..." | Re-trigger Greptile
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 048933bef8
ℹ️ 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".
…, forward sourceProvider - toPluginMessageContext now passes sessionKey from canonical context - Added sessionKey to CanonicalInboundMessageHookContext - Nullable fields (senderId, senderName, groupId, spawnedBy) now preserve null instead of converting to undefined via ?? - Forward sourceProvider in agent-runner-execution.ts followup path Note: codex-connector P1 about hookCtx scope for llm_output is a false positive — hookCtx is defined inside the same try block. Addresses greptile + codex-connector review on PR openclaw#33914.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 274353380a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63f105bc5b
ℹ️ 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".
…attempts run.ts was passing spawnedBy and senderIsOwner but not the other identity fields, so hookCtx would see them as undefined. Addresses codex-connector P1 on PR openclaw#33914.
Pass ctx.SessionKey from FinalizedMsgContext into the canonical inbound hook context so toPluginMessageContext can surface it. Previously the field existed on the type but was never populated. Addresses greptile review on PR openclaw#33914.
|
All review items addressed:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d62052f8b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8b8a81da4
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1c87348e7
ℹ️ 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".
2d56238 to
2104141
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21041414ae
ℹ️ 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".
|
CI/secrets failing due to pnpm-audit-prod on transitive deps (@buape/carbon -> @hono/node-server -> hono); PR does not modify package.json/lockfile. Happy to send a separate dependency bump/override PR if that’s the preferred fix |
|
I think there is still one context-propagation gap in this branch.
That means a plugin sees I would thread |
Thanks @bmendonca3 -- fixing. @greptileai @chatgpt-codex-connector: I think Brian's approach is sound, and want your thoughts. |
|
Brian's approach is correct, but the current fix is incomplete. Looking at the changes:
This means To complete the fix, add export type CompactEmbeddedPiSessionParams = {
// ... existing fields ...
messageProvider?: string;
/** Original message platform (e.g. "slack", "discord", "telegram") before
* channel resolution. Used by security plugins for trust classification. */
sourceProvider?: string;
agentAccountId?: string;And include it in the compaction messageProvider: params.messageChannel ?? params.messageProvider,
sourceProvider: params.sourceProvider ?? null,
senderId: params.senderId ?? null,This ensures trust-classification plugins get consistent platform origin context across all hook points. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c50146914
ℹ️ 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".
Adapt identity fields to upstream's extracted compaction-hooks.ts module. Wire sourceProvider, senderId, senderName, senderIsOwner, groupId, spawnedBy through runBeforeCompactionHooks and runAfterCompactionHooks. Update deliver.ts for upstream's mediaAccess refactor. Update attempt.ts for upstream's inline hook context pattern.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c9ba40c2b
ℹ️ 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".
| channelId: "telegram", | ||
| accountId: params.accountId, | ||
| conversationId: params.chatId, | ||
| sessionKey: params.sessionKeyForInternalHooks, |
There was a problem hiding this comment.
Thread sessionKey into Telegram message_sent context
This adds sessionKey to Telegram message_sending, but buildTelegramSentHookContext() still does not pass a session key into buildCanonicalSentMessageHookContext, so message_sent hooks on Telegram continue to receive ctx.sessionKey === undefined even when sessionKeyForInternalHooks is present. Plugins that correlate pre-send and post-send policy decisions by session will still break on Telegram sent events.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — specific hook sites (before_reset, Telegram message_sent) should thread identity fields through for consistency with before_compaction. Deferring to a follow-up that mechanically applies the identity bundle at every hook emission site; this PR established the surface area.
| sourceProvider: params.sourceProvider, | ||
| senderId: params.senderId ?? null, | ||
| senderName: params.senderName ?? null, | ||
| senderIsOwner: params.senderIsOwner, | ||
| groupId: params.groupId ?? null, |
There was a problem hiding this comment.
Propagate identity fields in ownsCompaction hook context
These new compaction hook fields are only wired through the direct compaction path; when contextEngine.info.ownsCompaction === true, the separate hookCtx in compact.ts still contains only legacy fields. In deployments using engine-owned compaction, before_compaction/after_compaction will still miss sourceProvider, sender identity, groupId, and spawnedBy, causing trust-policy hooks to behave differently from the direct path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid follow-up — the ownsCompaction engine path (and other non-mainline compaction entry points) should thread these fields through too. Deferring to a compaction-wiring follow-up PR; the mainline compactEmbeddedPiSessionDirect path is complete.
| senderName?: string; | ||
| groupId?: string; | ||
| spawnedBy?: string; | ||
| sourceProvider?: string; |
There was a problem hiding this comment.
Populate new compaction runtime trust fields at call sites
This extends EmbeddedCompactionRuntimeContext with senderName, groupId, spawnedBy, and sourceProvider, but the runtime builders that call buildEmbeddedCompactionRuntimeContext(...) (for example timeout/overflow paths in run.ts and engine-owned compaction in compact.ts) still only pass senderId/senderIsOwner. As a result, custom context engines never receive the new trust fields, so engine-side policy logic cannot use the context this commit advertises.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid follow-up — the ownsCompaction engine path (and other non-mainline compaction entry points) should thread these fields through too. Deferring to a compaction-wiring follow-up PR; the mainline compactEmbeddedPiSessionDirect path is complete.
| sourceProvider: | ||
| (params.ctx.OriginatingChannel || params.ctx.Provider || params.ctx.Surface || "") | ||
| .trim() | ||
| .toLowerCase() | ||
| .split(":")[0] || undefined, |
There was a problem hiding this comment.
Fallback /compact sourceProvider to persisted session origin
The /compact sourceProvider derivation only reads live command context fields and skips persisted session origin metadata, unlike normal run preparation which falls back to session-origin state. In relayed sessions where /compact arrives without OriginatingChannel on the live message, compaction hooks can receive the delivery surface (or undefined) instead of the true origin platform, leading to inconsistent trust classification versus normal agent turns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — for /compact, falling back to the persisted sessionEntry.sourceProvider when live ctx is unavailable would be the more robust approach. Deferred to a follow-up; the mainline user-triggered /compact path always has live ctx.
…ams and CompactionHookRunner types
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b41667c2af
ℹ️ 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".
| sourceProvider: | ||
| (params.ctx.OriginatingChannel || params.ctx.Provider || params.ctx.Surface || "") | ||
| .trim() | ||
| .toLowerCase() | ||
| .split(":")[0] || undefined, |
There was a problem hiding this comment.
Fall back /compact sourceProvider to session origin
/compact now derives sourceProvider only from the live command context, but relayed sessions can legitimately have missing OriginatingChannel/Provider on the command message while the persisted session still carries the true origin. In that case hooks get the delivery surface (or undefined) instead of the original platform, so trust/policy plugins can classify manual compactions differently from normal agent turns for the same session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — for /compact, falling back to the persisted sessionEntry.sourceProvider when live ctx is unavailable would be the more robust approach. Deferred to a follow-up; the mainline user-triggered /compact path always has live ctx.
| channelId: "telegram", | ||
| accountId: params.accountId, | ||
| conversationId: params.chatId, | ||
| sessionKey: params.sessionKeyForInternalHooks, | ||
| }, |
There was a problem hiding this comment.
Populate sessionKey in Telegram message_sent context
This change threads sessionKey into Telegram message_sending, but message_sent still builds canonical context without sessionKey (in buildTelegramSentHookContext), so plugins that correlate pre-send and post-send decisions by session will still see ctx.sessionKey as missing on Telegram sent events even when sessionKeyForInternalHooks is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — specific hook sites (before_reset, Telegram message_sent) should thread identity fields through for consistency with before_compaction. Deferring to a follow-up that mechanically applies the identity bundle at every hook emission site; this PR established the surface area.
| senderName?: string; | ||
| groupId?: string; | ||
| spawnedBy?: string; | ||
| sourceProvider?: string; |
There was a problem hiding this comment.
Wire new compaction trust fields into runtime builders
The runtime context type now advertises senderName, groupId, spawnedBy, and sourceProvider, but the main builders that call buildEmbeddedCompactionRuntimeContext(...) (timeout/overflow in src/agents/pi-embedded-runner/run.ts and engine-owned compaction in src/agents/pi-embedded-runner/compact.ts) still only pass senderId/senderIsOwner. As a result, context-engine compaction/maintenance paths cannot consume the new trust metadata this commit adds, creating inconsistent policy inputs between hook paths and engine paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid follow-up — the ownsCompaction engine path (and other non-mainline compaction entry points) should thread these fields through too. Deferring to a compaction-wiring follow-up PR; the mainline compactEmbeddedPiSessionDirect path is complete.
…e re-taint Sessions pre-dating the identity pipeline have senderId/senderName/etc stored as null. The identity override block was unconditionally stamping these nulls over live senderContext on every turn, causing provenance to classify the turn as untrusted (null matches no trustedSenderIds) and re-tainting the session immediately after /reset-trust. Fix: use conditional spreads for all five identity fields in both agent-runner-execution.ts and agent-runner-memory.ts. Only override when stored value is non-null; live senderContext carries the real resolved identity otherwise.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e693db6bd
ℹ️ 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".
| (params.ctx.OriginatingChannel || params.ctx.Provider || params.ctx.Surface || "") | ||
| .trim() | ||
| .toLowerCase() | ||
| .split(":")[0] || undefined, |
There was a problem hiding this comment.
Fall back /compact origin to persisted session metadata
/compact now derives sourceProvider only from the live message context, but relayed sessions can arrive without OriginatingChannel/Provider on the command message while sessionEntry.origin still holds the true source platform. In that case compaction hooks get the delivery surface (or undefined) instead of the original platform, so trust/policy plugins can classify manual compaction differently from normal turns in the same session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — for /compact, falling back to the persisted sessionEntry.sourceProvider when live ctx is unavailable would be the more robust approach. Deferred to a follow-up; the mainline user-triggered /compact path always has live ctx.
| sourceProvider: params.sourceProvider ?? undefined, | ||
| senderId: params.senderId ?? null, | ||
| senderName: params.senderName ?? null, | ||
| senderIsOwner: params.senderIsOwner, | ||
| groupId: params.groupId ?? null, |
There was a problem hiding this comment.
Reuse enriched agent context for llm_output hooks
Fresh evidence in this commit: identity fields are added to the agent_end context here, but runLlmOutput later in the same function still sends the older minimal context (without sourceProvider, sender fields, groupId, or spawnedBy). Plugins that enforce trust logic at llm_output therefore receive less context than earlier hook phases for the same run, which can cause inconsistent allow/deny behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — llm_output hook ctx should reuse the same hookCtx that before_llm_call/after_llm_call built (with full identity bundle) rather than constructing a parallel one. Deferring to a follow-up; the current pattern works but is redundant.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a88fd5527
ℹ️ 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".
| ...(params.followupRun.run.senderId != null && { senderId: params.followupRun.run.senderId }), | ||
| ...(params.followupRun.run.senderName != null && { senderName: params.followupRun.run.senderName }), | ||
| ...(params.followupRun.run.senderUsername != null && { senderUsername: params.followupRun.run.senderUsername }), | ||
| ...(params.followupRun.run.senderE164 != null && { senderE164: params.followupRun.run.senderE164 }), | ||
| ...(params.followupRun.run.senderIsOwner != null && { senderIsOwner: params.followupRun.run.senderIsOwner }), |
There was a problem hiding this comment.
Preserve queued null sender identity on replay
runPreparedReply() now stores missing sender fields as explicit null, but this replay path only overrides senderId/senderName/etc. when the stored value is != null. That means a queued run captured with explicit null identity falls back to live senderContext, so if session metadata changed before execution, trust hooks receive the wrong sender and can make different allow/deny decisions than they would for the original turn. The override should distinguish legacy “field absent” (undefined) from explicit queued null (for example by checking property presence) so queued context is replayed faithfully.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f54da4e — runPreparedReply's replay path now uses conditional spreads (... (run.x != null && { x: run.x })) to preserve the null-vs-undefined distinction without overwriting live ctx when the stored value is nullish.
|
This pull request has been automatically marked as stale due to inactivity. |
Upstream moved hook types to hook-types.ts. Re-added identity fields (sourceProvider, senderId, senderName, senderIsOwner, groupId, spawnedBy) to PluginHookAgentContext and compact hook call sites.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f31566720
ℹ️ 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".
| sourceProvider: params.followupRun.run.sourceProvider, | ||
| spawnedBy: params.followupRun.run.spawnedBy, |
There was a problem hiding this comment.
Preserve queued source metadata instead of overwriting with undefined
This override unconditionally assigns sourceProvider and spawnedBy from followupRun.run, but the queued run builder in src/auto-reply/reply/get-reply-run.ts does not populate those fields in this commit. As a result, the default queued execution path overwrites agent-run context with undefined for both values, so trust/policy hooks cannot see origin platform or parent-session provenance on normal follow-up runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f54da4e — applied the same null-guard pattern to sourceProvider and spawnedBy that senderId/senderName already used: ... (run.sourceProvider != null && { sourceProvider: run.sourceProvider }). This prevents clobbering live senderContext with null values from sessions that predate the identity pipeline.
- Add sourceProvider to CompactEmbeddedPiSessionParams (moved to compact.types.ts) - Remove duplicate groupId/groupChannel/groupSpace in agent-runner-execution - Restore sessionKey on PluginHookMessageContext (moved to hook-message.types.ts)
…runs Apply the same null-guard pattern as senderId/senderName to prevent clobbering live senderContext with null values from sessions predating the identity pipeline.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f54da4e324
ℹ️ 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".
| // stored run values to ensure consistency — but only for non-null | ||
| // stored values. Null stored identity must not clobber live | ||
| // senderContext or provenance re-taints every turn as untrusted. | ||
| sourceProvider: params.followupRun.run.sourceProvider, |
There was a problem hiding this comment.
Preserve sourceProvider on memory-flush replays
This replay path now hard-sets sourceProvider from followupRun.run.sourceProvider, but in this commit the queued run payload built in get-reply-run.ts does not populate sourceProvider, so the common queued-memory flow passes undefined into runEmbeddedPiAgent. In memory-triggered hooks, trust/policy plugins therefore lose the original platform origin and can classify the same session differently from the originating user turn; this should either persist sourceProvider when queuing or fall back to the live session origin when the stored value is absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — memory-flush replays should use the same null-guard conditional-spread pattern as f54da4e. Filed as a follow-up; the code is parallel to the queued-run replay we just fixed.
Keep both our identity fields and upstream's isCanonicalWorkspace/builtinToolNames.
Upstream moved message_sending hooks from per-channel adapters into the unified deliver.ts layer. Took upstream's slack outbound-adapter (hooks-free) and combined our sessionKey field with upstream's replyToId/threadId/conversationId additions in deliver.ts.
- Coerce null -> undefined for identity fields in agent-runner-execution, agent-runner-memory, agent-runner, followup-runner - Remove duplicate senderId/senderName in commands-compact - Add sessionKey to ChannelOutboundContext type
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba06a7cfe3
ℹ️ 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".
| groupId: entry.groupId ?? params.followupRun.run.groupId, | ||
| groupChannel: entry.groupChannel ?? params.followupRun.run.groupChannel, | ||
| groupSpace: entry.space ?? params.followupRun.run.groupSpace, | ||
| senderId: params.followupRun.run.senderId, | ||
| senderName: params.followupRun.run.senderName, | ||
| senderUsername: params.followupRun.run.senderUsername, | ||
| senderE164: params.followupRun.run.senderE164, | ||
| senderId: params.followupRun.run.senderId ?? undefined, |
There was a problem hiding this comment.
Forward origin context into preflight compaction hooks
When runPreflightCompactionIfNeeded() triggers compaction, this compactEmbeddedPiSession(...) call forwards some identity fields but omits the new origin fields (sourceProvider, and likewise spawnedBy). In the preflight path, before_compaction/after_compaction hooks therefore receive an incomplete trust context even though normal run hooks now include these fields, so plugins that gate behavior by origin can classify preflight compactions differently from the same session’s regular turns.
Useful? React with 👍 / 👎.
…ity-context # Conflicts: # src/auto-reply/reply/agent-runner-execution.ts
|
Codex automated review: keeping this open. Keep this PR open. Current main at e962381 still does not expose the requested trust identity fields on PluginHookAgentContext or the main agent/compaction hook contexts. The message-hook sessionKey portion is partly already present, but the core identity-context feature remains missing; this PR is also paired with the same-author open umbrella #36671 and has plugin-author comments showing real demand. Best possible solution: Keep this PR open for maintainer review or a narrowed rebase. The best path is an additive plugin SDK/context update that exposes sourceProvider, senderId, senderName, senderIsOwner, groupId, and spawnedBy on PluginHookAgentContext; derives a normalized origin provider consistently; preserves null semantics; threads the fields through early run, attempt, queued/memory, compaction, reset, and message-related hook contexts as appropriate; updates SDK/API/docs/tests; and drops any sessionKey message-hook hunks already implemented on main. What I checked:
Remaining risk / open question:
Codex Review notes: model gpt-5.5, reasoning high; reviewed against e962381dbf1e. |
# Conflicts: # src/auto-reply/reply/agent-runner-execution.ts # src/hooks/message-hook-mappers.ts # src/infra/outbound/deliver.ts # src/plugins/hook-message.types.ts
Summary
Enrich
PluginHookAgentContextwith sender identity fields that enable plugins to make trust decisions based on who triggered an agent run.New Fields
sourceProviderstring?"discord","slack","telegram") — preserves the original platform even after channel routingsenderIdstring?senderNamestring?senderIsOwnerboolean?groupIdstring?spawnedBystring?Also adds
sessionKeytoPluginHookMessageContext.Motivation
Security plugins (like our openclaw-provenance taint-tracking plugin) need to classify trust levels based on message origin. The current hook context provides
messageProviderandtrigger, but these don't capture:This follows the incremental pattern established in:
triggerandchannelId)sessionKeyto session hooks)cc @vincentkoc — this is part of a split from our larger hooks proposal. Following your lead on incremental, focused PRs.
AI-Assisted Development 🤖
This PR was developed with AI assistance (Claude/OpenClaw agent "Tank").
pnpm checkclean. Deployed locally and in use for ~4 weeks.Related: pre-LLM message interception
The sender identity and session context fields added here are automatically available to any hook that uses
PluginHookAgentContext— including thebefore_agent_replyhook proposed in #20067. That hook enables plugins to short-circuit the agent with a synthetic reply before the LLM runs. With sender identity context, abefore_agent_replyhandler can make trust-aware interception decisions: e.g., require approval flows for untrusted senders while letting the owner pass through, or route group-chat messages through different dialog wizards based on the sender's identity. Mutually beneficial — this PR enriches the context that #20067's hook consumes.