Skip to content

Reject split outbound agent session ownership#96078

Open
youngting520 wants to merge 2 commits into
openclaw:mainfrom
youngting520:bug/outbound-owner-mismatch
Open

Reject split outbound agent session ownership#96078
youngting520 wants to merge 2 commits into
openclaw:mainfrom
youngting520:bug/outbound-owner-mismatch

Conversation

@youngting520

@youngting520 youngting520 commented Jun 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes #96075.

Outbound send/action paths could accept an explicit agentId while also carrying an agent-scoped session key owned by another agent. That could split ownership across media policy, plugin dispatch, transcript mirroring, queueing, and internal hook correlation.

Why This Change Was Made

This adds a shared outbound ownership guard that parses agent:<owner>:... session keys and compares only the normalized owner segment against explicit agent owners. The guard is applied at both Gateway boundaries and lower outbound helpers so internal callers cannot bypass the Gateway check.

policyKey remains intentionally excluded from equality checks because routed ACP-style delivery can use a separate policy/origin session while the canonical session/mirror keys still describe the owning agent.

User Impact

Contradictory outbound requests such as agentId: "work" with sessionKey: "agent:main:..." now fail before dispatch, media resolution, durable queueing, mirroring, or hook correlation. Omitted/blank agent ids, legacy session keys, same-agent different conversation keys, and cross-agent delivery policy keys remain supported.

Compatibility and Stored Queue Behavior

This intentionally changes contradictory split-owner outbound requests from accepted dispatch to fail-closed validation. The rejected shape is an explicit agent owner paired with an agent-scoped session key owned by a different agent, for example agentId: "work" with sessionKey: "agent:main:slack:channel:c1".

I am not preserving explicit-agent precedence for that contradictory shape because it can route media policy, plugin dispatch, transcript mirroring, queueing, and hook correlation under different owners. Same-agent session keys, omitted or blank agent ids, legacy/non-agent session keys, and cross-agent policyKey routing remain allowed.

There is no serialized queue-shape, schema-version, or migration change. Recovery validates the existing session and mirror fields. A previously stored entry whose canonical owners disagree is permanently marked failed through the existing queue failure path instead of being replayed across agent boundaries. This is the intended compatibility behavior; policyKey remains excluded from the recovery equality check.

Evidence

Exact-head behavior proof

Exact reviewed head:

HEAD=78e7ee8724ba103ed24686f3bf37cd85f1e0e318

The canonical PR CI completed successfully on that exact SHA: https://github.com/openclaw/openclaw/actions/runs/29933742723

Selected terminal output from the exact-head Node shards:

✓ src/gateway/server-methods/send.test.ts (84 tests)
✓ src/infra/outbound/message-action-runner.send-validation.test.ts (33 tests)
✓ src/infra/outbound/deliver.test.ts (163 tests)
✓ src/infra/outbound/delivery-queue.recovery.test.ts (53 tests)
✓ src/infra/outbound/delivery-queue.reconnect-drain.test.ts (19 tests)
✓ src/infra/outbound/outbound-send-service.test.ts (32 tests)
✓ src/infra/outbound/message.test.ts (16 tests)
✓ src/infra/outbound/session-owner.test.ts (8 tests)
CI workflow: 58 jobs succeeded, 11 scope-disabled jobs skipped, 0 failed

Those exact-head suites exercise the final side-effect boundaries:

  • deliver.test.ts rejects mismatched canonical ownership before queue or platform I/O and asserts zero enqueue/platform calls.
  • deliver.test.ts allows same-owner control and mirror keys with a cross-agent policyKey and reaches the adapter once.
  • delivery-queue.recovery.test.ts permanently fails mismatched or malformed stored entries before admission, backoff, adapter lookup, or replay.
  • delivery-queue.recovery.test.ts replays a valid stored entry whose canonical owner is main while its policyKey belongs to policy-owner.
  • Gateway and message-action tests reject mismatches before channel/plugin discovery.

Additional checks

  • git diff --check
  • Exact-head canonical PR CI, including build artifacts, prod/test types, lint, guards, docs, Gateway methods, outbound delivery, and queue recovery
  • .agents/skills/autoreview/scripts/autoreview --mode uncommitted for the recovery contract follow-up: clean, no accepted/actionable findings

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: L labels Jun 23, 2026
@youngting520
youngting520 marked this pull request as ready for review June 23, 2026 11:04
@youngting520

youngting520 commented Jun 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi maintainers, this PR is ready for review when you have bandwidth.

Summary:

I intentionally left policyKey out of the equality check because routed delivery can use a separate policy/origin session. If this approach looks right, I’d appreciate maintainer review and landing guidance.

@youngting520
youngting520 force-pushed the bug/outbound-owner-mismatch branch from 01f18f2 to 0b58b05 Compare June 23, 2026 14:43
@clawsweeper

clawsweeper Bot commented Jun 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 20, 2026, 2:43 PM ET / 18:43 UTC.

Summary
The branch adds a shared validator that rejects mismatched explicit agent IDs and agent-scoped outbound session or mirror keys across Gateway, message-action, send-service, delivery, recovery, tests, and Gateway protocol docs.

PR surface: Source +311, Tests +694, Docs +4. Total +1009 across 17 files.

Reproducibility: no. high-confidence current-main runtime reproduction was established in this review. The linked report and proposed tests identify a concrete source-level path, but the supplied real-run evidence is for an earlier PR head rather than the current one.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: serialized state: src/infra/outbound/delivery-queue-recovery.ts, serialized state: src/infra/outbound/session-owner.test.ts, serialized state: src/infra/outbound/session-owner.ts, unknown-data-model-change: src/infra/outbound/session-owner.test.ts, unknown-data-model-change: src/infra/outbound/session-owner.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #96075
Summary: This PR is the explicit candidate implementation for the open outbound split-ownership report.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until stronger real behavior proof is added.

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

Rank-up moves:

  • [P1] Add redacted exact-head terminal or live output covering the final rejection-before-dispatch path and the allowed policy-key route.
  • [P1] Obtain maintainer confirmation of the fail-closed and queued-recovery compatibility contract.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR body includes useful terminal output from the real outbound helper path, but it was captured at 0b58b05 and does not prove the latest 6e1fa96 head after later Gateway and recovery changes; add redacted exact-head terminal or live output before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Merging deliberately rejects a request shape that the PR describes as previously dispatched, so existing internal integrations or operators using contradictory explicit agent and session owners could begin receiving invalid-request failures.
  • [P1] The new invariant spans Gateway sends, tool actions, direct outbound helpers, delivery, and persisted queue recovery; maintainer agreement is needed on whether all of those owner-bearing keys must share one canonical owner and whether dead-lettering old queued entries is acceptable.

Maintainer options:

  1. Confirm the fail-closed ownership contract (recommended)
    Approve the compatibility posture only after reviewing the impact on existing callers and queued deliveries, then require refreshed exact-head behavior proof.
  2. Retain compatibility for contradictory callers
    Choose explicit-agent precedence or an explicitly opted-in strict path if currently supported integrations must continue dispatching these requests.
  3. Pause pending ownership-policy direction
    Leave the PR open but unmerged until the outbound owner and upgrade policy is settled.

Next step before merge

  • [P1] A maintainer must choose the fail-closed compatibility and queued-recovery policy; refreshed contributor-owned proof is also required, so this is not a safe automated repair task.

Maintainer decision needed

  • Question: Should OpenClaw fail closed for every explicit-agent and canonical-session owner mismatch, including recovery of already queued outbound entries, rather than retain explicit-agent precedence for existing contradictory callers?
  • Rationale: The patch intentionally changes compatibility behavior across several public and internal outbound boundaries; source-level review can establish mechanics, but cannot choose the acceptable upgrade posture for existing integrations and queued work.
  • Likely owner: vincentkoc — The PR discussion specifically routes outbound/Gateway contract review to this person, though feature-history attribution could not be independently verified.
  • Options:
    • Adopt canonical-owner rejection (recommended): Confirm that explicit agent IDs, canonical control keys, and transcript mirror keys must always name the same normalized agent, then require exact-head proof before landing.
    • Preserve explicit-agent precedence: Keep the existing contradictory request shape working and narrow this PR to diagnostics or a separately opted-in strict mode if that remains the desired compatibility contract.
    • Defer the contract change: Pause this PR and retain the linked issue until an owner defines the supported cross-agent routing and queued-delivery migration policy.

Security
Cleared: The visible diff adds validation and tests without dependency, workflow, permission, secret, package-resolution, or artifact-execution changes.

Review details

Best possible solution:

Adopt one explicitly documented ownership contract for control and transcript keys, then land a consistent guard only with fresh exact-head proof that demonstrates rejection before dispatch and acceptance of supported routed-policy cases.

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

No high-confidence current-main runtime reproduction was established in this review. The linked report and proposed tests identify a concrete source-level path, but the supplied real-run evidence is for an earlier PR head rather than the current one.

Is this the best way to solve the issue?

Unclear. A shared guard is the maintainable shape if the canonical-owner invariant is accepted, but maintainers must first decide whether fail-closed rejection and queue dead-lettering are the correct compatibility contract.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4aaea7b07291.

Label changes

Label justifications:

  • P2: This is a bounded outbound correctness and compatibility hardening change with no evidence of a currently widespread user-facing outage.
  • merge-risk: 🚨 compatibility: The branch intentionally converts contradictory explicit-agent and scoped-session requests from dispatching to invalid-request failures.
  • merge-risk: 🚨 session-state: The validation governs session and transcript mirror ownership and also changes handling of queued entries during recovery.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body includes useful terminal output from the real outbound helper path, but it was captured at 0b58b05 and does not prove the latest 6e1fa96 head after later Gateway and recovery changes; add redacted exact-head terminal or live output before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +311, Tests +694, Docs +4. Total +1009 across 17 files.

View PR surface stats
Area Files Added Removed Net
Source 7 320 9 +311
Tests 9 711 17 +694
Docs 1 4 0 +4
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 17 1035 26 +1009

What I checked:

  • Linked canonical report: The PR explicitly implements the open report describing split outbound ownership; it is a candidate fix rather than a superseding replacement until merged. (6e1fa96582a5)
  • Proposed boundary coverage: The PR introduces the shared ownership helper and calls it at Gateway, lower outbound helpers, delivery, and queued-delivery recovery, with regression tests for mismatches, legacy keys, same-owner keys, mirrors, and policy-key exclusion. (src/infra/outbound/session-owner.ts:1, 6e1fa96582a5)
  • Compatibility posture requires owner confirmation: The PR body states that an explicit agent ID paired with another agent's scoped session key changes from accepted dispatch to fail-closed validation; this is a deliberate compatibility contract choice, not a purely mechanical bug fix. Public docs: docs/gateway/protocol.md. (docs/gateway/protocol.md:488, 6e1fa96582a5)
  • Exact-head proof gap: The supplied real-behavior transcript was run at 0b58b05, while the reviewed PR head is 6e1fa96 after multiple later force-pushes. It therefore does not demonstrate the final Gateway and recovery paths. (0b58b0592049)
  • Prior review continuity: The latest completed ClawSweeper cycle already required both a human compatibility-contract decision and refreshed contributor-owned proof for the changed final head; no prior line-level finding remains to re-raise. (67dece7b745c)

Likely related people:

  • vincentkoc: The contributor explicitly requested a sanity check from this person for the outbound/Gateway ownership boundary; no stronger local feature-history attribution was available in this read-only review environment. (role: requested domain reviewer; confidence: low; files: src/gateway/server-methods/send.ts, src/infra/outbound/deliver.ts, src/infra/outbound/session-owner.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.
Review history (4 earlier review cycles)
  • reviewed 2026-06-30T22:09:46.283Z sha 0b58b05 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-19T03:56:19.355Z sha af7feb2 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-19T23:09:18.745Z sha b5599c1 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-20T11:30:56.394Z sha 67dece7 :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot mentioned this pull request Jun 23, 2026
Open
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 23, 2026
@youngting520

youngting520 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 24, 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. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 24, 2026
@youngting520

youngting520 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Quick clarification for whoever picks this up for human review: I believe the ClawSweeper “Stored data model” note is a false positive. src/infra/outbound/session-owner.ts is a pure stateless validation helper. It only parses agent:<owner>:... session keys and compares the normalized owner segment; it does not persist, serialize, migrate, or change any stored schema/state. So there should be no migration surface here.

The only intentional compatibility change is the fail-closed rejection of contradictory split-owner requests: an explicit agentId paired with an agent-scoped sessionKey owned by a different agent. I think that is the right tradeoff because it aligns outbound send/action behavior with the documented ownership contract: docs/gateway/protocol.md says that when both sessionKey and agentId are present, the resolved session agent must match agentId, and src/infra/outbound/session-context.ts documents OutboundSessionContext.key as the canonical hook/session key for the producing agent runtime.

Same-agent keys, omitted/blank agent ids, legacy/non-agent keys, and cross-agent policyKey routing are still accepted. The only rejected shape is the contradictory owner pair, for example agentId: "work" with sessionKey: "agent:main:...", which can split media policy, action dispatch, mirroring, queueing, and hook correlation across different owners.

I’m happy to adjust if maintainers prefer a different compatibility posture here. cc @vincentkoc if you have bandwidth to sanity-check the outbound/gateway boundary. Thanks!

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. labels Jun 27, 2026
@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
@youngting520
youngting520 force-pushed the bug/outbound-owner-mismatch branch from 0b58b05 to af7feb2 Compare July 19, 2026 03:51
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation app: web-ui App: web-ui size: XL and removed size: L labels Jul 19, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 19, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the stale Marked as stale due to inactivity label Jul 19, 2026
@youngting520
youngting520 force-pushed the bug/outbound-owner-mismatch branch from af7feb2 to 473ea2e Compare July 19, 2026 22:58
@openclaw-barnacle openclaw-barnacle Bot removed the app: web-ui App: web-ui label Jul 19, 2026
@youngting520
youngting520 force-pushed the bug/outbound-owner-mismatch branch from 473ea2e to b5599c1 Compare July 19, 2026 23:03
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 19, 2026
@youngting520
youngting520 force-pushed the bug/outbound-owner-mismatch branch from b5599c1 to 67dece7 Compare July 20, 2026 11:27
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. label Jul 20, 2026
@youngting520
youngting520 force-pushed the bug/outbound-owner-mismatch branch from 67dece7 to cab9644 Compare July 20, 2026 17:39
@youngting520
youngting520 force-pushed the bug/outbound-owner-mismatch branch from cab9644 to 6e1fa96 Compare July 20, 2026 18:39
@youngting520

youngting520 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

@vincentkoc could you confirm the outbound/Gateway compatibility contract on the current exact head 78e7ee8724ba103ed24686f3bf37cd85f1e0e318 when you have bandwidth?

The requested decision is specifically whether these three rules are acceptable together:

  1. An explicit agentId that disagrees with an agent-scoped canonical session or mirror key fails closed before dispatch, queueing, mirroring, or hooks.
  2. A previously stored contradictory queue entry is permanently marked failed during recovery instead of being replayed across agent boundaries.
  3. policyKey remains excluded from equality checks, so routed delivery may intentionally use another agent's policy/origin session.

There is no queue schema/version or migration change. Exact-head CI is green, and the PR body now contains the current-SHA terminal evidence and recovery behavior details.

@youngting520

youngting520 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 22, 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.

Re-review progress:

@clawsweeper

clawsweeper Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: Reject split outbound agent session ownership This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

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

docs Improvements or additions to documentation gateway Gateway runtime merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XL status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Outbound message ownership can split when agentId and sessionKey disagree

2 participants

@youngting520 @Patrick-Erichsen