fix(plugin): route inbound by the one common session id#227
Conversation
|
@sanil-23 is attempting to deploy a commit to the Vezures Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates conversation UUID handling to use shared thread identifiers across encoding, routing, server reply correlation, and closed-target handling. Adds reverse binding for peer-minted conversation IDs and adjusts tests for the new ChangesPer-pair thread binding and routing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5feac032a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // OpenHuman uses one shared session id, not our per-conversation wrapper uuid) → | ||
| // resolve via the harness-session index. Both resolve to the live local session. | ||
| export function labelForConversationUuid(agentAddress, id) { | ||
| const sessionUuid = sessionUuidForConversation(id) ?? peekSessionUuidForHarness(id); |
There was a problem hiding this comment.
For default Codex sessions, mcp/server.mjs:127 generates harness_session_id values like tp-codex-... when no real CODEX_SESSION_ID/CODEX_THREAD_ID is present, but inbound tp.to_session_uuid is only forwarded through safeUuid(tp.to_session_uuid) in mcp/format.mjs:162, which drops those non-hex IDs. In that common case this new peekSessionUuidForHarness fallback is never reached; the reply is treated as untargeted and goes to the primary session, misrouting whenever the intended live session is not primary. Please either decode/pass harness-session IDs separately or allow the same ID shape that this registry fallback is meant to resolve.
Useful? React with 👍 / 👎.
sanil-23
force-pushed
the
fix/plugin-shared-session-id
branch
from
5feac03 to
801213a
Compare
sanil-23
changed the title
…ndex) The session id is per-pair: the conversation uuid we mint for (this session, peer) IS the session id. A peer (e.g. OpenHuman) that addresses a reply by that same per-pair id — carried back in wrapper_session_id, with no separate to_session_uuid — was not routed, because reply resolution only consulted the to_session_uuid slot. In resolveBound, when there's no to_session_uuid, resolve fromSessionUuid (= wrapper_session_id = the per-pair session id) through our conversation index (labelForConversationUuid → sessionUuid → live session). It matches ONLY ids we minted, so an ordinary peer's own wrapper convUuid falls through to policy routing unchanged. routing-test: the per-pair id we minted routes back to its session; an id we never minted falls through. 39 routing + 25 registry checks pass.
sanil-23
force-pushed
the
fix/plugin-shared-session-id
branch
from
801213a to
eb24ec2
Compare
A reply now reuses the id of the thread it answers as its own scope.wrapper_session_id, instead of minting a fresh conversation uuid and echoing the peer's id in tp.to_session_uuid. Every message — whoever sends it — carries exactly one session id, so both peers key the conversation on the same id in both directions (incl. peer-initiated threads). - dispatchSend: conversationUuid = replied-to thread id (replyConvUuid) ?? fresh mint; only a brand-new thread we open mints an id - registry.bindConversationUuid: adopt a peer-minted thread id into the conversation index (reverse-only, idempotent) so later inbound routes deterministically to the owning session, not just via primary policy - format: stop emitting tp.to_session_uuid (inbound read path kept for backward compat with older peers) - daemon: closed-session notice carries the shared id as conversationUuid - registry-test: cover bindConversationUuid (adopt / route / idempotent) Verified: 39 routing + 30 registry offline checks; live OpenHuman<->claude round-trip threads the reply back into the originating session.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ddaecce0e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (fromSessionUuid) { | ||
| const label = labelForConversationUuid(agentAddress, fromSessionUuid); | ||
| if (label) return { kind: "session", labels: [label] }; |
There was a problem hiding this comment.
When a peer replies by echoing one of our minted wrapper_session_id values in fromSessionUuid and the owning session has gone offline, labelForConversationUuid returns null because it only matches live sessions; this new branch then falls through to routeTarget, so under the default primary policy the bound thread is delivered to another live session. This regresses the reuse-proof behavior already used for toSessionUuid: known-but-not-live conversation IDs should be held/reaped, while only truly unknown IDs should fall through to policy routing.
Useful? React with 👍 / 👎.
| // "peer session id" to echo. (Historically tp.to_session_uuid carried the peer's id | ||
| // back; that dual-id addressing is gone — routing keys on wrapper_session_id alone. | ||
| // tp.to_session above is an explicit label target, orthogonal to the session id.) |
There was a problem hiding this comment.
Removing the tp.to_session_uuid emission here leaves this package's own test script failing: npm test --prefix sdk/plugin-tinyplace stops in envelope-test.mjs with FAIL to_session_uuid roundtrips as toSessionUuid. If dual-ID addressing is intentionally gone, the stale envelope test needs to be updated or removed; otherwise the encoding path should keep serializing the field so the package test suite remains green.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/plugin-tinyplace/mcp/registry.mjs`:
- Around line 196-206: The bindConversationUuid flow is still racy because the
readEntryFile check and writeFileSync are not atomic, so concurrent callers can
both bind the same conversation. Update bindConversationUuid to use a
first-writer-wins file creation strategy like resolveConversationUuid, using
openSync with "wx" (or equivalent atomic create) for the binding file and only
writing once the exclusive handle is acquired. Keep the existing guard/return
behavior in bindConversationUuid and preserve the best-effort error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9e0e80c-7ee3-43d8-ada0-5be9344d0922
📒 Files selected for processing (7)
sdk/plugin-tinyplace/hooks/agent-daemon.mjssdk/plugin-tinyplace/mcp/format.mjssdk/plugin-tinyplace/mcp/registry.mjssdk/plugin-tinyplace/mcp/routing.mjssdk/plugin-tinyplace/mcp/server.mjssdk/plugin-tinyplace/registry-test.mjssdk/plugin-tinyplace/routing-test.mjs
…d ids Address review on tinyhumansai#227 — commit to the single shared session id fully: - Remove tp.to_session_uuid entirely (no emit AND no decode) — every message carries exactly one id in scope.wrapper_session_id. dispatchSend and the daemon/routing no longer thread a separate peer id. - routing (P1): a shared id we minted/adopted whose owner is offline names a SPECIFIC closed session — hold it (unrouted) and reap-as-closed, instead of falling through to policy and misdelivering to another live session. Only a truly unknown id falls through (first-contact → primary). - reapClosedTargets + the daemon isolated-response gate now key on a known shared id (conversation index) rather than to_session_uuid. - registry.bindConversationUuid: atomic first-owner-wins via openSync(wx) CAS (was read-then-write, TOCTOU). - envelope-test / routing-test: cover no-emit/no-decode + known-but-offline → held (label-reuse safety) instead of the old dual-id assertions. All offline suites green (envelope 40, registry 30, routing 38, + harness/ adapter/branch/lock/inject/hooks).
`classify_message` bucketed inbound harness DMs on `harness_session_id`, but the tiny.place plugin now sends one shared conversation id per thread in `scope.wrapper_session_id` (and reuses it on reply). Key on that instead via a new `SessionEnvelopeV1::session_key()` (wrapper id, `harness_session_id` fallback for legacy envelopes), so a reply threads back into the session that originated the conversation — in both directions. Pairs with the plugin change (tinyhumansai/tiny.place#227). Verified live: OpenHuman-initiated DM under session S -> claude reply reused S -> ingested back under S and drove the orchestration graph.
|
Thanks for the review — addressed in
Companion OpenHuman change: tinyhumansai/openhuman#4582. |
1 participant
Summary
The plugin resolves a reply to a local session only through the per-conversation
convUuidindex. A peer that keys on one common session id — e.g. OpenHuman stores ourscope.harness_session_idand carries that same id in the reply'swrapper_session_id, with no per-conversationto_session_uuid— misses that index, so the message falls through to primary/unrouted (and never threads back to the originating session).There is no separate from/to session identity here: both sides key on the one shared
harness_session_id.Fix
peekSessionUuidForHarness(id)— a read-only reverse ofresolveSessionUuid; unlike it, never mints, so an unknown/attacker-supplied id can't create a binding during inbound routing.labelForSharedSessionId(agent, id)— resolves a shared session id to the owning live local session.resolveBound, when there's noto_session_uuid, route by the common session id carried infromSessionUuid(wrapper_session_id). It matches only our own sessions, so an ordinary peer's wrapperconvUuid(not one of our harness ids) resolves to null and falls through to the existing label/policy routing — unchanged.Backward compatible: the
convUuid/to_session_uuidpath is tried first and untouched.Tests
routing-test.mjs: shared id → routed to its session + landed in the right inbox;labelForSharedSessionIdresolves the live label; an unknown id is not hijacked (falls through to policy);peekof an unknown id returns null (no mint). 42/42 routing + 25/25 registry checks pass.Context
Pairs with OpenHuman: its
SessionEnvelopeV1already carries the one common id inscope.harness_session_id/wrapper_session_id— no extratp/to/from fields needed. Useful on its own.Summary by CodeRabbit