test(e2e): multi-tenant comms visibility transport proof (RIG-3531) - #1042
Open
rigel-mintaka wants to merge 2 commits into
Open
test(e2e): multi-tenant comms visibility transport proof (RIG-3531)#1042rigel-mintaka wants to merge 2 commits into
rigel-mintaka wants to merge 2 commits into
Conversation
|
Compass engineering docs preview: https://compass-comms-rig3531-tenant.compass-eng-docs.pages.dev Deployed from |
rigel-mintaka
marked this pull request as ready for review
September 9, 2026 07:41
T4 of the multi-actor test-coverage record: the zero-container e2e leg that proves cross-tenant comms visibility survives the real door — real per-account bearers, real HTTP/2 server-streams, real cross-process seq ordering. Three assertions over observer-scoped clients (the admin bearer sees everything, so a negative visibility claim is unassertable through it): - owner-1's live stream receives its own private-channel post (subscribed before the post, so the event travels the live tail, not replay). - owner-2, subscribed at sinceSeq=0 after the private post, receives a globally-visible canary as its FIRST matching event and never the private post. The absence is proven by in-order per-subscriber delivery, not by a sleep: the private post sits in the replay window owner-2 is served, so a leak would necessarily arrive ahead of the canary. A second owner-1 subscription opened after the post replays it, so the negative is about the visibility filter rather than the event being unreachable. - cross-owner OpenDM collapses to NOT_FOUND byte-identically to an unknown handle — both arms asserted, because indistinguishability is the contract. Verified biting against production: disabling visibleToActor's MessagePosted membership arm (subscribe.go) fails the leg via the LEAK assertion. Test-only; no production code changed. Refs RIG-3531
…IG-3531) Review findings on the T4 leg: one high, three medium, four low. The high one was load-bearing. Assertion 3's cross-owner arm was never proven to be cross-owner — the only setup guard was `agent1ID != agent2ID`, which holds for any two distinct accounts including two agents under the SAME owner. Agent ownership here is emergent: nothing in CreateAgentRequest names an owner, so placement depends entirely on which observer client was passed. With both agents under one owner the cross-owner handle misses the resolver instead of biting OpenDM's same-owner check, still returns NOT_FOUND, and every assertion passes — the test proves `unknown == unknown` and reads as a cross-owner proof. Verified by counterfactual: creating agent-2 over owner-1's client left the pre-fix test GREEN. `createAgentAs` now returns the server-resolved owner (`Account.Agent.OwnerUserId`, populated at mapping.go:33 — no new RPC) and the placement is asserted before assertion 3 runs. Assertion 3 also compared only `connect.Code` while its own comment claimed byte-identical indistinguishability. Both arms reach NOT_FOUND through different branches — cross-owner via the same-owner check (comms.go:683), unknown via the resolver miss (resolve.go:88) — and share a code only by funnelling into the same edgeError arm. What actually closes the oracle is notFoundHandle re-keying both to the submitted handle, which is a MESSAGE oracle a code compare cannot see. Now compares the messages with the submitted handle redacted. Verified by mutating production to leak the peer's owner id into the message while holding the code at NOT_FOUND: the old assertion passed, the new one fails. Assertion 1 claimed live-tail delivery it did not bind. SubscribeComms returns before the server registers the subscriber (connect's CallServerStream does not wait for a response frame), so the post could be served from the replay snapshot and satisfy the assertion anyway. Gated owner-1's stream with awaitSubscriptionLive, the happens-before primitive already applied to owner-2. awaitSubscriptionLive was the file's only unbounded wait — a bare Receive on a stream riding a deadline-free ctx, against a header claiming every wait is ctx-bounded. Now pumps in a goroutine raced against a derived deadline, the shape AwaitDelivery uses. Remaining doc comments narrowed to what the code proves: the canary arrives by founding membership, not by SHARED grouping; the t4- handle prefixes keep handles distinct within this leg rather than providing cross-leg isolation (per-fixture stacks already do); and assertion 2's "never" is bounded to "never ahead of the canary, MessagePosted only". Round 2 found one more, in the fix itself: errMessage degraded a non-connect error to err.Error() instead of failing. That is the same class it was added to close — a transport fault (stream reset, TLS, deadline) hitting both arms yields two identical strings containing neither submitted handle, the redaction removes nothing, and the comparison passes having observed no rejection at all. Now rejectionMessage(t, ...), fatal on the miss. Verified by mutation: substituting a bare error fails at the observation point with the reason, where before both arms would have compared equal. Also corrected three citations that pointed at neighbouring lines (comms.go:677, resolve.go:86-90, resolve.go:137 — each verified to carry the text cited) and recorded why the message compare is full-text rather than narrowed. Round 3 caught the fix's own comment overclaiming. It named a stream reset, a TLS fault and an rpcTimeout expiry as reaching the fatal, but connect codes every client-side error (wrapIfUncoded, connect@v1.20.0/error.go:279-287), so all three pass errors.As and the branch is effectively unreachable. A deadline expiry would in fact return "context deadline exceeded" on both arms and compare equal — what stops that is the absolute NOT_FOUND assertion at each call site, not this helper. Comment and fatal text now say so, including a warning not to weaken those assertions to a cross-vs-unknown code compare. Refs RIG-3531
mattwilkinsonn
added this pull request to stack #1031
September 10, 2026 01:56
mattwilkinsonn
approved these changes
Sep 10, 2026
|
🚫 This stack was removed from the merge queue because it was canceled by Matt Wilkinson (a GitHub user). See more details here.
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
rigel-mintaka
force-pushed
the
compass-comms/rig3531-tenant-transport
branch
from
September 10, 2026 02:26
ac8ae71 to
831af1f
Compare
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
2 participants
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.
This PR is part of a stack containing 3 PRs:
mainT4 of the multi-actor test-coverage record: the zero-container e2e leg that
proves cross-tenant comms visibility survives the real door — real per-account
bearers, real HTTP/2 server-streams, real cross-process seq ordering.
Three assertions over observer-scoped clients (the admin bearer sees everything,
so a negative visibility claim is unassertable through it):
before the post, so the event travels the live tail, not replay).
globally-visible canary as its FIRST matching event and never the private
post. The absence is proven by in-order per-subscriber delivery, not by a
sleep: the private post sits in the replay window owner-2 is served, so a
leak would necessarily arrive ahead of the canary. A second owner-1
subscription opened after the post replays it, so the negative is about the
visibility filter rather than the event being unreachable.
handle — both arms asserted, because indistinguishability is the contract.
Verified biting against production: disabling visibleToActor's MessagePosted
membership arm (subscribe.go) fails the leg via the LEAK assertion.
Test-only; no production code changed.
Refs RIG-3531