feat(store): unique sibling channel-group names with a repairing migration (RIG-3030)

.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,

docs/specs/product/compass.md

@@ -591,24 +591,53 @@ 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 `/~<handle>/` 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 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
 
 - **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
 
 - **Given** two visible groups named `svc` under different parent groups
 - **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 `/~<other-handle>/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
+
+#### 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

go/internal/store/channel_group_names_migration_pgtest_test.go

@@ -0,0 +1,195 @@
+//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 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),
+			('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),
+			('group-n', '__dm__', 'owner-e', 'boot', 'group-i'),
+			('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'),
+			('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 {
+		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
+		"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
+		"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 {
+			t.Errorf("group %s name = %q, want %q", id, got[id], name)
+		}
+	}
+
+	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()
+			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()
+	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)
+	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, "+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, value string
+		if err := rows.Scan(&id, &value); err != nil {
+			t.Fatalf("scan repaired group: %v", err)
+		}
+		got[id] = value
+	}
+	if err := rows.Err(); err != nil {
+		t.Fatalf("iterate repaired groups: %v", err)
+	}
+	return got
+}

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 {