fix(#764, #775): run workspace package suites in CI; land the role-holders audit row

.github/workflows/ci.yml

@@ -131,6 +131,24 @@ jobs:
       - name: Test (node --test via tsx)
         run: npm run test
 
+      # #764 — the workspace packages carry their own test suites, and until
+      # this step NO workflow ran them: `npm run test` above globs only
+      # `middleware/test/**`. That was found the expensive way — #759 broke two
+      # conductor-core tests (a fixture warning plus a published-schema drift
+      # that would make consumers REJECT valid graphs) while every required
+      # check stayed green; a reviewer caught it by hand.
+      #
+      # `@omadia/plugin-ui-helpers` is deliberately absent: it declares a test
+      # script but contains zero test files, so including it fails on
+      # "no test files found" — and papering over that with --passWithNoTests
+      # would create a permanently-green no-op, the exact failure family this
+      # repo keeps finding (#640, #752). If it gains tests, add it here.
+      - name: Test (workspace package suites)
+        run: |
+          npm run test -w @omadia/canvas-core
+          npm run test -w @omadia/conductor-core
+          npm run test -w @omadia/plugin-api
+
       # `--test-timeout` is applied to the FILE, not to each leaf (#566): a
       # file whose leaves are all fast but whose total crosses the ceiling is
       # killed as a unit, and the failure is reported against whichever leaf

middleware/src/auth/adminAuditLog.ts

@@ -108,3 +108,35 @@ function rowToEntry(row: AuditRow): AuditEntry {
     createdAt: row.created_at,
   };
 }
+
+/**
+ * #775 — map a conductor role-holder change to an audit entry WITHOUT ever
+ * putting a non-uuid into `actor_id`.
+ *
+ * The bug this fixes: the index.ts closure passed the session sub (an email,
+ * e.g. `demo@byte5.de`) as `actor.id`; `admin_audit.actor_id` is a uuid
+ * column, so EVERY write failed — loudly in the log, but the audit trail for
+ * baton moves (the entire point of #759) stayed empty on any install using
+ * email subs, i.e. the normal local-auth case.
+ *
+ * Mapping: the uuid goes to `id` only when the session actually carried one;
+ * the sub always goes to `email` — same treatment the adminUsers routes give
+ * it, and for the `'operator'` fallback it is the only place the actor can be
+ * preserved at all (the column is free-text, like `target`).
+ */
+export function roleChangeAuditEntry(entry: {
+  actor: string;
+  actorUserId?: string;
+  roleKey: string;
+  action: 'add' | 'remove';
+  holderId: string;
+  holdersAfter: readonly string[];
+}): AuditEntryInput {
+  return {
+    actor: { id: entry.actorUserId, email: entry.actor },
+    action: 'conductor.role_holders_change',
+    target: `conductor-role:${entry.roleKey}`,
+    before: { action: entry.action, holderId: entry.holderId },
+    after: { holders: [...entry.holdersAfter] },
+  };
+}

middleware/src/conductor/routes.ts

@@ -98,6 +98,10 @@ export interface ConductorRouterDeps {
    */
   auditRoleChange?: (entry: {
     actor: string;
+    /** #775 — the session's omadia user uuid, when the session carries one.
+     *  `actor` above is the SUB (an email under local auth), which must never
+     *  be written to the uuid `admin_audit.actor_id` column. */
+    actorUserId?: string;
     roleKey: string;
     action: 'add' | 'remove';
     holderId: string;
@@ -330,6 +334,7 @@ export function createConductorRouter(deps: ConductorRouterDeps): Router {
         try {
           await deps.auditRoleChange({
             actor: req.session?.sub ?? 'operator',
+            actorUserId: req.session?.omadia_user_id,
             roleKey: key,
             action,
             holderId,

middleware/src/index.ts

@@ -238,7 +238,7 @@ import {
   EntraProvider,
 } from './auth/providers/EntraProvider.js';
 import { runAuthBootstrap } from './auth/bootstrap.js';
-import { AdminAuditLog } from './auth/adminAuditLog.js';
+import { AdminAuditLog, roleChangeAuditEntry } from './auth/adminAuditLog.js';
 import {
   PlatformSettingsStore,
   SETTING_AUTH_ACTIVE_PROVIDERS,
@@ -3424,13 +3424,10 @@ async function main(): Promise<void> {
       // instance: `adminAudit` is constructed further down this block, and the
       // route handler only dereferences it at request time.
       auditRoleChange: async (entry) => {
-        await adminAudit?.record({
-          actor: { id: entry.actor },
-          action: 'conductor.role_holders_change',
-          target: `conductor-role:${entry.roleKey}`,
-          before: { action: entry.action, holderId: entry.holderId },
-          after: { holders: entry.holdersAfter },
-        });
+        // #775 — the mapping lives in roleChangeAuditEntry so it is testable:
+        // the previous inline version put an EMAIL into the uuid actor_id
+        // column and every audit write failed.
+        await adminAudit?.record(roleChangeAuditEntry(entry));
       },
       webhooksEnabled: config.CONDUCTOR_WEBHOOKS_ENABLED,
       // #330 — guardrails for agent-generated ephemeral workflows (env-tunable).

middleware/test/conductorCancelAndStrictApproval.test.ts

@@ -313,7 +313,7 @@ describe('#759 routes: cancel endpoint + role-holder audit', () => {
     const app = express();
     app.use(express.json());
     app.use((req, _res, next) => {
-      (req as express.Request & { session?: unknown }).session = { sub: 'op-1' } as never;
+      (req as express.Request & { session?: unknown }).session = { sub: 'op-1', omadia_user_id: 'uuid-op-1' } as never;
       next();
     });
     const deps = {
@@ -384,6 +384,10 @@ describe('#759 routes: cancel endpoint + role-holder audit', () => {
     assert.equal(audited.length, 1);
     assert.deepEqual(audited[0], {
       actor: 'op-1',
+      // #775 — the session uuid travels alongside the sub, because the sub is
+      // an email under local auth and must never land in the uuid actor_id
+      // column. The harness session carries omadia_user_id for exactly this.
+      actorUserId: 'uuid-op-1',
       roleKey: 'approvers',
       action: 'add',
       holderId: 'op-1',

middleware/test/roleChangeAudit775.pg.test.ts

@@ -0,0 +1,88 @@
+/**
+ * #775 — the end-to-end proof at the layer where the bug actually lived.
+ *
+ * The defect was a DATABASE-level cast failure: an email in the uuid
+ * `actor_id` column made every `conductor.role_holders_change` insert throw,
+ * so the unit-level mapper tests alone cannot prove the fix — this suite runs
+ * `roleChangeAuditEntry` output through the real `AdminAuditLog.record`
+ * against the real `admin_audit` DDL (migration 0002, applied verbatim).
+ */
+import assert from 'node:assert/strict';
+import { readFileSync } from 'node:fs';
+import path from 'node:path';
+import { after, before, describe, it } from 'node:test';
+import { fileURLToPath } from 'node:url';
+
+import { Pool } from 'pg';
+
+import { probePgTest } from './_helpers/pgTestDb.js';
+
+import { AdminAuditLog, roleChangeAuditEntry } from '../src/auth/adminAuditLog.js';
+
+const { url: PG_URL, reachable: pgAvailable } = await probePgTest({
+  label: 'roleChangeAudit775',
+  vars: ['GRAPH_PG_TEST_URL', 'MEMORY_PG_TEST_URL', 'DATABASE_URL'],
+  timeoutMs: 1_500,
+});
+
+const HERE = path.dirname(fileURLToPath(import.meta.url));
+// The REAL DDL, not a copy: a hand-duplicated schema here could drift from
+// the migration and green-light an entry the production table rejects —
+// which is the exact failure class this suite exists to prevent.
+const SCHEMA = readFileSync(
+  path.join(HERE, '../src/auth/migrations/0002_admin_audit.sql'),
+  'utf8',
+);
+
+describe('#775 role-holders audit against real Postgres', { skip: !pgAvailable }, () => {
+  let pool: Pool;
+
+  before(async () => {
+    pool = new Pool({ connectionString: PG_URL });
+    await pool.query('DROP TABLE IF EXISTS admin_audit');
+    await pool.query(SCHEMA);
+  });
+
+  after(async () => {
+    await pool.query('DROP TABLE IF EXISTS admin_audit');
+    await pool.end();
+  });
+
+  it('an email-sub session lands a row (the exact case that used to throw)', async () => {
+    const log = new AdminAuditLog(pool);
+
+    await log.record(
+      roleChangeAuditEntry({
+        actor: 'demo@byte5.de',
+        roleKey: 'release-approver',
+        action: 'add',
+        holderId: 'user:u-2',
+        holdersAfter: ['user:u-1', 'user:u-2'],
+      }),
+    );
+
+    const rows = await pool.query(
+      "SELECT actor_id, actor_email, action, target FROM admin_audit WHERE action = 'conductor.role_holders_change'",
+    );
+    assert.equal(rows.rowCount, 1, 'the audit row must actually land');
+    assert.equal(rows.rows[0].actor_id, null);
+    assert.equal(rows.rows[0].actor_email, 'demo@byte5.de');
+    assert.equal(rows.rows[0].target, 'conductor-role:release-approver');
+  });
+
+  it('the OLD mapping is rejected by the real column — the regression stays impossible to miss', async () => {
+    // This is the mutation-check as a permanent test: anyone who reverts the
+    // closure to `actor: { id: entry.actor }` reproduces exactly this insert,
+    // and this asserts the database still refuses it.
+    const log = new AdminAuditLog(pool);
+    await assert.rejects(
+      () =>
+        log.record({
+          actor: { id: 'demo@byte5.de' },
+          action: 'conductor.role_holders_change',
+          target: 'conductor-role:x',
+        }),
+      /invalid input syntax for type uuid/,
+    );
+  });
+});

middleware/test/roleChangeAudit775.test.ts

@@ -0,0 +1,61 @@
+/**
+ * #775 — the conductor role-holders audit entry must actually land.
+ *
+ * The defect: `auditRoleChange` passed the session sub (an EMAIL under local
+ * auth) as `actor.id`; `admin_audit.actor_id` is a uuid column, so every
+ * write failed and the audit trail for baton moves — the whole point of
+ * #759 — stayed empty on the normal local-auth install. Loud in the log,
+ * invisible on the PR: no test drove the closure with an email sub.
+ */
+import assert from 'node:assert/strict';
+import { describe, test } from 'node:test';
+
+import { roleChangeAuditEntry } from '../src/auth/adminAuditLog.js';
+
+describe('#775 roleChangeAuditEntry', () => {
+  test('an email sub never lands in the uuid id slot', () => {
+    const entry = roleChangeAuditEntry({
+      actor: 'demo@byte5.de',
+      roleKey: 'release-approver',
+      action: 'add',
+      holderId: 'user:u-2',
+      holdersAfter: ['user:u-1', 'user:u-2'],
+    });
+
+    assert.equal(entry.actor.id, undefined, 'no uuid in the session -> id stays empty');
+    assert.equal(entry.actor.email, 'demo@byte5.de');
+    assert.equal(entry.action, 'conductor.role_holders_change');
+    assert.equal(entry.target, 'conductor-role:release-approver');
+  });
+
+  test('a session uuid is threaded to id, the sub still to email', () => {
+    const entry = roleChangeAuditEntry({
+      actor: 'demo@byte5.de',
+      actorUserId: '3f2c8d1e-0000-4000-8000-000000000001',
+      roleKey: 'release-approver',
+      action: 'remove',
+      holderId: 'user:u-2',
+      holdersAfter: ['user:u-1'],
+    });
+
+    assert.equal(entry.actor.id, '3f2c8d1e-0000-4000-8000-000000000001');
+    assert.equal(entry.actor.email, 'demo@byte5.de');
+  });
+
+  test('the operator fallback is preserved rather than dropped', () => {
+    // No session at all: the closure used to lose nothing here by accident
+    // (the string 'operator' also failed the uuid cast). The mapper keeps it
+    // in the free-text email column — an audit row with no actor at all
+    // would be worse than one with a non-address marker.
+    const entry = roleChangeAuditEntry({
+      actor: 'operator',
+      roleKey: 'r',
+      action: 'add',
+      holderId: 'user:u-1',
+      holdersAfter: ['user:u-1'],
+    });
+
+    assert.equal(entry.actor.id, undefined);
+    assert.equal(entry.actor.email, 'operator');
+  });
+});