feat(orchestration): auto-accept contact requests from linked agents (O2)#4709
feat(orchestration): auto-accept contact requests from linked agents (O2)#4709sanil-23 wants to merge 3 commits into
Conversation
…(O2) tiny.place's relay drops any DM to a peer that hasn't accepted a contact request, and the plugin never auto-accepts (accepting a contact is a trust decision). So a wrapped agent that re-establishes contact with its paired OpenHuman "Master" is blocked forever behind a pending request — its session_info intro and entire session stream never arrive. This is the e2e gate for session_info. Add auto_accept_linked_contact_requests: poll /contacts/requests and accept every *incoming pending* request whose requester is already in the local linked-agent set (agent_orchestration::pairing::linked_agent_ids) — and ONLY those. Non-linked requesters are left pending for the human; generic auto-accept stays off (the trust boundary). Wired into the existing orchestration message-drain supervisor, running before each mailbox drain so a freshly-accepted agent's DMs are picked up the same cycle. Fail-closed: linked_agent_ids returns an empty set on any pairing-store read error, so a read failure auto-accepts nothing (every request stays pending) rather than opening the gate. The empty-set case also short-circuits before the network round-trip. The trust check reuses the DM-ingest matcher (now pub(crate)) so the base58 pairing-store form and a base64 wire form of the same Ed25519 key unify — otherwise a linked agent's request could be missed and the gate stay shut. Tests: linked requester accepted; non-linked left pending; empty/read-error linked set accepts nothing; base58/base64 unification; incoming-pending request parsing (agentId → contact.requester fallback, skips accepted/outgoing).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an auto-accept gate for pending tiny.place contact requests from locally linked agents in ChangesAuto-accept linked contact requests
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
agent
![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: b10dc27eec
ℹ️ 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".
| ); | ||
| let mut accepted = 0usize; | ||
| for agent_id in to_accept { | ||
| match accept_request(config, &agent_id).await { |
There was a problem hiding this comment.
When a request uses the base64 form of a key already stored as base58 (the case agent_id_in_linked_set now admits), this passes that base64 string into accept_request, which persists a separate Linked record because persist_record only removes exact agent_id matches. That leaves both encodings linked; a later decline/block/remove of either one does not clear the other, so linked_agent_ids can continue authorizing a peer the user just unlinked. Please canonicalize to the existing linked record, or de-dupe by decoded key, when accepting.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/agent_orchestration/pairing.rs (1)
331-441: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo end-to-end test for the network+accept path of the gate.
Tests cover
incoming_pending_requesters,request_view_requester(indirectly),requesters_to_auto_accept, and the fail-closedlinked_agent_idspath, butauto_accept_linked_contact_requestsitself (the/contacts/requestsfetch + per-requesteraccept_requestloop + accepted-count tally) isn't exercised end-to-end. The repo already has a wiremock-based pattern for tiny.place-adjacent flows (e.g.src/openhuman/file_storage/tools_tests.rsper the coverage matrix); consider reusing it here to catch integration-level regressions in this trust-sensitive gate.Also applies to: 593-711
🤖 Prompt for 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. In `@src/openhuman/agent_orchestration/pairing.rs` around lines 331 - 441, The end-to-end network-and-accept flow in auto_accept_linked_contact_requests is not covered, so add an integration test that exercises the full path: mock tiny.place’s /contacts/requests response, verify requesters are filtered via requesters_to_auto_accept, and confirm accept_request is called for each linked requester while the accepted count is returned correctly. Reuse the existing wiremock-style testing pattern used elsewhere in the codebase, and anchor the test against auto_accept_linked_contact_requests, incoming_pending_requesters, and accept_request so regressions in the trust gate are caught.
🤖 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.
Nitpick comments:
In `@src/openhuman/agent_orchestration/pairing.rs`:
- Around line 331-441: The end-to-end network-and-accept flow in
auto_accept_linked_contact_requests is not covered, so add an integration test
that exercises the full path: mock tiny.place’s /contacts/requests response,
verify requesters are filtered via requesters_to_auto_accept, and confirm
accept_request is called for each linked requester while the accepted count is
returned correctly. Reuse the existing wiremock-style testing pattern used
elsewhere in the codebase, and anchor the test against
auto_accept_linked_contact_requests, incoming_pending_requesters, and
accept_request so regressions in the trust gate are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44ab33c5-c13a-461c-b220-be965d88f346
📒 Files selected for processing (4)
docs/TEST-COVERAGE-MATRIX.mdsrc/openhuman/agent_orchestration/pairing.rssrc/openhuman/orchestration/ingest.rssrc/openhuman/orchestration/ops.rs
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
PR #4709 — feat(orchestration): auto-accept contact requests from linked agents (O2)
Walkthrough
This PR closes the delivery gate that stops tiny.place's relay from dropping a paired agent's session_info intro: on each 15s orchestration drain cycle it polls /contacts/requests and auto-accepts every incoming pending request whose requester is already in the local linked-agent set — and only those. The decision logic is cleanly decomposed into three pure, unit-tested helpers (incoming_pending_requesters → requesters_to_auto_accept) plus a thin network wrapper, and it reuses the DM-ingest matcher (agent_id_in_linked_set, promoted to pub(crate)) so base58/base64 encodings of the same Ed25519 key unify. The trust boundary is narrowed, not widened (blocked/declined agents are excluded, fail-closed on store read error), the tests are focused and meaningful, and the wiring is minimal and well-commented.
Overall this is a well-scoped, defensively-designed change. My comments are about robustness at the seams — id canonicalization on accept, re-accept churn if the relay doesn't clear the request, and a slightly misleading doc line — none of which block the core logic.
Changes
| File | Summary |
|---|---|
src/openhuman/agent_orchestration/pairing.rs |
New auto_accept_linked_contact_requests() + pure helpers incoming_pending_requesters / request_view_requester / requesters_to_auto_accept; 5 new unit tests. |
src/openhuman/orchestration/ingest.rs |
agent_id_in_linked_set promoted to pub(crate); doc updated to note it's shared with the auto-accept gate. |
src/openhuman/orchestration/ops.rs |
One call added to the existing drain supervisor loop, before drain_mailbox_once; errors logged, never abort the loop. |
docs/TEST-COVERAGE-MATRIX.md |
Adds row 6.3.10 for the new gate. |
Actionable comments (3)
⚠️ Major
1. src/openhuman/agent_orchestration/pairing.rs:378-379 — accept with the canonical linked id, not the raw wire id
requesters_to_auto_accept returns the raw incoming requester string, which the PR's own justification says "may carry the base64 Ed25519 key" (that's why the base58/base64 unification exists). That raw id is then passed straight to accept_request, which (a) POSTs /contacts/{base64...}/accept — percent-encoding +///= — and (b) persist_records a new record keyed by the base64 string, while the existing linked record is keyed by base58. Because persist_record dedupes by exact-string agent_id (pairing.rs:283), you now hold two Linked records for one identity, which accumulates on every base64-form request and surfaces as duplicate rows in the pairing UI. The safer form is to accept using the already-linked canonical id you matched against, not the wire echo.
Suggested change:
// requesters_to_auto_accept: return the linked-set id that matched, so the
// downstream accept/persist uses the canonical (base58) form already on file.
fn requesters_to_auto_accept(incoming: &[String], linked: &HashSet<String>) -> Vec<String> {
incoming
.iter()
.filter_map(|id| {
linked
.iter()
.find(|linked_id| agent_id_in_linked_set(id, &std::iter::once((*linked_id).clone()).collect()))
.cloned()
})
.collect()
}(Or, more simply, look up the canonical id in the accept loop before calling accept_request.) If the relay genuinely requires the exact id form it sent, disregard the POST half — but the duplicate-record half still stands and is worth resolving.
💡 Refactor / suggestion
2. src/openhuman/agent_orchestration/pairing.rs:361-393 — no guard against re-accepting the same pending request every cycle
The "no per-cycle churn" guarantee rests entirely on the relay clearing the pending request the moment accept succeeds (design note in the PR body). If it ever doesn't — relay race, an accept that no-ops because the contact is already accepted server-side, key-form mismatch from finding #1 — the same requester is re-selected every 15s, re-POSTed, re-persisted, and re-publish_global(OrchestrationPairingChanged) fires forever, spamming the event bus / UI refresh. Since the gate fires precisely when local=Linked but relay=pending, there's no local state that naturally converges. A cheap in-memory "recently auto-accepted" guard bounds the blast radius if the assumption is ever violated.
Suggested change (sketch):
// module-level, alongside STORE_LOCKS
static RECENT_AUTO_ACCEPTED: LazyLock<Mutex<HashSet<String>>> =
LazyLock::new(|| Mutex::new(HashSet::new()));
// in the accept loop: skip ids accepted in a prior pass, insert on success.
// (Clear/expire the set when a request is confirmed gone, or cap its size.)At minimum, please confirm in a comment that the relay is observed to drop the request from incoming on accept, since the whole no-churn argument depends on it.
💡 Nitpick / doc
3. src/openhuman/agent_orchestration/pairing.rs:344-348 — "key rotation" is listed as auto-unblocked but isn't
The doc lists "fresh daemon, reconnect, key rotation" as the re-contact cases this gate unblocks. A rotated key is a different Ed25519 identity, so it will not be in linked_agent_ids() and agent_id_in_linked_set won't match it — that request is correctly left pending for the human. That's the right security behavior, but the doc implies the opposite. Suggest dropping "key rotation" from the auto-accepted list (or noting it stays pending by design).
Suggested change:
// so a wrapped agent that re-establishes contact (fresh daemon, reconnect)
// is otherwise blocked behind a pending request ...
// (A rotated key is a new identity → not linked → left pending for the human.)Nitpicks (2)
src/openhuman/agent_orchestration/pairing.rs:365—limit=100silently caps how many pending requests are scanned per pass; a linked requester beyond the 100th stays blocked until earlier ones clear. Mirrorslist(), so it's consistent, but worth a comment or pagination if request volume can be high.src/openhuman/agent_orchestration/pairing.rs:377-392— the accept loop's error branch andacceptedcounting aren't exercised by any test (the pure helpers are well covered). Not required, but a small test injecting a stubbed accept would close the last uncovered branch of the new function.
Questions for the author (1)
src/openhuman/agent_orchestration/pairing.rs:415— for an incoming request, is the relay's top-levelagentIdguaranteed to be the counterpart (requester) and never our own addressee? The parse assumes so; if the relay ever echoes our own id there, it's harmless (won't be in the linked set) but worth confirming against the actual payload shape.
Verified / looks good
- Trust boundary is correctly narrowed:
linked_agent_idsfiltersstatus == Linked, so blocked (Blocked) and declined (record removed) agents are excluded from auto-accept. - Fail-closed behavior is real and tested: a corrupt pairing store → empty linked set → zero accepts, with the network round-trip short-circuited.
- base58/base64 unification correctly reuses the single DM-ingest matcher rather than duplicating decode logic; the shared-matcher doc on
ingest.rsis accurate. - Supervisor wiring runs auto-accept before
drain_mailbox_once(so a fresh accept is drained the same cycle) and never aborts the loop on error. - Pure helpers are genuinely pure and the 5 new unit tests exercise linked/unlinked/mixed/empty/fail-closed and the parse's pending/incoming/fallback branches.
| ); | ||
| let mut accepted = 0usize; | ||
| for agent_id in to_accept { | ||
| match accept_request(config, &agent_id).await { |
There was a problem hiding this comment.
requesters_to_auto_accept returns the raw incoming requester string, which the PR itself says "may carry the base64 Ed25519 key." That raw id flows straight into accept_request, which persist_records a new record keyed by the base64 string — while the existing linked record is keyed by base58. Since persist_record dedupes by exact-string agent_id (line 283), you end up with two Linked records for one identity, accumulating on every base64-form request (duplicate rows in the pairing UI), and the POST hits /contacts/{base64%2B%2F%3D...}/accept.
Prefer resolving to the already-linked canonical id you matched against before calling accept_request, e.g. look up the matching linked-set entry in the accept loop and pass that instead of &agent_id. If the relay strictly needs the exact id form it sent, the duplicate-record half still stands.
| to_accept.len() | ||
| ); | ||
| let mut accepted = 0usize; | ||
| for agent_id in to_accept { |
There was a problem hiding this comment.
💡 Suggestion — no guard against re-accepting the same pending request every cycle.
The "no per-cycle churn" guarantee rests entirely on the relay dropping the request from incoming the instant accept succeeds. If it ever doesn't (relay race, an accept that no-ops because the contact is already accepted server-side, or the id-form mismatch from the other comment), the same requester is re-selected every 15s → re-POST + re-persist + publish_global(OrchestrationPairingChanged) firing forever, spamming the event bus / UI. The gate fires precisely when local=Linked but relay=pending, so no local state naturally converges.
A cheap module-level RECENT_AUTO_ACCEPTED: HashSet<String> guard (skip ids accepted in a prior pass, insert on success, cap/expire) bounds the blast radius. At minimum, please confirm in a comment that the relay is observed to clear the request on accept — the whole no-churn argument depends on it.
| /// | ||
| /// Why this is the delivery gate: tiny.place's relay DROPS any DM to a peer that | ||
| /// has not accepted a contact request, so a wrapped agent that re-establishes | ||
| /// contact (fresh daemon, reconnect, key rotation) is otherwise blocked behind a |
There was a problem hiding this comment.
💡 Doc nit — "key rotation" is listed as auto-unblocked but isn't.
This lists "fresh daemon, reconnect, key rotation" as the re-contact cases the gate unblocks. A rotated key is a different Ed25519 identity, so it won't be in linked_agent_ids() and agent_id_in_linked_set won't match it — that request is (correctly) left pending for the human. Right behavior, but the doc implies the opposite. Suggest dropping "key rotation" here or noting it stays pending by design.
| let requests: Value = client | ||
| .http() | ||
| .get_agent_auth::<Value>( | ||
| "/contacts/requests", |
There was a problem hiding this comment.
Nitpick — limit=100 silently caps the scan. A linked requester beyond the 100th pending request stays blocked until earlier ones clear. Consistent with list(), so fine, but worth a comment or pagination if request volume can be high.
|
|
||
| /// The counterpart (requester) address of a single incoming contact-request view. | ||
| fn request_view_requester(view: &Value) -> Option<String> { | ||
| if let Some(id) = view.get("agentId").and_then(Value::as_str) { |
There was a problem hiding this comment.
Question — is the relay's top-level agentId on an incoming request guaranteed to be the counterpart (requester) and never our own addressee? The parse assumes so. If the relay ever echoes our own id here it's harmless (won't be in the linked set), but worth confirming against the real payload shape.
… linked id
Address PR review (Codex P2 / M3gA-Mind Major): requesters_to_auto_accept
returned the raw incoming requester string, which the base58/base64 unification
now admits in a *different* encoding than the stored linked record. Passing that
raw id into accept_request → persist_record (which dedupes by exact-string
agent_id) would persist a SECOND Linked record for one identity — duplicate rows
in the pairing UI, a POST to /contacts/{base64...}/accept, and worst of all a
later decline/block of one encoding leaving the other still authorizing the peer.
Fix: resolve each match to its canonical stored linked id via a new
ingest::resolve_linked_id (agent_id_in_linked_set now delegates to it), and
accept/persist under that. requesters_to_auto_accept returns canonical ids.
Also from review: drop the misleading 'key rotation' from the re-contact list (a
rotated key is a different identity → correctly left pending); document why the
gate has no per-cycle churn (relay drops accepted requests from the pending
/contacts/requests set) and the limit=100 scan cap.
Tests updated to assert canonicalization (base64 wire id → base58 stored id) at
both the ingest resolver and the pairing gate.
|
Thanks for the reviews. Addressed in ✅ Canonical linked id on accept (Codex P2 + @M3gA-Mind Major, ✅ "key rotation" doc nit (@M3gA-Mind, ✅ Per-cycle churn (@M3gA-Mind, ✅ ↩︎ ↩︎ End-to-end network+accept test (@coderabbitai) — fair. The pure decision/parse core ( |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd pairing.rs
fd ingest.rs
echo "---resolve_linked_id---"
rg -n "resolve_linked_id" src/openhuman/agent_orchestration/pairing.rs src/openhuman/orchestration/ingest.rs -A 20
echo "---requesters_to_auto_accept---"
rg -n "requesters_to_auto_accept" src/openhuman/agent_orchestration/pairing.rs -A 20
echo "---agent_id_in_linked_set---"
rg -n "agent_id_in_linked_set" src/openhuman/orchestration/ingest.rs -A 20Length of output: 23833
The two deferred items are reasonable as-is:
No blocking concerns remain from my side. Nice fix on the canonical-id dedup bug — that was a real latent data-integrity issue. (੭ˊ꒳ˋ)੭ nice catch, well fixed |
2 participants
Summary
contact_request, OpenHuman now auto-accepts it iff the requester's agent address is already in the linked-agent set (agent_orchestration::pairing::linked_agent_ids) — otherwise the request is left pending for the human.auto_accept_linked_contact_requests()polls/contacts/requestsand is wired into the existing orchestration message-drain supervisor, running before each mailbox drain so a freshly-accepted agent's DMs are picked up the same cycle.pub(crate)) so the base58 pairing-store form and the base64 wire form of the same Ed25519 key unify.Problem
tiny.place's relay drops any DM to a peer that hasn't accepted a contact request, and the plugin never auto-accepts by design (the relay's own rule is "never auto-accept"). So when a wrapped coding-agent (Claude/Codex/OpenHuman) that is already paired with its OpenHuman "Master" re-establishes contact (fresh daemon, reconnect, key rotation), its
session_infointro — and its entire session stream — is blocked forever behind a pending contact request.This is the e2e gate for the
session_infofeature: without it, all of the schema/emit/receiver work is invisible because the intro DM is silently dropped by the relay.The linked-agent store already exists (
pairing::linked_agent_ids), but nocontact_request → contact_acceptwiring did (git grep contact_acceptreturned nothing onmain). This PR adds exactly that gate — nothing more.Solution
auto_accept_linked_contact_requests(config)(src/openhuman/agent_orchestration/pairing.rs): loadslinked_agent_ids(); short-circuits with no network call when the set is empty (the common case and the fail-closed read-error case); fetches/contacts/requests; and for every incoming pending request whose requester is in the linked set, accepts it via the existingaccept_request()(which POSTs/contacts/{id}/accept, persists aLinkedrecord, and publishesOrchestrationPairingChanged).incoming_pending_requesters()(parse the raw/contacts/requestsJSON, resolvingagentId→contact.requesterfallback, mirroring the frontend'scontactAddress) andrequesters_to_auto_accept()(the trust decision).orchestration::ingest::agent_id_in_linked_set(promoted topub(crate)), which unifies base58/base64 encodings of the same key. Without this, a linked agent's request could be missed and the gate stay shut.src/openhuman/orchestration/ops.rs): one call added to the existing 15s drain-supervisor loop, beforedrain_mailbox_once. Contact requests "surface" the same way DMs do — by polling — so the poll lives alongside the mailbox drain. Errors are logged and never abort the loop.Design note:
accept_request()is reused as-is; re-accepting an already-linked agent is idempotent relay-side and simply refreshes the local record — the request also leaves the pending queue after accept, so there is no per-cycle churn.Submission Checklist
cargo test -p openhuman agent_orchestration::pairing(11 passed) andorchestration::ingest(17 passed).docs/TEST-COVERAGE-MATRIX.md## Related/contacts/requests,/contacts/{id}/accept) already used by the pairing surfacesession_infodelivery planImpact
[orchestration].enabled.Related
session_infodelivery plan §4)session_infovariant) complete the e2e path; the emitter also needs a prekey-presence retry/backoff (spec §5 caveat 2 — contact-accepted ≠ messageable).AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
oh-auto-acceptb10dc27eeValidation Run
pnpm --filter openhuman-app format:check(no frontend changes)pnpm typecheck(no TypeScript changes)GGML_NATIVE=OFF cargo test -p openhuman --lib agent_orchestration::pairing→ 11 passed;orchestration::ingest→ 17 passedcargo fmt --checkclean;GGML_NATIVE=OFF cargo check --libclean (pre-existing warnings only)Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
agent_id_in_linked_setbehavior is identical (visibility-only change).Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes