fix(#764, #775): run workspace package suites in CI; land the role-holders audit row - #820
Merged
Conversation
…lders audit row 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.
This was referenced Aug 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #764. Closes #775. Two findings from the same review wave — both small, both sharp, one branch.
#764 — three test suites ran nowhere
npm run testin CI globs onlymiddleware/test/**. The workspace packages' own suites — canvas-core (vitest), conductor-core (vitest, 99 tests), plugin-api (node:test) — ran in no workflow at all. Twice proven non-hypothetical:New step in the middleware job runs all three.
@omadia/plugin-ui-helpersis deliberately absent and documented in the step comment: it declares atestscript but contains zero test files, so including it fails on "no test files found" — and papering over that with--passWithNoTestswould create a permanently-green no-op, the exact failure family this repo keeps finding (#640 guard-skip, #752 absent ratchet). If it gains tests, add it.#775 — the baton-move audit trail was empty
The
auditRoleChangeclosure passed the session sub (an email under local auth) asactor.id;admin_audit.actor_idis a uuid column, so every insert threw. Loud in the log, invisible in the trail — the entire point of #759.Fix, per the issue's own sketch:
req.session.omadia_user_id(a real uuid) asactorUserId;roleChangeAuditEntry()inadminAuditLog.ts: uuid →actor_idonly when the session carries one; the sub always → the free-textactor_email(theadminUsersroutes' precedent, and for the'operator'fallback the only place the actor survives at all);Proof, at the layer where the bug lived
The defect was a database-level cast failure, so unit tests alone cannot prove the fix:
omadia_user_id, and the deep-equal asserts it arrives in the entry.roleChangeAudit775.pg.test.ts) runs the mapper output through the realAdminAuditLog.recordagainst the real migration DDL — read fromsrc/auth/migrations/0002_admin_audit.sql, not copied, because a hand-duplicated schema could drift and green-light what production rejects. Verified 2/2 against an ephemeralpostgres:16-alpine.invalid input syntax for type uuid. That is the mutation check promoted to a test — anyone reverting the closure reproduces exactly that insert.Mutation evidence
Verification
Full middleware suite 7048 tests / 0 fail after extending the one existing expectation that legitimately gained the new field;
typecheck:testratchet: no regressions; lint clean. Blast radius: one CI step (additive), one closure inindex.ts(now one line), one additive entry field inconductor/routes.ts, one exported mapper + tests.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.