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/docs/specs/product/compass.md b/docs/specs/product/compass.md index 092b33897..8aea03ae2 100644 --- a/docs/specs/product/compass.md +++ b/docs/specs/product/compass.md @@ -591,17 +591,25 @@ 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 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 @@ -609,6 +617,27 @@ 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 + +#### 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/gen/compass/v1/comms.pb.go b/go/gen/compass/v1/comms.pb.go index 8b1a56686..de1ce18cb 100644 --- a/go/gen/compass/v1/comms.pb.go +++ b/go/gen/compass/v1/comms.pb.go @@ -2732,15 +2732,19 @@ 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. 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"` 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 @@ -3045,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 new file mode 100644 index 000000000..8e3a35c51 --- /dev/null +++ b/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 +} 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 73ab5c47b..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) @@ -61,24 +65,29 @@ 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) + } return ChannelGroup{}, fmt.Errorf("store: insert channel group: %w", err) } if err := tx.Commit(ctx); err != nil { 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 } @@ -248,11 +257,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 @@ -306,25 +316,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 { @@ -337,23 +363,33 @@ 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%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], "~") { + // 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) + } + } 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 { @@ -366,7 +402,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%s", + ErrInvalidArgument, ref, groupRefHints(groups, handles, matches)) } for _, group := range candidates { return group, nil @@ -374,6 +415,60 @@ 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. 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 { + 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 + } + 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 + } + 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) + } + } + if len(hints) == 0 { + return "" + } + slices.Sort(hints) + hints = slices.Compact(hints) + 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 // 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 0f9700bd5..29505c546 100644 --- a/go/internal/store/channels_test.go +++ b/go/internal/store/channels_test.go @@ -12,6 +12,192 @@ import ( "testing" ) +// 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) + 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) + } +} + +// 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) + } + } +} + +// 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) + } + } +} + +// 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") + 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) + } + 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) + } + _, 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) + } + } +} + 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/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/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 e208f0428..fbb0f45a9 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)) ) ` @@ -334,6 +335,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 +409,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 +437,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, which keys top-level names. +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,19 +470,21 @@ 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)) +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 ` 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 +502,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..8f5588ec2 100644 --- a/go/internal/store/db/querier.go +++ b/go/internal/store/db/querier.go @@ -229,10 +229,11 @@ 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) - // 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) @@ -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, 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) InsertCoordinationGroup(ctx context.Context, arg InsertCoordinationGroupParams) error 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/group_ref_test.go b/go/internal/store/group_ref_test.go index a155831ed..19b138385 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,31 @@ 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}, + {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 { 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 +83,81 @@ 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) + } + } + } +} + +// 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/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/0003_channel_group_names.sql b/go/internal/store/migrations/0003_channel_group_names.sql new file mode 100644 index 000000000..8fc83ccf0 --- /dev/null +++ b/go/internal/store/migrations/0003_channel_group_names.sql @@ -0,0 +1,107 @@ +-- Agent tools read '/' as a path separator and address a group by sibling name, +-- 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'; + +-- 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 +-- compass_system. +SET LOCAL ROLE compass_system; +-- 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[]; + duplicate RECORD; + candidate TEXT; + suffix INTEGER; +BEGIN + 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 '%/%' + RETURNING id + ) + SELECT coalesce(array_agg(id), '{}') INTO rewritten_ids FROM rewritten; + + FOR duplicate IN + SELECT id, name_scope, parent_group_id, name, duplicate_number + FROM ( + 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 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 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 + 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 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 + ); + 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; + +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 ((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 +-- 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); + +-- 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 971ef9da1..56da3ac7e 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, 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)) +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,10 +102,11 @@ 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)) +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 @@ -123,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 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..1c9649274 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. Top-level 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 8ffce239f..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,7 +1277,9 @@ 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. 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; */ @@ -1296,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; */ @@ -1453,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 8ffce239f..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,7 +1277,9 @@ 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. 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; */ @@ -1296,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; */ @@ -1453,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 3955c6e4d..f40ad8992 100644 --- a/proto/compass/v1/comms.proto +++ b/proto/compass/v1/comms.proto @@ -651,15 +651,19 @@ message ListAccountsResponse { } message CreateChannelGroupRequest { - // Leaf name of the group, e.g. "matt". + // 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; 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; } @@ -703,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; }