Skip to content

fix: expose sessions_send requester return path#97311

Closed
henryyuen168-ship-it wants to merge 3 commits into
openclaw:mainfrom
henryyuen168-ship-it:fix/inter-session-return-path
Closed

fix: expose sessions_send requester return path#97311
henryyuen168-ship-it wants to merge 3 commits into
openclaw:mainfrom
henryyuen168-ship-it:fix/inter-session-return-path

Conversation

@henryyuen168-ship-it

@henryyuen168-ship-it henryyuen168-ship-it commented Jun 27, 2026 •
edited
Loading

Copy link
Copy Markdown

Summary

  • expose the concrete requester session key only in the initial sessions_send A2A target prompt as source-reply context
  • instruct A2A targets to return visible replies through message(action="send"), matching sourceReplyDeliveryMode: "message_tool_only"
  • keep shared ping-pong reply and final announce session lines placeholdered, and explicitly avoid reverse sessions_send back to the requester

Why

A WhatsApp group-originated Koro 2 request to Alfred lost its original return path. Alfred received placeholder context (<REQUESTER_SESSION>) and sent the result to agent:koro:main / internal UI instead of the originating WhatsApp group session (agent:koro:whatsapp:group:[email protected]).

Verification

  • RED: node scripts/test-projects.mjs src/agents/tools/sessions-send-helpers.test.ts failed on the missing concrete requester session before the fix.
  • GREEN: node scripts/test-projects.mjs src/agents/tools/sessions-send-helpers.test.ts src/agents/openclaw-tools.sessions.test.ts src/agents/tools/sessions-send-tool.a2a.test.ts passed 3 files / 65 tests after the source-reply follow-up.
  • GREEN: node scripts/test-projects.mjs src/agents/tools/sessions-send-helpers.test.ts src/agents/tools/sessions-send-tool.a2a.test.ts src/agents/tools/sessions.test.ts src/agents/tools/agent-step.test.ts src/agents/openclaw-tools.sessions.test.ts passed 5 files / 108 tests after the source-reply follow-up.
  • git diff --check -- src/agents/tools/sessions-send-helpers.ts src/agents/tools/sessions-send-helpers.test.ts src/agents/openclaw-tools.sessions.test.ts passed.

Real Behavior Proof

Redacted live relay test on 2026-06-27:

  • Source requester session: agent:koro:whatsapp:group:[email protected].
  • Target session: agent:alfred:main.
  • Alfred main resent the Sandweiss SAT packet back to the exact requester session during the live mitigation test.
  • Koro 2 posted visibly to the originating WhatsApp group exactly once.
  • Gateway evidence: WhatsApp group [email protected], message id 3EB02408D91490BA94EE86.
  • No self-route to agent:koro:main was observed in the relay closeout.

Review

ClawSweeper first flagged concrete requester keys leaking through shared reply/announce contexts. Commit 962de1b78b scoped concrete requester context to the initial target prompt only.

ClawSweeper then flagged the reverse sessions_send instruction as conflicting with the existing message_tool_only source-reply contract and duplicate-delivery guard work. Commit 883e75b0ee replaces the reverse-send instruction with message(action="send") guidance and adds regression expectations that the initial target prompt does not contain sessions_send(sessionKey:).

Risk

The requester session key is visible to the first A2A target run only as source context. Visible source-channel output stays on the message tool path, while reply and announce contexts remain placeholdered to reduce prompt churn and avoid requester-side self-routing hints.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jun 27, 2026
@clawsweeper

clawsweeper Bot commented Jun 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed June 27, 2026, 9:47 PM ET / 01:47 UTC.

Summary
The PR changes A2A sessions_send prompt context and tests so the initial target prompt includes requester return guidance through message(action="send") while reply and announce prompts keep placeholdered session lines.

PR surface: Source +11, Tests +68. Total +79 across 3 files.

Reproducibility: yes. at source level, with medium confidence: current main starts A2A target runs with placeholdered extra prompt context plus message_tool_only provenance, and the PR body reports a live WhatsApp relay failure/fix. I did not run an independent live WhatsApp or multi-agent reproduction in this read-only review.

Review metrics: 1 noteworthy metric.

  • Model-facing A2A prompt guidance: 1 concrete session-key exposure reintroduced, 1 return instruction added. The changed system prompt can affect both visible reply routing and target-session prompt-cache reuse before runtime tests catch it.

Stored data model
Persistent data-model change detected: serialized state: src/agents/tools/sessions-send-helpers.test.ts, serialized state: src/agents/tools/sessions-send-helpers.ts, unknown-data-model-change: src/agents/tools/sessions-send-helpers.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦞 diamond lobster
Patch quality: 🦐 gold shrimp
Result: needs maintainer review before merge.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Keep concrete requester session keys out of extraSystemPrompt while preserving the message(action="send") guidance.
  • Replace live-looking WhatsApp identifiers in committed tests and redact the PR proof text.

Risk before merge

  • [P1] The PR reintroduces concrete requester session keys into extraSystemPrompt, which current shipped code intentionally placeholders to preserve target-session prompt-cache reuse.
  • [P1] The branch and PR proof include live-looking WhatsApp group/message identifiers that should be replaced with synthetic fixtures and redacted proof text before merge.
  • [P1] The prompt-level instruction overlaps fix: prevent A2A target from reverse-calling sessions_send to requester (#39476) #91161; until the code-level guard lands, sessions_send remains available to A2A targets on current main.

Maintainer options:

  1. Keep the prompt stable before merge (recommended)
    Preserve the placeholdered requester session line in extraSystemPrompt, add only the message-tool return guidance, and update tests to prove the concrete key remains in provenance rather than the system prompt.
  2. Redact sensitive-looking identifiers
    Replace the committed WhatsApp group fixture with synthetic values and have the author or a maintainer redact equivalent group/message identifiers from the PR proof text.
  3. Coordinate with the reverse-send guard
    Maintainers may pause this prompt repair until fix: prevent A2A target from reverse-calling sessions_send to requester (#39476) #91161 is resolved if they want prompt guidance and tool-surface guarding reviewed together.

Next step before merge

  • [P2] A narrow branch repair can keep A2A extra prompts stable and synthetic, but PR-body proof redaction and final coordination with the reverse-send guard still need author or maintainer attention.

Security
Needs attention: The code-execution surface is unchanged, but the branch commits live-looking WhatsApp identifiers and expands concrete requester-session exposure in model-facing prompt context.

Review findings

  • [P2] Keep requester keys out of the system prompt — src/agents/tools/sessions-send-helpers.ts:63-66
  • [P2] Replace the live WhatsApp fixture with synthetic IDs — src/agents/tools/sessions-send-helpers.test.ts:125
Review details

Best possible solution:

Keep the message(action="send") source-reply guidance, keep requester/target session keys placeholdered in extraSystemPrompt, rely on existing provenance/runtime routing for concrete source context, redact fixtures/proof, and coordinate with #91161 for the code-level reverse-send guard.

Do we have a high-confidence way to reproduce the issue?

Yes at source level, with medium confidence: current main starts A2A target runs with placeholdered extra prompt context plus message_tool_only provenance, and the PR body reports a live WhatsApp relay failure/fix. I did not run an independent live WhatsApp or multi-agent reproduction in this read-only review.

Is this the best way to solve the issue?

No as written: adding explicit message(action="send") guidance is the right direction, but moving the concrete requester key into extraSystemPrompt regresses the stable prompt-cache contract. The narrower fix is to keep the placeholdered session line and rely on provenance/runtime routing for the concrete return path.

Full review comments:

  • [P2] Keep requester keys out of the system prompt — src/agents/tools/sessions-send-helpers.ts:63-66
    This puts the concrete requester session key back into extraSystemPrompt, but current shipped code intentionally keeps those high-cardinality values placeholdered because the CLI runner hashes extraSystemPrompt for session reuse and prompt-cache stability. Add the message(action="send") guidance without changing the requester session line; the concrete source session is already available through provenance and the annotated inter-session prompt.
    Confidence: 0.87
  • [P2] Replace the live WhatsApp fixture with synthetic IDs — src/agents/tools/sessions-send-helpers.test.ts:125
    This test fixture appears to copy the live WhatsApp group identifier from the PR proof. Real private channel identifiers should not be committed; use a synthetic group/session key while keeping the source-reply assertions.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.87

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 78f7de01c1aa.

Label changes

Label justifications:

  • P2: This is a focused A2A message-routing bug fix with limited blast radius, but the PR still needs prompt-cache and identifier-redaction repairs before merge.
  • merge-risk: 🚨 message-delivery: The PR changes model-facing A2A return guidance while the reverse sessions_send tool surface remains available, so a bad prompt boundary could still misroute or duplicate visible replies.
  • merge-risk: 🚨 security-boundary: The branch commits a live-looking WhatsApp group identifier and expands concrete requester-session exposure in model-facing prompt context.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body provides after-fix live relay output showing the requester session, target session, one visible source-channel reply, and no self-route; identifier redaction remains a separate security concern.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix live relay output showing the requester session, target session, one visible source-channel reply, and no self-route; identifier redaction remains a separate security concern.
Evidence reviewed

PR surface:

Source +11, Tests +68. Total +79 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 1 18 7 +11
Tests 2 74 6 +68
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 92 13 +79

Security concerns:

  • [medium] Live-looking WhatsApp identifier in committed fixture — src/agents/tools/sessions-send-helpers.test.ts:125
    The test fixture contains a WhatsApp group session key matching the PR proof narrative; committed tests should use synthetic IDs, and the public proof text should redact group/message identifiers.
    Confidence: 0.9

Acceptance criteria:

  • [P1] node scripts/test-projects.mjs src/agents/tools/sessions-send-helpers.test.ts src/agents/openclaw-tools.sessions.test.ts src/agents/tools/sessions-send-tool.a2a.test.ts.
  • [P1] git diff --check -- src/agents/tools/sessions-send-helpers.ts src/agents/tools/sessions-send-helpers.test.ts src/agents/openclaw-tools.sessions.test.ts.

What I checked:

  • Root and scoped policy read: Root AGENTS.md and the scoped agent/tool guides were read and applied because this PR changes agent tool prompt context, message delivery guidance, proof handling, and sensitive identifier exposure. (AGENTS.md:1, 78f7de01c1aa)
  • PR head reintroduces requester key into system prompt: At PR head, buildAgentSessionLines renders the concrete requester session key when includeRequesterReturnInstruction is set, so the target run's extra system prompt varies by requester session. (src/agents/tools/sessions-send-helpers.ts:62, 883e75b0ee29)
  • Current and shipped behavior placeholders session keys: Current main and v2026.6.10 keep requester/target session keys placeholdered in A2A extra system prompt, with an inline rationale that concrete high-cardinality keys churn prompt-cache reuse. (src/agents/tools/sessions-send-helpers.ts:62, 78f7de01c1aa)
  • Concrete source metadata already exists outside extraSystemPrompt: The sessions_send target run already carries sourceSessionKey in inputProvenance and annotates the user message with sourceSession=..., so the concrete value does not need to be moved into the stable system prompt. (src/agents/tools/sessions-send-tool.ts:585, 78f7de01c1aa)
  • Message-tool-only routing contract: Current main already tells message-tool-only turns to send visible replies with message(action=send) and to omit target because it defaults to the current source channel/conversation. (src/agents/system-prompt.ts:525, 78f7de01c1aa)
  • Extra system prompt participates in CLI reuse hashing: The CLI runner hashes extraSystemPrompt for reusable session selection, so adding requester-specific keys there can invalidate otherwise reusable target sessions. (src/agents/cli-runner/prepare.ts:349, 78f7de01c1aa)

Likely related people:

  • Sunjae-k: Merged PR fix(agents): stabilize a2a prompt cache context #90173 intentionally placeholdered A2A session keys in extraSystemPrompt while keeping concrete routing metadata in provenance. (role: introduced stable prompt-cache behavior; confidence: high; commits: 5d42ad6654a8; files: src/agents/tools/sessions-send-helpers.ts, src/agents/tools/sessions-send-helpers.test.ts, src/agents/openclaw-tools.sessions.test.ts)
  • vincentkoc: Recent path history shows merged sessions_send routing and explicit-key work in the same send path affected by this PR. (role: recent sessions_send contributor; confidence: high; commits: 3659ff8bbf71, 0ea08076c3b5; files: src/agents/tools/sessions-send-tool.ts, src/agents/tools/message-tool.ts)
  • steipete: History shows earlier A2A/session helper and inter-session provenance work that this prompt and routing path relies on. (role: feature-history contributor; confidence: high; commits: 817e6e810b7d, 041fab7b7275; files: src/agents/tools/sessions-send-tool.ts, src/agents/tools/sessions-send-tool.a2a.ts, src/sessions/input-provenance.ts)
  • jalehman: Authored the merged sessions_send announce thread delivery fix in the same A2A announce lifecycle. (role: adjacent announce-routing contributor; confidence: medium; commits: 8359e5f584d3; files: src/agents/tools/sessions-send-tool.a2a.ts, src/agents/tools/sessions-announce-target.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jun 27, 2026
@henryyuen168-ship-it

henryyuen168-ship-it commented Jun 27, 2026

Copy link
Copy Markdown
Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 27, 2026
@henryyuen168-ship-it

henryyuen168-ship-it commented Jun 28, 2026

Copy link
Copy Markdown
Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 28, 2026
This was referenced Jul 1, 2026
Closed
Merged
Open
Open
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 16, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 16, 2026
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 23, 2026

Copy link
Copy Markdown

Closing due to inactivity.
If you believe this PR should be revived, post in #clawtributors on Discord to talk to a maintainer.
That channel is the escape hatch for high-quality PRs that get auto-closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. size: S stale Marked as stale due to inactivity status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@henryyuen168-ship-it