fix(ci): the decoupling ratchet was timing out before it ran - #752
Conversation
`core decoupling ratchet (#470)` reported `cancelled` on two PRs in a row, which reads like a transient. It was not. Measured on the job's own step timings: Ensure ripgrep 15:35:47 -> 15:40:54 (5:07) cancelled Check core is ... skipped The job's `timeout-minutes: 5` fired inside the install step, so the step that does the actual checking never executed. On the run that DID pass, the same install took 3:28 — the job routinely had about ninety seconds of headroom. A timed-out job reports as `cancelled` and skips its remaining steps, so a REQUIRED check was silently not running: not red, not passing, just absent. Same failure family as a permanently-green guard-skip. Two changes: - `apt-get update` is the expensive half. It refetches every package index to install one small binary. Try the cached indices first and only refresh when that actually fails; worst case costs a few seconds more than today. `rg` is genuinely required — `check-core-decoupling.mjs` shells out to it via `execFileSync('rg', …)`, so the step cannot simply be dropped. - Timeout 5 -> 10, so a slow mirror cannot take out a required check. The guard is an explicit `if` rather than `cmd && exit 0`: whether `set -e` aborts on the first half of an `&&` list is exactly the kind of shell subtlety that produced a silent CI failure in this repo before. Both branches verified under `bash -e`.
|
Verified on this PR's own run — which was the stated test.
Skipping Worth noting the shape of this one, because it is the third time this pattern has cost time in this repo: the gate was not red and not green — it was absent. #750 and #751 both still carry a |
…lders audit row (#820) 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.
core decoupling ratchet (#470)reportedcancelledon two PRs in a row. That reads like a transient — a push cancelling an in-flight run. It was not: when a push cancels a run, every job is cancelled, and here the siblings all succeeded on the same SHA while only this one died.What actually happened
From the job's own step timings:
The job's
timeout-minutes: 5fired inside the install step, so the step that does the actual checking never executed.And the run that passed was not comfortable either — the same install took 3:28 there. The job routinely had about ninety seconds of headroom, which is why this surfaces as an intermittent rather than a permanent failure.
A timed-out job reports as
cancelledand skips its remaining steps. So a required check was silently not running: not red, not passing, just absent. That is the same failure family as a permanently-green guard-skip — the gate is gone and nothing about the PR page says so.The fix
apt-get updateis the expensive half. It refetches every package index in order to install one small binary. The step now tries the cached indices first and only refreshes when that actually fails; worst case costs a few seconds more than today, best case skips minutes.rgis genuinely required and the step cannot just be deleted —scripts/check-core-decoupling.mjsshells out to it (execFileSync('rg', args, …)at line 119).Timeout 5 → 10, so a slow package mirror cannot take out a required check. The install is the only unbounded thing in the job; the check itself is a single
rgover the repo.One deliberate detail
The guard is an explicit
if:not
command -v rg >/dev/null && exit 0. Whetherset -eaborts on the first half of an&&list is precisely the kind of shell subtlety that has produced a silent CI failure in this repo before (theprintf | grep -qEPIPE inversion that left the release pipeline dead for a month). Both branches were verified underbash -e.Verification
The real verification is this PR's own run: if the ratchet check comes back
successrather thancancelled, the step completed and the check executed. Worth glancing at its duration too — the point is headroom, not just a pass.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.