hooks: support { skip: true } in before_agent_start to prevent agent run#32802
hooks: support { skip: true } in before_agent_start to prevent agent run#32802haosenwang1018 wants to merge 5 commits intoopenclaw:mainfrom
Conversation
… instead of cancelling
Greptile SummaryThis PR adds Beyond the headline feature, the PR also ships four unrelated but independently sensible fixes:
Key findings:
Confidence Score: 3/5
Last reviewed commit: 956838b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return undefined; | ||
| } | ||
| return context.currentThreadTs; |
There was a problem hiding this comment.
Case-insensitive Matrix room ID comparison
Matrix room IDs are case-sensitive by the Matrix specification (e.g., !abc123:matrix.org and !ABC123:matrix.org are distinct rooms). The .toLowerCase() comparison on both sides could cause a room mismatch if room IDs happen to differ only by case — while unlikely in practice, it's a silent correctness risk.
The Telegram equivalent (resolveTelegramAutoThreadId) does a direct equality comparison without case-folding, which is the correct default for these identifiers.
| return undefined; | |
| } | |
| return context.currentThreadTs; | |
| if (normalizedTo !== normalizedChannel) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/outbound/message-action-params.ts
Line: 108-110
Comment:
**Case-insensitive Matrix room ID comparison**
Matrix room IDs are case-sensitive by the Matrix specification (e.g., `!abc123:matrix.org` and `!ABC123:matrix.org` are distinct rooms). The `.toLowerCase()` comparison on both sides could cause a room mismatch if room IDs happen to differ only by case — while unlikely in practice, it's a silent correctness risk.
The Telegram equivalent (`resolveTelegramAutoThreadId`) does a direct equality comparison without case-folding, which is the correct default for these identifiers.
```suggestion
if (normalizedTo !== normalizedChannel) {
```
How can I resolve this? If you propose a fix, please make it concise.| const summary = | ||
| (preparation.previousSummary ?? "No conversation history to summarize.") + fileOpsSummary; |
There was a problem hiding this comment.
previousSummary empty-string edge case
The ?? (nullish coalescing) operator only falls back for null / undefined. If preparation.previousSummary is "" (an empty string set by an upstream caller), the fallback text "No conversation history to summarize." is skipped and summary becomes "" + fileOpsSummary. This could leave the stored summary starting with file-op content and no human-readable header.
Consider using the logical-OR operator (or an explicit falsy check) if an empty string should also trigger the fallback:
| const summary = | |
| (preparation.previousSummary ?? "No conversation history to summarize.") + fileOpsSummary; | |
| const summary = | |
| (preparation.previousSummary || "No conversation history to summarize.") + fileOpsSummary; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-extensions/compaction-safeguard.ts
Line: 219-220
Comment:
**`previousSummary` empty-string edge case**
The `??` (nullish coalescing) operator only falls back for `null` / `undefined`. If `preparation.previousSummary` is `""` (an empty string set by an upstream caller), the fallback text `"No conversation history to summarize."` is skipped and `summary` becomes `"" + fileOpsSummary`. This could leave the stored summary starting with file-op content and no human-readable header.
Consider using the logical-OR operator (or an explicit falsy check) if an empty string should also trigger the fallback:
```suggestion
const summary =
(preparation.previousSummary || "No conversation history to summarize.") + fileOpsSummary;
```
How can I resolve this? If you propose a fix, please make it concise.| (acc, next) => ({ | ||
| ...mergeBeforePromptBuild(acc, next), | ||
| ...mergeBeforeModelResolve(acc, next), | ||
| skip: acc?.skip || next.skip, |
There was a problem hiding this comment.
skip merge produces false when both sides are false
acc?.skip || next.skip evaluates to false (not undefined) when both values are explicitly false. While the downstream consumer (if (legacyBeforeAgentStartResult?.skip)) only checks truthiness and is unaffected, this differs from the behaviour of all other optional fields on the result (which remain undefined when absent). It's a minor inconsistency but worth noting in case other consumers inspect the property more strictly.
A more explicit form that preserves undefined semantics:
| skip: acc?.skip || next.skip, | |
| skip: (acc?.skip || next.skip) || undefined, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/hooks.ts
Line: 308
Comment:
**`skip` merge produces `false` when both sides are `false`**
`acc?.skip || next.skip` evaluates to `false` (not `undefined`) when both values are explicitly `false`. While the downstream consumer (`if (legacyBeforeAgentStartResult?.skip)`) only checks truthiness and is unaffected, this differs from the behaviour of all other optional fields on the result (which remain `undefined` when absent). It's a minor inconsistency but worth noting in case other consumers inspect the property more strictly.
A more explicit form that preserves `undefined` semantics:
```suggestion
skip: (acc?.skip || next.skip) || undefined,
```
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 956838b0bd
ℹ️ 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".
| if (normalizedTo.toLowerCase() !== normalizedChannel.toLowerCase()) { | ||
| return undefined; | ||
| } | ||
| return context.currentThreadTs; |
There was a problem hiding this comment.
resolveMatrixAutoThreadId unconditionally returns currentThreadTs for same-room sends, but Matrix tool context can set currentThreadTs from ReplyToId when the inbound message is a plain reply (not a thread). In that case, message actions with replyToMode=first|all will now inject a threadId and send as m.thread replies in the wrong context, instead of normal room replies; this changes delivery behavior for non-thread conversations.
Useful? React with 👍 / 👎.
| ((ctx: ChannelOutboundContext) => { | ||
| log.warn("sendMedia not implemented; degrading to sendText", { | ||
| channel: params.channel, | ||
| }); | ||
| return sendText({ ...ctx, mediaUrl: undefined }); |
There was a problem hiding this comment.
The new sendMedia fallback always calls sendText, but outbound delivery invokes sendMedia once per media URL and only the first URL carries the caption. For text-only adapters (no sendMedia), payloads with multiple media URLs now trigger extra empty text sends (or channel-side validation failures on blank messages) for the second and later items, which is a regression from expected single-message degradation behavior.
Useful? React with 👍 / 👎.
|
Strong +1 from a production user. I'm running OpenClaw on a Mac mini with Claude Max ($200/mo subscription) + local Ollama (Qwen 3.5:4b). I built a The classification works perfectly (0.98 confidence, 2.3s latency), but Real-world routing stats from testing: ~60-70% of messages are simple enough for local Qwen. That's a lot of wasted Claude tokens on "what time is it" and "turn off the lamp." Would love to see this merged. |
2 participants
Summary
before_agent_starthook can only modify agent runs (systemPrompt, prependContext, modelOverride) but cannot skip them entirely. This forces group message monitoring plugins to trigger a full LLM agent run for every observed message, wasting resources and bloating session history.skip?: booleantoPluginHookBeforeAgentStartResult. When abefore_agent_starthook returns{ skip: true }, the agent run is skipped entirely — no LLM call, no session write. The run returns immediately with an empty payload.conversationIdinmessage_sendingctx) is not addressed here.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
{ skip: true }frombefore_agent_starthook handlers to skip the agent run entirely. No LLM call is made and no session state is written.Security Impact (required)
NoNoNoNoNoRepro + Verification
Environment
Steps
before_agent_starthook that returns{ skip: true }Expected
Actual
Evidence
Human Verification (required)
hooks.before-agent-start.test.ts(10 existing + 6 new)Compatibility / Migration
YesNoNoFailure Recovery (if this breaks)
{ skip: true }or the commit is revertedRisks and Mitigations
{ skip: true }unconditionally would silently prevent all agent runs.[hooks] before_agent_start returned skip=true; skipping agent run) so operators can diagnose via logs.