Skip to content

fix: prefer sessionKey over label in sessions_send#39551

Closed
1034378361 wants to merge 2 commits into
openclaw:mainfrom
1034378361:fix/sessions-send-sessionkey-priority
Closed

fix: prefer sessionKey over label in sessions_send#39551
1034378361 wants to merge 2 commits into
openclaw:mainfrom
1034378361:fix/sessions-send-sessionkey-priority

Conversation

@1034378361

@1034378361 1034378361 commented Mar 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Summary

  • Problem: sessions_send rejected calls when sessionKey and label were both present, even if sessionKey already identified the target unambiguously.
  • Why it matters: LLM-generated tool calls can include redundant target fields (sessionKey, label, and agentId) from the same schema, which caused avoidable runtime failures.
  • What changed: sessions_send now treats sessionKey as authoritative and ignores label / agentId when sessionKey is present. Added a regression test for the mixed-parameter case.
  • What did NOT change (scope boundary): This does not change label-only resolution behavior, cross-agent permission checks, or the missing-target error path.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

  • Closes #
  • Related #

User-visible / Behavior Changes

  • sessions_send no longer fails with Provide either sessionKey or label (not both). when sessionKey is present alongside label / agentId.
  • When sessionKey is provided, it is now used as the source of truth for target selection.

Security Impact (required)

  • New permissions/capabilities? (No)
  • Secrets/tokens handling changed? (No)
  • New/changed network calls? (No)
  • Command/tool execution surface changed? (No)
  • Data access scope changed? (No)
  • If any Yes, explain risk + mitigation:

Repro + Verification

Environment

  • OS: Linux
  • Runtime/container: local source checkout
  • Model/provider: N/A
  • Integration/channel (if any): N/A
  • Relevant config (redacted): default test config with mocked gateway calls

Steps

  1. Call sessions_send with sessionKey, label, and agentId together.
  2. Observe current behavior before the fix: the tool rejects the call with Provide either sessionKey or label (not both).
  3. Apply the fix and rerun the sessions tool tests.

Expected

  • If sessionKey is present, sessions_send should use it directly.
  • Redundant label / agentId should not cause the call to fail.
  • label-only resolution should still work as before.
  • Missing target should still return Either sessionKey or label is required.

Actual

  • After this change, mixed-input calls succeed by resolving through sessionKey.
  • Existing label-only and missing-target behavior remains intact.

Evidence

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

  • Verified scenarios:
    • corepack pnpm exec vitest run src/agents/tools/sessions.test.ts
    • corepack pnpm exec vitest run src/agents/openclaw-tools.sessions.test.ts
    • Confirmed the new regression test passes:
      • prefers sessionKey when sessionKey, label, and agentId are all provided
  • Edge cases checked:
    • label-only behavior remains covered
    • missing-target error remains Either sessionKey or label is required
    • higher-level sessions tools tests still pass
  • What you did not verify:
    • Full end-to-end behavior through a live GitHub CI run
    • Additional gateway-specific test files beyond the focused sessions tool coverage

Compatibility / Migration

  • Backward compatible? (Yes)
  • Config/env changes? (No)
  • Migration needed? (No)
  • If yes, exact upgrade steps:

Failure Recovery (if this breaks)

  • How to disable/revert this change quickly:
    • Revert this commit.
  • Files/config to restore:
    • src/agents/tools/sessions-send-tool.ts
    • src/agents/tools/sessions.test.ts
  • Known bad symptoms reviewers should watch for:
    • Unexpected target selection changes when sessionKey is present
    • Regressions in label-only resolution

Risks and Mitigations

  • Risk: A caller may accidentally pass mismatched sessionKey and label, and the tool will now prefer sessionKey.
    • Mitigation: This is intentional and aligns behavior with the most specific target identifier; the regression test documents the precedence rule.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S labels Mar 8, 2026
@greptile-apps

greptile-apps Bot commented Mar 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a regression where sessions_send rejected calls that included both sessionKey and label/agentId, even though sessionKey alone was sufficient to identify the target. The root cause was a strict mutual-exclusion guard that produced a runtime error for LLM-generated tool calls that naturally echo all available target fields.

Changes:

  • Removes the sessionKey + label → error guard and replaces it with a precedence rule: when sessionKey is present, labelParam and labelAgentIdParam are cleared before the label-resolution branch executes.
  • Adds .trim() || undefined normalization to sessionKeyParam, consistent with how labelParam and labelAgentIdParam were already handled — prevents a whitespace-only sessionKey from triggering the wrong branch.
  • Adds a regression test (prefers sessionKey when sessionKey, label, and agentId are all provided) that verifies sessions.resolve is not called and the agent call uses the correct sessionKey.
  • Updates the tool description to reflect the new precedence behavior.
  • Refactors visibility checking into a dedicated resolveVisibleSessionReference helper for code clarity.

One thing to be aware of: When sessionKey and label are both present but point to different targets, the tool now silently ignores label and uses sessionKey. There is no warning surfaced to the caller. The PR author acknowledged this trade-off, and the result object already includes the resolved sessionKey so callers can verify which session was targeted.

All downstream permission checks (resolveVisibleSessionReference, visibility guards) remain in place.

Confidence Score: 4/5

  • Safe to merge — the change is small, well-scoped, and all downstream authorization checks remain in place.
  • The fix is targeted and correct: the sessionKey-only path already bypassed the label-resolution branch before this PR, so clearing labelParam/labelAgentIdParam when sessionKey is present does not introduce a new code path — it just prevents the old mutual-exclusion guard from firing. The regression test covers the core scenario and all permission checks are preserved. Minor gap: no explicit test for the case where sessionKey is present but agentToAgent.enabled is false (to confirm the visibility guard still blocks it), but this is a pre-existing behavioral path, not something changed by this PR. No files require special attention.
  • No files require special attention.

Last reviewed commit: 852bbe2

@1034378361

1034378361 commented Mar 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Adding a small clarification on the intended precedence rule: the sessionKey-first behavior in this patch is deliberate, not an accidental side effect.

The motivation is compatibility with real LLM-generated tool calls, which often echo multiple target-identifying fields from the same schema (sessionKey, label, and agentId) in a single call. In that situation, sessionKey is the most specific identifier we already have, so the patch treats it as authoritative instead of rejecting the call.

This does not broaden access or bypass existing checks:

  • label-only resolution still follows the existing sessions.resolve path
  • visibility / sandbox / agent-to-agent permission checks are unchanged
  • the missing-target error path is unchanged

So the behavior change is intentionally narrow: when sessionKey is present, prefer it and ignore redundant label / agentId fields rather than fail on a mixed-input call.

@AlexAnys AlexAnys mentioned this pull request Mar 15, 2026
Closed
20 tasks
@openclaw-barnacle

openclaw-barnacle Bot commented Apr 24, 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 Apr 24, 2026
@clawsweeper

clawsweeper Bot commented Apr 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close as superseded. The mixed-selector bug is still real on current main, but the active replacement PR #74009 covers the same runtime, test, tool-description, and changelog surfaces, while this branch still has concrete security and test-isolation blockers.

So I’m closing this here and keeping the remaining discussion on the canonical linked item.

Review details

Best possible solution:

Finish the fix in one canonical branch, preferably #74009 or its successor: remove the mixed sessionKey+label rejection, keep the public selector contract narrow, avoid returning resolved canonical keys on denied sends, and isolate the new tests before running the focused sessions test and changed gate.

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

Yes. Source inspection on current main gives a high-confidence reproduction path: call sessions_send with non-empty sessionKey and label, and the guard at src/agents/tools/sessions-send-tool.ts:109 returns the reported error before target resolution.

Is this the best way to solve the issue?

No as this branch stands. Preferring sessionKey is the right narrow direction, but this PR is superseded by #74009 and still needs the denied-key disclosure and persistent mock implementation issues resolved before any equivalent patch should merge.

Security review:

Security review needs attention: Security review needs attention because the diff can turn currently rejected mixed-selector calls into a session-id resolution path that echoes a resolved key after access denial.

  • [medium] Resolved session key disclosure on denied sends — src/agents/tools/sessions-send-tool.ts:103
    The patch removes the early mixed-selector rejection and adds a sessionId selector path. If a session-id-shaped selector resolves but visibility or A2A policy denies the send, the existing denial response can return the resolved displayKey, exposing session existence and the canonical key to a caller that was not allowed to send.
    Confidence: 0.82

What I checked:

  • Current main still rejects mixed selectors: sessions_send on current main reads sessionKey, label, and agentId, then returns Provide either sessionKey or label (not both). when both sessionKey and label are present. (src/agents/tools/sessions-send-tool.ts:109, 53593f0683fa)
  • Session-id-shaped values resolve before access checks: The shared resolver treats non-key or session-id-looking input as a gateway sessions.resolve lookup, so a sessionId value can be converted into a canonical key before send visibility/A2A denial runs. (src/agents/tools/sessions-resolution.ts:417, 53593f0683fa)
  • Denied send responses echo the resolved display key: After resolution, denied visibility/A2A sends return sessionKey: displayKey; with the PR's mixed-selector path this can expose a canonical key for a session-id-shaped selector that current main rejects earlier. (src/agents/tools/sessions-send-tool.ts:248, 53593f0683fa)
  • PR diff broadens the selector surface: The PR head adds a separate sessionId schema field and makes readStringParam(params, "sessionKey") ?? readStringParam(params, "sessionId") authoritative over label and agentId. (src/agents/tools/sessions-send-tool.ts:103, 4a08e3c5bb8d)
  • PR tests leave persistent gateway mock implementations installed: The added mixed-selector tests use callGatewayMock.mockImplementation(...), while the sessions_send gating beforeEach only calls mockClear(), so later tests in the same describe can inherit the implementation. (src/agents/tools/sessions.test.ts:654, 4a08e3c5bb8d)
  • Active canonical replacement exists: Open PR fix(agents): prefer sessionKey in sessions_send #74009 changes the same four surfaces for the same sessions_send precedence behavior, keeps session-id values under the existing sessionKey field, and its body/patch explicitly supersede overlapping contributor work. (115455ea2224)

Likely related people:

  • steipete: Current blame for the sessions_send selector/denial region points to recent maintenance, and recent inter-session prompt work also modified the same send path. (role: recent maintainer; confidence: high; commits: 0d7d1aa09cf3, c5c08c074a44; files: src/agents/tools/sessions-send-tool.ts, src/agents/tool-description-presets.ts)
  • scotthuang: Authored recent merged A2A skip/ownership changes in sessions_send, directly adjacent to the visibility-sensitive behavior affected by this PR. (role: adjacent owner; confidence: medium; commits: 8a7c21407ae3, 1c3fbbd72a01; files: src/agents/tools/sessions-send-tool.ts, src/acp/session-interaction-mode.ts)
  • Takhoffman: Recent sessions_send reply carryover and test work touched the same runtime and test file involved in this PR's behavior and test-isolation issue. (role: adjacent owner; confidence: medium; commits: 5b4669632a9d, 2c1d16e261da, 6651511e90e5; files: src/agents/tools/sessions-send-tool.ts, src/agents/tools/sessions.test.ts, src/agents/tools/sessions-resolution.ts)
  • vincentkoc: Pushed the repair commit on this branch and has recent adjacent agent/session helper work, but the strongest current-main ownership trail points to the other sessions_send maintainers. (role: adjacent maintainer; confidence: low; commits: 4a08e3c5bb8d, 74e7b8d47b18; files: src/agents/tools/sessions-send-tool.ts, src/agents/tools/sessions.test.ts, src/agents/tools/sessions-resolution.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 53593f0683fa.

@openclaw-barnacle openclaw-barnacle Bot removed the stale Marked as stale due to inactivity label Apr 26, 2026
This was referenced Apr 27, 2026
Closed
Closed
@vincentkoc

vincentkoc commented Apr 28, 2026

Copy link
Copy Markdown
Member

ProjectClownfish pushed a narrow repair to this branch so the original contributor path can stay canonical.

Source PR: #39551
Validation: pnpm -s vitest run src/agents/tools/sessions.test.ts; pnpm check:changed
Contributor credit is preserved in the branch history and PR context.

@vincentkoc vincentkoc force-pushed the fix/sessions-send-sessionkey-priority branch from 852bbe2 to 4a08e3c Compare April 28, 2026 18:31
@vincentkoc vincentkoc added clownfish:human-review clawsweeper Tracked by ClawSweeper automation and removed clownfish:human-review labels Apr 28, 2026
@openclaw-clownfish openclaw-clownfish Bot mentioned this pull request Apr 29, 2026
Closed
@clawsweeper clawsweeper Bot mentioned this pull request Apr 29, 2026
Closed
This was referenced Apr 29, 2026
Closed
Closed
@clawsweeper clawsweeper Bot closed this May 1, 2026
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 clawsweeper Tracked by ClawSweeper automation size: S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@1034378361 @vincentkoc