test: the testing standard, an honest coverage measurement, and the two highest ranked gaps (#803, #813)

.github/workflows/ci.yml

@@ -426,6 +426,7 @@ jobs:
             # live database, and neither ran anywhere before: the package was
             # absent from this list, so the escalation those guards catch
             # would have shipped again with the suite reporting green.
+            # tools/lint-go-db-test-wiring.mjs fails if either is dropped.
             go test -tags integration -count=1 -p 1 -v ./internal/accounts/... ./internal/agenttask/... ./internal/egress/... ./internal/payments/... ./internal/profiles/... ./internal/rag/... ./internal/tenants/... ./internal/signup/... ./internal/routing/... ./internal/litellmconfig/... ./internal/catalog/... ./internal/providers/... ./internal/platform ./internal/platform/db/...
           else
             # -tags integration additionally compiles in
@@ -596,7 +597,7 @@ jobs:
 
       - name: Install repo-level dev deps
         if: needs.changes.outputs.run != 'false'
-        run: npm ci --ignore-scripts || npm install --ignore-scripts
+        run: npm ci --ignore-scripts
 
       # Merge-gate integrity: exactly one job may publish each required
       # check name, and every required context must have a producer.
@@ -638,6 +639,24 @@ jobs:
       - if: needs.changes.outputs.run != 'false'
         run: npm run lint:no-client-cost-fields
 
+      # Issue #813, spec wiring. Moved to web-unit (see that job): asking
+      # Playwright which projects a config declares needs the web console's
+      # own node_modules, which this dependency-light job deliberately does
+      # not install.
+
+      # Issue #659, #797. A Go package carrying a database-gated test skips
+      # silently when it is absent from the RLS step's package list, which is
+      # how the platform package went from never running to green in one line.
+      # Fixture regression test: two matrix legs, one shell `if` with no
+      # `else` wrapping a single go test line, so only one leg's package is
+      # actually reached. The other leg's DSN-gated package must be reported
+      # unpaired, not credited via the directory the wrapped line shares with
+      # every leg regardless of the branch that selects it.
+      - if: needs.changes.outputs.run != 'false'
+        run: npm run lint:go-db-test-wiring:test
+      - if: needs.changes.outputs.run != 'false'
+        run: npm run lint:go-db-test-wiring
+
       # Self-checks for the provisioning scripts (pure python, no network, no
       # framework). Wired into CI here because nothing ran them before: the
       # `make test-scripts` target existed but no workflow invoked it, so every
@@ -719,6 +738,36 @@ jobs:
         if: needs.changes.outputs.run != 'false'
         run: npm run e2e:verify-collection
 
+      # Issue #813, camouflage shape 14. A Playwright spec that no workflow
+      # runs emits no signal at all, and one that only a nightly runs
+      # protects nothing on the merge path. Moved here, not left in
+      # repo-policy-lints: it asks Playwright directly which projects a
+      # config declares (see tools/verify-spec-wiring.mjs's projectsOf),
+      # rather than trusting a hand-maintained copy that went stale under
+      # PR #799 and produced a false positive. That needs this job's own
+      # node_modules (Playwright installed), which repo-policy-lints
+      # deliberately does not carry. Fixture regression test for the
+      # event-condition classifier runs first, in a check that names the
+      # case, rather than as a silently wrong count in the guard below.
+      # tools/verify-spec-wiring.mjs parses workflow YAML with the root
+      # devDependency 'yaml'. This job's only other install is the
+      # apps/web-console one above, set by defaults.run.working-directory, so
+      # without this the guard dies on ERR_MODULE_NOT_FOUND. The sibling
+      # root-scoped steps below survive because they import nothing.
+      - name: Install repo-level dev deps
+        if: needs.changes.outputs.run != 'false'
+        working-directory: .
+        run: npm ci --ignore-scripts
+
+      - name: Spec wiring guard test
+        if: needs.changes.outputs.run != 'false'
+        working-directory: .
+        run: npm run lint:spec-wiring:test
+      - name: Spec wiring guard
+        if: needs.changes.outputs.run != 'false'
+        working-directory: .
+        run: npm run lint:spec-wiring
+
       - name: Unit tests (vitest)
         if: needs.changes.outputs.run != 'false'
         run: npm run test:unit

.gitignore

@@ -31,6 +31,9 @@ package-lock.json
 # apps/desktop predate that rule and are already tracked, which is why they
 # need no negation here.
 !/apps/agent-console/package-lock.json
+# The repo root is one of those: repo-policy-lints and the web-console job
+# both install root dev deps with `npm ci` before running tools/*.mjs.
+!/package-lock.json
 
 # Rust (apps/desktop-sandbox) — Cargo.lock is committed for reproducible
 # builds of the codex-bwrap binary; only the build output is ignored.

apps/control-plane/internal/platform/membership_role_rls_test.go

@@ -0,0 +1,222 @@
+package platform_test
+
+import (
+	"context"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/jackc/pgx/v5/pgxpool"
+
+	"github.com/sakibsadmanshajib/hive/apps/control-plane/internal/platform"
+)
+
+// IsWorkspaceOwner is the predicate behind PermBillingWrite on
+// PUT /api/v1/budgets/{ws} and POST /api/v1/spend-alerts/{ws}. PR #768 mounted
+// both of those routes, so this predicate now sits on a money path.
+//
+// The unit tests for it stub RoleStore, which proves the service maps the
+// owner role to true and says nothing about the SQL underneath. The SQL is
+// where issue #803 lives: GetMembershipRole selected role without constraining
+// account_memberships.status, so a row with role=owner and status=invited
+// passed the owner check on a billing write.
+//
+// These tests connect on an unscoped pool rather than SET ROLE hive_app, unlike
+// the tenant_users suite in role_rls_test.go. hive_app holds no grant on
+// public.accounts or public.account_memberships, so this account-scoped path
+// does not run as that role. The defect under test is the predicate, not RLS.
+
+const (
+	skipNoTestDSN  = "HIVE_TEST_DB_URL not set"
+	errNotTestDSN  = "refusing to run: HIVE_TEST_DB_URL must name a database whose name carries the test marker"
+	errParseDSN    = "parse HIVE_TEST_DB_URL: %v"
+	errConnect     = "connect: %v"
+	errSeedPool    = "seed pool: %v"
+	errSeedUser    = "seed auth user: %v"
+	errSeedAccount = "seed account: %v"
+	errSeedMember  = "seed membership: %v"
+	errOwnerCall   = "IsWorkspaceOwner: %v"
+)
+
+// The marker is checked on the parsed database name, not on the whole DSN.
+// Searching the raw string matches a host, a user, a password or a query
+// parameter that happens to contain the substring, and this suite inserts and
+// deletes rows, so a loose check is a licence to write to whatever it was
+// pointed at.
+func requireAccountRoleTestDSN(t *testing.T) string {
+	t.Helper()
+	dsn := os.Getenv("HIVE_TEST_DB_URL")
+	if dsn == "" {
+		t.Skip(skipNoTestDSN)
+	}
+	cfg, err := pgxpool.ParseConfig(dsn)
+	if err != nil {
+		t.Fatalf(errParseDSN, err)
+	}
+	if !strings.Contains(strings.ToLower(cfg.ConnConfig.Database), "test") {
+		t.Fatal(errNotTestDSN)
+	}
+	return dsn
+}
+
+func newAccountRoleTestPool(t *testing.T) *pgxpool.Pool {
+	t.Helper()
+	cfg, err := pgxpool.ParseConfig(requireAccountRoleTestDSN(t))
+	if err != nil {
+		t.Fatalf(errParseDSN, err)
+	}
+	cfg.MaxConns = 1
+
+	pool, err := pgxpool.NewWithConfig(context.Background(), cfg)
+	if err != nil {
+		t.Fatalf(errConnect, err)
+	}
+	t.Cleanup(pool.Close)
+	return pool
+}
+
+// seedSubjectAccountMembership inserts the subject user, a separate user who created
+// and owns the account, the account itself, and one account_memberships row
+// putting the subject on that account with the given role and status.
+//
+// The creator is a separate user deliberately. An earlier version of this
+// fixture set accounts.owner_user_id to the subject and then inserted the same
+// subject as status='invited', so the two rows contradicted each other: the
+// account said the user owned it outright while the membership said the
+// invitation was still outstanding. A test asserting "not an owner" against a
+// row that also says "owner" proves nothing about which of the two the
+// predicate reads. Here every case is one shape, a user added to somebody
+// else's workspace, and the only variable is the role and status on the
+// membership row, which is the predicate under test.
+func seedSubjectAccountMembership(t *testing.T, accountID, userID uuid.UUID, role, status string) {
+	t.Helper()
+	dsn := requireAccountRoleTestDSN(t)
+	ctx := context.Background()
+
+	setup, err := pgxpool.New(ctx, dsn)
+	if err != nil {
+		t.Fatalf(errSeedPool, err)
+	}
+	defer setup.Close()
+
+	creatorID := uuid.New()
+	for _, seeded := range []uuid.UUID{creatorID, userID} {
+		if _, err := setup.Exec(ctx, insertUserSQL, seeded,
+			"membership-role-"+seeded.String()+"@hive-test.invalid"); err != nil {
+			t.Fatalf(errSeedUser, err)
+		}
+	}
+	if _, err := setup.Exec(ctx, insertAccountSQL, accountID,
+		"membership-role-"+accountID.String(), creatorID); err != nil {
+		t.Fatalf(errSeedAccount, err)
+	}
+	if _, err := setup.Exec(ctx, insertMembershipSQL, accountID, userID, role, status); err != nil {
+		t.Fatalf(errSeedMember, err)
+	}
+
+	t.Cleanup(func() {
+		cleanup, err := pgxpool.New(context.Background(), dsn)
+		if err != nil {
+			return
+		}
+		defer cleanup.Close()
+		// account_memberships cascades from accounts, and accounts.owner_user_id
+		// references auth.users, so the account row goes first.
+		_, _ = cleanup.Exec(context.Background(), dropAccountSQL, accountID)
+		_, _ = cleanup.Exec(context.Background(), dropUserSQL, userID)
+		_, _ = cleanup.Exec(context.Background(), dropUserSQL, creatorID)
+	})
+}
+
+const (
+	insertUserSQL = `INSERT INTO auth.users (id, email) VALUES ($1, $2)
+		ON CONFLICT (id) DO NOTHING`
+
+	insertAccountSQL = `INSERT INTO public.accounts
+		(id, slug, display_name, account_type, owner_user_id)
+		VALUES ($1, $2, 'membership role test', 'business', $3)
+		ON CONFLICT (id) DO NOTHING`
+
+	insertMembershipSQL = `INSERT INTO public.account_memberships
+		(account_id, user_id, role, status)
+		VALUES ($1, $2, $3, $4)
+		ON CONFLICT (account_id, user_id)
+		DO UPDATE SET role = EXCLUDED.role, status = EXCLUDED.status`
+
+	dropAccountSQL = `DELETE FROM public.accounts WHERE id = $1`
+	dropUserSQL    = `DELETE FROM auth.users WHERE id = $1`
+)
+
+// Positive control. Without it, a GetMembershipRole that returned the empty
+// role for every input would pass every negative case below, and the suite
+// would report that a completely broken predicate is safe.
+func TestIsWorkspaceOwner_ActiveOwnerIsOwner(t *testing.T) {
+	pool := newAccountRoleTestPool(t)
+	accountID, userID := uuid.New(), uuid.New()
+	seedSubjectAccountMembership(t, accountID, userID, "owner", "active")
+
+	svc := platform.NewRoleService(platform.NewPgxRoleStore(pool))
+	isOwner, err := svc.IsWorkspaceOwner(context.Background(), userID, accountID)
+	if err != nil {
+		t.Fatalf(errOwnerCall, err)
+	}
+	if !isOwner {
+		t.Fatal("an active owner membership must be recognized as workspace owner")
+	}
+}
+
+// Issue #803. account_memberships.status is constrained to active or invited
+// today, so invited is the reachable non-active value: an owner who was invited
+// to a workspace and has not accepted. That row must not authorize a billing
+// write on PUT /api/v1/budgets/{ws} or POST /api/v1/spend-alerts/{ws}.
+func TestIsWorkspaceOwner_NonActiveOwnerIsNotOwner(t *testing.T) {
+	pool := newAccountRoleTestPool(t)
+	svc := platform.NewRoleService(platform.NewPgxRoleStore(pool))
+	ctx := context.Background()
+
+	cases := []struct {
+		name   string
+		role   string
+		status string
+	}{
+		{"invited owner is not owner", "owner", "invited"},
+		{"active member is not owner", "member", "active"},
+		{"invited member is not owner", "member", "invited"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			accountID, userID := uuid.New(), uuid.New()
+			seedSubjectAccountMembership(t, accountID, userID, tc.role, tc.status)
+
+			isOwner, err := svc.IsWorkspaceOwner(ctx, userID, accountID)
+			if err != nil {
+				t.Fatalf(errOwnerCall, err)
+			}
+			if isOwner {
+				t.Fatalf(errNotOwnerFmt, tc.role, tc.status)
+			}
+		})
+	}
+}
+
+const errNotOwnerFmt = "role=%s status=%s must not authorize a workspace-owner operation"
+
+// A membership row in another workspace must not carry over. This is the
+// account-scoped twin of the foreign-tenant case in role_rls_test.go.
+func TestIsWorkspaceOwner_ForeignWorkspaceMembershipDoesNotCarry(t *testing.T) {
+	pool := newAccountRoleTestPool(t)
+	accountA, accountB := uuid.New(), uuid.New()
+	userID := uuid.New()
+	seedSubjectAccountMembership(t, accountA, userID, "owner", "active")
+	seedSubjectAccountMembership(t, accountB, uuid.New(), "owner", "active")
+
+	svc := platform.NewRoleService(platform.NewPgxRoleStore(pool))
+	isOwner, err := svc.IsWorkspaceOwner(context.Background(), userID, accountB)
+	if err != nil {
+		t.Fatalf(errOwnerCall, err)
+	}
+	if isOwner {
+		t.Fatal("owning workspace A must not report ownership of workspace B")
+	}
+}

apps/web-console/playwright-spec-manifest.json

@@ -14,7 +14,17 @@
     "and e2e/chat-coverage/playwright.chat-coverage.config.ts.",
     "The owui projects and the live chat-coverage project gate their testMatch on credentials,",
     "so the guard lists those configs with placeholders to keep collection deterministic",
-    "everywhere."
+    "everywhere.",
+    "",
+    "There used to be a `configs` object here, pinning each config to the projects it declares.",
+    "It was a hand-maintained copy that this doc claimed was 'verified by the same guard from the",
+    "same listing', which was not true: nothing here re-derived it, so it went stale the moment a",
+    "config's projects changed anywhere but this file. PR #799 added a project to",
+    "playwright.config.ts while a sibling branch's copy of this file was mid-flight, and the",
+    "guard blamed the workflow that correctly selected the new project instead of its own stale",
+    "copy. tools/verify-spec-wiring.mjs now asks Playwright directly which projects a config",
+    "declares (an unfindable --project name always names every real one in its own error), so",
+    "there is nothing here left to go stale."
   ],
   "specs": {
     "e2e/chat-coverage/auth.setup.ts": ["chat-coverage-setup"],

apps/web-console/scripts/verify-spec-collection.mjs

@@ -96,8 +96,12 @@ const CONFIGS = [
 
 // Where Playwright test modules live. Anything matching TEST_FILE under these
 // is expected to be collected by at least one project.
+// `.test.ts` is here because the chromium project sets a testDir and no
+// testMatch, so Playwright's default pattern collects that suffix too. Walking
+// for `.spec.ts` alone would leave a file the runner does load invisible to the
+// DARK check below.
 const TEST_ROOTS = ["e2e", "tests/e2e"];
-const TEST_FILE = /\.(spec|setup)\.ts$/;
+const TEST_FILE = /\.(spec|test|setup)\.ts$/;
 
 function listFilesOnDisk() {
   const found = [];
@@ -194,18 +198,29 @@ function listOneConfig({ label, args, env }) {
 }
 
 function main() {
-  const manifest = JSON.parse(readFileSync(MANIFEST, "utf8")).specs;
+  const document = JSON.parse(readFileSync(MANIFEST, "utf8"));
+  const manifest = document.specs;
 
   const collected = new Map();
+  const problems = [];
+
+  // tools/verify-spec-wiring.mjs used to resolve a workflow's `--config` and
+  // `--project` arguments through a `configs` object pinned here from this
+  // same listing. That was a hand-maintained copy of what this loop already
+  // computes below, and it drifted (issue: PR #799 added a project to
+  // playwright.config.ts while a sibling branch's copy of this file was
+  // mid-flight, and the wiring guard blamed the workflow instead of its own
+  // stale copy). It now asks Playwright directly, the same way this loop
+  // does, so there is nothing left to pin or compare here.
   for (const config of CONFIGS) {
-    for (const [file, projects] of listOneConfig(config)) {
+    const byFile = listOneConfig(config);
+    for (const [file, fileProjects] of byFile) {
       const merged = collected.get(file) ?? new Set();
-      for (const project of projects) merged.add(project);
+      for (const project of fileProjects) merged.add(project);
       collected.set(file, merged);
     }
   }
 
-  const problems = [];
   const asLine = (file, projects) =>
     `    ${JSON.stringify(file)}: ${JSON.stringify([...projects].sort())}`;