feat(comms): OpenDM handler + AsAccount adapter + relay dispatch + spawn auto-open (RIG-2964) - #757
Merged
Conversation
|
Compass engineering docs preview: https://compass-server-rig-2964-peer.compass-eng-docs.pages.dev Deployed from |
rigel-mintaka
added a commit
that referenced
this pull request
Aug 30, 2026
… tests (RIG-2964) Addresses the T3 review (PR #757): - **medium (security):** `dmChannelName` was not injective — the `--` delimiter collides when a handle itself contains `--` (the handle grammar permits consecutive hyphens), so pair {a, b--c} and {a--b, c} both derived `dm--a--b--c` and the second open resumed onto the first's channel, cross-adding members (a same-owner private-DM confidentiality break). Switch the separator to `:`, a byte the handle grammar excludes, so `dm:<lo>:<hi>` is injective. New pgtest opens both pairs and asserts DISTINCT channels + uncorrupted membership (RED-verified against the old delimiter). - **medium (test):** add the R8 headline-safety test — a post-spawn DM-open FAILURE (and a nil opener) never rolls back the spawn: peer placed, `dm_channel_name` empty. - **low (test):** assert a DM RESUME emits no second ChannelChanged (the created-only guard), canary-gated. - **low (perf):** `emitDMCreated` takes the already-read channel instead of re-reading it. Refs RIG-2964 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…awn auto-open (RIG-2964) Peer-DM task T3: wire the `OpenDM` RPC end to end and auto-open the manager↔peer DM at spawn. Builds on T1 (proto, #738) and T2 (store `dm.go`, #746), both merged. ### What changed - **`Comms.OpenDM` handler** (`internal/comms/comms.go`) — replaces the T1 `CodeUnimplemented` stub. Resolves the peer owner-namespaced (`resolveAgentAccount`), guards self-DM (`invalid_argument`), enforces same-owner authz collapsed oracle-safe to the merged `not_found` (a cross-owner peer is byte-identical to an unknown one, mirroring `ReparentAgent`), derives the deterministic sorted-handle name, and runs the whole open in one store tx. - **`internal/comms/dm.go`** (new) — `dmChannelName` (sorted-handle `dm--<lo>--<hi>`), `openDMTx` (one-tx `LockOwnerDMTx` → `EnsureOwnerDMGroupTx` → `UpsertDMChannelTx`, mirroring the store's `openDM` test helper and `EnsureCoordinationChannel`), and `emitDMCreated` (best-effort post-commit `ChannelChanged`, create-only). - **`OpenDMAsAccount` adapter** (`internal/comms/agent_caller.go`) — the agent-tool entry, mirroring `UpdateChannelMembersAsAccount` (`errNoActor` guard + `WithActor` + the shared handler path). - **Relay dispatch** — `CommsCaller` gains `OpenDMAsAccount`; `executeCall` gains the `open_dm` arm; `fakeCommsCaller` gains the recorder. - **Spawn auto-open (R8)** (`server/lifecycle.go`) — `lifecycleService` gains a narrow `dmOpener` seam; `SpawnAsAccount` sets `DmChannelName` from one post-spawn site covering fresh/resume/idempotent paths (OpenDM is resolve-or-create, so a re-spawn returns the same DM). A post-spawn open failure is logged and returns an empty `dm_channel_name` — never a spawn rollback (the DM is recoverable next turn via `comms_open_dm`). ### Tests - `internal/comms/dm_open_pgtest_test.go` (new): same-owner create (kind=DM ∧ mandatory ∧ both parties ∧ deterministic name); reopen resumes the same channel in either handle order; unknown / cross-owner → `not_found`; self → `invalid_argument`; empty account → `errNoActor`; `ChannelChanged` on create; `AsAccount` resolve-or-create. - `internal/runnerhub/relay_open_dm_test.go` (new): the `open_dm` arm forwards under the bound account, result oneof + `call_id` round-trip, tool error rendered in-band. - `server/lifecycle_pgtest_test.go`: spawn returns a live `dm_channel_name` whose channel is a real DM; idempotent re-spawn returns the same name. All green against a real Postgres; build + vet + gofmt + golangci-lint (0 issues) clean. Spec-impact: none. Refs RIG-2964 Co-authored-by: Matt Wilkinson <matt@rigel.build>
… tests (RIG-2964) Addresses the T3 review (PR #757): - **medium (security):** `dmChannelName` was not injective — the `--` delimiter collides when a handle itself contains `--` (the handle grammar permits consecutive hyphens), so pair {a, b--c} and {a--b, c} both derived `dm--a--b--c` and the second open resumed onto the first's channel, cross-adding members (a same-owner private-DM confidentiality break). Switch the separator to `:`, a byte the handle grammar excludes, so `dm:<lo>:<hi>` is injective. New pgtest opens both pairs and asserts DISTINCT channels + uncorrupted membership (RED-verified against the old delimiter). - **medium (test):** add the R8 headline-safety test — a post-spawn DM-open FAILURE (and a nil opener) never rolls back the spawn: peer placed, `dm_channel_name` empty. - **low (test):** assert a DM RESUME emits no second ChannelChanged (the created-only guard), canary-gated. - **low (perf):** `emitDMCreated` takes the already-read channel instead of re-reading it. Refs RIG-2964 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
force-pushed
the
compass-server/rig-2964-peer-dm-comms-hub
branch
from
August 30, 2026 22:47
1b5e331 to
0f5eaeb
Compare
…t test comments (RIG-2964) Round-2 review lows: three test doc-comments illustrated the pre-PR 2-arg newLifecycleService signature (now 3-arg with the dmOpener seam), and the resume-emit test comment described a 'scope after create' the assertion does not do (it counts all ChannelChanged for the DM and requires exactly 1). Comment-only — no behavior change. Refs RIG-2964 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
added a commit
that referenced
this pull request
Aug 30, 2026
…ly-lands-in-DM (RIG-2967) Peer-DM task T6: the integration proof of the full tasking loop over the real spine. Stacks on T3 (#757). No production code — consumes T0-T3 (design.md T6:831 "Interfaces: none new"). ### What changed - **`server/dm_e2e_pgtest_test.go`** (new, `//go:build pgtest && unix`) — assembles production's exact delivery wire inline (a real Postgres store, the real `runnerhub.Hub` with a `recordingRunner` door, a real comms service on the comms bus, and the delivery consumer driven by the REAL resume-based waker whose DM opener is that same comms service, so a spawn's auto-open runs through the real T3 handler — mirrors `sinks.go`). Reuses the full-stack placement harness (`attachFakeRunner`/`recordingRunner`) and the offline-mention e2e's control accessors. ### Tests - `TestPeerDMTaskingLoopEndToEnd` — open (kind=DM ∧ mandatory ∧ both members ∧ created) → A posts naming the DM channel+topic (T0 name-addressed `create_topic`) → B comes live, the message is cursor-swept as a deliver whose recorded source channel+topic names ARE the DM's (the T0 denorm the reply-routing fix hangs on) → B replies naming that same DM channel+topic and the reply lands in the SAME DM topic, asserted two-sided (present in the DM ∧ absent from B's home channel — the dogfood-symptom misroute regression reddens both halves) → re-open is idempotent (same id, created=false) → an outsider same-owner agent's list on the DM collapses to `not_found` (D9 visibility clip). - `TestPeerDMSpawnPathDelivers` — a manager spawns a peer; the spawn auto-opens the manager↔peer DM and returns a live `dm_channel_name` whose channel satisfies the DM invariants; the spawn runs the real hub Provision→Start so the peer is genuinely hub-live, and a manager post into that DM reaches it via the live fan-out, event-gated on the recording runner. Every async wait event-gates on an observed wire fact (`waitForControlDelivers`) — never a sleep. Both green against a real Postgres; vet + gofmt clean. Spec-impact: none. Refs RIG-2967 Co-authored-by: Matt Wilkinson <matt@rigel.build>
mattwilkinsonn
approved these changes
Aug 30, 2026
mattwilkinsonn
added a commit
that referenced
this pull request
Aug 30, 2026
…ly-lands-in-DM (RIG-2967) (#758) * feat(comms): OpenDM handler + AsAccount adapter + relay dispatch + spawn auto-open (RIG-2964) Peer-DM task T3: wire the `OpenDM` RPC end to end and auto-open the manager↔peer DM at spawn. Builds on T1 (proto, #738) and T2 (store `dm.go`, #746), both merged. ### What changed - **`Comms.OpenDM` handler** (`internal/comms/comms.go`) — replaces the T1 `CodeUnimplemented` stub. Resolves the peer owner-namespaced (`resolveAgentAccount`), guards self-DM (`invalid_argument`), enforces same-owner authz collapsed oracle-safe to the merged `not_found` (a cross-owner peer is byte-identical to an unknown one, mirroring `ReparentAgent`), derives the deterministic sorted-handle name, and runs the whole open in one store tx. - **`internal/comms/dm.go`** (new) — `dmChannelName` (sorted-handle `dm--<lo>--<hi>`), `openDMTx` (one-tx `LockOwnerDMTx` → `EnsureOwnerDMGroupTx` → `UpsertDMChannelTx`, mirroring the store's `openDM` test helper and `EnsureCoordinationChannel`), and `emitDMCreated` (best-effort post-commit `ChannelChanged`, create-only). - **`OpenDMAsAccount` adapter** (`internal/comms/agent_caller.go`) — the agent-tool entry, mirroring `UpdateChannelMembersAsAccount` (`errNoActor` guard + `WithActor` + the shared handler path). - **Relay dispatch** — `CommsCaller` gains `OpenDMAsAccount`; `executeCall` gains the `open_dm` arm; `fakeCommsCaller` gains the recorder. - **Spawn auto-open (R8)** (`server/lifecycle.go`) — `lifecycleService` gains a narrow `dmOpener` seam; `SpawnAsAccount` sets `DmChannelName` from one post-spawn site covering fresh/resume/idempotent paths (OpenDM is resolve-or-create, so a re-spawn returns the same DM). A post-spawn open failure is logged and returns an empty `dm_channel_name` — never a spawn rollback (the DM is recoverable next turn via `comms_open_dm`). ### Tests - `internal/comms/dm_open_pgtest_test.go` (new): same-owner create (kind=DM ∧ mandatory ∧ both parties ∧ deterministic name); reopen resumes the same channel in either handle order; unknown / cross-owner → `not_found`; self → `invalid_argument`; empty account → `errNoActor`; `ChannelChanged` on create; `AsAccount` resolve-or-create. - `internal/runnerhub/relay_open_dm_test.go` (new): the `open_dm` arm forwards under the bound account, result oneof + `call_id` round-trip, tool error rendered in-band. - `server/lifecycle_pgtest_test.go`: spawn returns a live `dm_channel_name` whose channel is a real DM; idempotent re-spawn returns the same name. All green against a real Postgres; build + vet + gofmt + golangci-lint (0 issues) clean. Spec-impact: none. Refs RIG-2964 Co-authored-by: Matt Wilkinson <matt@rigel.build> * fix(comms): injective DM name (colon sep) + R8 rollback-safety & emit tests (RIG-2964) Addresses the T3 review (PR #757): - **medium (security):** `dmChannelName` was not injective — the `--` delimiter collides when a handle itself contains `--` (the handle grammar permits consecutive hyphens), so pair {a, b--c} and {a--b, c} both derived `dm--a--b--c` and the second open resumed onto the first's channel, cross-adding members (a same-owner private-DM confidentiality break). Switch the separator to `:`, a byte the handle grammar excludes, so `dm:<lo>:<hi>` is injective. New pgtest opens both pairs and asserts DISTINCT channels + uncorrupted membership (RED-verified against the old delimiter). - **medium (test):** add the R8 headline-safety test — a post-spawn DM-open FAILURE (and a nil opener) never rolls back the spawn: peer placed, `dm_channel_name` empty. - **low (test):** assert a DM RESUME emits no second ChannelChanged (the created-only guard), canary-gated. - **low (perf):** `emitDMCreated` takes the already-read channel instead of re-reading it. Refs RIG-2964 Co-authored-by: Matt Wilkinson <matt@rigel.build> * docs(comms): correct stale newLifecycleService signature + resume-emit test comments (RIG-2964) Round-2 review lows: three test doc-comments illustrated the pre-PR 2-arg newLifecycleService signature (now 3-arg with the dmOpener seam), and the resume-emit test comment described a 'scope after create' the assertion does not do (it counts all ChannelChanged for the DM and requires exactly 1). Comment-only — no behavior change. Refs RIG-2964 Co-authored-by: Matt Wilkinson <matt@rigel.build> * test(server): e2e pgtest — peer-DM tasking loop open→post→deliver→reply-lands-in-DM (RIG-2967) Peer-DM task T6: the integration proof of the full tasking loop over the real spine. Stacks on T3 (#757). No production code — consumes T0-T3 (design.md T6:831 "Interfaces: none new"). ### What changed - **`server/dm_e2e_pgtest_test.go`** (new, `//go:build pgtest && unix`) — assembles production's exact delivery wire inline (a real Postgres store, the real `runnerhub.Hub` with a `recordingRunner` door, a real comms service on the comms bus, and the delivery consumer driven by the REAL resume-based waker whose DM opener is that same comms service, so a spawn's auto-open runs through the real T3 handler — mirrors `sinks.go`). Reuses the full-stack placement harness (`attachFakeRunner`/`recordingRunner`) and the offline-mention e2e's control accessors. ### Tests - `TestPeerDMTaskingLoopEndToEnd` — open (kind=DM ∧ mandatory ∧ both members ∧ created) → A posts naming the DM channel+topic (T0 name-addressed `create_topic`) → B comes live, the message is cursor-swept as a deliver whose recorded source channel+topic names ARE the DM's (the T0 denorm the reply-routing fix hangs on) → B replies naming that same DM channel+topic and the reply lands in the SAME DM topic, asserted two-sided (present in the DM ∧ absent from B's home channel — the dogfood-symptom misroute regression reddens both halves) → re-open is idempotent (same id, created=false) → an outsider same-owner agent's list on the DM collapses to `not_found` (D9 visibility clip). - `TestPeerDMSpawnPathDelivers` — a manager spawns a peer; the spawn auto-opens the manager↔peer DM and returns a live `dm_channel_name` whose channel satisfies the DM invariants; the spawn runs the real hub Provision→Start so the peer is genuinely hub-live, and a manager post into that DM reaches it via the live fan-out, event-gated on the recording runner. Every async wait event-gates on an observed wire fact (`waitForControlDelivers`) — never a sleep. Both green against a real Postgres; vet + gofmt clean. Spec-impact: none. Refs RIG-2967 Co-authored-by: Matt Wilkinson <matt@rigel.build> * docs(server): document manager-offline + exact-count invariants in peer-DM e2e (RIG-2967) Round-2 review lows: comment the load-bearing invariants the spawn-path e2e relies on — the manager is intentionally not hub-live so its DM post fires the deliver immediately (offline-author path, no author-settle hold), and the exact-count deliver check is airtight because B's only owed message is the single DM post. Also clarified the got (event-gate) vs dd (denorm re-snapshot) split. Comment-only — no behavior change. Refs RIG-2967 Co-authored-by: Matt Wilkinson <matt@rigel.build> --------- Co-authored-by: Matt Wilkinson <matt@rigel.build>
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 2 PRs:
mainPeer-DM task T3: wire the
OpenDMRPC end to end and auto-open the manager↔peer DM at spawn. Builds on T1 (proto, #738) and T2 (storedm.go, #746), both merged.What changed
Comms.OpenDMhandler (internal/comms/comms.go) — replaces the T1CodeUnimplementedstub. Resolves the peer owner-namespaced (resolveAgentAccount), guards self-DM (invalid_argument), enforces same-owner authz collapsed oracle-safe to the mergednot_found(a cross-owner peer is byte-identical to an unknown one, mirroringReparentAgent), derives the deterministic sorted-handle name, and runs the whole open in one store tx.internal/comms/dm.go(new) —dmChannelName(sorted-handledm--<lo>--<hi>),openDMTx(one-txLockOwnerDMTx→EnsureOwnerDMGroupTx→UpsertDMChannelTx, mirroring the store'sopenDMtest helper andEnsureCoordinationChannel), andemitDMCreated(best-effort post-commitChannelChanged, create-only).OpenDMAsAccountadapter (internal/comms/agent_caller.go) — the agent-tool entry, mirroringUpdateChannelMembersAsAccount(errNoActorguard +WithActor+ the shared handler path).CommsCallergainsOpenDMAsAccount;executeCallgains theopen_dmarm;fakeCommsCallergains the recorder.server/lifecycle.go) —lifecycleServicegains a narrowdmOpenerseam;SpawnAsAccountsetsDmChannelNamefrom one post-spawn site covering fresh/resume/idempotent paths (OpenDM is resolve-or-create, so a re-spawn returns the same DM). A post-spawn open failure is logged and returns an emptydm_channel_name— never a spawn rollback (the DM is recoverable next turn viacomms_open_dm).Tests
internal/comms/dm_open_pgtest_test.go(new): same-owner create (kind=DM ∧ mandatory ∧ both parties ∧ deterministic name); reopen resumes the same channel in either handle order; unknown / cross-owner →not_found; self →invalid_argument; empty account →errNoActor;ChannelChangedon create;AsAccountresolve-or-create.internal/runnerhub/relay_open_dm_test.go(new): theopen_dmarm forwards under the bound account, result oneof +call_idround-trip, tool error rendered in-band.server/lifecycle_pgtest_test.go: spawn returns a livedm_channel_namewhose channel is a real DM; idempotent re-spawn returns the same name.All green against a real Postgres; build + vet + gofmt + golangci-lint (0 issues) clean.
Spec-impact: none. Refs RIG-2964
Co-authored-by: Matt Wilkinson matt@rigel.build