From 25f785ec1ddbdfbab4db443bd7cb7ba120f20306 Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 5 Oct 2026 13:41:51 -0400 Subject: [PATCH 1/9] feat(store): unique sibling channel-group names with a repairing migration (RIG-3030) Group names become unique among one owner's siblings and may not contain a slash, so a slash path from main's group-ref resolver names at most one of an owner's groups. The migration repairs existing rows as compass_system: slash names are rewritten, duplicates get a free -N suffix with the pre-existing valid name kept, and reserved top-level groups are untouched. CreateChannelGroup maps the new unique violation to AlreadyExists. Co-authored-by: Matt Wilkinson --- .squawk.toml | 30 ++-- go/gen/compass/v1/comms.pb.go | 3 +- ...annel_group_names_migration_pgtest_test.go | 152 ++++++++++++++++++ go/internal/store/channels.go | 3 + go/internal/store/channels_test.go | 32 ++++ go/internal/store/coordination.go | 9 +- go/internal/store/dm.go | 7 +- .../migrations/0009_channel_group_names.sql | 71 ++++++++ .../src/gen/compass/v1/comms_pb.ts | 3 +- .../src/gen/compass/v1/comms_pb.ts | 3 +- proto/compass/v1/comms.proto | 3 +- 11 files changed, 289 insertions(+), 27 deletions(-) create mode 100644 go/internal/store/channel_group_names_migration_pgtest_test.go create mode 100644 go/internal/store/migrations/0009_channel_group_names.sql diff --git a/.squawk.toml b/.squawk.toml index 744243838..260eab6ad 100644 --- a/.squawk.toml +++ b/.squawk.toml @@ -1,15 +1,15 @@ # squawk — Postgres migration-safety linter for go/internal/store/migrations/. # Gate config for tools/sql-migration-gate/moon.yml. # -# WHY THESE EXCLUSIONS: every migration so far (0001_init.sql, then the -# append-only files after it) creates tables that are empty when it runs, inside -# one transaction (see the migration runner + advisory lock). squawk's default -# rule set is calibrated for changes to a large LIVE table, where a full-table -# rewrite or a non-CONCURRENT index build takes a blocking lock that stalls -# traffic. On a new empty table that hazard does not exist, so the rules below -# are accepted. Revisit this list when a migration first alters or indexes a -# populated table. The gate stays RED on genuinely unsafe NEW DDL (e.g. adding -# a NOT NULL column with no default), which these exclusions do not silence. +# WHY THESE EXCLUSIONS: every migration runs inside one transaction under the +# runner's advisory lock (see the migration runner). squawk's default rule set +# is calibrated for a large LIVE table, where a full-table rewrite or a +# non-CONCURRENT index build stalls traffic for its duration. Most migrations +# here create empty tables; the ones that touch populated tables (e.g. +# channel_groups) touch small ones, where the lock is brief. Revisit this list +# when a migration first alters or indexes a large populated table. The gate +# stays RED on genuinely unsafe NEW DDL (e.g. adding a NOT NULL column with no +# default), which these exclusions do not silence. pg_version = "16.0" @@ -22,13 +22,13 @@ pg_version = "16.0" assume_in_transaction = true excluded_rules = [ - # CREATE INDEX (non-CONCURRENTLY) and the lock it takes only block writes on a - # populated table. Every index here is built on an empty pre-live table inside - # the bootstrap transaction; CONCURRENTLY is in fact illegal inside a txn. + # A non-CONCURRENT index blocks writes while it builds. Indexes here land on + # new empty tables or on small populated ones, so the block is brief; and + # CONCURRENTLY is illegal inside the runner's transaction anyway. "require-concurrent-index-creation", - # lock_timeout / statement_timeout guard against a slow blocking op stalling - # live traffic. There is no live traffic during a pre-live bootstrap; the whole - # migration runs under one advisory-locked transaction. + # lock_timeout / statement_timeout guard a slow blocking op against stalling + # live traffic. Ours are short ops on empty or small tables; a migration that + # can queue behind long transactions sets its own lock_timeout. "require-timeout-settings", # IF NOT EXISTS makes a migration re-runnable after a partial failure. This # migration is transactional (all-or-nothing) and version-gated by the runner, diff --git a/go/gen/compass/v1/comms.pb.go b/go/gen/compass/v1/comms.pb.go index 8b1a56686..05bc69a64 100644 --- a/go/gen/compass/v1/comms.pb.go +++ b/go/gen/compass/v1/comms.pb.go @@ -2732,7 +2732,8 @@ func (x *ListAccountsResponse) GetAccounts() []*Account { type CreateChannelGroupRequest struct { state protoimpl.MessageState `protogen:"open.v1"` - // Leaf name of the group, e.g. "matt". + // Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name an + // owner already uses under the same parent is ALREADY_EXISTS. Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` // Parent group; empty for a top-level group. ParentGroupId string `protobuf:"bytes,2,opt,name=parent_group_id,json=parentGroupId,proto3" json:"parent_group_id,omitempty"` diff --git a/go/internal/store/channel_group_names_migration_pgtest_test.go b/go/internal/store/channel_group_names_migration_pgtest_test.go new file mode 100644 index 000000000..c8e718ccd --- /dev/null +++ b/go/internal/store/channel_group_names_migration_pgtest_test.go @@ -0,0 +1,152 @@ +//go:build pgtest + +package store + +import ( + "slices" + "strings" + "testing" + + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/RigelBuild/compass/go/internal/pgtest" +) + +// TestChannelGroupNamesMigrationRepairsExistingRows replays the channel-group-names +// migration over pre-constraint rows: slashes and duplicate siblings are renamed, +// a valid original keeps its name, and reserved top-level names stay untouched. +func TestChannelGroupNamesMigrationRepairsExistingRows(t *testing.T) { + ctx := t.Context() + dsn := pgtest.RequireDSN(t) + pool, err := pgxpool.New(ctx, dsn) + if err != nil { + t.Fatalf("pgxpool.New: %v", err) + } + t.Cleanup(pool.Close) + + migs, err := loadMigrations() + if err != nil { + t.Fatalf("loadMigrations: %v", err) + } + // Found by name: the slot is renumbered whenever a concurrent migration lands first. + target := slices.IndexFunc(migs, func(m migration) bool { return strings.HasSuffix(m.name, "_channel_group_names.sql") }) + if target < 0 { + t.Fatal("channel-group-names migration not embedded") + } + conn, err := pool.Acquire(ctx) + if err != nil { + t.Fatalf("acquire migration connection: %v", err) + } + defer conn.Release() + if err := ensureMigrationsTable(ctx, conn); err != nil { + t.Fatalf("ensure migrations table: %v", err) + } + for _, m := range migs[:target] { + if err := applyMigration(ctx, conn, m); err != nil { + t.Fatalf("apply v%d: %v", m.version, err) + } + } + + // The owner connection skips RLS only as superuser, so seed as the system role. + tx, err := pool.Begin(ctx) + if err != nil { + t.Fatalf("begin seed: %v", err) + } + defer func() { _ = tx.Rollback(ctx) }() // no-op after Commit; a failed seed already fails the test + if _, err := tx.Exec(ctx, "SET LOCAL ROLE "+systemRole); err != nil { + t.Fatalf("set seed role: %v", err) + } + if _, err := tx.Exec(ctx, ` + INSERT INTO tenants (id, slug, display_name, created_at_unix_ms) + VALUES ('boot', 'default', 'Default', 1), ('boot-2', 'second', 'Second', 2); + INSERT INTO channel_groups (id, name, owner_user_id, tenant_id, parent_group_id) VALUES + ('group-a', 'duplicate', 'owner-a', 'boot', NULL), + ('group-b', 'duplicate', 'owner-a', 'boot', NULL), + ('group-c', 'duplicate-2', 'owner-a', 'boot', NULL), + ('group-d', 'path/name', 'owner-b', 'boot', NULL), + ('group-e', '__dm__', 'owner-c', 'boot', NULL), + ('group-f', '__dm__', 'owner-c', 'boot', NULL), + ('group-g', 'a/b', 'owner-d', 'boot', NULL), + ('group-h', 'a-b', 'owner-d', 'boot', NULL), + ('group-i', 'parent', 'owner-e', 'boot', NULL), + ('group-j', 'nested', 'owner-e', 'boot', 'group-i'), + ('group-k', 'nested', 'owner-e', 'boot', 'group-i'), + ('group-l', 'tenant-name', 'owner-f', 'boot', NULL), + ('group-m', 'tenant-name', 'owner-f', 'boot-2', NULL)`); err != nil { + t.Fatalf("seed pre-migration groups: %v", err) + } + if err := tx.Commit(ctx); err != nil { + t.Fatalf("commit seed: %v", err) + } + + if err := applyMigration(ctx, conn, migs[target]); err != nil { + t.Fatalf("apply channel-group-names migration: %v", err) + } + + got := readGroupNamesAsSystem(t, pool) + want := map[string]string{ + "group-a": "duplicate", // lowest id keeps the name + "group-b": "duplicate-3", // -2 is taken by group-c + "group-c": "duplicate-2", // a pre-existing valid name is never clobbered + "group-d": "path-name", // slash rewritten + "group-e": "__dm__", // reserved top-level names are exempt + "group-f": "__dm__", // reserved top-level names are exempt + "group-g": "a-b-2", // a rewritten name yields to the valid original + "group-h": "a-b", // the valid original keeps its name + "group-i": "parent", // nested parent untouched + "group-j": "nested", // nested siblings dedupe too + "group-k": "nested-2", // nested siblings dedupe too + "group-l": "tenant-name", // owner ids are global, so tenants share a namespace + "group-m": "tenant-name-2", // owner ids are global, so tenants share a namespace + } + for id, name := range want { + if got[id] != name { + t.Errorf("group %s name = %q, want %q", id, got[id], name) + } + } + + var indexExists, checkExists bool + if err := pool.QueryRow(ctx, `SELECT + EXISTS (SELECT 1 FROM pg_indexes WHERE schemaname = current_schema() + AND indexname = 'channel_groups_owner_parent_name_key'), + EXISTS (SELECT 1 FROM pg_constraint WHERE conname = 'channel_groups_name_no_slash' + AND conrelid = 'channel_groups'::regclass AND convalidated)`).Scan(&indexExists, &checkExists); err != nil { + t.Fatalf("inspect constraints: %v", err) + } + if !indexExists { + t.Error("sibling unique index does not exist") + } + if !checkExists { + t.Error("validated no-slash check does not exist") + } +} + +func readGroupNamesAsSystem(t *testing.T, pool *pgxpool.Pool) map[string]string { + t.Helper() + ctx := t.Context() + tx, err := pool.Begin(ctx) + if err != nil { + t.Fatalf("begin read: %v", err) + } + defer func() { _ = tx.Rollback(ctx) }() // read-only tx; rollback is cleanup only + if _, err := tx.Exec(ctx, "SET LOCAL ROLE "+systemRole); err != nil { + t.Fatalf("set read role: %v", err) + } + rows, err := tx.Query(ctx, `SELECT id, name FROM channel_groups`) + if err != nil { + t.Fatalf("read repaired groups: %v", err) + } + defer rows.Close() + got := make(map[string]string) + for rows.Next() { + var id, name string + if err := rows.Scan(&id, &name); err != nil { + t.Fatalf("scan repaired group: %v", err) + } + got[id] = name + } + if err := rows.Err(); err != nil { + t.Fatalf("iterate repaired groups: %v", err) + } + return got +} diff --git a/go/internal/store/channels.go b/go/internal/store/channels.go index 73ab5c47b..37711413f 100644 --- a/go/internal/store/channels.go +++ b/go/internal/store/channels.go @@ -68,6 +68,9 @@ func (s *Store) CreateChannelGroup(ctx context.Context, ownerUserID AccountID, g OwnerUserID: string(ownerUserID), Visibility: int16(g.Visibility), //nolint:gosec // G115: ChannelGroupVisibility is a CHECK-constrained 0/1 enum (channel_groups.visibility), always within int16 }); err != nil { + if pgErrIs(err, pgUniqueViolation) && pgConstraintName(err) == "channel_groups_owner_parent_name_key" { + return ChannelGroup{}, fmt.Errorf("%w: sibling group %q already exists", ErrConflict, g.Name) + } return ChannelGroup{}, fmt.Errorf("store: insert channel group: %w", err) } if err := tx.Commit(ctx); err != nil { diff --git a/go/internal/store/channels_test.go b/go/internal/store/channels_test.go index 0f9700bd5..7337e9f4d 100644 --- a/go/internal/store/channels_test.go +++ b/go/internal/store/channels_test.go @@ -12,6 +12,38 @@ import ( "testing" ) +// TestCreateChannelGroupSiblingNamesUnique: a name is unique among one owner's +// siblings, so agent tools can address it; other parents and owners may reuse it. +func TestCreateChannelGroupSiblingNamesUnique(t *testing.T) { + ctx := t.Context() + s := newTestStore(t) + owner := mustUser(t, s, "owner") + other := mustUser(t, s, "other") + mk := func(by AccountID, name string, parent ChannelGroupID) error { + _, err := s.CreateChannelGroup(ctx, by, NewChannelGroup{Name: name, ParentGroupID: parent, Visibility: VisibilityOwner}) + return err + } + parent, err := s.CreateChannelGroup(ctx, owner.ID, NewChannelGroup{Name: "parent", Visibility: VisibilityOwner}) + if err != nil { + t.Fatalf("CreateChannelGroup(parent): %v", err) + } + otherParent, err := s.CreateChannelGroup(ctx, owner.ID, NewChannelGroup{Name: "other-parent", Visibility: VisibilityOwner}) + if err != nil { + t.Fatalf("CreateChannelGroup(other-parent): %v", err) + } + if err := mk(owner.ID, "same", parent.ID); err != nil { + t.Fatalf("first sibling: %v", err) + } + sentinelIs(t, mk(owner.ID, "same", parent.ID), ErrConflict, "duplicate nested sibling") + sentinelIs(t, mk(owner.ID, "parent", ""), ErrConflict, "duplicate top-level sibling") + if err := mk(owner.ID, "same", otherParent.ID); err != nil { + t.Fatalf("same name under another parent: %v", err) + } + if err := mk(other.ID, "parent", ""); err != nil { + t.Fatalf("same top-level name for another owner: %v", err) + } +} + func TestCreateChannelGroupCeilingRejectsWiderChild(t *testing.T) { ctx := context.Background() s := newTestStore(t) diff --git a/go/internal/store/coordination.go b/go/internal/store/coordination.go index 70a260015..84989268e 100644 --- a/go/internal/store/coordination.go +++ b/go/internal/store/coordination.go @@ -90,8 +90,8 @@ func (s *Store) EnsureOwnerCoordinationGroupTx(ctx context.Context, tx pgx.Tx, o // A fixed reserved name scoped to the owner — deterministic, so every // reconcile for this owner resolves the identical group. The get-half is // VISIBILITY-DISCRIMINATED (AND visibility = $3, bound to VisibilityOwner): - // CreateChannelGroup has no reserved-name guard, so a user CAN plant a - // top-level group named __coordination__ at any visibility. A wider + // a top-level group named __coordination__ may predate the reserved-name + // guard or come from a raw insert, at any visibility. A wider // (VisibilityShared) planted group must NEVER be adopted — inserting the // OWNER_ONLY coordination channel into a SHARED group would make an // owner-private channel visible to every account (channelVisiblePredicate), @@ -101,9 +101,8 @@ func (s *Store) EnsureOwnerCoordinationGroupTx(ctx context.Context, tx pgx.Tx, o // correctly-named top-level planted group is harmless: it has the exact shape // the reconcile would itself create. The caller holds the per-owner advisory // lock (LockOwnerCoordinationTx), so the get-then-create cannot race a - // concurrent reconcile for the same owner into two groups; channel_groups has - // no unique index on (name, owner, parent), and CreateChannelGroup refuses the - // reserved top-level name, so only the system insert can plant such a row. + // concurrent reconcile for the same owner into two groups; the sibling-name + // index exempts top-level reserved names, and the API guard rejects new user inserts. qtx := db.New(tx) existing, err := qtx.GetCoordinationGroup(ctx, db.GetCoordinationGroupParams{ diff --git a/go/internal/store/dm.go b/go/internal/store/dm.go index 0432c0dbe..bc0fd1efb 100644 --- a/go/internal/store/dm.go +++ b/go/internal/store/dm.go @@ -23,7 +23,8 @@ const dmGroupName = "__dm__" const coordinationGroupName = "__coordination__" // isReservedGroupName reports whether name is a system group name that a -// caller may not claim at top level. +// caller may not claim at top level. A new name must also join the exemption in +// channel_groups_owner_parent_name_key, which needs a migration. func isReservedGroupName(name string) bool { return name == dmGroupName || name == coordinationGroupName || name == linearRoutingGroupName } @@ -48,8 +49,8 @@ type DMChannelSpec struct { // owner-private, never lattice-shared), un-parented — differing only in the // reserved name (__dm__ vs __coordination__) so the two reserved namespaces stay // disjoint. The get-half is VISIBILITY-DISCRIMINATED (AND visibility = $3, bound -// to VisibilityOwner): CreateChannelGroup has no reserved-name guard, so a user -// CAN plant a top-level group named __dm__ at any visibility; a wider +// to VisibilityOwner): a top-level group named __dm__ may predate the +// reserved-name guard or come from a raw insert, at any visibility; a wider // (VisibilityShared) planted group must NEVER be adopted (it would host // owner-private DMs in a shared group — a cross-tenant leak), so the // discriminator excludes it and the create-half INSERTs the correct diff --git a/go/internal/store/migrations/0009_channel_group_names.sql b/go/internal/store/migrations/0009_channel_group_names.sql new file mode 100644 index 000000000..91a180728 --- /dev/null +++ b/go/internal/store/migrations/0009_channel_group_names.sql @@ -0,0 +1,71 @@ +-- Agent tools read '/' as a path separator and address a group by sibling name, +-- so a name holds no '/' and is unique among its siblings. Owner ids are global, +-- so the key is owner/parent/name; tenant_id is not part of it. + +-- Under FORCE RLS a non-superuser owner sees no rows, so the repair runs as +-- compass_system. +SET LOCAL ROLE compass_system; + +-- Rewrite separators first, then suffix duplicates. A valid original keeps its +-- name; each free suffix is found first so no existing name is overwritten. +DO $$ +DECLARE + rewritten_ids TEXT[]; + duplicate RECORD; + candidate TEXT; + suffix INTEGER; +BEGIN + WITH rewritten AS ( + UPDATE channel_groups SET name = REPLACE(name, '/', '-') + WHERE name LIKE '%/%' + RETURNING id + ) + SELECT COALESCE(array_agg(id), '{}') INTO rewritten_ids FROM rewritten; + + FOR duplicate IN + SELECT id, owner_user_id, parent_group_id, name, duplicate_number + FROM ( + SELECT id, owner_user_id, parent_group_id, name, + row_number() OVER ( + PARTITION BY owner_user_id, COALESCE(parent_group_id, ''), name + ORDER BY id = ANY (rewritten_ids), id + ) AS duplicate_number + FROM channel_groups + ) AS ranked + WHERE duplicate_number > 1 + ORDER BY owner_user_id, COALESCE(parent_group_id, ''), name, duplicate_number + LOOP + IF duplicate.parent_group_id IS NULL + AND duplicate.name IN ('__dm__', '__linear__', '__coordination__') THEN + CONTINUE; + END IF; + suffix := duplicate.duplicate_number; + LOOP + candidate := duplicate.name || '-' || suffix::text; + EXIT WHEN NOT EXISTS ( + SELECT 1 + FROM channel_groups AS sibling + WHERE sibling.owner_user_id = duplicate.owner_user_id + AND COALESCE(sibling.parent_group_id, '') = COALESCE(duplicate.parent_group_id, '') + AND sibling.name = candidate + AND sibling.id <> duplicate.id + ); + suffix := suffix + 1; + END LOOP; + UPDATE channel_groups SET name = candidate WHERE id = duplicate.id; + END LOOP; +END $$; + +-- DDL runs as the table owner, not the system role. +RESET ROLE; + +-- Top-level reserved names stay exempt: a planted wider look-alike must not +-- block the system's own owner-visible group. +CREATE UNIQUE INDEX channel_groups_owner_parent_name_key + ON channel_groups (owner_user_id, COALESCE(parent_group_id, ''), name) + WHERE NOT (parent_group_id IS NULL AND name IN ('__dm__', '__linear__', '__coordination__')); + +-- The repair above leaves no '/' behind. The validating scan holds the write lock +-- only over this small table, inside the runner's single transaction. +-- squawk-ignore constraint-missing-not-valid +ALTER TABLE channel_groups ADD CONSTRAINT channel_groups_name_no_slash CHECK (POSITION('/' IN name) = 0); diff --git a/packages/compass-agent/src/gen/compass/v1/comms_pb.ts b/packages/compass-agent/src/gen/compass/v1/comms_pb.ts index 8ffce239f..7c506da3a 100644 --- a/packages/compass-agent/src/gen/compass/v1/comms_pb.ts +++ b/packages/compass-agent/src/gen/compass/v1/comms_pb.ts @@ -1277,7 +1277,8 @@ export const ListAccountsResponseSchema: GenMessage = /*@_ */ export type CreateChannelGroupRequest = Message$1<"compass.v1.CreateChannelGroupRequest"> & { /** - * Leaf name of the group, e.g. "matt". + * Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name an + * owner already uses under the same parent is ALREADY_EXISTS. * * @generated from field: string name = 1; */ diff --git a/packages/compass-client/src/gen/compass/v1/comms_pb.ts b/packages/compass-client/src/gen/compass/v1/comms_pb.ts index 8ffce239f..7c506da3a 100644 --- a/packages/compass-client/src/gen/compass/v1/comms_pb.ts +++ b/packages/compass-client/src/gen/compass/v1/comms_pb.ts @@ -1277,7 +1277,8 @@ export const ListAccountsResponseSchema: GenMessage = /*@_ */ export type CreateChannelGroupRequest = Message$1<"compass.v1.CreateChannelGroupRequest"> & { /** - * Leaf name of the group, e.g. "matt". + * Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name an + * owner already uses under the same parent is ALREADY_EXISTS. * * @generated from field: string name = 1; */ diff --git a/proto/compass/v1/comms.proto b/proto/compass/v1/comms.proto index 3955c6e4d..204443f8b 100644 --- a/proto/compass/v1/comms.proto +++ b/proto/compass/v1/comms.proto @@ -651,7 +651,8 @@ message ListAccountsResponse { } message CreateChannelGroupRequest { - // Leaf name of the group, e.g. "matt". + // Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name an + // owner already uses under the same parent is ALREADY_EXISTS. string name = 1; // Parent group; empty for a top-level group. string parent_group_id = 2; From cc4b524e27cc18df0b993d0bf3551de245f5f17b Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 5 Oct 2026 14:44:08 -0400 Subject: [PATCH 2/9] fix(store): lock and time-bound the group-name migration; cover nested reserved names (RIG-3030) Co-authored-by: Matt Wilkinson --- .../channel_group_names_migration_pgtest_test.go | 6 +++++- .../store/migrations/0009_channel_group_names.sql | 14 ++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/go/internal/store/channel_group_names_migration_pgtest_test.go b/go/internal/store/channel_group_names_migration_pgtest_test.go index c8e718ccd..360cfedff 100644 --- a/go/internal/store/channel_group_names_migration_pgtest_test.go +++ b/go/internal/store/channel_group_names_migration_pgtest_test.go @@ -72,7 +72,9 @@ func TestChannelGroupNamesMigrationRepairsExistingRows(t *testing.T) { ('group-j', 'nested', 'owner-e', 'boot', 'group-i'), ('group-k', 'nested', 'owner-e', 'boot', 'group-i'), ('group-l', 'tenant-name', 'owner-f', 'boot', NULL), - ('group-m', 'tenant-name', 'owner-f', 'boot-2', NULL)`); err != nil { + ('group-m', 'tenant-name', 'owner-f', 'boot-2', NULL), + ('group-n', '__dm__', 'owner-e', 'boot', 'group-i'), + ('group-o', '__dm__', 'owner-e', 'boot', 'group-i')`); err != nil { t.Fatalf("seed pre-migration groups: %v", err) } if err := tx.Commit(ctx); err != nil { @@ -98,6 +100,8 @@ func TestChannelGroupNamesMigrationRepairsExistingRows(t *testing.T) { "group-k": "nested-2", // nested siblings dedupe too "group-l": "tenant-name", // owner ids are global, so tenants share a namespace "group-m": "tenant-name-2", // owner ids are global, so tenants share a namespace + "group-n": "__dm__", // the reserved exemption is top-level only + "group-o": "__dm__-2", // the reserved exemption is top-level only } for id, name := range want { if got[id] != name { diff --git a/go/internal/store/migrations/0009_channel_group_names.sql b/go/internal/store/migrations/0009_channel_group_names.sql index 91a180728..5c1cf1149 100644 --- a/go/internal/store/migrations/0009_channel_group_names.sql +++ b/go/internal/store/migrations/0009_channel_group_names.sql @@ -1,10 +1,16 @@ -- Agent tools read '/' as a path separator and address a group by sibling name, --- so a name holds no '/' and is unique among its siblings. Owner ids are global, --- so the key is owner/parent/name; tenant_id is not part of it. +-- so a name holds no '/' and is unique among one account's siblings. Owner ids +-- are global, so the key is owner/parent/name; tenant_id is not part of it. + +-- Fail fast rather than queue every channel read behind a long transaction. +SET LOCAL lock_timeout = '5s'; -- Under FORCE RLS a non-superuser owner sees no rows, so the repair runs as -- compass_system. SET LOCAL ROLE compass_system; +-- Freeze writers from the repair through the index build, or a concurrent +-- create could commit a duplicate the repair never saw. +LOCK TABLE channel_groups IN SHARE ROW EXCLUSIVE MODE; -- Rewrite separators first, then suffix duplicates. A valid original keeps its -- name; each free suffix is found first so no existing name is overwritten. @@ -65,7 +71,7 @@ CREATE UNIQUE INDEX channel_groups_owner_parent_name_key ON channel_groups (owner_user_id, COALESCE(parent_group_id, ''), name) WHERE NOT (parent_group_id IS NULL AND name IN ('__dm__', '__linear__', '__coordination__')); --- The repair above leaves no '/' behind. The validating scan holds the write lock --- only over this small table, inside the runner's single transaction. +-- The repair above leaves no '/' behind. This takes ACCESS EXCLUSIVE until the +-- migration commits; brief on a small table. -- squawk-ignore constraint-missing-not-valid ALTER TABLE channel_groups ADD CONSTRAINT channel_groups_name_no_slash CHECK (POSITION('/' IN name) = 0); From c95861e9d7d8cf55074a0a795fcb436acad9cd67 Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 5 Oct 2026 22:51:11 -0400 Subject: [PATCH 3/9] fix(store): take the group-name lock inside the repair block (RIG-3030) sqlc-vet replays migrations through autocommit psql, where a bare LOCK TABLE errors. Co-authored-by: Matt Wilkinson --- go/internal/store/migrations/0009_channel_group_names.sql | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/go/internal/store/migrations/0009_channel_group_names.sql b/go/internal/store/migrations/0009_channel_group_names.sql index 5c1cf1149..23f7e87c7 100644 --- a/go/internal/store/migrations/0009_channel_group_names.sql +++ b/go/internal/store/migrations/0009_channel_group_names.sql @@ -8,10 +8,6 @@ SET LOCAL lock_timeout = '5s'; -- Under FORCE RLS a non-superuser owner sees no rows, so the repair runs as -- compass_system. SET LOCAL ROLE compass_system; --- Freeze writers from the repair through the index build, or a concurrent --- create could commit a duplicate the repair never saw. -LOCK TABLE channel_groups IN SHARE ROW EXCLUSIVE MODE; - -- Rewrite separators first, then suffix duplicates. A valid original keeps its -- name; each free suffix is found first so no existing name is overwritten. DO $$ @@ -21,6 +17,10 @@ DECLARE candidate TEXT; suffix INTEGER; BEGIN + -- Freeze writers through the index build so no concurrent create commits a + -- duplicate the repair never saw. Inside DO so autocommit replays accept it. + LOCK TABLE channel_groups IN SHARE ROW EXCLUSIVE MODE; + WITH rewritten AS ( UPDATE channel_groups SET name = REPLACE(name, '/', '-') WHERE name LIKE '%/%' From 3f572c2c62df11516355411e48791d6ef4da75a8 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 6 Oct 2026 00:41:25 -0400 Subject: [PATCH 4/9] feat(store): key group names by namespace owner; anchor and owner-qualify group refs (RIG-4458) A leading / anchors a ref at top level and /~handle/ picks the owner, so every visible group stays addressable while ambiguity still errors (DL-291). Co-authored-by: Matt Wilkinson --- docs/specs/product/compass.md | 30 +++- go/gen/compass/v1/comms.pb.go | 25 ++-- .../gen/compass/v1/agent_gateway.pb.go | 5 +- ...annel_group_names_migration_pgtest_test.go | 44 +++++- go/internal/store/channels.go | 131 ++++++++++++++---- go/internal/store/channels_test.go | 65 +++++++++ go/internal/store/coordination_pgtest_test.go | 2 +- go/internal/store/db/channels.sql.go | 61 ++++++-- go/internal/store/db/coordination.sql.go | 5 +- go/internal/store/db/dm.sql.go | 5 +- go/internal/store/db/linear_routing.sql.go | 5 +- go/internal/store/db/models.go | 13 +- go/internal/store/db/querier.go | 4 +- go/internal/store/group_ref_test.go | 60 +++++++- .../store/linear_routing_pgtest_test.go | 2 +- .../migrations/0009_channel_group_names.sql | 33 +++-- go/internal/store/queries/channels.sql | 15 +- go/internal/store/queries/coordination.sql | 5 +- go/internal/store/queries/dm.sql | 5 +- go/internal/store/queries/linear_routing.sql | 5 +- go/internal/store/types.go | 5 +- go/server/linear_responder_pgtest_test.go | 4 +- .../src/gen/compass/v1/agent_gateway_pb.ts | 5 +- .../src/gen/compass/v1/comms_pb.ts | 25 ++-- .../src/gen/compass/v1/comms_pb.ts | 25 ++-- proto/compass/v1/agent_gateway.proto | 5 +- proto/compass/v1/comms.proto | 25 ++-- 27 files changed, 481 insertions(+), 133 deletions(-) diff --git a/docs/specs/product/compass.md b/docs/specs/product/compass.md index 092b33897..dcec1909e 100644 --- a/docs/specs/product/compass.md +++ b/docs/specs/product/compass.md @@ -591,17 +591,23 @@ caller is a founding member by construction. Agent tools SHALL never pass group ids. `create_channel` and `create_channel_group` SHALL name the parent group as a leaf name, or as a slash path from the root when the name contains `/`; a group name SHALL NOT contain -`/`. Resolution SHALL use the caller's visible groups; unknown or invisible -groups SHALL return not-found. A leaf that -names more than one visible group, or a path whose last step matches more than -one visible group, SHALL return invalid-argument. The human RPC SHALL take ids +`/`. A leading `/` SHALL anchor the path at the top level (`/infra`), and a +leading `/~/` SHALL keep only top-level groups in that user's namespace +(`/~matt/eng`). Resolution SHALL use the caller's visible groups; unknown or +invisible groups SHALL return not-found. A ref that names more than one visible +group SHALL return invalid-argument, never pick one, and the error SHALL name +the anchored or owner-qualified ref for each match. The human RPC SHALL take ids and reject the name fields. +A group name SHALL be unique among its siblings within one user's namespace. An +agent's groups belong to its owning user's namespace, so a user and that user's +agents cannot create same-named siblings. + #### Scenario: An ambiguous leaf name is rejected - **Given** two visible groups with the same leaf name - **When** an agent tool names that group by its leaf name -- **Then** the call returns invalid-argument +- **Then** the call returns invalid-argument naming a ref for each group #### Scenario: A slash path selects one of two same-named groups @@ -609,6 +615,20 @@ and reject the name fields. - **When** an agent tool names one with its root slash path - **Then** the call resolves to the group at that path +#### Scenario: An owner qualifier selects between two users' top-level groups + +- **Given** the caller's own top-level group `eng` and another user's shared + top-level group `eng` +- **When** an agent tool names `/~/eng` +- **Then** the call resolves to the other user's group + +#### Scenario: An anchor selects the top-level group over a nested one + +- **Given** a top-level group `infra` and a group `infra` nested under `eng` +- **When** an agent tool names `/infra` +- **Then** the call resolves to the top-level group, and `eng/infra` resolves to + the nested one + ### Requirement: The `SubscribeComms` fan-out is visibility-scoped The comms bus fans every event to every subscriber, so the server SHALL filter diff --git a/go/gen/compass/v1/comms.pb.go b/go/gen/compass/v1/comms.pb.go index 05bc69a64..45fbd9e84 100644 --- a/go/gen/compass/v1/comms.pb.go +++ b/go/gen/compass/v1/comms.pb.go @@ -2732,16 +2732,19 @@ func (x *ListAccountsResponse) GetAccounts() []*Account { type CreateChannelGroupRequest struct { state protoimpl.MessageState `protogen:"open.v1"` - // Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name an - // owner already uses under the same parent is ALREADY_EXISTS. + // Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name already + // used under the same parent in the same user's namespace (a user and that + // user's agents share one) is ALREADY_EXISTS. Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` // Parent group; empty for a top-level group. ParentGroupId string `protobuf:"bytes,2,opt,name=parent_group_id,json=parentGroupId,proto3" json:"parent_group_id,omitempty"` Visibility ChannelGroupVisibility `protobuf:"varint,3,opt,name=visibility,proto3,enum=compass.v1.ChannelGroupVisibility" json:"visibility,omitempty"` - // Agent tool path only. Names the parent as a leaf or root slash path (`a/b/c`) - // when it contains `/`. Unknown or invisible is NOT_FOUND; an ambiguous leaf - // or path is INVALID_ARGUMENT. CommsService rejects this field; human callers - // use parent_group_id. + // Agent tool path only. Names the parent as a leaf (`eng`), a slash path from + // the root (`eng/infra`), a top-level-anchored path (`/infra`), or a path + // qualified by the top-level group's owner handle (`/~matt/eng`). Unknown or + // invisible is NOT_FOUND; an ambiguous ref is INVALID_ARGUMENT and names the + // anchored or qualified ref to use. CommsService rejects this field; human + // callers use parent_group_id. ParentGroupName string `protobuf:"bytes,4,opt,name=parent_group_name,json=parentGroupName,proto3" json:"parent_group_name,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache @@ -3046,10 +3049,12 @@ type CreateChannelRequest struct { ParentAgentHandle string `protobuf:"bytes,5,opt,name=parent_agent_handle,json=parentAgentHandle,proto3" json:"parent_agent_handle,omitempty"` // TREE requires parent_agent_handle. MembershipMode ChannelMembershipMode `protobuf:"varint,6,opt,name=membership_mode,json=membershipMode,proto3,enum=compass.v1.ChannelMembershipMode" json:"membership_mode,omitempty"` - // Agent tool path only. Names the parent as a leaf or root slash path (`a/b/c`) - // when it contains `/`. Unknown or invisible is NOT_FOUND; an ambiguous leaf - // or path is INVALID_ARGUMENT. CommsService rejects this field; human callers - // use group_id. + // Agent tool path only. Names the parent as a leaf (`eng`), a slash path from + // the root (`eng/infra`), a top-level-anchored path (`/infra`), or a path + // qualified by the top-level group's owner handle (`/~matt/eng`). Unknown or + // invisible is NOT_FOUND; an ambiguous ref is INVALID_ARGUMENT and names the + // anchored or qualified ref to use. CommsService rejects this field; human + // callers use group_id. GroupName string `protobuf:"bytes,7,opt,name=group_name,json=groupName,proto3" json:"group_name,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache diff --git a/go/internal/gen/compass/v1/agent_gateway.pb.go b/go/internal/gen/compass/v1/agent_gateway.pb.go index 569ad7912..a684b1eb2 100644 --- a/go/internal/gen/compass/v1/agent_gateway.pb.go +++ b/go/internal/gen/compass/v1/agent_gateway.pb.go @@ -115,8 +115,9 @@ type CommsCallRequest struct { state protoimpl.MessageState `protogen:"open.v1"` CallId string `protobuf:"bytes,1,opt,name=call_id,json=callId,proto3" json:"call_id,omitempty"` // create_channel and create_channel_group use group_name / parent_group_name - // as a leaf or root slash path, resolved within visible groups. Unknown or - // invisible is NOT_FOUND; ambiguous leaf or path is INVALID_ARGUMENT. Setting + // as a leaf, a root slash path, a top-level-anchored `/path`, or an + // owner-qualified `/~handle/path`, resolved within visible groups. Unknown or + // invisible is NOT_FOUND; an ambiguous ref is INVALID_ARGUMENT. Setting // group_id / parent_group_id is INVALID_ARGUMENT. // // Types that are valid to be assigned to Call: diff --git a/go/internal/store/channel_group_names_migration_pgtest_test.go b/go/internal/store/channel_group_names_migration_pgtest_test.go index 360cfedff..e1364b5fd 100644 --- a/go/internal/store/channel_group_names_migration_pgtest_test.go +++ b/go/internal/store/channel_group_names_migration_pgtest_test.go @@ -59,6 +59,13 @@ func TestChannelGroupNamesMigrationRepairsExistingRows(t *testing.T) { if _, err := tx.Exec(ctx, ` INSERT INTO tenants (id, slug, display_name, created_at_unix_ms) VALUES ('boot', 'default', 'Default', 1), ('boot-2', 'second', 'Second', 2); + INSERT INTO accounts (id, handle, display_name, tenant_id) VALUES + ('owner-k', 'owner-k', 'Owner K', 'boot'), + ('agent-k1', 'agent-k1', 'Agent K1', 'boot'), + ('agent-k2', 'agent-k2', 'Agent K2', 'boot'); + INSERT INTO user_accounts (account_id, tenant_id) VALUES ('owner-k', 'boot'); + INSERT INTO agent_accounts (account_id, owner_user_id, tenant_id) VALUES + ('agent-k1', 'owner-k', 'boot'), ('agent-k2', 'owner-k', 'boot'); INSERT INTO channel_groups (id, name, owner_user_id, tenant_id, parent_group_id) VALUES ('group-a', 'duplicate', 'owner-a', 'boot', NULL), ('group-b', 'duplicate', 'owner-a', 'boot', NULL), @@ -74,7 +81,11 @@ func TestChannelGroupNamesMigrationRepairsExistingRows(t *testing.T) { ('group-l', 'tenant-name', 'owner-f', 'boot', NULL), ('group-m', 'tenant-name', 'owner-f', 'boot-2', NULL), ('group-n', '__dm__', 'owner-e', 'boot', 'group-i'), - ('group-o', '__dm__', 'owner-e', 'boot', 'group-i')`); err != nil { + ('group-o', '__dm__', 'owner-e', 'boot', 'group-i'), + ('group-p', 'team', 'owner-k', 'boot', NULL), + ('group-q', 'team', 'agent-k1', 'boot', NULL), + ('group-r', 'svc', 'agent-k1', 'boot', 'group-p'), + ('group-s', 'svc', 'agent-k2', 'boot', 'group-p')`); err != nil { t.Fatalf("seed pre-migration groups: %v", err) } if err := tx.Commit(ctx); err != nil { @@ -102,6 +113,10 @@ func TestChannelGroupNamesMigrationRepairsExistingRows(t *testing.T) { "group-m": "tenant-name-2", // owner ids are global, so tenants share a namespace "group-n": "__dm__", // the reserved exemption is top-level only "group-o": "__dm__-2", // the reserved exemption is top-level only + "group-p": "team", // an agent's group shares its owner's namespace + "group-q": "team-2", // an agent's group shares its owner's namespace + "group-r": "svc", // two agents of one owner share a namespace + "group-s": "svc-2", // two agents of one owner share a namespace } for id, name := range want { if got[id] != name { @@ -109,6 +124,13 @@ func TestChannelGroupNamesMigrationRepairsExistingRows(t *testing.T) { } } + namespaces := readGroupNamespacesAsSystem(t, pool) + for id, owner := range map[string]string{"group-a": "owner-a", "group-p": "owner-k", "group-q": "owner-k", "group-s": "owner-k"} { + if namespaces[id] != owner { + t.Errorf("group %s namespace = %q, want %q", id, namespaces[id], owner) + } + } + var indexExists, checkExists bool if err := pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE schemaname = current_schema() @@ -126,6 +148,18 @@ func TestChannelGroupNamesMigrationRepairsExistingRows(t *testing.T) { } func readGroupNamesAsSystem(t *testing.T, pool *pgxpool.Pool) map[string]string { + t.Helper() + return readGroupColumnAsSystem(t, pool, "name") +} + +func readGroupNamespacesAsSystem(t *testing.T, pool *pgxpool.Pool) map[string]string { + t.Helper() + return readGroupColumnAsSystem(t, pool, "namespace_owner_id") +} + +// readGroupColumnAsSystem maps each group id to one text column; column is a +// test constant, never input. +func readGroupColumnAsSystem(t *testing.T, pool *pgxpool.Pool, column string) map[string]string { t.Helper() ctx := t.Context() tx, err := pool.Begin(ctx) @@ -136,18 +170,18 @@ func readGroupNamesAsSystem(t *testing.T, pool *pgxpool.Pool) map[string]string if _, err := tx.Exec(ctx, "SET LOCAL ROLE "+systemRole); err != nil { t.Fatalf("set read role: %v", err) } - rows, err := tx.Query(ctx, `SELECT id, name FROM channel_groups`) + rows, err := tx.Query(ctx, "SELECT id, "+column+" FROM channel_groups") if err != nil { t.Fatalf("read repaired groups: %v", err) } defer rows.Close() got := make(map[string]string) for rows.Next() { - var id, name string - if err := rows.Scan(&id, &name); err != nil { + var id, value string + if err := rows.Scan(&id, &value); err != nil { t.Fatalf("scan repaired group: %v", err) } - got[id] = name + got[id] = value } if err := rows.Err(); err != nil { t.Fatalf("iterate repaired groups: %v", err) diff --git a/go/internal/store/channels.go b/go/internal/store/channels.go index 37711413f..f6a83c1b8 100644 --- a/go/internal/store/channels.go +++ b/go/internal/store/channels.go @@ -61,13 +61,14 @@ func (s *Store) CreateChannelGroup(ctx context.Context, ownerUserID AccountID, g } id := newID() - if err := s.q.WithTx(tx).InsertChannelGroup(ctx, db.InsertChannelGroupParams{ + namespaceOwner, err := s.q.WithTx(tx).InsertChannelGroup(ctx, db.InsertChannelGroupParams{ ID: id, Name: g.Name, Column3: string(g.ParentGroupID), OwnerUserID: string(ownerUserID), Visibility: int16(g.Visibility), //nolint:gosec // G115: ChannelGroupVisibility is a CHECK-constrained 0/1 enum (channel_groups.visibility), always within int16 - }); err != nil { + }) + if err != nil { if pgErrIs(err, pgUniqueViolation) && pgConstraintName(err) == "channel_groups_owner_parent_name_key" { return ChannelGroup{}, fmt.Errorf("%w: sibling group %q already exists", ErrConflict, g.Name) } @@ -77,11 +78,12 @@ func (s *Store) CreateChannelGroup(ctx context.Context, ownerUserID AccountID, g return ChannelGroup{}, fmt.Errorf("store: commit create group: %w", err) } return ChannelGroup{ - ID: ChannelGroupID(id), - Name: g.Name, - ParentGroupID: g.ParentGroupID, - OwnerUserID: ownerUserID, - Visibility: g.Visibility, + ID: ChannelGroupID(id), + Name: g.Name, + ParentGroupID: g.ParentGroupID, + OwnerUserID: ownerUserID, + NamespaceOwnerID: AccountID(namespaceOwner), + Visibility: g.Visibility, }, nil } @@ -251,11 +253,12 @@ func (s *Store) ListChannelGroups(ctx context.Context, visibleTo AccountID) ([]C var groups []ChannelGroup for _, row := range rows { groups = append(groups, ChannelGroup{ - ID: ChannelGroupID(row.ID), - Name: row.Name, - ParentGroupID: ChannelGroupID(row.ParentGroupID), - OwnerUserID: AccountID(row.OwnerUserID), - Visibility: ChannelGroupVisibility(row.Visibility), + ID: ChannelGroupID(row.ID), + Name: row.Name, + ParentGroupID: ChannelGroupID(row.ParentGroupID), + OwnerUserID: AccountID(row.OwnerUserID), + NamespaceOwnerID: AccountID(row.NamespaceOwnerID), + Visibility: ChannelGroupVisibility(row.Visibility), }) } return groups, nil @@ -309,25 +312,41 @@ func (s *Store) ChannelVisibleTo(ctx context.Context, actor AccountID, channelID return visible, nil } -// ChannelGroupByRefForViewer resolves an agent's group reference (a leaf name, or -// a slash path from the root) within the groups visible to viewer. +// ChannelGroupByRefForViewer resolves an agent's group reference (a leaf name, a +// slash path from the root, or an anchored `/path` or `/~owner/path`) within the +// groups visible to viewer. func (s *Store) ChannelGroupByRefForViewer(ctx context.Context, viewer AccountID, ref string) (ChannelGroup, error) { groups, err := s.ListChannelGroups(ctx, viewer) if err != nil { return ChannelGroup{}, err } - return resolveGroupRef(groups, ref) + owners := make([]string, 0, len(groups)) + for _, group := range groups { + owners = append(owners, string(group.NamespaceOwnerID)) + } + rows, err := s.q.GlobalHandlesByAccountIDs(ctx, owners) + if err != nil { + return ChannelGroup{}, fmt.Errorf("store: read group owner handles: %w", err) + } + handles := make(map[AccountID]string, len(rows)) + for _, row := range rows { + handles[AccountID(row.AccountID)] = row.Handle + } + return resolveGroupRef(groups, handles, ref) } // resolveGroupRef picks one group from the viewer's visible set. Unknown and -// invisible both give ErrNotFound; a leaf matches at any depth, a path walks from -// the root and may pass through same-named groups if it ends on exactly one. -func resolveGroupRef(groups []ChannelGroup, ref string) (ChannelGroup, error) { +// invisible both give ErrNotFound; a bare leaf matches at any depth, a path walks +// from the root and may pass through same-named groups if it ends on exactly one. +// A leading '/' anchors at the top level, and `/~handle/` keeps only top-level +// groups in that user's namespace. handles maps namespace owner ids to handles. +func resolveGroupRef(groups []ChannelGroup, handles map[AccountID]string, ref string) (ChannelGroup, error) { if ref == "" { return ChannelGroup{}, fmt.Errorf("%w: group name is required", ErrInvalidArgument) } - if !strings.Contains(ref, "/") { + anchored := strings.HasPrefix(ref, "/") + if !anchored && !strings.Contains(ref, "/") { var matches []ChannelGroup for _, group := range groups { if group.Name == ref { @@ -340,23 +359,32 @@ func resolveGroupRef(groups []ChannelGroup, ref string) (ChannelGroup, error) { case 1: return matches[0], nil default: - return ChannelGroup{}, fmt.Errorf("%w: group name %q is ambiguous — it names %d visible groups; use a slash path from the root", ErrInvalidArgument, ref, len(matches)) + return ChannelGroup{}, fmt.Errorf("%w: group name %q is ambiguous — it names %d visible groups; use one of %s", + ErrInvalidArgument, ref, len(matches), groupRefHints(groups, handles, matches)) } } - segments := strings.Split(ref, "/") + segments := strings.Split(strings.TrimPrefix(ref, "/"), "/") if slices.Contains(segments, "") { return ChannelGroup{}, fmt.Errorf("%w: group path %q has an empty segment", ErrInvalidArgument, ref) } + owner := "" + if anchored && strings.HasPrefix(segments[0], "~") { + owner = strings.TrimPrefix(segments[0], "~") + segments = segments[1:] + if owner == "" || len(segments) == 0 { + return ChannelGroup{}, fmt.Errorf("%w: group path %q needs an owner handle and a group, e.g. /~matt/eng", ErrInvalidArgument, ref) + } + } candidates := make(map[ChannelGroupID]ChannelGroup) - for _, segment := range segments { + for i, segment := range segments { next := make(map[ChannelGroupID]ChannelGroup) for _, group := range groups { if group.Name != segment { continue } - if len(candidates) == 0 { - if group.ParentGroupID == "" { + if i == 0 { + if group.ParentGroupID == "" && (owner == "" || handles[group.NamespaceOwnerID] == owner) { next[group.ID] = group } } else if _, ok := candidates[group.ParentGroupID]; ok { @@ -369,7 +397,12 @@ func resolveGroupRef(groups []ChannelGroup, ref string) (ChannelGroup, error) { candidates = next } if len(candidates) > 1 { - return ChannelGroup{}, fmt.Errorf("%w: group path %q is ambiguous", ErrInvalidArgument, ref) + matches := make([]ChannelGroup, 0, len(candidates)) + for _, group := range candidates { + matches = append(matches, group) + } + return ChannelGroup{}, fmt.Errorf("%w: group path %q is ambiguous; use one of %s", + ErrInvalidArgument, ref, groupRefHints(groups, handles, matches)) } for _, group := range candidates { return group, nil @@ -377,6 +410,54 @@ func resolveGroupRef(groups []ChannelGroup, ref string) (ChannelGroup, error) { return ChannelGroup{}, fmt.Errorf("%w: group %q", ErrNotFound, ref) } +// groupRefHints names, for each match, the anchored path when it is unique among +// the visible groups and otherwise the owner-qualified path, so the caller can retry. +func groupRefHints(groups []ChannelGroup, handles map[AccountID]string, matches []ChannelGroup) string { + byID := make(map[ChannelGroupID]ChannelGroup, len(groups)) + for _, group := range groups { + byID[group.ID] = group + } + type refForms struct{ anchored, qualified string } + formsOf := func(group ChannelGroup) refForms { + path := group.Name + // The depth cap stops a malformed parent cycle from looping. + for depth := 0; group.ParentGroupID != "" && depth < len(groups); depth++ { + parent, ok := byID[group.ParentGroupID] + if !ok { + return refForms{} + } + group = parent + path = group.Name + "/" + path + } + forms := refForms{anchored: "/" + path} + if handle, ok := handles[group.NamespaceOwnerID]; ok { + forms.qualified = "/~" + handle + "/" + path + } + return forms + } + anchoredCount := make(map[string]int, len(groups)) + for _, group := range groups { + if forms := formsOf(group); forms.anchored != "" { + anchoredCount[forms.anchored]++ + } + } + hints := make([]string, 0, len(matches)) + for _, match := range matches { + forms := formsOf(match) + switch { + case forms.anchored != "" && anchoredCount[forms.anchored] == 1: + hints = append(hints, forms.anchored) + case forms.qualified != "": + hints = append(hints, forms.qualified) + case forms.anchored != "": + hints = append(hints, forms.anchored) + } + } + slices.Sort(hints) + hints = slices.Compact(hints) + return strings.Join(hints, ", ") + " (a leading / anchors at the top level; /~owner/ picks the top-level group's owner)" +} + // ChannelByNameForViewer resolves a channel NAME to its Channel within the set // visible to viewer — the viewer-scoped name→id resolve the agent tool edge runs // ahead of the id-typed store calls (peer-DM record R1). Channel names are not diff --git a/go/internal/store/channels_test.go b/go/internal/store/channels_test.go index 7337e9f4d..17ad23421 100644 --- a/go/internal/store/channels_test.go +++ b/go/internal/store/channels_test.go @@ -44,6 +44,71 @@ func TestCreateChannelGroupSiblingNamesUnique(t *testing.T) { } } +// TestCreateChannelGroupSiblingNamesUniquePerNamespace: an agent's groups live in +// its owner's namespace, so the owner and its agents share one set of sibling names. +func TestCreateChannelGroupSiblingNamesUniquePerNamespace(t *testing.T) { + ctx := t.Context() + s := newTestStore(t) + owner := mustUser(t, s, "owner") + first := mustAgent(t, s, owner.ID, "first") + second := mustAgent(t, s, owner.ID, "second") + other := mustUser(t, s, "other") + mk := func(by AccountID, name string, parent ChannelGroupID) error { + _, err := s.CreateChannelGroup(ctx, by, NewChannelGroup{Name: name, ParentGroupID: parent, Visibility: VisibilityOwner}) + return err + } + team, err := s.CreateChannelGroup(ctx, owner.ID, NewChannelGroup{Name: "team", Visibility: VisibilityOwner}) + if err != nil { + t.Fatalf("CreateChannelGroup(team): %v", err) + } + if team.NamespaceOwnerID != owner.ID { + t.Fatalf("owner group namespace = %q, want %q", team.NamespaceOwnerID, owner.ID) + } + sentinelIs(t, mk(first.ID, "team", ""), ErrConflict, "agent top-level name its owner already uses") + + agentGroup, err := s.CreateChannelGroup(ctx, first.ID, NewChannelGroup{Name: "svc", ParentGroupID: team.ID, Visibility: VisibilityOwner}) + if err != nil { + t.Fatalf("CreateChannelGroup(agent svc): %v", err) + } + if agentGroup.NamespaceOwnerID != owner.ID { + t.Fatalf("agent group namespace = %q, want owner %q", agentGroup.NamespaceOwnerID, owner.ID) + } + sentinelIs(t, mk(second.ID, "svc", team.ID), ErrConflict, "two agents of one owner under one parent") + if err := mk(other.ID, "team", ""); err != nil { + t.Fatalf("same top-level name in another user's namespace: %v", err) + } +} + +// TestChannelGroupByRefForViewerQualifiesOwner: a viewer's own top-level group +// and a stranger's shared one share a name; only the owner-qualified form picks one. +func TestChannelGroupByRefForViewerQualifiesOwner(t *testing.T) { + ctx := t.Context() + s := newTestStore(t) + viewer := mustUser(t, s, "viewer") + viewerAgent := mustAgent(t, s, viewer.ID, "helper") + stranger := mustUser(t, s, "stranger") + own, err := s.CreateChannelGroup(ctx, viewerAgent.ID, NewChannelGroup{Name: "eng", Visibility: VisibilityOwner}) + if err != nil { + t.Fatalf("CreateChannelGroup(viewer eng): %v", err) + } + shared, err := s.CreateChannelGroup(ctx, stranger.ID, NewChannelGroup{Name: "eng", Visibility: VisibilityShared}) + if err != nil { + t.Fatalf("CreateChannelGroup(stranger eng): %v", err) + } + + _, err = s.ChannelGroupByRefForViewer(ctx, viewerAgent.ID, "eng") + sentinelIs(t, err, ErrInvalidArgument, "bare cross-owner eng") + for ref, want := range map[string]ChannelGroupID{"/~viewer/eng": own.ID, "/~stranger/eng": shared.ID} { + got, err := s.ChannelGroupByRefForViewer(ctx, viewerAgent.ID, ref) + if err != nil { + t.Fatalf("ChannelGroupByRefForViewer(%q): %v", ref, err) + } + if got.ID != want { + t.Fatalf("ChannelGroupByRefForViewer(%q) = %q, want %q", ref, got.ID, want) + } + } +} + func TestCreateChannelGroupCeilingRejectsWiderChild(t *testing.T) { ctx := context.Background() s := newTestStore(t) diff --git a/go/internal/store/coordination_pgtest_test.go b/go/internal/store/coordination_pgtest_test.go index 863203c3d..d70c85a18 100644 --- a/go/internal/store/coordination_pgtest_test.go +++ b/go/internal/store/coordination_pgtest_test.go @@ -365,7 +365,7 @@ func TestReconcileIgnoresMisVisibilityUserGroup(t *testing.T) { // A top-level SHARED __coordination__ can still exist via paths other than // CreateChannelGroup, so plant it with raw SQL. if _, err := s.pool.Exec(ctx, - "INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility) VALUES ($1,$2,NULL,$3,$4)", + "INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, namespace_owner_id, visibility) VALUES ($1,$2,NULL,$3,$3,$4)", newID(), coordinationGroupName, string(owner.ID), int16(VisibilityShared)); err != nil { t.Fatalf("plant shared __coordination__ group: %v", err) } diff --git a/go/internal/store/db/channels.sql.go b/go/internal/store/db/channels.sql.go index e208f0428..25a03325e 100644 --- a/go/internal/store/db/channels.sql.go +++ b/go/internal/store/db/channels.sql.go @@ -334,6 +334,36 @@ func (q *Queries) GetChannelGroupVisibility(ctx context.Context, id string) (int return visibility, err } +const globalHandlesByAccountIDs = `-- name: GlobalHandlesByAccountIDs :many +SELECT account_id, handle FROM account_handles +WHERE owner_user_id IS NULL AND account_id = ANY($1::text[]) +` + +type GlobalHandlesByAccountIDsRow struct { + AccountID string + Handle string +} + +func (q *Queries) GlobalHandlesByAccountIDs(ctx context.Context, dollar_1 []string) ([]GlobalHandlesByAccountIDsRow, error) { + rows, err := q.db.Query(ctx, globalHandlesByAccountIDs, dollar_1) + if err != nil { + return nil, err + } + defer rows.Close() + var items []GlobalHandlesByAccountIDsRow + for rows.Next() { + var i GlobalHandlesByAccountIDsRow + if err := rows.Scan(&i.AccountID, &i.Handle); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const insertAgentWorkspaceIgnore = `-- name: InsertAgentWorkspaceIgnore :exec INSERT INTO agent_workspaces (id, agent_account_id) VALUES ($1, $2) @@ -378,10 +408,12 @@ func (q *Queries) InsertChannel(ctx context.Context, arg InsertChannelParams) er return err } -const insertChannelGroup = `-- name: InsertChannelGroup :exec +const insertChannelGroup = `-- name: InsertChannelGroup :one -INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility) -VALUES ($1, $2, NULLIF($3, ''), $4, $5) +INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility, namespace_owner_id) +VALUES ($1, $2, NULLIF($3, ''), $4, $5, + COALESCE((SELECT owner_user_id FROM agent_accounts WHERE account_id = $4), $4)) +RETURNING namespace_owner_id ` type InsertChannelGroupParams struct { @@ -404,15 +436,18 @@ type InsertChannelGroupParams struct { // groupVisiblePredicate). The copies MUST stay textually identical so the stream // edge's single-id visibility check cannot drift from the list read (the // anti-drift guarantee the design record requires). -func (q *Queries) InsertChannelGroup(ctx context.Context, arg InsertChannelGroupParams) error { - _, err := q.db.Exec(ctx, insertChannelGroup, +// An agent's group lives in its owner's namespace, so sibling names are unique per user. +func (q *Queries) InsertChannelGroup(ctx context.Context, arg InsertChannelGroupParams) (string, error) { + row := q.db.QueryRow(ctx, insertChannelGroup, arg.ID, arg.Name, arg.Column3, arg.OwnerUserID, arg.Visibility, ) - return err + var namespace_owner_id string + err := row.Scan(&namespace_owner_id) + return namespace_owner_id, err } const listChannelGroups = `-- name: ListChannelGroups :many @@ -434,7 +469,7 @@ viewer AS ( UNION ALL SELECT $1 AS uid ) -SELECT g.id, g.name, COALESCE(g.parent_group_id, '') AS parent_group_id, g.owner_user_id, g.visibility +SELECT g.id, g.name, COALESCE(g.parent_group_id, '') AS parent_group_id, g.owner_user_id, g.visibility, g.namespace_owner_id FROM channel_groups g JOIN effective e ON e.id = g.id WHERE (e.eff_vis = 1 OR g.owner_user_id IN (SELECT uid FROM viewer)) @@ -442,11 +477,12 @@ ORDER BY g.name ` type ListChannelGroupsRow struct { - ID string - Name string - ParentGroupID string - OwnerUserID string - Visibility int16 + ID string + Name string + ParentGroupID string + OwnerUserID string + Visibility int16 + NamespaceOwnerID string } func (q *Queries) ListChannelGroups(ctx context.Context, accountID string) ([]ListChannelGroupsRow, error) { @@ -464,6 +500,7 @@ func (q *Queries) ListChannelGroups(ctx context.Context, accountID string) ([]Li &i.ParentGroupID, &i.OwnerUserID, &i.Visibility, + &i.NamespaceOwnerID, ); err != nil { return nil, err } diff --git a/go/internal/store/db/coordination.sql.go b/go/internal/store/db/coordination.sql.go index 35f4d1a99..2e3508221 100644 --- a/go/internal/store/db/coordination.sql.go +++ b/go/internal/store/db/coordination.sql.go @@ -139,8 +139,9 @@ func (q *Queries) InsertCoordinationChannel(ctx context.Context, arg InsertCoord } const insertCoordinationGroup = `-- name: InsertCoordinationGroup :exec -INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility) -VALUES ($1, $2, NULL, $3, $4) +INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility, namespace_owner_id) +VALUES ($1, $2, NULL, $3, $4, + COALESCE((SELECT owner_user_id FROM agent_accounts WHERE account_id = $3), $3)) ` type InsertCoordinationGroupParams struct { diff --git a/go/internal/store/db/dm.sql.go b/go/internal/store/db/dm.sql.go index e239e3af7..b2f7f99d7 100644 --- a/go/internal/store/db/dm.sql.go +++ b/go/internal/store/db/dm.sql.go @@ -114,8 +114,9 @@ func (q *Queries) InsertDMChannel(ctx context.Context, arg InsertDMChannelParams } const insertOwnerDMGroup = `-- name: InsertOwnerDMGroup :exec -INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility) -VALUES ($1, $2, NULL, $3, $4) +INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility, namespace_owner_id) +VALUES ($1, $2, NULL, $3, $4, + COALESCE((SELECT owner_user_id FROM agent_accounts WHERE account_id = $3), $3)) ` type InsertOwnerDMGroupParams struct { diff --git a/go/internal/store/db/linear_routing.sql.go b/go/internal/store/db/linear_routing.sql.go index 6d9bc1504..519c6e13a 100644 --- a/go/internal/store/db/linear_routing.sql.go +++ b/go/internal/store/db/linear_routing.sql.go @@ -88,8 +88,9 @@ func (q *Queries) InsertLinearRoutingChannel(ctx context.Context, arg InsertLine } const insertLinearRoutingGroup = `-- name: InsertLinearRoutingGroup :exec -INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility) -VALUES ($1, $2, NULL, $3, $4) +INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility, namespace_owner_id) +VALUES ($1, $2, NULL, $3, $4, + COALESCE((SELECT owner_user_id FROM agent_accounts WHERE account_id = $3), $3)) ` type InsertLinearRoutingGroupParams struct { diff --git a/go/internal/store/db/models.go b/go/internal/store/db/models.go index 45cf884d5..67a19f5cd 100644 --- a/go/internal/store/db/models.go +++ b/go/internal/store/db/models.go @@ -145,12 +145,13 @@ type Channel struct { } type ChannelGroup struct { - ID string - Name string - ParentGroupID pgtype.Text - OwnerUserID string - Visibility int16 - TenantID string + ID string + Name string + ParentGroupID pgtype.Text + OwnerUserID string + Visibility int16 + TenantID string + NamespaceOwnerID string } type ChannelMember struct { diff --git a/go/internal/store/db/querier.go b/go/internal/store/db/querier.go index 2c921b45f..c44b3e328 100644 --- a/go/internal/store/db/querier.go +++ b/go/internal/store/db/querier.go @@ -229,6 +229,7 @@ type Querier interface { GetTopicChannel(ctx context.Context, id string) (string, error) // The caller is bound to one tenant by the store's scoped query path. GetTourState(ctx context.Context, accountID string) (GetTourStateRow, error) + GlobalHandlesByAccountIDs(ctx context.Context, dollar_1 []string) ([]GlobalHandlesByAccountIDsRow, error) // Scope grants are managed for user accounts; agents inherit their owner's rows. // The SELECT runs under RLS, so a user from another tenant inserts nothing. GrantForgeScope(ctx context.Context, arg GrantForgeScopeParams) (int64, error) @@ -277,7 +278,8 @@ type Querier interface { // groupVisiblePredicate). The copies MUST stay textually identical so the stream // edge's single-id visibility check cannot drift from the list read (the // anti-drift guarantee the design record requires). - InsertChannelGroup(ctx context.Context, arg InsertChannelGroupParams) error + // An agent's group lives in its owner's namespace, so sibling names are unique per user. + InsertChannelGroup(ctx context.Context, arg InsertChannelGroupParams) (string, error) InsertChannelPin(ctx context.Context, arg InsertChannelPinParams) error InsertCoordinationChannel(ctx context.Context, arg InsertCoordinationChannelParams) (string, error) InsertCoordinationGroup(ctx context.Context, arg InsertCoordinationGroupParams) error diff --git a/go/internal/store/group_ref_test.go b/go/internal/store/group_ref_test.go index a155831ed..cfb25800a 100644 --- a/go/internal/store/group_ref_test.go +++ b/go/internal/store/group_ref_test.go @@ -2,6 +2,7 @@ package store import ( "errors" + "strings" "testing" ) @@ -17,7 +18,14 @@ func TestResolveGroupRef(t *testing.T) { {ID: "root-svc-2", Name: "svc"}, {ID: "root-svc-child", Name: "nested", ParentGroupID: "root-svc-1"}, {ID: "beta-svc-leaf", Name: "leaf", ParentGroupID: "beta-svc"}, + // The viewer's own top-level eng beside a stranger's shared one. + {ID: "viewer-eng", Name: "eng", NamespaceOwnerID: "viewer-id"}, + {ID: "stranger-eng", Name: "eng", NamespaceOwnerID: "stranger-id"}, + // A top-level infra beside an infra nested under the viewer's eng. + {ID: "top-infra", Name: "infra", NamespaceOwnerID: "viewer-id"}, + {ID: "eng-infra", Name: "infra", ParentGroupID: "viewer-eng", NamespaceOwnerID: "viewer-id"}, } + handles := map[AccountID]string{"viewer-id": "viewer", "stranger-id": "stranger"} tests := []struct { name string @@ -35,14 +43,28 @@ func TestResolveGroupRef(t *testing.T) { {name: "missing intermediate path segment", ref: "alpha/missing/svc", wantErr: ErrNotFound}, {name: "path ends in same-named siblings", ref: "alpha/dup", wantErr: ErrInvalidArgument}, {name: "empty ref", ref: "", wantErr: ErrInvalidArgument}, - {name: "leading slash", ref: "/a", wantErr: ErrInvalidArgument}, {name: "trailing slash", ref: "a/", wantErr: ErrInvalidArgument}, {name: "doubled slash", ref: "a//b", wantErr: ErrInvalidArgument}, + {name: "bare slash", ref: "/", wantErr: ErrInvalidArgument}, + {name: "cross-owner top-level leaf is ambiguous", ref: "eng", wantErr: ErrInvalidArgument}, + {name: "anchored cross-owner top level stays ambiguous", ref: "/eng", wantErr: ErrInvalidArgument}, + {name: "owner qualifier picks the viewer's group", ref: "/~viewer/eng", wantID: "viewer-eng"}, + {name: "owner qualifier picks the stranger's group", ref: "/~stranger/eng", wantID: "stranger-eng"}, + {name: "owner qualifier with a path", ref: "/~viewer/eng/infra", wantID: "eng-infra"}, + {name: "unknown owner qualifier", ref: "/~nobody/eng", wantErr: ErrNotFound}, + {name: "owner qualifier needs a group", ref: "/~viewer", wantErr: ErrInvalidArgument}, + {name: "empty owner qualifier", ref: "/~/eng", wantErr: ErrInvalidArgument}, + {name: "tilde is only an owner qualifier when anchored", ref: "~viewer/eng", wantErr: ErrNotFound}, + {name: "top-level vs nested leaf is ambiguous", ref: "infra", wantErr: ErrInvalidArgument}, + {name: "anchor picks the top-level group", ref: "/infra", wantID: "top-infra"}, + {name: "unanchored path picks the nested group", ref: "eng/infra", wantID: "eng-infra"}, + {name: "anchored path through an ambiguous top level", ref: "/eng/infra", wantID: "eng-infra"}, + {name: "anchor never matches a nested group", ref: "/leaf", wantErr: ErrNotFound}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := resolveGroupRef(groups, tt.ref) + got, err := resolveGroupRef(groups, handles, tt.ref) if tt.wantErr != nil { if !errors.Is(err, tt.wantErr) { t.Fatalf("resolveGroupRef(%q) error = %v, want %v", tt.ref, err, tt.wantErr) @@ -58,3 +80,37 @@ func TestResolveGroupRef(t *testing.T) { }) } } + +// TestResolveGroupRefAmbiguityNamesTheFix checks that an ambiguous ref names a +// ref that resolves each match, so a caller can retry without guessing. +func TestResolveGroupRefAmbiguityNamesTheFix(t *testing.T) { + groups := []ChannelGroup{ + {ID: "viewer-eng", Name: "eng", NamespaceOwnerID: "viewer-id"}, + {ID: "stranger-eng", Name: "eng", NamespaceOwnerID: "stranger-id"}, + {ID: "top-infra", Name: "infra", NamespaceOwnerID: "viewer-id"}, + {ID: "eng-infra", Name: "infra", ParentGroupID: "viewer-eng", NamespaceOwnerID: "viewer-id"}, + } + handles := map[AccountID]string{"viewer-id": "viewer", "stranger-id": "stranger"} + + for _, tt := range []struct { + ref string + hints map[string]ChannelGroupID + }{ + {ref: "eng", hints: map[string]ChannelGroupID{"/~viewer/eng": "viewer-eng", "/~stranger/eng": "stranger-eng"}}, + {ref: "infra", hints: map[string]ChannelGroupID{"/infra": "top-infra", "/eng/infra": "eng-infra"}}, + } { + _, err := resolveGroupRef(groups, handles, tt.ref) + if !errors.Is(err, ErrInvalidArgument) { + t.Fatalf("resolveGroupRef(%q) error = %v, want invalid argument", tt.ref, err) + } + for hint, wantID := range tt.hints { + if !strings.Contains(err.Error(), hint) { + t.Errorf("resolveGroupRef(%q) error %q does not name %q", tt.ref, err, hint) + } + got, err := resolveGroupRef(groups, handles, hint) + if err != nil || got.ID != wantID { + t.Errorf("hint %q resolves to %q, %v; want %q", hint, got.ID, err, wantID) + } + } + } +} diff --git a/go/internal/store/linear_routing_pgtest_test.go b/go/internal/store/linear_routing_pgtest_test.go index 4717e952b..55e238d61 100644 --- a/go/internal/store/linear_routing_pgtest_test.go +++ b/go/internal/store/linear_routing_pgtest_test.go @@ -45,7 +45,7 @@ func TestEnsureLinearRoutingChannelSkipsSharedGroup(t *testing.T) { // look-alike group and its routing channel with raw SQL. sharedID, plantedID := newID(), newID() if _, err := s.pool.Exec(t.Context(), - "INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility, tenant_id) VALUES ($1,$2,NULL,$3,$4,$5)", + "INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, namespace_owner_id, visibility, tenant_id) VALUES ($1,$2,NULL,$3,$3,$4,$5)", sharedID, linearRoutingGroupName, string(admin.ID), int16(VisibilityShared), string(s.resolveTenant(t.Context()))); err != nil { t.Fatalf("plant shared __linear__ group: %v", err) } diff --git a/go/internal/store/migrations/0009_channel_group_names.sql b/go/internal/store/migrations/0009_channel_group_names.sql index 23f7e87c7..64844d617 100644 --- a/go/internal/store/migrations/0009_channel_group_names.sql +++ b/go/internal/store/migrations/0009_channel_group_names.sql @@ -1,15 +1,21 @@ -- Agent tools read '/' as a path separator and address a group by sibling name, --- so a name holds no '/' and is unique among one account's siblings. Owner ids --- are global, so the key is owner/parent/name; tenant_id is not part of it. +-- so a name holds no '/' and is unique among one namespace's siblings. An +-- agent's groups live in its owner's namespace. Owner ids are global, so the key +-- is namespace/parent/name; tenant_id is not part of it. -- Fail fast rather than queue every channel read behind a long transaction. SET LOCAL lock_timeout = '5s'; +-- The default exists only for the backfill; it is dropped below so every insert +-- must name the namespace. +ALTER TABLE channel_groups ADD COLUMN namespace_owner_id TEXT NOT NULL DEFAULT ''; + -- Under FORCE RLS a non-superuser owner sees no rows, so the repair runs as -- compass_system. SET LOCAL ROLE compass_system; --- Rewrite separators first, then suffix duplicates. A valid original keeps its --- name; each free suffix is found first so no existing name is overwritten. +-- Backfill namespaces, rewrite separators, then suffix duplicates. A valid +-- original keeps its name; each free suffix is found first so no existing name +-- is overwritten. DO $$ DECLARE rewritten_ids TEXT[]; @@ -21,6 +27,11 @@ BEGIN -- duplicate the repair never saw. Inside DO so autocommit replays accept it. LOCK TABLE channel_groups IN SHARE ROW EXCLUSIVE MODE; + UPDATE channel_groups AS g + SET namespace_owner_id = COALESCE( + (SELECT a.owner_user_id FROM agent_accounts AS a WHERE a.account_id = g.owner_user_id), + g.owner_user_id); + WITH rewritten AS ( UPDATE channel_groups SET name = REPLACE(name, '/', '-') WHERE name LIKE '%/%' @@ -29,17 +40,17 @@ BEGIN SELECT COALESCE(array_agg(id), '{}') INTO rewritten_ids FROM rewritten; FOR duplicate IN - SELECT id, owner_user_id, parent_group_id, name, duplicate_number + SELECT id, namespace_owner_id, parent_group_id, name, duplicate_number FROM ( - SELECT id, owner_user_id, parent_group_id, name, + SELECT id, namespace_owner_id, parent_group_id, name, row_number() OVER ( - PARTITION BY owner_user_id, COALESCE(parent_group_id, ''), name + PARTITION BY namespace_owner_id, COALESCE(parent_group_id, ''), name ORDER BY id = ANY (rewritten_ids), id ) AS duplicate_number FROM channel_groups ) AS ranked WHERE duplicate_number > 1 - ORDER BY owner_user_id, COALESCE(parent_group_id, ''), name, duplicate_number + ORDER BY namespace_owner_id, COALESCE(parent_group_id, ''), name, duplicate_number LOOP IF duplicate.parent_group_id IS NULL AND duplicate.name IN ('__dm__', '__linear__', '__coordination__') THEN @@ -51,7 +62,7 @@ BEGIN EXIT WHEN NOT EXISTS ( SELECT 1 FROM channel_groups AS sibling - WHERE sibling.owner_user_id = duplicate.owner_user_id + WHERE sibling.namespace_owner_id = duplicate.namespace_owner_id AND COALESCE(sibling.parent_group_id, '') = COALESCE(duplicate.parent_group_id, '') AND sibling.name = candidate AND sibling.id <> duplicate.id @@ -65,10 +76,12 @@ END $$; -- DDL runs as the table owner, not the system role. RESET ROLE; +ALTER TABLE channel_groups ALTER COLUMN namespace_owner_id DROP DEFAULT; + -- Top-level reserved names stay exempt: a planted wider look-alike must not -- block the system's own owner-visible group. CREATE UNIQUE INDEX channel_groups_owner_parent_name_key - ON channel_groups (owner_user_id, COALESCE(parent_group_id, ''), name) + ON channel_groups (namespace_owner_id, COALESCE(parent_group_id, ''), name) WHERE NOT (parent_group_id IS NULL AND name IN ('__dm__', '__linear__', '__coordination__')); -- The repair above leaves no '/' behind. This takes ACCESS EXCLUSIVE until the diff --git a/go/internal/store/queries/channels.sql b/go/internal/store/queries/channels.sql index 971ef9da1..5181f051f 100644 --- a/go/internal/store/queries/channels.sql +++ b/go/internal/store/queries/channels.sql @@ -11,9 +11,16 @@ -- edge's single-id visibility check cannot drift from the list read (the -- anti-drift guarantee the design record requires). --- name: InsertChannelGroup :exec -INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility) -VALUES ($1, $2, NULLIF($3, ''), $4, $5); +-- name: InsertChannelGroup :one +-- An agent's group lives in its owner's namespace, so sibling names are unique per user. +INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility, namespace_owner_id) +VALUES ($1, $2, NULLIF($3, ''), $4, $5, + COALESCE((SELECT owner_user_id FROM agent_accounts WHERE account_id = $4), $4)) +RETURNING namespace_owner_id; + +-- name: GlobalHandlesByAccountIDs :many +SELECT account_id, handle FROM account_handles +WHERE owner_user_id IS NULL AND account_id = ANY($1::text[]); -- name: GetChannelGroupVisibility :one SELECT visibility FROM channel_groups WHERE id = $1; @@ -95,7 +102,7 @@ viewer AS ( UNION ALL SELECT $1 AS uid ) -SELECT g.id, g.name, COALESCE(g.parent_group_id, '') AS parent_group_id, g.owner_user_id, g.visibility +SELECT g.id, g.name, COALESCE(g.parent_group_id, '') AS parent_group_id, g.owner_user_id, g.visibility, g.namespace_owner_id FROM channel_groups g JOIN effective e ON e.id = g.id WHERE (e.eff_vis = 1 OR g.owner_user_id IN (SELECT uid FROM viewer)) diff --git a/go/internal/store/queries/coordination.sql b/go/internal/store/queries/coordination.sql index 543d38a9e..3d1851de2 100644 --- a/go/internal/store/queries/coordination.sql +++ b/go/internal/store/queries/coordination.sql @@ -10,8 +10,9 @@ SELECT id FROM channel_groups WHERE owner_user_id = $1 AND name = $2 AND parent_group_id IS NULL AND visibility = $3; -- name: InsertCoordinationGroup :exec -INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility) -VALUES ($1, $2, NULL, $3, $4); +INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility, namespace_owner_id) +VALUES ($1, $2, NULL, $3, $4, + COALESCE((SELECT owner_user_id FROM agent_accounts WHERE account_id = $3), $3)); -- name: GetCoordinationChannelByName :one SELECT id, COALESCE(owner_account_id, '') AS owner_account_id diff --git a/go/internal/store/queries/dm.sql b/go/internal/store/queries/dm.sql index aa431a61e..07de4346d 100644 --- a/go/internal/store/queries/dm.sql +++ b/go/internal/store/queries/dm.sql @@ -14,8 +14,9 @@ SELECT id FROM channel_groups WHERE owner_user_id = $1 AND name = $2 AND parent_group_id IS NULL AND visibility = $3; -- name: InsertOwnerDMGroup :exec -INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility) -VALUES ($1, $2, NULL, $3, $4); +INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility, namespace_owner_id) +VALUES ($1, $2, NULL, $3, $4, + COALESCE((SELECT owner_user_id FROM agent_accounts WHERE account_id = $3), $3)); -- name: GetDMChannelByName :one SELECT id, kind FROM channels WHERE group_id = $1 AND name = $2; diff --git a/go/internal/store/queries/linear_routing.sql b/go/internal/store/queries/linear_routing.sql index 2e402b9ff..d58146a72 100644 --- a/go/internal/store/queries/linear_routing.sql +++ b/go/internal/store/queries/linear_routing.sql @@ -9,8 +9,9 @@ ORDER BY id LIMIT 1; -- name: InsertLinearRoutingGroup :exec -INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility) -VALUES ($1, $2, NULL, $3, $4); +INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility, namespace_owner_id) +VALUES ($1, $2, NULL, $3, $4, + COALESCE((SELECT owner_user_id FROM agent_accounts WHERE account_id = $3), $3)); -- name: GetLinearRoutingChannel :one SELECT id, kind FROM channels WHERE group_id = $1 AND name = $2; diff --git a/go/internal/store/types.go b/go/internal/store/types.go index 22a46d3b8..ae6895593 100644 --- a/go/internal/store/types.go +++ b/go/internal/store/types.go @@ -219,7 +219,10 @@ type ChannelGroup struct { // OwnerUserID is the user whose space this group is; empty for a shared // group. Server-set to the creating caller. OwnerUserID AccountID - Visibility ChannelGroupVisibility + // NamespaceOwnerID is the user whose namespace holds the group: the creator, + // or an agent creator's owner. Sibling names are unique within it. + NamespaceOwnerID AccountID + Visibility ChannelGroupVisibility } // Channel is a named conversation within a group (comms.proto:183-195). Per the diff --git a/go/server/linear_responder_pgtest_test.go b/go/server/linear_responder_pgtest_test.go index e772a96f5..d34758b1f 100644 --- a/go/server/linear_responder_pgtest_test.go +++ b/go/server/linear_responder_pgtest_test.go @@ -92,8 +92,8 @@ func plantRoutingLookalikes(t *testing.T, st *store.Store, dsn string, adminID s // The store refuses the reserved group name (store.linearRoutingGroupName), so plant with raw SQL. const sharedID, plantedID = "linear-lookalike-group", "linear-lookalike-channel" execSQL(t, ctx, dsn, - `INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility, tenant_id) - SELECT $1, '__linear__', NULL, a.id, $2, a.tenant_id FROM accounts a WHERE a.id = $3`, + `INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, namespace_owner_id, visibility, tenant_id) + SELECT $1, '__linear__', NULL, a.id, a.id, $2, a.tenant_id FROM accounts a WHERE a.id = $3`, sharedID, int16(store.VisibilityShared), string(adminID)) execSQL(t, ctx, dsn, `INSERT INTO channels (id, name, group_id, kind, post_policy, owner_account_id, mandatory_subscription, tenant_id) diff --git a/packages/compass-agent/src/gen/compass/v1/agent_gateway_pb.ts b/packages/compass-agent/src/gen/compass/v1/agent_gateway_pb.ts index 0973eb476..eced6a07a 100644 --- a/packages/compass-agent/src/gen/compass/v1/agent_gateway_pb.ts +++ b/packages/compass-agent/src/gen/compass/v1/agent_gateway_pb.ts @@ -65,8 +65,9 @@ export type CommsCallRequest = Message<"compass.v1.CommsCallRequest"> & { /** * create_channel and create_channel_group use group_name / parent_group_name - * as a leaf or root slash path, resolved within visible groups. Unknown or - * invisible is NOT_FOUND; ambiguous leaf or path is INVALID_ARGUMENT. Setting + * as a leaf, a root slash path, a top-level-anchored `/path`, or an + * owner-qualified `/~handle/path`, resolved within visible groups. Unknown or + * invisible is NOT_FOUND; an ambiguous ref is INVALID_ARGUMENT. Setting * group_id / parent_group_id is INVALID_ARGUMENT. * * @generated from oneof compass.v1.CommsCallRequest.call diff --git a/packages/compass-agent/src/gen/compass/v1/comms_pb.ts b/packages/compass-agent/src/gen/compass/v1/comms_pb.ts index 7c506da3a..7bdf7af8d 100644 --- a/packages/compass-agent/src/gen/compass/v1/comms_pb.ts +++ b/packages/compass-agent/src/gen/compass/v1/comms_pb.ts @@ -1277,8 +1277,9 @@ export const ListAccountsResponseSchema: GenMessage = /*@_ */ export type CreateChannelGroupRequest = Message$1<"compass.v1.CreateChannelGroupRequest"> & { /** - * Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name an - * owner already uses under the same parent is ALREADY_EXISTS. + * Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name already + * used under the same parent in the same user's namespace (a user and that + * user's agents share one) is ALREADY_EXISTS. * * @generated from field: string name = 1; */ @@ -1297,10 +1298,12 @@ export type CreateChannelGroupRequest = Message$1<"compass.v1.CreateChannelGroup visibility: ChannelGroupVisibility; /** - * Agent tool path only. Names the parent as a leaf or root slash path (`a/b/c`) - * when it contains `/`. Unknown or invisible is NOT_FOUND; an ambiguous leaf - * or path is INVALID_ARGUMENT. CommsService rejects this field; human callers - * use parent_group_id. + * Agent tool path only. Names the parent as a leaf (`eng`), a slash path from + * the root (`eng/infra`), a top-level-anchored path (`/infra`), or a path + * qualified by the top-level group's owner handle (`/~matt/eng`). Unknown or + * invisible is NOT_FOUND; an ambiguous ref is INVALID_ARGUMENT and names the + * anchored or qualified ref to use. CommsService rejects this field; human + * callers use parent_group_id. * * @generated from field: string parent_group_name = 4; */ @@ -1454,10 +1457,12 @@ export type CreateChannelRequest = Message$1<"compass.v1.CreateChannelRequest"> membershipMode: ChannelMembershipMode; /** - * Agent tool path only. Names the parent as a leaf or root slash path (`a/b/c`) - * when it contains `/`. Unknown or invisible is NOT_FOUND; an ambiguous leaf - * or path is INVALID_ARGUMENT. CommsService rejects this field; human callers - * use group_id. + * Agent tool path only. Names the parent as a leaf (`eng`), a slash path from + * the root (`eng/infra`), a top-level-anchored path (`/infra`), or a path + * qualified by the top-level group's owner handle (`/~matt/eng`). Unknown or + * invisible is NOT_FOUND; an ambiguous ref is INVALID_ARGUMENT and names the + * anchored or qualified ref to use. CommsService rejects this field; human + * callers use group_id. * * @generated from field: string group_name = 7; */ diff --git a/packages/compass-client/src/gen/compass/v1/comms_pb.ts b/packages/compass-client/src/gen/compass/v1/comms_pb.ts index 7c506da3a..7bdf7af8d 100644 --- a/packages/compass-client/src/gen/compass/v1/comms_pb.ts +++ b/packages/compass-client/src/gen/compass/v1/comms_pb.ts @@ -1277,8 +1277,9 @@ export const ListAccountsResponseSchema: GenMessage = /*@_ */ export type CreateChannelGroupRequest = Message$1<"compass.v1.CreateChannelGroupRequest"> & { /** - * Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name an - * owner already uses under the same parent is ALREADY_EXISTS. + * Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name already + * used under the same parent in the same user's namespace (a user and that + * user's agents share one) is ALREADY_EXISTS. * * @generated from field: string name = 1; */ @@ -1297,10 +1298,12 @@ export type CreateChannelGroupRequest = Message$1<"compass.v1.CreateChannelGroup visibility: ChannelGroupVisibility; /** - * Agent tool path only. Names the parent as a leaf or root slash path (`a/b/c`) - * when it contains `/`. Unknown or invisible is NOT_FOUND; an ambiguous leaf - * or path is INVALID_ARGUMENT. CommsService rejects this field; human callers - * use parent_group_id. + * Agent tool path only. Names the parent as a leaf (`eng`), a slash path from + * the root (`eng/infra`), a top-level-anchored path (`/infra`), or a path + * qualified by the top-level group's owner handle (`/~matt/eng`). Unknown or + * invisible is NOT_FOUND; an ambiguous ref is INVALID_ARGUMENT and names the + * anchored or qualified ref to use. CommsService rejects this field; human + * callers use parent_group_id. * * @generated from field: string parent_group_name = 4; */ @@ -1454,10 +1457,12 @@ export type CreateChannelRequest = Message$1<"compass.v1.CreateChannelRequest"> membershipMode: ChannelMembershipMode; /** - * Agent tool path only. Names the parent as a leaf or root slash path (`a/b/c`) - * when it contains `/`. Unknown or invisible is NOT_FOUND; an ambiguous leaf - * or path is INVALID_ARGUMENT. CommsService rejects this field; human callers - * use group_id. + * Agent tool path only. Names the parent as a leaf (`eng`), a slash path from + * the root (`eng/infra`), a top-level-anchored path (`/infra`), or a path + * qualified by the top-level group's owner handle (`/~matt/eng`). Unknown or + * invisible is NOT_FOUND; an ambiguous ref is INVALID_ARGUMENT and names the + * anchored or qualified ref to use. CommsService rejects this field; human + * callers use group_id. * * @generated from field: string group_name = 7; */ diff --git a/proto/compass/v1/agent_gateway.proto b/proto/compass/v1/agent_gateway.proto index fec6b96de..a758e9859 100644 --- a/proto/compass/v1/agent_gateway.proto +++ b/proto/compass/v1/agent_gateway.proto @@ -106,8 +106,9 @@ service AgentGateway { message CommsCallRequest { string call_id = 1; // create_channel and create_channel_group use group_name / parent_group_name - // as a leaf or root slash path, resolved within visible groups. Unknown or - // invisible is NOT_FOUND; ambiguous leaf or path is INVALID_ARGUMENT. Setting + // as a leaf, a root slash path, a top-level-anchored `/path`, or an + // owner-qualified `/~handle/path`, resolved within visible groups. Unknown or + // invisible is NOT_FOUND; an ambiguous ref is INVALID_ARGUMENT. Setting // group_id / parent_group_id is INVALID_ARGUMENT. oneof call { // post, list and update_members carry a channel NAME in channel_id, resolved diff --git a/proto/compass/v1/comms.proto b/proto/compass/v1/comms.proto index 204443f8b..5a98c91f1 100644 --- a/proto/compass/v1/comms.proto +++ b/proto/compass/v1/comms.proto @@ -651,16 +651,19 @@ message ListAccountsResponse { } message CreateChannelGroupRequest { - // Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name an - // owner already uses under the same parent is ALREADY_EXISTS. + // Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name already + // used under the same parent in the same user's namespace (a user and that + // user's agents share one) is ALREADY_EXISTS. string name = 1; // Parent group; empty for a top-level group. string parent_group_id = 2; ChannelGroupVisibility visibility = 3; - // Agent tool path only. Names the parent as a leaf or root slash path (`a/b/c`) - // when it contains `/`. Unknown or invisible is NOT_FOUND; an ambiguous leaf - // or path is INVALID_ARGUMENT. CommsService rejects this field; human callers - // use parent_group_id. + // Agent tool path only. Names the parent as a leaf (`eng`), a slash path from + // the root (`eng/infra`), a top-level-anchored path (`/infra`), or a path + // qualified by the top-level group's owner handle (`/~matt/eng`). Unknown or + // invisible is NOT_FOUND; an ambiguous ref is INVALID_ARGUMENT and names the + // anchored or qualified ref to use. CommsService rejects this field; human + // callers use parent_group_id. string parent_group_name = 4; } @@ -704,10 +707,12 @@ message CreateChannelRequest { string parent_agent_handle = 5; // TREE requires parent_agent_handle. ChannelMembershipMode membership_mode = 6; - // Agent tool path only. Names the parent as a leaf or root slash path (`a/b/c`) - // when it contains `/`. Unknown or invisible is NOT_FOUND; an ambiguous leaf - // or path is INVALID_ARGUMENT. CommsService rejects this field; human callers - // use group_id. + // Agent tool path only. Names the parent as a leaf (`eng`), a slash path from + // the root (`eng/infra`), a top-level-anchored path (`/infra`), or a path + // qualified by the top-level group's owner handle (`/~matt/eng`). Unknown or + // invisible is NOT_FOUND; an ambiguous ref is INVALID_ARGUMENT and names the + // anchored or qualified ref to use. CommsService rejects this field; human + // callers use group_id. string group_name = 7; } From 095b663322a16455ca1a9f4051bfce4a68b6e382 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 6 Oct 2026 00:56:13 -0400 Subject: [PATCH 5/9] fix(store): renumber the group-name migration onto the collapsed baseline (RIG-3030) Co-authored-by: Matt Wilkinson --- ...{0009_channel_group_names.sql => 0002_channel_group_names.sql} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename go/internal/store/migrations/{0009_channel_group_names.sql => 0002_channel_group_names.sql} (100%) diff --git a/go/internal/store/migrations/0009_channel_group_names.sql b/go/internal/store/migrations/0002_channel_group_names.sql similarity index 100% rename from go/internal/store/migrations/0009_channel_group_names.sql rename to go/internal/store/migrations/0002_channel_group_names.sql From 7504503b1d30dc9820875ad6b13886c9b5ca6873 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 6 Oct 2026 01:28:48 -0400 Subject: [PATCH 6/9] fix(store): show namespace groups to the namespace; fill namespace on old-binary inserts (RIG-4458) An agent-created owner group is visible to its user and sibling agents, matching the OWNER contract. A trigger fills namespace_owner_id for inserts that omit it. Co-authored-by: Matt Wilkinson --- .../channel_groups_reserved_pgtest_test.go | 3 + go/internal/store/channels.go | 27 ++++-- go/internal/store/channels_test.go | 89 +++++++++++++++++++ go/internal/store/db/authz.sql.go | 6 +- go/internal/store/db/channels.sql.go | 6 +- go/internal/store/db/querier.go | 2 +- go/internal/store/group_ref_test.go | 47 ++++++++++ .../migrations/0002_channel_group_names.sql | 41 ++++++--- go/internal/store/queries/authz.sql | 6 +- go/internal/store/queries/channels.sql | 6 +- 10 files changed, 202 insertions(+), 31 deletions(-) diff --git a/go/internal/store/channel_groups_reserved_pgtest_test.go b/go/internal/store/channel_groups_reserved_pgtest_test.go index d8ee93a1e..c562e008a 100644 --- a/go/internal/store/channel_groups_reserved_pgtest_test.go +++ b/go/internal/store/channel_groups_reserved_pgtest_test.go @@ -29,6 +29,9 @@ func TestCreateChannelGroupRefusesReservedTopLevelNames(t *testing.T) { {"dm nested", NewChannelGroup{Name: dmGroupName, ParentGroupID: parent.ID, Visibility: VisibilityOwner}, false}, {"slash top level", NewChannelGroup{Name: "a/b", Visibility: VisibilityOwner}, true}, {"slash nested", NewChannelGroup{Name: "a/b", ParentGroupID: parent.ID, Visibility: VisibilityOwner}, true}, + {"tilde top level", NewChannelGroup{Name: "~matt", Visibility: VisibilityOwner}, true}, + {"tilde nested", NewChannelGroup{Name: "~ops", ParentGroupID: parent.ID, Visibility: VisibilityOwner}, true}, + {"inner tilde", NewChannelGroup{Name: "a~b", Visibility: VisibilityOwner}, false}, {"ordinary", NewChannelGroup{Name: "ordinary", Visibility: VisibilityShared}, false}, } for _, tc := range cases { diff --git a/go/internal/store/channels.go b/go/internal/store/channels.go index f6a83c1b8..a72b1b14d 100644 --- a/go/internal/store/channels.go +++ b/go/internal/store/channels.go @@ -21,6 +21,10 @@ func (s *Store) CreateChannelGroup(ctx context.Context, ownerUserID AccountID, g if strings.Contains(g.Name, "/") { return ChannelGroup{}, fmt.Errorf("%w: group name %q cannot contain '/'", ErrInvalidArgument, g.Name) } + // A leading '~' would read as an owner qualifier in an anchored group ref. + if strings.HasPrefix(g.Name, "~") { + return ChannelGroup{}, fmt.Errorf("%w: group name %q cannot start with '~'", ErrInvalidArgument, g.Name) + } // System groups own these names at top level; nested reuse is an ordinary group. if g.ParentGroupID == "" && isReservedGroupName(g.Name) { return ChannelGroup{}, fmt.Errorf("%w: group name %q is reserved", ErrInvalidArgument, g.Name) @@ -359,7 +363,7 @@ func resolveGroupRef(groups []ChannelGroup, handles map[AccountID]string, ref st case 1: return matches[0], nil default: - return ChannelGroup{}, fmt.Errorf("%w: group name %q is ambiguous — it names %d visible groups; use one of %s", + return ChannelGroup{}, fmt.Errorf("%w: group name %q is ambiguous — it names %d visible groups%s", ErrInvalidArgument, ref, len(matches), groupRefHints(groups, handles, matches)) } } @@ -369,7 +373,8 @@ func resolveGroupRef(groups []ChannelGroup, handles map[AccountID]string, ref st } owner := "" if anchored && strings.HasPrefix(segments[0], "~") { - owner = strings.TrimPrefix(segments[0], "~") + // Handles are lowercase, and callers often write them as mentions. + owner = strings.ToLower(strings.TrimPrefix(strings.TrimPrefix(segments[0], "~"), "@")) segments = segments[1:] if owner == "" || len(segments) == 0 { return ChannelGroup{}, fmt.Errorf("%w: group path %q needs an owner handle and a group, e.g. /~matt/eng", ErrInvalidArgument, ref) @@ -401,7 +406,7 @@ func resolveGroupRef(groups []ChannelGroup, handles map[AccountID]string, ref st for _, group := range candidates { matches = append(matches, group) } - return ChannelGroup{}, fmt.Errorf("%w: group path %q is ambiguous; use one of %s", + return ChannelGroup{}, fmt.Errorf("%w: group path %q is ambiguous%s", ErrInvalidArgument, ref, groupRefHints(groups, handles, matches)) } for _, group := range candidates { @@ -411,7 +416,8 @@ func resolveGroupRef(groups []ChannelGroup, handles map[AccountID]string, ref st } // groupRefHints names, for each match, the anchored path when it is unique among -// the visible groups and otherwise the owner-qualified path, so the caller can retry. +// the visible groups and otherwise the owner-qualified path, so the caller can +// retry. It returns "" when no match has a ref that would resolve it. func groupRefHints(groups []ChannelGroup, handles map[AccountID]string, matches []ChannelGroup) string { byID := make(map[ChannelGroupID]ChannelGroup, len(groups)) for _, group := range groups { @@ -429,7 +435,11 @@ func groupRefHints(groups []ChannelGroup, handles map[AccountID]string, matches group = parent path = group.Name + "/" + path } - forms := refForms{anchored: "/" + path} + var forms refForms + // A legacy top-level name starting with '~' would parse as an owner qualifier. + if !strings.HasPrefix(group.Name, "~") { + forms.anchored = "/" + path + } if handle, ok := handles[group.NamespaceOwnerID]; ok { forms.qualified = "/~" + handle + "/" + path } @@ -449,13 +459,14 @@ func groupRefHints(groups []ChannelGroup, handles map[AccountID]string, matches hints = append(hints, forms.anchored) case forms.qualified != "": hints = append(hints, forms.qualified) - case forms.anchored != "": - hints = append(hints, forms.anchored) } } + if len(hints) == 0 { + return "" + } slices.Sort(hints) hints = slices.Compact(hints) - return strings.Join(hints, ", ") + " (a leading / anchors at the top level; /~owner/ picks the top-level group's owner)" + return "; use one of " + strings.Join(hints, ", ") + " (a leading / anchors at the top level; /~owner/ picks the top-level group's owner)" } // ChannelByNameForViewer resolves a channel NAME to its Channel within the set diff --git a/go/internal/store/channels_test.go b/go/internal/store/channels_test.go index 17ad23421..910f3faf4 100644 --- a/go/internal/store/channels_test.go +++ b/go/internal/store/channels_test.go @@ -109,6 +109,95 @@ func TestChannelGroupByRefForViewerQualifiesOwner(t *testing.T) { } } +// TestOwnerGroupVisibleAcrossNamespace: an owner-only group an agent creates +// belongs to its user's namespace, so the user and sibling agents see it too. +func TestOwnerGroupVisibleAcrossNamespace(t *testing.T) { + ctx := t.Context() + s := newTestStore(t) + owner := mustUser(t, s, "owner") + a1 := mustAgent(t, s, owner.ID, "a1") + a2 := mustAgent(t, s, owner.ID, "a2") + stranger := mustUser(t, s, "stranger") + strangerAgent := mustAgent(t, s, stranger.ID, "spy") + secret, err := s.CreateChannelGroup(ctx, a1.ID, NewChannelGroup{Name: "secret", Visibility: VisibilityOwner}) + if err != nil { + t.Fatalf("CreateChannelGroup(secret): %v", err) + } + for _, viewer := range []AccountID{owner.ID, a1.ID, a2.ID} { + visible, err := s.ChannelGroupVisibleTo(ctx, viewer, secret.ID) + if err != nil || !visible { + t.Fatalf("ChannelGroupVisibleTo(%s) = %v, %v; want true", viewer, visible, err) + } + got, err := s.ChannelGroupByRefForViewer(ctx, viewer, "secret") + if err != nil || got.ID != secret.ID { + t.Fatalf("ChannelGroupByRefForViewer(%s, secret) = %q, %v; want %q", viewer, got.ID, err, secret.ID) + } + } + // A sibling agent may nest under it, as it may under its owner's groups. + if _, err := s.CreateChannelGroup(ctx, a2.ID, NewChannelGroup{Name: "child", ParentGroupID: secret.ID, Visibility: VisibilityOwner}); err != nil { + t.Fatalf("sibling agent nests under namespace group: %v", err) + } + for _, viewer := range []AccountID{stranger.ID, strangerAgent.ID} { + visible, err := s.ChannelGroupVisibleTo(ctx, viewer, secret.ID) + if err != nil || visible { + t.Fatalf("ChannelGroupVisibleTo(stranger %s) = %v, %v; want false", viewer, visible, err) + } + _, err = s.ChannelGroupByRefForViewer(ctx, viewer, "secret") + sentinelIs(t, err, ErrNotFound, "stranger resolves owner-only namespace group") + _, err = s.CreateChannelGroup(ctx, viewer, NewChannelGroup{Name: "intruder", ParentGroupID: secret.ID, Visibility: VisibilityOwner}) + sentinelIs(t, err, ErrNotFound, "stranger nests under owner-only namespace group") + } +} + +// TestChannelGroupInsertWithoutNamespaceIsFilled: an older binary inserts no +// namespace, so the trigger derives it from the creator during a rolling deploy. +func TestChannelGroupInsertWithoutNamespaceIsFilled(t *testing.T) { + ctx := t.Context() + s := newTestStore(t) + owner := mustUser(t, s, "owner") + agent := mustAgent(t, s, owner.ID, "legacy") + for _, creator := range []AccountID{owner.ID, agent.ID} { + id := newID() + if _, err := s.pool.Exec(ctx, + "INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility) VALUES ($1, $2, NULL, $3, $4)", + id, "legacy-"+id, string(creator), int16(VisibilityOwner)); err != nil { + t.Fatalf("insert without namespace by %s: %v", creator, err) + } + var namespace string + if err := s.pool.QueryRow(ctx, "SELECT namespace_owner_id FROM channel_groups WHERE id = $1", id).Scan(&namespace); err != nil { + t.Fatalf("read namespace: %v", err) + } + if namespace != string(owner.ID) { + t.Fatalf("namespace for creator %s = %q, want %q", creator, namespace, owner.ID) + } + } +} + +// TestChannelGroupByRefForViewerNestedCrossNamespaceStaysAmbiguous pins today's +// behavior: the same nested path in two namespaces is never auto-picked. +func TestChannelGroupByRefForViewerNestedCrossNamespaceStaysAmbiguous(t *testing.T) { + ctx := t.Context() + s := newTestStore(t) + alice := mustUser(t, s, "alice") + bob := mustUser(t, s, "bob") + pub, err := s.CreateChannelGroup(ctx, alice.ID, NewChannelGroup{Name: "pub", Visibility: VisibilityShared}) + if err != nil { + t.Fatalf("CreateChannelGroup(pub): %v", err) + } + for _, by := range []AccountID{alice.ID, bob.ID} { + if _, err := s.CreateChannelGroup(ctx, by, NewChannelGroup{Name: "infra", ParentGroupID: pub.ID, Visibility: VisibilityShared}); err != nil { + t.Fatalf("CreateChannelGroup(pub/infra by %s): %v", by, err) + } + } + for _, ref := range []string{"pub/infra", "/pub/infra", "/~alice/pub/infra"} { + got, err := s.ChannelGroupByRefForViewer(ctx, alice.ID, ref) + sentinelIs(t, err, ErrInvalidArgument, "nested cross-namespace "+ref) + if got.ID != "" { + t.Fatalf("ChannelGroupByRefForViewer(%q) picked %q", ref, got.ID) + } + } +} + func TestCreateChannelGroupCeilingRejectsWiderChild(t *testing.T) { ctx := context.Background() s := newTestStore(t) diff --git a/go/internal/store/db/authz.sql.go b/go/internal/store/db/authz.sql.go index 10e111e53..bdb571b18 100644 --- a/go/internal/store/db/authz.sql.go +++ b/go/internal/store/db/authz.sql.go @@ -45,7 +45,9 @@ SELECT EXISTS ( -- bare-SHARED group nested under an OWNER parent would authorize -- creates it should not). OR g.visibility = $3 - OR g.owner_user_id = (SELECT owner_user_id FROM agent_accounts WHERE account_id = $2))) + OR g.owner_user_id = (SELECT owner_user_id FROM agent_accounts WHERE account_id = $2) + -- A group an agent created lives in its owner's namespace. + OR g.namespace_owner_id = COALESCE((SELECT owner_user_id FROM agent_accounts WHERE account_id = $2), $2))) ` type GroupCreateAuthorizedParams struct { @@ -54,7 +56,7 @@ type GroupCreateAuthorizedParams struct { Visibility int16 } -// Feeds requireGroupCreateAuthz: owner, agent-owner, or SHARED-visibility group. +// Feeds requireGroupCreateAuthz: owner, agent-owner, same namespace, or SHARED-visibility group. func (q *Queries) GroupCreateAuthorized(ctx context.Context, arg GroupCreateAuthorizedParams) (bool, error) { row := q.db.QueryRow(ctx, groupCreateAuthorized, arg.ID, arg.OwnerUserID, arg.Visibility) var exists bool diff --git a/go/internal/store/db/channels.sql.go b/go/internal/store/db/channels.sql.go index 25a03325e..d11378beb 100644 --- a/go/internal/store/db/channels.sql.go +++ b/go/internal/store/db/channels.sql.go @@ -55,7 +55,8 @@ viewer AS ( SELECT EXISTS ( SELECT 1 FROM channel_groups g JOIN effective e ON e.id = g.id - WHERE g.id = $2 AND (e.eff_vis = 1 OR g.owner_user_id IN (SELECT uid FROM viewer)) + WHERE g.id = $2 AND (e.eff_vis = 1 OR g.owner_user_id IN (SELECT uid FROM viewer) + OR g.namespace_owner_id IN (SELECT uid FROM viewer)) ) ` @@ -472,7 +473,8 @@ viewer AS ( SELECT g.id, g.name, COALESCE(g.parent_group_id, '') AS parent_group_id, g.owner_user_id, g.visibility, g.namespace_owner_id FROM channel_groups g JOIN effective e ON e.id = g.id -WHERE (e.eff_vis = 1 OR g.owner_user_id IN (SELECT uid FROM viewer)) +WHERE (e.eff_vis = 1 OR g.owner_user_id IN (SELECT uid FROM viewer) + OR g.namespace_owner_id IN (SELECT uid FROM viewer)) ORDER BY g.name ` diff --git a/go/internal/store/db/querier.go b/go/internal/store/db/querier.go index c44b3e328..6a05de53d 100644 --- a/go/internal/store/db/querier.go +++ b/go/internal/store/db/querier.go @@ -233,7 +233,7 @@ type Querier interface { // Scope grants are managed for user accounts; agents inherit their owner's rows. // The SELECT runs under RLS, so a user from another tenant inserts nothing. GrantForgeScope(ctx context.Context, arg GrantForgeScopeParams) (int64, error) - // Feeds requireGroupCreateAuthz: owner, agent-owner, or SHARED-visibility group. + // Feeds requireGroupCreateAuthz: owner, agent-owner, same namespace, or SHARED-visibility group. GroupCreateAuthorized(ctx context.Context, arg GroupCreateAuthorizedParams) (bool, error) HasForgeScope(ctx context.Context, arg HasForgeScopeParams) (bool, error) HotTailBytes(ctx context.Context, arg HotTailBytesParams) (int64, error) diff --git a/go/internal/store/group_ref_test.go b/go/internal/store/group_ref_test.go index cfb25800a..19b138385 100644 --- a/go/internal/store/group_ref_test.go +++ b/go/internal/store/group_ref_test.go @@ -60,6 +60,9 @@ func TestResolveGroupRef(t *testing.T) { {name: "unanchored path picks the nested group", ref: "eng/infra", wantID: "eng-infra"}, {name: "anchored path through an ambiguous top level", ref: "/eng/infra", wantID: "eng-infra"}, {name: "anchor never matches a nested group", ref: "/leaf", wantErr: ErrNotFound}, + {name: "qualifier ignores case", ref: "/~Viewer/eng", wantID: "viewer-eng"}, + {name: "qualifier strips one leading @", ref: "/~@stranger/eng", wantID: "stranger-eng"}, + {name: "qualifier strips only one @", ref: "/~@@stranger/eng", wantErr: ErrNotFound}, } for _, tt := range tests { @@ -114,3 +117,47 @@ func TestResolveGroupRefAmbiguityNamesTheFix(t *testing.T) { } } } + +// TestResolveGroupRefHintForLegacyTildeName: a pre-guard top-level name that +// starts with '~' would parse as an owner qualifier, so its hint is qualified. +func TestResolveGroupRefHintForLegacyTildeName(t *testing.T) { + groups := []ChannelGroup{ + {ID: "top-ops", Name: "~ops", NamespaceOwnerID: "viewer-id"}, + {ID: "eng", Name: "eng", NamespaceOwnerID: "viewer-id"}, + {ID: "eng-ops", Name: "~ops", ParentGroupID: "eng", NamespaceOwnerID: "viewer-id"}, + } + handles := map[AccountID]string{"viewer-id": "viewer"} + _, err := resolveGroupRef(groups, handles, "~ops") + if !errors.Is(err, ErrInvalidArgument) { + t.Fatalf("resolveGroupRef(~ops) error = %v, want invalid argument", err) + } + for item := range strings.SplitSeq(err.Error(), ", ") { + if strings.HasSuffix(item, "use one of /~ops") || strings.HasPrefix(item, "/~ops") { + t.Errorf("error %q offers the unparseable anchored ref /~ops", err) + } + } + for hint, wantID := range map[string]ChannelGroupID{"/~viewer/~ops": "top-ops", "/eng/~ops": "eng-ops"} { + if !strings.Contains(err.Error(), hint) { + t.Errorf("error %q does not name %q", err, hint) + } + if got, err := resolveGroupRef(groups, handles, hint); err != nil || got.ID != wantID { + t.Errorf("hint %q resolves to %q, %v; want %q", hint, got.ID, err, wantID) + } + } +} + +// TestResolveGroupRefNoHintOmitsClause: with no ref to offer, the error must not +// promise one. +func TestResolveGroupRefNoHintOmitsClause(t *testing.T) { + groups := []ChannelGroup{ + {ID: "x-1", Name: "x", NamespaceOwnerID: "ghost-1"}, + {ID: "x-2", Name: "x", NamespaceOwnerID: "ghost-2"}, + } + _, err := resolveGroupRef(groups, nil, "x") + if !errors.Is(err, ErrInvalidArgument) { + t.Fatalf("resolveGroupRef(x) error = %v, want invalid argument", err) + } + if strings.Contains(err.Error(), "use one of") { + t.Errorf("error %q offers an empty hint list", err) + } +} diff --git a/go/internal/store/migrations/0002_channel_group_names.sql b/go/internal/store/migrations/0002_channel_group_names.sql index 64844d617..51a97940e 100644 --- a/go/internal/store/migrations/0002_channel_group_names.sql +++ b/go/internal/store/migrations/0002_channel_group_names.sql @@ -6,8 +6,9 @@ -- Fail fast rather than queue every channel read behind a long transaction. SET LOCAL lock_timeout = '5s'; --- The default exists only for the backfill; it is dropped below so every insert --- must name the namespace. +-- The default exists only for the backfill and is dropped below; the trigger +-- fills the column for writers that omit it. ADD COLUMN holds ACCESS EXCLUSIVE +-- until commit, so no create can slip a duplicate past the repair. ALTER TABLE channel_groups ADD COLUMN namespace_owner_id TEXT NOT NULL DEFAULT ''; -- Under FORCE RLS a non-superuser owner sees no rows, so the repair runs as @@ -23,34 +24,30 @@ DECLARE candidate TEXT; suffix INTEGER; BEGIN - -- Freeze writers through the index build so no concurrent create commits a - -- duplicate the repair never saw. Inside DO so autocommit replays accept it. - LOCK TABLE channel_groups IN SHARE ROW EXCLUSIVE MODE; - UPDATE channel_groups AS g - SET namespace_owner_id = COALESCE( + SET namespace_owner_id = coalesce( (SELECT a.owner_user_id FROM agent_accounts AS a WHERE a.account_id = g.owner_user_id), g.owner_user_id); WITH rewritten AS ( - UPDATE channel_groups SET name = REPLACE(name, '/', '-') + UPDATE channel_groups SET name = replace(name, '/', '-') WHERE name LIKE '%/%' RETURNING id ) - SELECT COALESCE(array_agg(id), '{}') INTO rewritten_ids FROM rewritten; + SELECT coalesce(array_agg(id), '{}') INTO rewritten_ids FROM rewritten; FOR duplicate IN SELECT id, namespace_owner_id, parent_group_id, name, duplicate_number FROM ( SELECT id, namespace_owner_id, parent_group_id, name, row_number() OVER ( - PARTITION BY namespace_owner_id, COALESCE(parent_group_id, ''), name + PARTITION BY namespace_owner_id, coalesce(parent_group_id, ''), name ORDER BY id = ANY (rewritten_ids), id ) AS duplicate_number FROM channel_groups ) AS ranked WHERE duplicate_number > 1 - ORDER BY namespace_owner_id, COALESCE(parent_group_id, ''), name, duplicate_number + ORDER BY namespace_owner_id, coalesce(parent_group_id, ''), name, duplicate_number LOOP IF duplicate.parent_group_id IS NULL AND duplicate.name IN ('__dm__', '__linear__', '__coordination__') THEN @@ -63,7 +60,7 @@ BEGIN SELECT 1 FROM channel_groups AS sibling WHERE sibling.namespace_owner_id = duplicate.namespace_owner_id - AND COALESCE(sibling.parent_group_id, '') = COALESCE(duplicate.parent_group_id, '') + AND coalesce(sibling.parent_group_id, '') = coalesce(duplicate.parent_group_id, '') AND sibling.name = candidate AND sibling.id <> duplicate.id ); @@ -81,10 +78,26 @@ ALTER TABLE channel_groups ALTER COLUMN namespace_owner_id DROP DEFAULT; -- Top-level reserved names stay exempt: a planted wider look-alike must not -- block the system's own owner-visible group. CREATE UNIQUE INDEX channel_groups_owner_parent_name_key - ON channel_groups (namespace_owner_id, COALESCE(parent_group_id, ''), name) + ON channel_groups (namespace_owner_id, coalesce(parent_group_id, ''), name) WHERE NOT (parent_group_id IS NULL AND name IN ('__dm__', '__linear__', '__coordination__')); -- The repair above leaves no '/' behind. This takes ACCESS EXCLUSIVE until the -- migration commits; brief on a small table. -- squawk-ignore constraint-missing-not-valid -ALTER TABLE channel_groups ADD CONSTRAINT channel_groups_name_no_slash CHECK (POSITION('/' IN name) = 0); +ALTER TABLE channel_groups ADD CONSTRAINT channel_groups_name_no_slash CHECK (position('/' IN name) = 0); + +-- Binaries from before this migration insert without the column during a +-- rolling deploy, so derive it from the creator the way the backfill does. +CREATE FUNCTION channel_groups_fill_namespace() RETURNS TRIGGER +LANGUAGE plpgsql AS $$ +BEGIN + IF NEW.namespace_owner_id IS NULL THEN + NEW.namespace_owner_id := coalesce( + (SELECT a.owner_user_id FROM agent_accounts AS a WHERE a.account_id = NEW.owner_user_id), + NEW.owner_user_id); + END IF; + RETURN NEW; +END $$; + +CREATE TRIGGER channel_groups_fill_namespace BEFORE INSERT ON channel_groups + FOR EACH ROW EXECUTE FUNCTION channel_groups_fill_namespace(); diff --git a/go/internal/store/queries/authz.sql b/go/internal/store/queries/authz.sql index d2f4d101c..8f03d8593 100644 --- a/go/internal/store/queries/authz.sql +++ b/go/internal/store/queries/authz.sql @@ -10,7 +10,7 @@ SELECT EXISTS (SELECT 1 FROM topics t JOIN channel_members cm ON cm.channel_id = t.channel_id WHERE t.id = $1 AND cm.account_id = $2); -- name: GroupCreateAuthorized :one --- Feeds requireGroupCreateAuthz: owner, agent-owner, or SHARED-visibility group. +-- Feeds requireGroupCreateAuthz: owner, agent-owner, same namespace, or SHARED-visibility group. SELECT EXISTS ( SELECT 1 FROM channel_groups g WHERE g.id = $1 AND ( @@ -26,7 +26,9 @@ SELECT EXISTS ( -- bare-SHARED group nested under an OWNER parent would authorize -- creates it should not). OR g.visibility = $3 - OR g.owner_user_id = (SELECT owner_user_id FROM agent_accounts WHERE account_id = $2))); + OR g.owner_user_id = (SELECT owner_user_id FROM agent_accounts WHERE account_id = $2) + -- A group an agent created lives in its owner's namespace. + OR g.namespace_owner_id = COALESCE((SELECT owner_user_id FROM agent_accounts WHERE account_id = $2), $2))); -- name: AgentWorkspaceVisible :one -- Feeds isAgentWorkspaceVisible: membership on the agent's home channel. diff --git a/go/internal/store/queries/channels.sql b/go/internal/store/queries/channels.sql index 5181f051f..9d30b9943 100644 --- a/go/internal/store/queries/channels.sql +++ b/go/internal/store/queries/channels.sql @@ -105,7 +105,8 @@ viewer AS ( SELECT g.id, g.name, COALESCE(g.parent_group_id, '') AS parent_group_id, g.owner_user_id, g.visibility, g.namespace_owner_id FROM channel_groups g JOIN effective e ON e.id = g.id -WHERE (e.eff_vis = 1 OR g.owner_user_id IN (SELECT uid FROM viewer)) +WHERE (e.eff_vis = 1 OR g.owner_user_id IN (SELECT uid FROM viewer) + OR g.namespace_owner_id IN (SELECT uid FROM viewer)) ORDER BY g.name; -- name: ChannelGroupVisibleTo :one @@ -130,7 +131,8 @@ viewer AS ( SELECT EXISTS ( SELECT 1 FROM channel_groups g JOIN effective e ON e.id = g.id - WHERE g.id = $2 AND (e.eff_vis = 1 OR g.owner_user_id IN (SELECT uid FROM viewer)) + WHERE g.id = $2 AND (e.eff_vis = 1 OR g.owner_user_id IN (SELECT uid FROM viewer) + OR g.namespace_owner_id IN (SELECT uid FROM viewer)) ); -- name: ListChannels :many From f2e3511e21184f93f60d72ef3b2981e1d4168f74 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 6 Oct 2026 17:33:04 -0400 Subject: [PATCH 7/9] fix(store): key nested group names on the parent alone (RIG-4651) A shared group has no per-user namespaces, so a child name exists once under its parent whoever creates it. Top-level names stay per namespace. Co-authored-by: Matt Wilkinson --- docs/specs/product/compass.md | 15 +++++++++--- ...annel_group_names_migration_pgtest_test.go | 7 +++++- go/internal/store/channels_test.go | 24 +++++++++---------- go/internal/store/db/channels.sql.go | 2 +- go/internal/store/db/querier.go | 2 +- .../migrations/0002_channel_group_names.sql | 24 +++++++++++-------- go/internal/store/queries/channels.sql | 2 +- go/internal/store/types.go | 2 +- 8 files changed, 48 insertions(+), 30 deletions(-) diff --git a/docs/specs/product/compass.md b/docs/specs/product/compass.md index dcec1909e..8aea03ae2 100644 --- a/docs/specs/product/compass.md +++ b/docs/specs/product/compass.md @@ -599,9 +599,11 @@ group SHALL return invalid-argument, never pick one, and the error SHALL name the anchored or owner-qualified ref for each match. The human RPC SHALL take ids and reject the name fields. -A group name SHALL be unique among its siblings within one user's namespace. An -agent's groups belong to its owning user's namespace, so a user and that user's -agents cannot create same-named siblings. +A top-level group name SHALL be unique within one user's namespace. An agent's +groups belong to its owning user's namespace, so a user and that user's agents +cannot create same-named top-level groups. A nested group name SHALL be unique +under its parent, whoever creates it: a shared group has no per-user +namespaces, so a second child of the same name is a conflict. #### Scenario: An ambiguous leaf name is rejected @@ -629,6 +631,13 @@ agents cannot create same-named siblings. - **Then** the call resolves to the top-level group, and `eng/infra` resolves to the nested one +#### Scenario: A shared group holds one child of each name + +- **Given** a shared group `pub` with a child `infra` made by one user +- **When** another user creates `infra` under `pub` +- **Then** the call returns a conflict, and `pub/infra` resolves to the one + child for both users + ### Requirement: The `SubscribeComms` fan-out is visibility-scoped The comms bus fans every event to every subscriber, so the server SHALL filter diff --git a/go/internal/store/channel_group_names_migration_pgtest_test.go b/go/internal/store/channel_group_names_migration_pgtest_test.go index e1364b5fd..8e3a35c51 100644 --- a/go/internal/store/channel_group_names_migration_pgtest_test.go +++ b/go/internal/store/channel_group_names_migration_pgtest_test.go @@ -85,7 +85,10 @@ func TestChannelGroupNamesMigrationRepairsExistingRows(t *testing.T) { ('group-p', 'team', 'owner-k', 'boot', NULL), ('group-q', 'team', 'agent-k1', 'boot', NULL), ('group-r', 'svc', 'agent-k1', 'boot', 'group-p'), - ('group-s', 'svc', 'agent-k2', 'boot', 'group-p')`); err != nil { + ('group-s', 'svc', 'agent-k2', 'boot', 'group-p'), + ('group-t', 'pub', 'owner-g', 'boot', NULL), + ('group-u', 'infra', 'owner-g', 'boot', 'group-t'), + ('group-v', 'infra', 'owner-h', 'boot', 'group-t')`); err != nil { t.Fatalf("seed pre-migration groups: %v", err) } if err := tx.Commit(ctx); err != nil { @@ -117,6 +120,8 @@ func TestChannelGroupNamesMigrationRepairsExistingRows(t *testing.T) { "group-q": "team-2", // an agent's group shares its owner's namespace "group-r": "svc", // two agents of one owner share a namespace "group-s": "svc-2", // two agents of one owner share a namespace + "group-u": "infra", // nested names are unique per parent + "group-v": "infra-2", // nested names are unique per parent, across namespaces } for id, name := range want { if got[id] != name { diff --git a/go/internal/store/channels_test.go b/go/internal/store/channels_test.go index 910f3faf4..9fa0b9f16 100644 --- a/go/internal/store/channels_test.go +++ b/go/internal/store/channels_test.go @@ -173,9 +173,9 @@ func TestChannelGroupInsertWithoutNamespaceIsFilled(t *testing.T) { } } -// TestChannelGroupByRefForViewerNestedCrossNamespaceStaysAmbiguous pins today's -// behavior: the same nested path in two namespaces is never auto-picked. -func TestChannelGroupByRefForViewerNestedCrossNamespaceStaysAmbiguous(t *testing.T) { +// TestCreateChannelGroupNestedNameUniquePerParent: a group shared by a whole +// tenant has no per-user namespaces, so a child name exists once under it. +func TestCreateChannelGroupNestedNameUniquePerParent(t *testing.T) { ctx := t.Context() s := newTestStore(t) alice := mustUser(t, s, "alice") @@ -184,16 +184,16 @@ func TestChannelGroupByRefForViewerNestedCrossNamespaceStaysAmbiguous(t *testing if err != nil { t.Fatalf("CreateChannelGroup(pub): %v", err) } - for _, by := range []AccountID{alice.ID, bob.ID} { - if _, err := s.CreateChannelGroup(ctx, by, NewChannelGroup{Name: "infra", ParentGroupID: pub.ID, Visibility: VisibilityShared}); err != nil { - t.Fatalf("CreateChannelGroup(pub/infra by %s): %v", by, err) - } + infra, err := s.CreateChannelGroup(ctx, alice.ID, NewChannelGroup{Name: "infra", ParentGroupID: pub.ID, Visibility: VisibilityShared}) + if err != nil { + t.Fatalf("CreateChannelGroup(pub/infra by alice): %v", err) } - for _, ref := range []string{"pub/infra", "/pub/infra", "/~alice/pub/infra"} { - got, err := s.ChannelGroupByRefForViewer(ctx, alice.ID, ref) - sentinelIs(t, err, ErrInvalidArgument, "nested cross-namespace "+ref) - if got.ID != "" { - t.Fatalf("ChannelGroupByRefForViewer(%q) picked %q", ref, got.ID) + _, err = s.CreateChannelGroup(ctx, bob.ID, NewChannelGroup{Name: "infra", ParentGroupID: pub.ID, Visibility: VisibilityShared}) + sentinelIs(t, err, ErrConflict, "second pub/infra from another namespace") + for _, viewer := range []AccountID{alice.ID, bob.ID} { + got, err := s.ChannelGroupByRefForViewer(ctx, viewer, "pub/infra") + if err != nil || got.ID != infra.ID { + t.Fatalf("ChannelGroupByRefForViewer(%s, pub/infra) = %q, %v; want %q", viewer, got.ID, err, infra.ID) } } } diff --git a/go/internal/store/db/channels.sql.go b/go/internal/store/db/channels.sql.go index d11378beb..fbb0f45a9 100644 --- a/go/internal/store/db/channels.sql.go +++ b/go/internal/store/db/channels.sql.go @@ -437,7 +437,7 @@ type InsertChannelGroupParams struct { // groupVisiblePredicate). The copies MUST stay textually identical so the stream // edge's single-id visibility check cannot drift from the list read (the // anti-drift guarantee the design record requires). -// An agent's group lives in its owner's namespace, so sibling names are unique per user. +// An agent's group lives in its owner's namespace, which keys top-level names. func (q *Queries) InsertChannelGroup(ctx context.Context, arg InsertChannelGroupParams) (string, error) { row := q.db.QueryRow(ctx, insertChannelGroup, arg.ID, diff --git a/go/internal/store/db/querier.go b/go/internal/store/db/querier.go index 6a05de53d..8f5588ec2 100644 --- a/go/internal/store/db/querier.go +++ b/go/internal/store/db/querier.go @@ -278,7 +278,7 @@ type Querier interface { // groupVisiblePredicate). The copies MUST stay textually identical so the stream // edge's single-id visibility check cannot drift from the list read (the // anti-drift guarantee the design record requires). - // An agent's group lives in its owner's namespace, so sibling names are unique per user. + // An agent's group lives in its owner's namespace, which keys top-level names. InsertChannelGroup(ctx context.Context, arg InsertChannelGroupParams) (string, error) InsertChannelPin(ctx context.Context, arg InsertChannelPinParams) error InsertCoordinationChannel(ctx context.Context, arg InsertCoordinationChannelParams) (string, error) diff --git a/go/internal/store/migrations/0002_channel_group_names.sql b/go/internal/store/migrations/0002_channel_group_names.sql index 51a97940e..8fc83ccf0 100644 --- a/go/internal/store/migrations/0002_channel_group_names.sql +++ b/go/internal/store/migrations/0002_channel_group_names.sql @@ -1,7 +1,7 @@ -- Agent tools read '/' as a path separator and address a group by sibling name, --- so a name holds no '/' and is unique among one namespace's siblings. An --- agent's groups live in its owner's namespace. Owner ids are global, so the key --- is namespace/parent/name; tenant_id is not part of it. +-- so a name holds no '/'. A top-level name is unique per namespace (an agent's +-- groups live in its owner's); a nested name is unique per parent, whoever made +-- it. Owner ids are global, so tenant_id is not part of the key. -- Fail fast rather than queue every channel read behind a long transaction. SET LOCAL lock_timeout = '5s'; @@ -37,17 +37,19 @@ BEGIN SELECT coalesce(array_agg(id), '{}') INTO rewritten_ids FROM rewritten; FOR duplicate IN - SELECT id, namespace_owner_id, parent_group_id, name, duplicate_number + SELECT id, name_scope, parent_group_id, name, duplicate_number FROM ( - SELECT id, namespace_owner_id, parent_group_id, name, + SELECT id, parent_group_id, name, + CASE WHEN parent_group_id IS NULL THEN namespace_owner_id ELSE '' END AS name_scope, row_number() OVER ( - PARTITION BY namespace_owner_id, coalesce(parent_group_id, ''), name + PARTITION BY CASE WHEN parent_group_id IS NULL THEN namespace_owner_id ELSE '' END, + coalesce(parent_group_id, ''), name ORDER BY id = ANY (rewritten_ids), id ) AS duplicate_number FROM channel_groups ) AS ranked WHERE duplicate_number > 1 - ORDER BY namespace_owner_id, coalesce(parent_group_id, ''), name, duplicate_number + ORDER BY name_scope, coalesce(parent_group_id, ''), name, duplicate_number LOOP IF duplicate.parent_group_id IS NULL AND duplicate.name IN ('__dm__', '__linear__', '__coordination__') THEN @@ -59,8 +61,9 @@ BEGIN EXIT WHEN NOT EXISTS ( SELECT 1 FROM channel_groups AS sibling - WHERE sibling.namespace_owner_id = duplicate.namespace_owner_id - AND coalesce(sibling.parent_group_id, '') = coalesce(duplicate.parent_group_id, '') + WHERE coalesce(sibling.parent_group_id, '') = coalesce(duplicate.parent_group_id, '') + AND (duplicate.parent_group_id IS NOT NULL + OR sibling.namespace_owner_id = duplicate.name_scope) AND sibling.name = candidate AND sibling.id <> duplicate.id ); @@ -78,7 +81,8 @@ ALTER TABLE channel_groups ALTER COLUMN namespace_owner_id DROP DEFAULT; -- Top-level reserved names stay exempt: a planted wider look-alike must not -- block the system's own owner-visible group. CREATE UNIQUE INDEX channel_groups_owner_parent_name_key - ON channel_groups (namespace_owner_id, coalesce(parent_group_id, ''), name) + ON channel_groups ((CASE WHEN parent_group_id IS NULL THEN namespace_owner_id ELSE '' END), + coalesce(parent_group_id, ''), name) WHERE NOT (parent_group_id IS NULL AND name IN ('__dm__', '__linear__', '__coordination__')); -- The repair above leaves no '/' behind. This takes ACCESS EXCLUSIVE until the diff --git a/go/internal/store/queries/channels.sql b/go/internal/store/queries/channels.sql index 9d30b9943..56da3ac7e 100644 --- a/go/internal/store/queries/channels.sql +++ b/go/internal/store/queries/channels.sql @@ -12,7 +12,7 @@ -- anti-drift guarantee the design record requires). -- name: InsertChannelGroup :one --- An agent's group lives in its owner's namespace, so sibling names are unique per user. +-- An agent's group lives in its owner's namespace, which keys top-level names. INSERT INTO channel_groups (id, name, parent_group_id, owner_user_id, visibility, namespace_owner_id) VALUES ($1, $2, NULLIF($3, ''), $4, $5, COALESCE((SELECT owner_user_id FROM agent_accounts WHERE account_id = $4), $4)) diff --git a/go/internal/store/types.go b/go/internal/store/types.go index ae6895593..1c9649274 100644 --- a/go/internal/store/types.go +++ b/go/internal/store/types.go @@ -220,7 +220,7 @@ type ChannelGroup struct { // group. Server-set to the creating caller. OwnerUserID AccountID // NamespaceOwnerID is the user whose namespace holds the group: the creator, - // or an agent creator's owner. Sibling names are unique within it. + // or an agent creator's owner. Top-level names are unique within it. NamespaceOwnerID AccountID Visibility ChannelGroupVisibility } From 001d10680b74c418def2cd88049838057c280a19 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 6 Oct 2026 18:02:22 -0400 Subject: [PATCH 8/9] docs(proto): nested group names conflict under the parent (RIG-4651) Co-authored-by: Matt Wilkinson --- go/gen/compass/v1/comms.pb.go | 6 +++--- go/internal/store/channels_test.go | 4 ++-- packages/compass-agent/src/gen/compass/v1/comms_pb.ts | 6 +++--- packages/compass-client/src/gen/compass/v1/comms_pb.ts | 6 +++--- proto/compass/v1/comms.proto | 6 +++--- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/go/gen/compass/v1/comms.pb.go b/go/gen/compass/v1/comms.pb.go index 45fbd9e84..de1ce18cb 100644 --- a/go/gen/compass/v1/comms.pb.go +++ b/go/gen/compass/v1/comms.pb.go @@ -2732,9 +2732,9 @@ func (x *ListAccountsResponse) GetAccounts() []*Account { type CreateChannelGroupRequest struct { state protoimpl.MessageState `protogen:"open.v1"` - // Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name already - // used under the same parent in the same user's namespace (a user and that - // user's agents share one) is ALREADY_EXISTS. + // Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT. ALREADY_EXISTS + // for a nested name already used under the parent by anyone, or a top-level + // name already in the user's namespace (a user and their agents share one). Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` // Parent group; empty for a top-level group. ParentGroupId string `protobuf:"bytes,2,opt,name=parent_group_id,json=parentGroupId,proto3" json:"parent_group_id,omitempty"` diff --git a/go/internal/store/channels_test.go b/go/internal/store/channels_test.go index 9fa0b9f16..29505c546 100644 --- a/go/internal/store/channels_test.go +++ b/go/internal/store/channels_test.go @@ -12,8 +12,8 @@ import ( "testing" ) -// TestCreateChannelGroupSiblingNamesUnique: a name is unique among one owner's -// siblings, so agent tools can address it; other parents and owners may reuse it. +// TestCreateChannelGroupSiblingNamesUnique: a name is unique among its siblings, +// so agent tools can address it; another parent, or another owner at top level, may reuse it. func TestCreateChannelGroupSiblingNamesUnique(t *testing.T) { ctx := t.Context() s := newTestStore(t) diff --git a/packages/compass-agent/src/gen/compass/v1/comms_pb.ts b/packages/compass-agent/src/gen/compass/v1/comms_pb.ts index 7bdf7af8d..ad7b187da 100644 --- a/packages/compass-agent/src/gen/compass/v1/comms_pb.ts +++ b/packages/compass-agent/src/gen/compass/v1/comms_pb.ts @@ -1277,9 +1277,9 @@ export const ListAccountsResponseSchema: GenMessage = /*@_ */ export type CreateChannelGroupRequest = Message$1<"compass.v1.CreateChannelGroupRequest"> & { /** - * Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name already - * used under the same parent in the same user's namespace (a user and that - * user's agents share one) is ALREADY_EXISTS. + * Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT. ALREADY_EXISTS + * for a nested name already used under the parent by anyone, or a top-level + * name already in the user's namespace (a user and their agents share one). * * @generated from field: string name = 1; */ diff --git a/packages/compass-client/src/gen/compass/v1/comms_pb.ts b/packages/compass-client/src/gen/compass/v1/comms_pb.ts index 7bdf7af8d..ad7b187da 100644 --- a/packages/compass-client/src/gen/compass/v1/comms_pb.ts +++ b/packages/compass-client/src/gen/compass/v1/comms_pb.ts @@ -1277,9 +1277,9 @@ export const ListAccountsResponseSchema: GenMessage = /*@_ */ export type CreateChannelGroupRequest = Message$1<"compass.v1.CreateChannelGroupRequest"> & { /** - * Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name already - * used under the same parent in the same user's namespace (a user and that - * user's agents share one) is ALREADY_EXISTS. + * Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT. ALREADY_EXISTS + * for a nested name already used under the parent by anyone, or a top-level + * name already in the user's namespace (a user and their agents share one). * * @generated from field: string name = 1; */ diff --git a/proto/compass/v1/comms.proto b/proto/compass/v1/comms.proto index 5a98c91f1..f40ad8992 100644 --- a/proto/compass/v1/comms.proto +++ b/proto/compass/v1/comms.proto @@ -651,9 +651,9 @@ message ListAccountsResponse { } message CreateChannelGroupRequest { - // Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT; a name already - // used under the same parent in the same user's namespace (a user and that - // user's agents share one) is ALREADY_EXISTS. + // Leaf name of the group, e.g. "matt". '/' is INVALID_ARGUMENT. ALREADY_EXISTS + // for a nested name already used under the parent by anyone, or a top-level + // name already in the user's namespace (a user and their agents share one). string name = 1; // Parent group; empty for a top-level group. string parent_group_id = 2; From ae8ae974b4117921d28975b0d4f1dc47978065c5 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 6 Oct 2026 23:26:48 -0400 Subject: [PATCH 9/9] fix(store): renumber the migration after 0002_forge_scopes landed (RIG-3030) Co-authored-by: Matt Wilkinson --- ...{0002_channel_group_names.sql => 0003_channel_group_names.sql} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename go/internal/store/migrations/{0002_channel_group_names.sql => 0003_channel_group_names.sql} (100%) diff --git a/go/internal/store/migrations/0002_channel_group_names.sql b/go/internal/store/migrations/0003_channel_group_names.sql similarity index 100% rename from go/internal/store/migrations/0002_channel_group_names.sql rename to go/internal/store/migrations/0003_channel_group_names.sql