feat(store): reserved DM group + upsert + create-guard + convert-on-add (RIG-2963) - #746
Merged
Merged
Conversation
|
Compass engineering docs preview: https://compass-server-rig-2963-peer.compass-eng-docs.pages.dev Deployed from |
rigel-mintaka
force-pushed
the
compass-server/rig-2963-peer-dm-store
branch
2 times, most recently
from
August 30, 2026 20:41
9a9012b to
688d4dc
Compare
…dd (RIG-2963) Peer-DM T2 (design product/compass-agent-peer-dm §Plan T2, Decisions R3/R4). Store layer for agent peer-DMs, stacked on the T1 proto contract (#738). - New go/internal/store/dm.go: EnsureOwnerDMGroupTx (per-owner reserved __dm__ group, visibility-discriminated, mirrors EnsureOwnerCoordinationGroupTx); UpsertDMChannelTx (deterministic sorted-handle name, no suffix search, ON CONFLICT DO NOTHING + re-SELECT resume loop, born kind=DM + OPEN ownerless + mandatory, expand owner membership + seed cursors); verifyReconcileDMTx (R3 belt: wrong-kind squat -> ErrNotFound, mandatory + missing-member drift reconciled in-tx); LockOwnerDMTx (distinct 'dm:' advisory-lock key domain); isReservedDMGroupTx (create-guard discriminator). - channels.go: R3 create-guard in CreateChannel (reject a manual create into a reserved DM group with merged ErrNotFound); R4 convert-on-add in UpdateChannelMembers (a genuine member ADD on kind=DM requires ConvertChannelName, flips kind=CHANNEL + rename + group_id=NULL freeing the DM name, before the add) + two-party floor (a remove may not strand a DM below two agent parties). FOR UPDATE read now also returns kind. - inputs.go: MemberUpdatesOptions{ConvertChannelName}; UpdateChannelMembers signature widened; comms.go threads req.Msg.GetConvertChannelName(). - dm_pgtest_test.go: 9 pgtest cases (create invariants, idempotent resume either direction, concurrent-open race -> one channel, delivery-target predicate, manual create into reserved group -> not_found, squat belt, convert with/without name, fresh pair after convert, remove-below-two rejected). Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
force-pushed
the
compass-server/rig-2963-peer-dm-store
branch
from
August 30, 2026 20:47
688d4dc to
6c41eae
Compare
mattwilkinsonn
approved these changes
Aug 30, 2026
rigel-mintaka
added a commit
that referenced
this pull request
Aug 30, 2026
…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>
mattwilkinsonn
added a commit
that referenced
this pull request
Aug 30, 2026
…awn auto-open (RIG-2964) (#757) * 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> --------- Co-authored-by: Matt Wilkinson <matt@rigel.build>
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.
Peer-DM T2 (design product/compass-agent-peer-dm §Plan T2, Decisions R3/R4). Store layer for agent peer-DMs, stacked on the T1 proto contract (#738).
Co-authored-by: Matt Wilkinson matt@rigel.build