Skip to content

fix(agents): prefer explicit sessions_send keys#92047

Merged
vincentkoc merged 1 commit into
mainfrom
fix-sessions-send-sessionkey
Jun 10, 2026
Merged

fix(agents): prefer explicit sessions_send keys#92047
vincentkoc merged 1 commit into
mainfrom
fix-sessions-send-sessionkey

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 10, 2026

Copy link
Copy Markdown
Member

Summary

  • let explicit sessionKey targets win when stale label metadata is also present
  • keep denied session-id and thread-session-id paths from echoing resolved canonical session keys
  • carry forward the useful part of fix(agents): prefer sessionKey in sessions_send #74009 with the review/security fixes applied

Fixes #64699.
Refs #41199.
Supersedes #74009.

Validation

  • node scripts/run-vitest.mjs src/agents/tools/sessions.test.ts
  • git diff --check
  • ./node_modules/.bin/oxfmt --check --threads=1 CHANGELOG.md src/agents/tool-description-presets.ts src/agents/tools/sessions-send-tool.ts src/agents/tools/sessions.test.ts
  • direct check:changed script guards/lint/runtime/import lanes passed where runnable in this sparse Codex worktree
  • node scripts/check-changed.mjs --dry-run selected core, coreTests, docs; tsgo:core and tsgo:core:test were sparse-skipped by the repo guard because ui/config is not present in this worktree
  • .agents/skills/autoreview/scripts/autoreview --mode local

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S maintainer Maintainer-authored PR labels Jun 10, 2026
@vincentkoc vincentkoc force-pushed the fix-sessions-send-sessionkey branch from 131d0c2 to ab6cbeb Compare June 10, 2026 23:07
@vincentkoc @openclaw-clownfish
fix(agents): prefer explicit sessions_send keys
272776b 
Honor caller-provided sessionKey values when stale label metadata is also present, and keep denied session-id sends from echoing the resolved canonical session key.

Supersedes #74009 and fixes #64699.

Co-authored-by: openclaw-clownfish[bot] <280122609+openclaw-clownfish[bot]@users.noreply.github.com>
@vincentkoc vincentkoc force-pushed the fix-sessions-send-sessionkey branch from ab6cbeb to 272776b Compare June 10, 2026 23:18
@clawsweeper

clawsweeper Bot commented Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed June 10, 2026, 7:22 PM ET / 23:22 UTC.

Summary
The PR makes explicit sessionKey input override redundant label metadata, avoids echoing resolved canonical keys on selected denied paths, updates the model-facing description and snapshots, and adds focused regression tests.

PR surface: Source -6, Tests +104, Docs +1. Total +99 across 10 files.

Reproducibility: yes. Current main has a deterministic early rejection for any sessions_send invocation containing both sessionKey and label, exactly matching the reported failure path.

Review metrics: none identified.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🌊 off-meta tidepool
Patch quality: 🦐 gold shrimp
Result: ready for maintainer review.

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

Rank-up moves:

  • Remove the CHANGELOG.md modification.
  • Rerun the focused sessions test and git diff --check after cleanup.

Risk before merge

  • [P1] The direct CHANGELOG.md edit can conflict with release generation or create duplicate release notes if merged.

Maintainer options:

  1. Decide the mitigation before merge
    Remove the changelog line and retain the focused explicit-key precedence, denial non-disclosure behavior, regression tests, and generated prompt snapshots.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] A mechanical one-line cleanup is required before merge; after that, the protected maintainer-labeled PR can return to normal maintainer landing review.

Security
Cleared: The runtime change reduces canonical session-key disclosure and introduces no dependency, workflow, permission, secret-handling, or new code-execution surface.

Review findings

  • [P2] Remove the release-owned changelog entry — CHANGELOG.md:67
Review details

Best possible solution:

Remove the changelog line and retain the focused explicit-key precedence, denial non-disclosure behavior, regression tests, and generated prompt snapshots.

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

Yes. Current main has a deterministic early rejection for any sessions_send invocation containing both sessionKey and label, exactly matching the reported failure path.

Is this the best way to solve the issue?

Yes, after removing the changelog edit. Giving the explicit key deterministic precedence is narrower and more maintainable than adding parallel tools or complex schema alternatives, and the tests cover the associated disclosure-sensitive denial paths.

Full review comments:

  • [P2] Remove the release-owned changelog entry — CHANGELOG.md:67
    Normal PRs must not edit CHANGELOG.md; release generation owns this file. Remove this line to avoid conflicts or duplicate release notes, and retain the release-note context in the PR body or squash message.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

Codex review notes: reasoning high; reviewed against 1b23e738305c.

Label changes

Label changes:

  • add P2: This PR fixes a reproducible agent-to-agent message delivery failure with limited blast radius and one narrow pre-merge cleanup.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The author is a repository member, so the external-contributor real behavior proof gate does not apply; the submitted focused test validation is supplemental evidence.

Label justifications:

  • P2: This PR fixes a reproducible agent-to-agent message delivery failure with limited blast radius and one narrow pre-merge cleanup.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The author is a repository member, so the external-contributor real behavior proof gate does not apply; the submitted focused test validation is supplemental evidence.
Evidence reviewed

PR surface:

Source -6, Tests +104, Docs +1. Total +99 across 10 files.

View PR surface stats
Area Files Added Removed Net
Source 2 5 11 -6
Tests 7 120 16 +104
Docs 1 1 0 +1
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 10 126 27 +99

Acceptance criteria:

  • [P1] git diff --check.
  • [P1] node scripts/run-vitest.mjs src/agents/tools/sessions.test.ts.

What I checked:

Likely related people:

  • Peter Steinberger: The file shortlog shows the largest share of sessions-send implementation history, and commit 56e77f6 introduced the label lookup and persistence behavior relevant to this fix. (role: primary feature-history contributor; confidence: high; commits: 56e77f68432e; files: src/agents/tools/sessions-send-tool.ts)
  • shushushu: Commit 7926ace most recently carried the current resolution and visibility block on main during adjacent agent-runtime work. (role: recent area contributor; confidence: medium; commits: 7926acee98c3; files: src/agents/tools/sessions-send-tool.ts)
  • Vincent Koc: Current-main shortlog shows prior merged contributions to the sessions-send module beyond authorship of this proposal, indicating relevant implementation familiarity. (role: prior area contributor; confidence: medium; commits: 5181e4f7c82b; files: src/agents/tools/sessions-send-tool.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: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. labels Jun 10, 2026
@vincentkoc vincentkoc merged commit 3659ff8 into main Jun 10, 2026
184 of 188 checks passed
@vincentkoc vincentkoc deleted the fix-sessions-send-sessionkey branch June 10, 2026 23:26
@vincentkoc vincentkoc mentioned this pull request Jun 10, 2026
Closed
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 11, 2026
@vincentkoc @openclaw-clownfish
fix(agents): prefer explicit sessions_send keys (openclaw#92047)
dc598ca 
Honor caller-provided sessionKey values when stale label metadata is also present, and keep denied session-id sends from echoing the resolved canonical session key.

Supersedes openclaw#74009 and fixes openclaw#64699.

Co-authored-by: openclaw-clownfish[bot] <280122609+openclaw-clownfish[bot]@users.noreply.github.com>
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 maintainer Maintainer-authored PR P2 Normal backlog priority with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. size: S status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: sessions_send unexpectedly injects label, causing mutual-exclusion error with sessionKey

1 participant

@vincentkoc