Conversation
…anaged users send_signal() and send_broadcast() in perspective_instance.rs only searched AgentService::list_user_emails() when deciding whether a recipient is local. The main agent has no email, so signals addressed to it always fell through to the link language — which on a single executor does not loop back, causing signal delivery to silently fail whenever a managed email user and the main agent were both owners of the same neighbourhood. Fix both functions to also check the main agent's DID after the email-user loop. If the main agent is a neighbourhood owner (send_broadcast) or is the intended recipient (send_signal), the signal is now published directly to the local pubsub, matching the same short-circuit path used for managed users. Add a regression test "should exchange neighbourhood signals between main agent and managed user" in multi-user-simple.test.ts covering direct signals in both directions and a broadcast from the managed user to the main agent.
Contributor
📝 WalkthroughWalkthroughAdded local signal publishing capabilities to the neighbourhood perspective instance by introducing a helper closure for emitting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/js/tests/multi-user-simple.test.ts (2)
1572-1572: Consider declaring an explicitthis.timeout()for clarity.The test accumulates roughly 20 s of fixed sleeps and signal waits. Other suites in this file that take similar time (e.g., the multi-node tests) call
this.timeout(120000)explicitly. Adding one here documents the expected worst-case duration and prevents silent failures if the global Mocha timeout is ever reduced.- it("should exchange neighbourhood signals between main agent and managed user", async () => { + it("should exchange neighbourhood signals between main agent and managed user", async function() { + this.timeout(60_000); console.log("\n=== Testing signals between main agent and managed user ===");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/js/tests/multi-user-simple.test.ts` at line 1572, Add an explicit per-test Mocha timeout inside the "should exchange neighbourhood signals between main agent and managed user" test: insert a call like this.timeout(120000) at the start of the `it` callback (the anonymous async function) so the test documents its expected worst-case duration and won't fail silently if global timeouts change. Ensure the timeout call is placed before any awaits/sleeps in that `it` block.
1606-1609: Add retry logic around the neighbourhood-join visibility check (coding guideline).
joinFromUrlmay return before the joined perspective appears inperspective.all(), making thefind+expectsequence a potential source of flakiness. Per the project guideline, cross-agent data-visibility checks should use retry with timeout rather than a single query.♻️ Suggested retry wrapper
- await userClient.neighbourhood.joinFromUrl(neighbourhoodUrl); - const userPerspectives = await userClient.perspective.all(); - const userSharedPerspective = userPerspectives.find(p => p.sharedUrl === neighbourhoodUrl); - expect(userSharedPerspective).to.not.be.null; + await userClient.neighbourhood.joinFromUrl(neighbourhoodUrl); + const maxJoinWait = 10_000; + const joinStart = Date.now(); + let userSharedPerspective: any; + while (Date.now() - joinStart < maxJoinWait) { + const userPerspectives = await userClient.perspective.all(); + userSharedPerspective = userPerspectives.find(p => p.sharedUrl === neighbourhoodUrl); + if (userSharedPerspective) break; + await new Promise(r => setTimeout(r, 200)); + } + expect(userSharedPerspective).to.not.be.null;Based on learnings: "Add retry logic with appropriate timeouts to fix flaky tests related to cross-agent data visibility, rather than changing from Local to Network GetStrategy."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/js/tests/multi-user-simple.test.ts` around lines 1606 - 1609, The visibility check after userClient.neighbourhood.joinFromUrl is flaky because perspective.all() may not immediately include the joined perspective; update the test to poll/retry until the shared perspective appears or a timeout elapses: after calling userClient.neighbourhood.joinFromUrl(neighbourhoodUrl) repeatedly call await userClient.perspective.all() and check for a perspective with sharedUrl === neighbourhoodUrl (the userSharedPerspective check) with a short delay between attempts and a sensible overall timeout (e.g., a few seconds), then assert non-null only after the retry loop succeeds, failing the test if the timeout is reached.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust-executor/src/perspectives/perspective_instance.rs`:
- Around line 3050-3106: The current block only routes local broadcasts when
current_perspective_handle.owners is Some(...), so if owners is None or empty
the main agent never gets the local signal; move or duplicate the main-agent
delivery logic out of the owners Some branch and treat None/empty as implicit
main-agent ownership by checking AgentService::with_global_instance(|s|
s.did.clone()) and, if present, publishing a NeighbourhoodSignalFilter (use
persisted.clone(), payload.verify_signatures(), NEIGHBOURHOOD_SIGNAL_TOPIC and
get_global_pubsub().await.publish(...)) with recipient Some(main_agent_did);
also ensure you still preserve the existing loop that sends to managed users
when owners is Some and non-empty.
- Around line 2955-3013: The routing to the main agent currently only occurs if
current_perspective_handle.owners is Some and contains the main_agent_did, so
when owners is None or an empty list (which in this file denotes implicit
main-agent ownership) signals fall through; change the check in the main-agent
branch (the block using main_agent_did, current_perspective_handle.owners,
publish_local, persisted, payload, and remote_agent_did) to treat None or empty
owners as implicit main-agent ownership by using an expression like
owners.as_ref().map_or(true, |o| o.is_empty() || o.contains(&remote_agent_did))
(i.e. route locally when owners is None OR owners is empty OR owners contains
the main agent), then call publish_local(...) and return Ok(()) as before.
---
Nitpick comments:
In `@tests/js/tests/multi-user-simple.test.ts`:
- Line 1572: Add an explicit per-test Mocha timeout inside the "should exchange
neighbourhood signals between main agent and managed user" test: insert a call
like this.timeout(120000) at the start of the `it` callback (the anonymous async
function) so the test documents its expected worst-case duration and won't fail
silently if global timeouts change. Ensure the timeout call is placed before any
awaits/sleeps in that `it` block.
- Around line 1606-1609: The visibility check after
userClient.neighbourhood.joinFromUrl is flaky because perspective.all() may not
immediately include the joined perspective; update the test to poll/retry until
the shared perspective appears or a timeout elapses: after calling
userClient.neighbourhood.joinFromUrl(neighbourhoodUrl) repeatedly call await
userClient.perspective.all() and check for a perspective with sharedUrl ===
neighbourhoodUrl (the userSharedPerspective check) with a short delay between
attempts and a sensible overall timeout (e.g., a few seconds), then assert
non-null only after the retry loop succeeds, failing the test if the timeout is
reached.
lucksus
added a commit
that referenced
this pull request
Feb 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
send_signal() and send_broadcast() in perspective_instance.rs only searched
AgentService::list_user_emails() when deciding whether a recipient is local.
The main agent has no email, so signals addressed to it always fell through to
the link language — which on a single executor does not loop back, causing
signal delivery to silently fail whenever a managed email user and the main
agent were both owners of the same neighbourhood.
Fix both functions to also check the main agent's DID after the email-user
loop. If the main agent is a neighbourhood owner (send_broadcast) or is the
intended recipient (send_signal), the signal is now published directly to the
local pubsub, matching the same short-circuit path used for managed users.
Add a regression test "should exchange neighbourhood signals between main agent
and managed user" in multi-user-simple.test.ts covering direct signals in both
directions and a broadcast from the managed user to the main agent.
Summary by CodeRabbit
Bug Fixes
Tests