WhatsApp: send commentary live#57279
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82d9235a28
ℹ️ 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 deliveredCommentarySegmentIds = new Set(params.deliveredCommentarySegmentIds ?? []); | ||
| const resolvedAssistantTexts = (() => { | ||
| if (params.assistantOutputs) { |
There was a problem hiding this comment.
Fall back to assistantTexts when outputs are missing
buildEmbeddedRunPayloads() now treats any defined assistantOutputs as authoritative, but runEmbeddedPiAgent() always passes this field (src/agents/pi-embedded-runner/run.ts:1253-1256), including the empty-array case. Because an empty array is truthy, the fallback to assistantTexts is skipped, so turns that stream text but never reach message_end (for example timeout/abort races where assistantOutputs is never populated) can lose all user-facing reply text and surface as empty/incomplete payloads instead of returning the captured stream content.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds opt-in live commentary delivery for WhatsApp ( Key changes:
Issues found:
Confidence Score: 4/5Safe to merge for the opt-in path; one P1 error-swallowing bug in the commentary wait loop should be addressed first. The feature is well-scoped and well-tested. The mutation bug in createCommentaryAbortError is a real P1: unexpected errors from waitForCommentaryDeliveryRound will have their name silently overwritten to AbortError, causing them to match isRunnerAbortError and be swallowed rather than re-thrown. The blast radius is narrow but it will make commentary-related incidents invisible in production. The other two findings are P2 and do not block merge. src/agents/pi-embedded-subscribe.ts (createCommentaryAbortError mutation); src/agents/pi-embedded-runner/run/payloads.ts (dead fallbackAnswerText branch)
|
| const createCommentaryAbortError = (message: string, reason?: unknown): Error => { | ||
| if (reason instanceof Error) { | ||
| reason.name = "AbortError"; | ||
| return reason; | ||
| } | ||
| const error = new Error(message); | ||
| error.name = "AbortError"; | ||
| return error; | ||
| }; |
There was a problem hiding this comment.
createCommentaryAbortError mutates the caller's error object
When reason instanceof Error, this function sets reason.name = "AbortError" in-place and returns the same object. This mutates the original error before the caller checks it. In waitForCommentaryDeliveryBounded inside attempt.ts, the pattern is:
} catch (err) {
abortCommentaryDelivery(err); // <-- mutates err.name = "AbortError" if err instanceof Error
if (err === timeoutError) { … }
if (isRunnerAbortError(err)) { … return; } // <-- always true now for any Error
throw err; // <-- never reached for Error instances
}Because isRunnerAbortError checks err.name === "AbortError", any unexpected Error thrown from waitForCommentaryDeliveryRound() will have its name quietly rewritten and be silently swallowed with log.debug instead of being re-thrown. The fix is to wrap the reason in a new error rather than mutating the original:
const createCommentaryAbortError = (message: string, reason?: unknown): Error => {
const error = new Error(message);
error.name = "AbortError";
if (reason instanceof Error) {
error.cause = reason;
}
return error;
};Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-embedded-subscribe.ts
Line: 627-635
Comment:
**`createCommentaryAbortError` mutates the caller's error object**
When `reason instanceof Error`, this function sets `reason.name = "AbortError"` in-place and returns the same object. This mutates the original error before the caller checks it. In `waitForCommentaryDeliveryBounded` inside `attempt.ts`, the pattern is:
```ts
} catch (err) {
abortCommentaryDelivery(err); // <-- mutates err.name = "AbortError" if err instanceof Error
if (err === timeoutError) { … }
if (isRunnerAbortError(err)) { … return; } // <-- always true now for any Error
throw err; // <-- never reached for Error instances
}
```
Because `isRunnerAbortError` checks `err.name === "AbortError"`, any unexpected Error thrown from `waitForCommentaryDeliveryRound()` will have its name quietly rewritten and be silently swallowed with `log.debug` instead of being re-thrown. The fix is to wrap the reason in a new error rather than mutating the original:
```ts
const createCommentaryAbortError = (message: string, reason?: unknown): Error => {
const error = new Error(message);
error.name = "AbortError";
if (reason instanceof Error) {
error.cause = reason;
}
return error;
};
```
How can I resolve this? If you propose a fix, please make it concise.| if (params.abortSignal) { | ||
| const upstreamAbort = () => { | ||
| abortController.abort(params.abortSignal?.reason); | ||
| }; | ||
| if (params.abortSignal.aborted) { | ||
| upstreamAbort(); | ||
| } else { | ||
| params.abortSignal.addEventListener("abort", upstreamAbort, { once: true }); | ||
| } |
There was a problem hiding this comment.
upstreamAbort listener is never explicitly removed
The listener is added with { once: true } so it self-removes when the signal fires, but in normal operation commentary delivery completes before the run-level abort signal fires. In a long-running session with many commentary segments, O(N) listeners accumulate on params.abortSignal until run end. Consider removing it in the .finally block alongside the other cleanup, mirroring the pattern used in deliverWebReply.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-embedded-subscribe.ts
Line: 660-668
Comment:
**`upstreamAbort` listener is never explicitly removed**
The listener is added with `{ once: true }` so it self-removes when the signal fires, but in normal operation commentary delivery completes before the run-level abort signal fires. In a long-running session with many commentary segments, O(N) listeners accumulate on `params.abortSignal` until run end. Consider removing it in the `.finally` block alongside the other cleanup, mirroring the pattern used in `deliverWebReply`.
How can I resolve this? If you propose a fix, please make it concise.|
Closing this for now because the implementation is intentionally based on the latest stable release line, not current This feature worked as expected in manual WhatsApp verification, but we are not going to fold unrelated |
Summary
commentaryand returns it together with the final answer, which makes live progress updates unusable in chat.channels.whatsapp.commentaryDelivery: "off" | "live", threaded a channel-agnosticonCommentaryReplyseam through the embedded runner, and delivered finalized commentary segments live through the existing WhatsApp send path.off.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
git blame, prior PR, issue, or refactor if known): clean replacement for WhatsApp: send commentary updates live #43731 after the earlier branch became too stale relative to current review expectations.Regression Test Plan (if applicable)
src/agents/pi-embedded-subscribe.commentary.test.ts,src/agents/pi-embedded-runner/run/payloads.test.ts,extensions/whatsapp/src/auto-reply/deliver-reply.test.ts,extensions/whatsapp/src/auto-reply/monitor/process-message.inbound-context.test.ts,extensions/telegram/src/bot-message-dispatch.test.tsUser-visible / Behavior Changes
channels.whatsapp.commentaryDeliveryis unset oroff.channels.whatsapp.commentaryDelivery: "live", assistantcommentaryis delivered to WhatsApp in real time as separate messages.Diagram (if applicable)
Security Impact (required)
Yes, explain risk + mitigation: N/ARepro + Verification
Environment
channels.whatsapp.commentaryDelivery = "live"Steps
channels.whatsapp.commentaryDelivery = "live".commentarybefore the final answer.Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
mainstartup behavior unrelated to this feature.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
channels.whatsapp.commentaryDelivery = "live"to opt in.Risks and Mitigations
subscribeEmbeddedPiSession()with stable segment IDs and targeted seam coverage.