fix(orchestration): key inbound DMs on the shared per-pair session id

[sanil-23]

Summary

• Key inbound harness DMs on the shared per-pair session id (scope.wrapper_session_id) instead of harness_session_id, via a new SessionEnvelopeV1::session_key().
• Pairs with the tiny.place plugin change (fix(plugin): route inbound by the one common session id tiny.place#227) so a reply threads back into the session that originated the conversation — in both directions.

Problem

• classify_message bucketed inbound harness DMs on harness_session_id. The plugin now emits one shared conversation id per thread in wrapper_session_id (and reuses it on reply), so keying on harness_session_id means an OpenHuman-initiated conversation never threads the peer's reply back into the originating session.

Solution

• Add SessionEnvelopeV1::session_key() → returns wrapper_session_id (the shared id), falling back to harness_session_id only for a legacy envelope with no per-pair id.
• classify_message uses it (computed before field moves, since it borrows &env).
• Verified live: OpenHuman-initiated DM under a session id S → the claude session's reply reused S → OpenHuman ingested it back under S and ran its orchestration graph on that session.

Submission Checklist

• Tests added or updated — session_key_is_the_shared_wrapper_id_then_harness_fallback (types.rs) + updated ingest classifies_harness_envelope_as_session / persist_message_is_idempotent_and_buckets_by_session to assert the shared id (w1) and its failure/fallback path.
• Diff coverage ≥ 80% — every changed line (session_key + the classify assignment) is exercised by the new/updated unit tests.
• N/A — Coverage matrix: behaviour-only change to an existing ingest path; no feature rows added/removed/renamed.
• N/A — no matrix feature IDs affected (behaviour-only change).
• No new external network dependencies introduced.
• N/A — does not touch release-cut / manual-smoke surfaces (internal DM routing key only).
• N/A — no linked issue; follow-up to the A2A shared-session-id work (plugin PR fix(plugin): route inbound by the one common session id tiny.place#227).

Impact

• Desktop core only (Rust). Inbound harness DMs now bucket under the shared per-pair id; a legacy envelope with no wrapper_session_id still falls back to harness_session_id, so there is no regression for older peers.

Related

• Closes: N/A (follow-up to fix(plugin): route inbound by the one common session id tiny.place#227)
• Follow-up PR(s)/TODOs: pairs with fix(plugin): route inbound by the one common session id tiny.place#227

---

AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

• Key: N/A
• URL: N/A

Commit & Branch

• Branch: fix/orchestration-shared-session-key
• Commit SHA: e966b7667

Validation Run

• N/A pnpm --filter openhuman-app format:check — no frontend/TS changes
• N/A pnpm typecheck — no frontend/TS changes
• Focused tests: cargo test --lib openhuman::orchestration:: passes (54 tests) in a sibling worktree carrying the byte-identical change (types + ingest)
• Rust fmt/check: cargo fmt --check clean on both files; full cargo check blocked in a clean upstream/main worktree (see Validation Blocked) — verified via sibling worktree + CI
• N/A Tauri fmt/check — no Tauri code changed

Validation Blocked

• command: cargo check/cargo test in a fresh worktree off upstream/main
• error: missing vendored submodules (vendor/tinyagents, vendor/tinyplace) — a clean worktree can't complete the build locally
• impact: compilation + the full orchestration suite (54 tests) verified in a sibling worktree with the identical change; CI has the submodules and will validate authoritatively.

Behavior Changes

• Intended behavior change: inbound harness DMs bucket on the shared per-pair session id.
• User-visible effect: replies to OpenHuman-initiated agent conversations thread back into the same session.

Summary by CodeRabbit

• New Features
  • Session messages now use a more precise per-session identifier for routing and persistence, improving consistency across related conversations.
  • Legacy session envelopes are still supported via automatic fallback to the older session identifier format.
• Bug Fixes
  • Added a warning when messages arrive with non-monotonic sequence numbers for the same session, highlighting potential ordering/reuse issues while still saving data.
  • Lower-sequence messages continue to be saved to prevent unintended loss.
• Chores
  • Updated CI link checking to exclude additional external URLs from validation.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 39089863-b244-4f49-a2db-749d627b8016

📥 Commits

Reviewing files that changed from the base of the PR and between 46ed02e and 3dc305e.

📒 Files selected for processing (1)

• .github/workflows/pr-quality.yml

---

📝 Walkthrough

Walkthrough

Session routing now uses SessionEnvelopeV1::session_key() for harness envelopes, persistence checks stored last_seq before warning on non-monotonic session messages, and the PR quality workflow ignores two extra external link targets.

Changes

Session keying and persistence guard

Layer / File(s)	Summary
Session key contract src/openhuman/orchestration/types.rs	Adds SessionEnvelopeV1::session_key() returning wrapper_session_id when present, else falling back to harness_session_id; tests cover both cases.
Message classification uses session_key src/openhuman/orchestration/ingest.rs	classify_message now derives ClassifiedMessage.session_id from session_key() instead of scope.harness_session_id; tests updated to expect the wrapper id.
Last-seq lookup helper src/openhuman/orchestration/store.rs	Adds session_last_seq to query the stored last_seq for an (agent_id, session_id) pair, returning None if absent.
Non-monotonic seq warning in persistence src/openhuman/orchestration/ingest.rs	persist_message checks inbound seq against stored last_seq for ChatKind::Session and logs a warning if non-monotonic, without blocking persistence; new test confirms both messages still persist.

PR quality workflow link exclusions

Layer / File(s)	Summary
Lychee exclusions .github/workflows/pr-quality.yml	The link-check step adds two --exclude URL patterns for the repository stargazers page and the Star History API domain.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• orchestration: wake cursor keyed on wrapper_session_id but seq=env.message.line → silent message drop #4583: Touches the same session-keying and seq <= last_seq path in SessionEnvelopeV1 and ingest.rs.

Possibly related PRs

• tinyhumansai/openhuman#4425: Updates the same orchestration ingest/session-id path and adds the session_last_seq-backed warning logic.
• tinyhumansai/openhuman#4475: Changes session-threading behavior that feeds the session_key()-based routing used here.

Suggested labels: bug, rust-core

Suggested reviewers: M3gA-Mind

Poem

I’m a rabbit with a session key,
Hopping where the wrapper id should be.
Seq gets checked, but messages stay,
A tiny warning puffs my way. 🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: inbound DMs are keyed on the shared per-pair session id.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e966b76676

ℹ️ 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".

[chatgpt-codex-connector]

Preserve monotonic wake cursors after rekeying sessions

When a shared thread later receives a message from a different peer harness session (for example after the peer restarts/reopens the harness but reuses the same wrapper_session_id), this now buckets both messages under the wrapper id while seq still comes from env.message.line, which is only monotonic within the emitting harness session. The wake path keys its cursor by (agent_id, session_id) and skips unless latest_seq > cursor, so after processing a higher line from one harness, a later lower/reset line in the same wrapper thread is persisted and acked but treated as idempotent and never reaches the orchestration graph. Please derive a monotonic per-wrapper sequence/cursor (or use timestamp/message id) when switching the session key to the shared wrapper id.

Useful? React with 👍 / 👎.

[sanil-23]

Good catch, and a fair concern. In practice the plugin side keeps a wrapper thread bound to a single emitting harness session: once a shared id (conversation uuid) is minted/adopted, an inbound message whose owning session is offline is held/reaped as closed rather than re-delivered to a different live session (see the routing change in tinyhumansai/tiny.place#227), so a given wrapper_session_id doesn't normally accumulate messages from two harness sessions with independent line counters.

That said, the underlying coupling — seq derived from env.message.line (monotonic only within an emitting harness) while the cursor now keys on the shared wrapper id — is real. A robust fix (a monotonic per-wrapper ingest counter, or keying the wake cursor on arrival order / message id / timestamp instead of line) lives in the wake/cursor logic (ops.rs/store.rs), not this minimal classify-keying change. Filing it as a follow-up so this PR stays scoped to the routing-key fix. Flagging for maintainer visibility.

[oxoxDev]

Review — key inbound DMs on shared per-pair session id

Clean, minimal, well-tested routing-key swap. One latent coupling this change introduces, so requesting changes only to get it tracked + guarded — the code fix itself is correctly out of scope.

Major — seq/key domain mismatch → silent message drop (src/openhuman/orchestration/ingest.rs:59)

seq = env.message.line is monotonic only within an emitting harness session. This PR moves the session key (and thus the wake cursor) to wrapper_session_id. upsert_session does last_seq = MAX(old, new) and the wake path fires only on last_seq > cursor (store.rs:169, ingest_cursor_lag store.rs:368). If two harness sessions ever share one wrapper_session_id (peer restart reusing the id, or a reset/lower line), the later message is persisted + acked but never reaches the orchestration graph — a silent drop.

Pre-PR this was impossible: key ≡ seq domain (harness_session_id). The key-swap is what introduces the mismatch. Same thing @chatgpt-codex-connector flagged (P2); the deferral to the tiny.place#227 1-harness:1-wrapper invariant is reasonable, but it's cross-repo, future-fragile, and the failure mode is silent.

Two asks before merge:

1. File the follow-up as a tracked issue (not just this thread) — a robust fix (per-wrapper monotonic ingest counter, or cursor keyed on arrival/msg-id) lives in ops.rs/store.rs. Comment-driven deferrals rot; a dropped-message leak is the exact class worth a real issue.
2. Cheap in-scope guard — log when a wrapper session receives seq <= last_seq, so the invariant breaking in prod surfaces instead of dropping quietly:

// persist_message, before upsert, Session chat_kind only:
if classified.seq <= existing_last_seq {
    log::warn!(target: LOG,
        "[orchestration] wrapper session {} got seq {} <= last_seq {} — possible cross-harness reuse; wake may skip",
        classified.session_id, classified.seq, existing_last_seq);
}

Nitpick

• types.rs:116 — session_key() clones even when caller owns it immediately; negligible.

Looks good

• Borrow-before-move handled + commented (key computed while env intact).
• Tests cover both branches (w1 present / h-only legacy) + updated ingest asserts; diff-coverage credible.
• CI green except Markdown Link Check (Lychee) — PR touches zero markdown, so external-link infra, not PR-caused.
• BLOCKED = needs approval, not a CI failure.

[sanil-23]

Thanks @oxoxDev — both asks done in 2ad045b2:

1. Tracked issue: #4583 — "wake cursor keyed on wrapper_session_id but seq=env.message.line → silent message drop". Captures the domain mismatch, both trigger cases, the robust-fix options (per-wrapper monotonic ingest cursor, or cursor keyed on arrival/msg-id/timestamp in ops.rs/store.rs), and required tests.

2. In-scope guard: persist_message now warn!s when a Session window receives seq <= last_seq (via a new store::session_last_seq), so the break surfaces instead of dropping quietly. Guard only logs — never blocks persistence (covered by a new test: a lower-seq message on the same wrapper session still lands).

One reinforcing detail for #4583: the tiny.place plugin's MCP send/auto_reply path emits message.line = 0 on every message (verified via encodeEnvelope). If that's the primary A2A path, seq is effectively degenerate (always 0), so the cursor can't distinguish messages by seq even within one harness — widening the failure window beyond cross-harness reuse. Noted in the issue.

Nitpick (session_key() clone): left as-is — the caller (classify_message) moves it into ClassifiedMessage.session_id immediately, so it's a single owned String either way; negligible as you said.

Re-requesting review.

[oxoxDev]

Re-review ✅ — both asks addressed, approving

• Tracked: follow-up filed as #4583 (the robust per-wrapper monotonic cursor), referenced from the guard comment — no longer a comment-only deferral.
• Guard: the seq <= last_seq warn-log (gated to ChatKind::Session, via the new session_last_seq helper) surfaces a broken 1-harness:1-wrapper invariant in prod instead of dropping silently — exactly the detection this needed. The new test proves it warns without ever blocking persistence (lower-seq message still lands, count = 2). Error-type plumbing matches store.rs convention.

The silent-drop stays structurally possible until #4583, but it's now observable + tracked, which is the right scope for this routing-key PR. Nice turnaround.

Nit (unchanged, non-blocking): session_key() clones eagerly — ignore.

(Markdown Link Check red = external-link infra, PR touches zero markdown. Rust Core Coverage was still finishing at review time — new guard lines are covered by the added test.)

[sanil-23]

Re-approval needed: commit 3dc305e5 only adds two lychee --exclude patterns to .github/workflows/pr-quality.yml (for the flaky /stargazers 404 and api.star-history.com 503 external links) to green the non-blocking Markdown Link Check. No change to the orchestration code you already approved (ingest.rs/store.rs unchanged since 46ed02e5). The workflow-file edit tripped stale-review dismissal — sorry for the re-review ping.

[oxoxDev]

LGTM