From e8dd184c2f47dc1778eaf6975335030772c4c8f6 Mon Sep 17 00:00:00 2001 From: Marcel Wege Date: Fri, 21 Aug 2026 09:47:51 +0200 Subject: [PATCH] fix(#764, #775): run workspace package suites in CI; land the role-holders audit row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the same review wave, one branch because both are small and sharp. #764 — CI never ran the workspace packages' own test suites. `npm run test` globs only `middleware/test/**`; canvas-core (vitest), conductor-core (vitest) and plugin-api (node:test) ran nowhere. Not hypothetical twice over: #759 broke two conductor-core tests while every required check stayed green, and #725's 15 canvas-core validator tests were only ever run by hand during review. New CI step runs all three. `@omadia/plugin-ui-helpers` is deliberately absent: it declares a test script but contains zero test files, 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 to the step. #775 — the `conductor.role_holders_change` audit entry never landed: the index.ts closure passed the session sub (an EMAIL under local auth) as `actor.id`, and `admin_audit.actor_id` is a uuid column, so every insert threw. Loud in the log, empty in the audit trail — the entire point of #759. Fix: the conductor entry additionally threads the session's `omadia_user_id` (a real uuid), and the mapping lives in an exported `roleChangeAuditEntry` (adminAuditLog.ts): uuid to `actor_id` only when the session carries one, the sub always to the free-text `actor_email` — the same treatment the adminUsers routes give it, and for the `'operator'` fallback the only place the actor survives at all. Tests: 3 mapper units; the existing route test now proves the uuid threading end-to-end (harness session carries omadia_user_id, deep-equal asserts it); and a pg-gated suite runs the mapper output through the real `AdminAuditLog.record` against the REAL migration DDL (read from the migration file, not copied — a hand-duplicated schema could drift and green-light what production rejects). Its second case pins the regression permanently: the OLD mapping must keep failing on the real column with `invalid input syntax for type uuid`. Mutation checks: removing the uuid threading turns the route test red; reverting the closure to the old inline mapping reproduces exactly the insert the pg test asserts the database refuses. Verified against an ephemeral postgres:16-alpine (2/2). Full suite: 7048 tests, 1 pre-existing-shape failure fixed by extending the existing expectation (the new field), then green; typecheck:test ratchet held with no regressions; lint clean. --- .github/workflows/ci.yml | 18 ++++ middleware/package-lock.json | 2 +- middleware/src/auth/adminAuditLog.ts | 32 +++++++ middleware/src/conductor/routes.ts | 5 ++ middleware/src/index.ts | 13 ++- .../conductorCancelAndStrictApproval.test.ts | 6 +- middleware/test/roleChangeAudit775.pg.test.ts | 88 +++++++++++++++++++ middleware/test/roleChangeAudit775.test.ts | 61 +++++++++++++ 8 files changed, 215 insertions(+), 10 deletions(-) create mode 100644 middleware/test/roleChangeAudit775.pg.test.ts create mode 100644 middleware/test/roleChangeAudit775.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2974c8284..92cb4fa6f 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/middleware/package-lock.json b/middleware/package-lock.json index 9f0883764..fb6432a10 100644 --- a/middleware/package-lock.json +++ b/middleware/package-lock.json @@ -9525,7 +9525,7 @@ }, "packages/plugin-api": { "name": "@omadia/plugin-api", - "version": "1.5.0", + "version": "1.6.0", "license": "MIT", "engines": { "node": ">=20" diff --git a/middleware/src/auth/adminAuditLog.ts b/middleware/src/auth/adminAuditLog.ts index f7f9f12cd..a55567a31 100644 --- a/middleware/src/auth/adminAuditLog.ts +++ b/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] }, + }; +} diff --git a/middleware/src/conductor/routes.ts b/middleware/src/conductor/routes.ts index 60690c798..547bead4d 100644 --- a/middleware/src/conductor/routes.ts +++ b/middleware/src/conductor/routes.ts @@ -97,6 +97,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; @@ -323,6 +327,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, diff --git a/middleware/src/index.ts b/middleware/src/index.ts index 84464d5f0..d326ab6fc 100644 --- a/middleware/src/index.ts +++ b/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 { // 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, webhookInboundMaxPerMinute: config.CONDUCTOR_WEBHOOK_MAX_DELIVERIES_PER_MINUTE, diff --git a/middleware/test/conductorCancelAndStrictApproval.test.ts b/middleware/test/conductorCancelAndStrictApproval.test.ts index 08f7bfdac..44278929d 100644 --- a/middleware/test/conductorCancelAndStrictApproval.test.ts +++ b/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', diff --git a/middleware/test/roleChangeAudit775.pg.test.ts b/middleware/test/roleChangeAudit775.pg.test.ts new file mode 100644 index 000000000..df9cca839 --- /dev/null +++ b/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/, + ); + }); +}); diff --git a/middleware/test/roleChangeAudit775.test.ts b/middleware/test/roleChangeAudit775.test.ts new file mode 100644 index 000000000..afad2f1cc --- /dev/null +++ b/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'); + }); +});