fix(auto-reply): restore route replies for exec-event completions#70258
Conversation
Greptile SummaryThis PR fixes async Confidence Score: 5/5Safe to merge; the only finding is a minor test assertion gap that doesn't affect production behaviour. The implementation is correct and consistent with existing patterns. The null guard in No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/auto-reply/reply/dispatch-from-config.test.ts
Line: 466-472
Comment:
**Missing `accountId` assertion in new test**
The test sets up `deliveryContext.accountId: "acc-1"` and `AccountId: undefined` precisely to exercise the `routeAccountId` fallback — but the `routeReply` expectation never asserts `accountId`. The new `routeAccountId` variable is the other half of this fix (it's passed to four `routeReply` call-sites), so leaving it unverified means the test would still pass even if the account-ID propagation regressed.
```suggestion
expect(mocks.routeReply).toHaveBeenCalledWith(
expect.objectContaining({
channel: "telegram",
to: "telegram:999",
accountId: "acc-1",
}),
);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(auto-reply): route exec-event replie..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| expect(dispatcher.sendFinalReply).not.toHaveBeenCalled(); | ||
| expect(mocks.routeReply).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| channel: "telegram", | ||
| to: "telegram:999", | ||
| }), | ||
| ); |
There was a problem hiding this comment.
accountId assertion in new test
The test sets up deliveryContext.accountId: "acc-1" and AccountId: undefined precisely to exercise the routeAccountId fallback — but the routeReply expectation never asserts accountId. The new routeAccountId variable is the other half of this fix (it's passed to four routeReply call-sites), so leaving it unverified means the test would still pass even if the account-ID propagation regressed.
| expect(dispatcher.sendFinalReply).not.toHaveBeenCalled(); | |
| expect(mocks.routeReply).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| channel: "telegram", | |
| to: "telegram:999", | |
| }), | |
| ); | |
| expect(mocks.routeReply).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| channel: "telegram", | |
| to: "telegram:999", | |
| accountId: "acc-1", | |
| }), | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/dispatch-from-config.test.ts
Line: 466-472
Comment:
**Missing `accountId` assertion in new test**
The test sets up `deliveryContext.accountId: "acc-1"` and `AccountId: undefined` precisely to exercise the `routeAccountId` fallback — but the `routeReply` expectation never asserts `accountId`. The new `routeAccountId` variable is the other half of this fix (it's passed to four `routeReply` call-sites), so leaving it unverified means the test would still pass even if the account-ID propagation regressed.
```suggestion
expect(mocks.routeReply).toHaveBeenCalledWith(
expect.objectContaining({
channel: "telegram",
to: "telegram:999",
accountId: "acc-1",
}),
);
```
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: 3f07711469
ℹ️ 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".
| }); | ||
| const originatingTo = ctx.OriginatingTo; | ||
| const originatingTo = ctx.OriginatingTo ?? fallbackOriginatingTo; | ||
| const routeAccountId = ctx.AccountId ?? fallbackOriginatingAccountId; |
There was a problem hiding this comment.
For system-event turns, this change correctly derives routeAccountId from persisted delivery context, but the main final-reply path in sendFinalPayload still calls routeReply with ctx.AccountId instead of the fallback. When exec-event/cron-event/heartbeat completions lose live account metadata (the regression this commit targets), final replies can still route with an undefined or wrong account, which breaks multi-account channel delivery or applies incorrect per-account messaging config.
Useful? React with 👍 / 👎.
QC 多模型审核结果
有效票数: 2 票 主要问题测试缺少 建议补充accountId断言后再合并。 |
wzfukui
force-pushed
the
fix/openclaw-exec-event-delivery-context
branch
from
3f07711 to
863429f
Compare
|
Updated the PR branch against latest main, resolved the merge conflict, and added the requested routeReply accountId assertion for the persisted delivery-context fallback test. Local targeted test passed: pnpm test src/auto-reply/reply/dispatch-from-config.test.ts. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 863429fd24
ℹ️ 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".
| const effectiveOriginatingChannel = ctx.OriginatingChannel ?? fallbackOriginatingChannel; | ||
| const effectiveOriginatingTo = ctx.OriginatingTo ?? fallbackOriginatingTo; |
There was a problem hiding this comment.
When a heartbeat/cron-event/exec-event turn is missing live origin fields, this code now reconstructs effectiveOriginatingChannel/effectiveOriginatingTo from session state and routes outbound replies there, but sendPolicy is still resolved later from the raw context (ctx.OriginatingChannel ?? ctx.Surface ?? ctx.Provider). In that scenario, channel rules are evaluated against exec-event instead of the actual routed channel (for example, telegram), so channel-based deny rules can be bypassed and a reply is still delivered. Please feed the effective fallback route into policy resolution so suppression is evaluated against the true outbound destination.
Useful? React with 👍 / 👎.
|
Static review. LGTM-shape on the fallback direction. One design question + two gaps:
|
steipete
force-pushed
the
fix/openclaw-exec-event-delivery-context
branch
from
fd6c33c to
e85f6b6
Compare
4 participants
Summary
exec-eventreplies by falling back to persisted session delivery contextOriginatingChannelandOriginatingToare present on the live turnRoot cause
Async completion turns can lose direct channel metadata by the time the reply is generated. In that case the router currently skips channel route-reply delivery entirely, so channel plugins never receive the final completion message.
Testing
pnpm vitest run src/auto-reply/reply/dispatch-from-config.test.ts