Skip to content

Commit 7e74af3

Browse files
os-muskclaude
andauthored
test(metadata-protocol): execute the read-probe PostgreSQL arm on a live server (#17621) (#17890)
Fixes #17621 Clause-②: no #17175 compiled one non-raising catalog presence arm per dialect family in `packages/metadata-protocol/src/migrations/read-probe.ts` and executed two of the three: SQLite end to end through a real `SqlDriver`, MySQL on the live server. The **PostgreSQL** arm — `SELECT 1 WHERE to_regclass('"TABLE"') IS NOT NULL` — was pinned character-for-character against all four knex client spellings and run **nowhere**: this package had no live-PG harness, no `pg` dependency, and its CI step supplied `OS_TEST_MYSQL_URL` alone while filtering vitest to `live-mysql`. A text pin cannot close that gap. The failure this module is fenced against is an arm mis-compiled for one dialect: it raises, the `catch` that exists for the expected miss swallows it, and a stored-row data repair silently becomes a no-op. Whether `to_regclass` answers **zero rows** rather than raising is a claim about PostgreSQL, not about this repo's string concatenation. ## The three changes | # | change | note | |:--|:--|:--| | 1 | `pg` devDependency on `packages/metadata-protocol` | a lockfile move — measured below | | 2 | `src/migrations/seed-tenancy-backfill.live-postgres.test.ts` | sibling of the live-MySQL file, in its own derived schema | | 3 | `OS_TEST_POSTGRES_URL` + a second vitest substring on that CI step | wiring: the `postgres:16` service is already up in that job | Two riders, both consequences of the above rather than scope: - `@types/pg` devDependency — `pg` ships no typings, and this package's `typecheck` target reaches every `*.test.ts` by rule, so without it `tsc --noEmit` fails `TS7016`. **Measured**: it adds exactly one lockfile package entry; its three dependencies (`@types/node`, `pg-protocol`, `pg-types`) were already resolved. Dev-only, types-only, no runtime code. - `read-probe.test.ts`'s docblock recorded the Postgres arm as "NOT MEASURED against a live server anywhere", naming the three reasons this PR removes. That sentence is now false, so it names where each arm runs instead. ## Was (1) only a lockfile move? — triage's stop condition **Yes.** Adding `pg: ^8.22.0` and re-running `pnpm install` produced a **three-line** lockfile diff, and the three lines are the importer entry: ``` + pg: + specifier: ^8.22.0 + version: 8.22.0 ``` Zero new packages, zero new transitives: `pg@8.22.0` was already resolved for `packages/drivers/driver-sql`, which declares it as an **optional peerDependency** (`pg`, `mysql2`, `tedious` under `peerDependenciesMeta.optional`). The stop condition as triage set it — `pg` pulling something unexpected into the install graph — did not fire. The one package that **is** new is `@types/pg`, named above; it is not pulled by `pg`, it is required by change (2) plus the repo's own type-check-coverage rule, and it is reported here rather than folded in silently. ## Changeset: `patch`, measured — this falsifies the dispatching seat's expectation The seat's expectation was `skip-changeset`, explicitly flagged as an expectation and not a reading. Taken: - **`dist/` is byte-untouched.** Built the package, grepped `dist/` for this change's markers (`os17621_case`, `live-postgres`, `toPgPlaceholders`): **zero hits**, against a positive control (`readTablePresence`) that hits `dist/index.js` and `dist/index.cjs`. Built entry typings are **byte-identical** across the change — `dist/index.d.ts` sha256 `bace0e9e373ceef9f8150582b7e9d8ff3414d7358311c82058c005f3135c0360` from a build of BASE sources and from a build of the branch alike. - **But the published tarball does change.** `npm pack` ships 27 files, and `package.json` is one of them (1.9 kB). The packed manifest now carries `"pg": "^8.22.0"` and `"@types/pg": "^8.15.6"` under `devDependencies` — verified by extracting `package/package.json` out of the real tarball. `AGENTS.md`: `skip-changeset` is for a diff that **publishes nothing from any released package**. `@objectstack/metadata-protocol` is released at 17.4.0, and this diff publishes two manifest lines. That is the same standard the repo's own precedents apply — `trigger-schedule`'s `repository.directory` correction took a changeset because "the value ships inside the tarball", and `driver-sql` graded a test-only PR `patch` rather than `skip-changeset`. So: **`patch`**, ⛔ not `skip-changeset`, and `Check Changeset` should report `success`, not `skipped`. ## The point of the card: the arm is EXECUTED, in both directions ⛔ No `--project` narrowing anywhere — the package declares no vitest projects at all (no `projects` key, no workspace config), so there is no project filter to drop a named file into. **Against a live PostgreSQL 16.13** (`OS_TEST_POSTGRES_URL` + `OS_EXPECT_LIVE_DIALECT_MATRIX=1`) — **7 of 7 executed**, by name: ``` ✓ is pointed at a real PostgreSQL, in this file's own schema — without this the run proves nothing ✓ every statement the migration builds PARSES and runs on PostgreSQL ✓ [#17621] the catalog presence probe ANSWERS on PostgreSQL — both directions, on the live server ✓ [#17621] the scope is the SEARCH PATH — a same-named relation off it is not this one ✓ [#17621] the arm's QUOTED argument matches exactly — a case-folded neighbour is not this table ✓ repairs the split end to end, and reports the already-minted duplicates ✓ is idempotent — a second run finds no split Test Files 1 passed (1) Tests 7 passed (7) [#17621] live PostgreSQL 16.13 ... search_path=os_lv_seed_tenancy_backfill_live_postgre_ae7a6c870c26 ``` **Without a URL** the same file **skips** — reported as skipped, never as passed: ``` Test Files 1 skipped (1) Tests 7 skipped (7) exit 0 ``` **With `OS_EXPECT_LIVE_DIALECT_MATRIX=1` and no URL** it is a named red, so a dropped `env:` line cannot return the seam to zero coverage quietly: ``` × #17621 live PostgreSQL > OS_TEST_POSTGRES_URL must be set — this runner declared it provisioned a server Test Files 1 failed (1) Tests 1 failed | 7 skipped (8) exit 1 ``` ### Ablation — the suite can fail Committed the fix first, then re-compiled the Postgres arm to MySQL's `DATABASE()` form in `read-probe.ts`. On-disk proof before believing the run: anchor `to_regclass` 3 occurrences → 2, injected `DATABASE()` 2 → 3, blob hash `fd66ad50…` → `86ea33de…`. No rebuild was needed or performed, and that is itself the resolution evidence: the subject is same-package `./read-probe.js`, which vitest resolves to `src/`, so a stale `dist` cannot manufacture a false green here the way it can for a dependency resolved through `exports`. Result: **6 of the 7 cases go red**, reporting ``` expected { verdict: 'unreadable', …(2) } to deeply equal { verdict: 'absent', probe: 'catalog' } + "detail": "function database() does not exist" ``` — exactly the mis-compiled-arm shape the fence exists to keep out of `'absent'`. The one case that stays green is the provisioning/non-vacuity check, which does not exercise the arm; that is the correct partition. Restored with `git checkout HEAD -- PATH` and verified: blob hash back to `fd66ad50…`, `git diff HEAD` empty, `git status --porcelain` clean. ## Verification | what | reading | |:--|:--| | package suite, unnarrowed (`pnpm --filter @objectstack/metadata-protocol test`, live PG present) | **177 passed / 2 skipped (179 files)**, **2538 passed / 12 skipped (2550 tests)**, exit 0. The 2 skipped files are the live-MySQL pair — no MySQL server in this container. | | tiers | none. This package declares no vitest projects, so there is no tier split to report and no `--project` filter was used. | | `pnpm --filter @objectstack/metadata-protocol typecheck` | exit 0. Proved it really compiles the new file: `tsc --noEmit --listFiles` names `seed-tenancy-backfill.live-postgres.test.ts` (1 hit), positive control `read-probe.ts` present, negative control `packages/cli/src` absent, 741 files total. | | CI filter widening, measured | `vitest list --filesOnly live-mysql` selects **3** files; `... live-mysql live-postgres` selects the **same 3 plus** `seed-tenancy-backfill.live-postgres.test.ts`. Multiple positionals are OR-ed and each stays a substring. | | `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` | 100 derived commands, all run at the final head `66dff5cb02` with `dist` present. **100 exit 0.** `--ran` reconciliation: `100 derived, 100 run, 0 NOT-MEASURED, 0 UNRUN` — a derived zero, every line recorded its exit code. The command set re-derived at that head is identical to the one derived before the rework commit. | | `eslint . --no-inline-config --format json` | exit 0, **6671 files**, 0 errors, 0 warnings — the whole repo population, not a narrowing. | | `check:nul-bytes` | red once, and fixed here: the editing tool materialised the unit-separator escapes in `sequenceKeyHash` into raw `0x1F` bytes. Rewritten as escape text, gate green (8510 files scanned), and an independent `grep -naP` sweep over every changed file finds nothing. | | `check:live-db-isolation` | PASS — the new file is listed among the four live suites that derive their database, and it joins `live-mysql-database.isolation.test.ts`'s auto-discovered population (9 cases green, distinctness now measured over 4 files rather than 3). | Three gates first answered **exit 3 — PREREQUISITE NOT MET** in this fresh worktree (`check:dual-build-cjs-loads`, `check:lean-entry-closure`, `check:type-check-debt`); a full `turbo run build` over the workspace (72/72 tasks) was run and all three were then measured green. ⛔ None of those exit-3s was read as a pass. ## Clause-②: `no` — the three falsification attempts | condition | result | |:--|:--| | (a) diff moves an `export` line, or changes the built entry `.d.ts` | **negative, measured.** No `+`/`-` line in the diff contains `export`. `dist/index.d.ts` byte-identical across a BASE-sources build and a branch build (same sha256). | | (b) `pg` reaches the **published** dependency graph | **negative, measured.** The packed manifest's `dependencies` are unchanged (7 workspace links + `zod`); no `peerDependencies`, no `optionalDependencies`. `pg` and `@types/pg` sit in `devDependencies`, which no consumer installs. | | (c) `scripts/pm/check-widening-tells.mjs --declaration no --diff FILE` | exit 0 — **but read it as NOT MEASURED, not as a clean negative.** Its own output: "0 judged against a declared surface, 5 NOT MEASURED … no declared surface covers it" for all five files. (a) and (b) carry the weight. | ## Rework — the install-time defect this PR shipped red with, and the verification hole behind it The first push was **24-of-35 red** in CI while `origin/main` was clean, and every red job died at the same place: ``` ERR_PNPM_OUTDATED_LOCKFILE specifiers in the lockfile don't match specifiers in package.json: * 1 dependencies were removed: @types/pg@^8.15.6 ``` The lockfile was right and the manifest was wrong. `@types/pg` was added *after* the first commit on this branch; a later step of the `dist/index.d.ts` before/after measurement restored `packages/metadata-protocol` to `HEAD`, and `HEAD` at that moment predated the manifest edit. The pathspec did not cover `pnpm-lock.yaml`, so the lockfile kept the entry the manifest lost. Fixed by putting the one line back — the lockfile is byte-untouched, the mismatch closes from the manifest side. Two things worth carrying, both about verification rather than about `pg`: - **The restore check that missed it was `git status --porcelain -- PATH` coming back clean** — which it was, because reverting to `HEAD` *is* clean. A clean status proves the restore ran; it does not prove the restore preserved the state the run was supposed to leave behind. Against an edit made **after** the commit being restored to, those two readings point opposite ways. - **Nothing local could see the result.** A warm `node_modules` never re-runs the frozen-lockfile check, so the defect existed only at install time on a clean tree while 100/100 derived gates and the full package suite stayed honestly green — the same family as #17853, a verification that passes because it never executed the thing in question. ⇒ a diff touching `package.json` or `pnpm-lock.yaml` owes a clean-tree `pnpm install --frozen-lockfile`, and no amount of green elsewhere substitutes. Measured on a genuinely clean tree (fresh worktree, no `node_modules`): **exit 1 before the fix with CI's exact message, exit 0 after it**, and `git diff --name-only` names `package.json` alone. Re-anchored at the fixed head `66dff5cb02`: the live-PG suite is **7 passed (7)** again, the skip direction **7 skipped (7)** at exit 0, `typecheck` exit 0, the package suite 176 passed / 3 skipped (179 files) and 2531 passed / 19 skipped (2550 tests) with no live servers, all 100 derived gates exit 0, and `eslint . --no-inline-config` 6671 files with 0 findings. The changeset stays **`patch`**, and the reasoning holds harder now: the packed `package.json` gains **two** dependency lines rather than one. --- _Authored by Claude Code — session `session_01RuoNSXUbBoWHkNS4AknTrM` (https://claude.ai/code/session_01RuoNSXUbBoWHkNS4AknTrM). Attribution kept in prose here because this body was edited through raw REST, whose measured behaviour is to append its own footer block._ ## Acceptance notes - `noted, not filed:` the new file's per-file schema comes from `currentLiveMysqlDatabase()` — a MySQL-named resolver in a PostgreSQL suite. Deliberate, and the file's docblock says why: the derivation is dialect-neutral and already capped at PostgreSQL's 63-byte limit, and calling it **by that name** is what enrols the file in `live-mysql-database.isolation.test.ts`'s population, which is discovered by reading each test file for that exact call. A dialect-neutral alias would buy a better name at the price of this file's distinctness never being measured. Renaming the resolver repo-wide is mechanical and belongs to whoever adds the next live suite to this package or generalises `live-mysql-database.testkit.ts`. 承接者: the next live-DB suite in `packages/metadata-protocol`. - `noted, not filed:` `pr-automation.yml`'s documented `skip-changeset` clobber is already fixed on this tree — both labeler steps now route through `scripts/pr-labels.mjs` (POST + targeted DELETE, no whole-set PUT), so the "apply the label after the push settles" precaution is stale advice rather than a live hazard. Moot for this PR, which takes `patch`. 承接者: whoever next reads that precaution in a dispatch. ⛔ Draft, and left that way: landing is the PM seat's act. Not marked ready, auto-merge not armed. --- _Generated by [Claude Code](https://claude.ai/code)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent ed5a1e7 commit 7e74af3

6 files changed

Lines changed: 453 additions & 9 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
'@objectstack/metadata-protocol': patch
3+
---
4+
5+
Execute the read-probe's PostgreSQL catalog arm against a live server, closing the one dialect this package pinned as text and never ran.
6+
7+
`read-probe.ts` compiles one non-raising table-presence arm per dialect family. Two were executed against something real — SQLite end to end through a real `SqlDriver`, MySQL on the live server the `Temporal Conformance` job provisions. The PostgreSQL arm (`SELECT 1 WHERE to_regclass('"<table>"') IS NOT NULL`) was pinned character-for-character against all four knex client spellings and run nowhere: this package had no live-PG harness, no `pg` dependency, and its CI step supplied `OS_TEST_MYSQL_URL` alone while filtering vitest to `live-mysql`.
8+
9+
A text pin cannot close that gap, because the failure this module is fenced against is an arm mis-compiled for one dialect: it raises, the `catch` that exists for the expected miss swallows it, and a stored-row data repair silently becomes a no-op. Whether `to_regclass` answers ZERO ROWS rather than raising is a claim about PostgreSQL, not about this repo's string concatenation. `seed-tenancy-backfill.live-postgres.test.ts` now runs every statement the migration builds, both presence directions with the refusal control beside them, the search-path scoping the arm depends on, and the whole backfill end to end — on a live server, in its own derived schema. Ablated (the Postgres arm re-compiled to MySQL's `DATABASE()` form), six of its seven cases go red, reporting `verdict: 'unreadable'` with `detail: "function database() does not exist"` — the exact shape the fence exists to keep out of `'absent'`.
10+
11+
Grade: `patch`, measured rather than defaulted. Not `minor` — no new export, no widened accept-set, no runtime behaviour change of any kind. Not `skip-changeset` either, and that is the measurement worth recording: `dist/` is byte-untouched (grepped for this change's markers: zero hits, against a positive control that hits `dist/index.js` and `dist/index.cjs`), but `package.json` is one of the 27 files `npm pack` ships, and it now carries `pg` and `@types/pg` in `devDependencies`. `skip-changeset` is for a diff that publishes nothing from a released package; this one publishes two manifest lines a consumer never installs, which is still publishing.

.github/workflows/ci.yml

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1407,18 +1407,38 @@ jobs:
14071407
# paths, not shell globs — measured, the glob form matches zero files
14081408
# ("No test files found", exit 1). Loud rather than silent, but still
14091409
# wrong; the substring is the form that actually selects both.
1410+
#
1411+
# [#17621] TWO substrings now, because the dialect that can fail is no
1412+
# longer only MySQL. #17175 gave the two presence probes a catalog arm per
1413+
# dialect family, and its PostgreSQL arm — `to_regclass` — was pinned as
1414+
# TEXT and executed NOWHERE: this package had no live-PG harness and this
1415+
# step supplied `OS_TEST_MYSQL_URL` alone, so the one arm whose failure
1416+
# mode is a SILENTLY skipped data repair was the one arm no server ever
1417+
# ran. The postgres service this job already provisions for the driver-sql
1418+
# leg is what makes closing that wiring rather than infrastructure.
1419+
#
1420+
# Multiple positionals are OR-ed, and each is still a substring: measured,
1421+
# `live-mysql live-postgres` selects the three `live-mysql*` files the
1422+
# single argument selected plus `seed-tenancy-backfill.live-postgres`. ⛔ A
1423+
# single shorter substring (`live-`) would select the same set today and
1424+
# silently widen with the next unrelated file that happens to spell it.
14101425
- name: Build metadata-protocol and its dependencies
14111426
run: pnpm exec turbo run build --filter=@objectstack/metadata-protocol... --concurrency=4
14121427

1413-
- name: Run the metadata-protocol migration statements against live MySQL
1428+
- name: Run the metadata-protocol migration statements against live MySQL and PostgreSQL
14141429
env:
14151430
OS_TEST_MYSQL_URL: mysql://root:root@127.0.0.1:3306/conformance
1431+
# [#17621] The same URL the driver-sql leg above already receives, for
1432+
# the postgres service this job already runs. One line, and the
1433+
# read-probe's Postgres arm stops being the only one nothing executes.
1434+
OS_TEST_POSTGRES_URL: postgres://postgres:postgres@127.0.0.1:5432/postgres
14161435
# Same vacuous-pass guard as the driver-sql leg: this runner
14171436
# provisioned the server, so a missing URL is a defect in the runner
1418-
# and must be a red rather than a skip.
1437+
# and must be a red rather than a skip. It now covers BOTH URLs: each
1438+
# live file above turns its own missing URL into a named failure.
14191439
OS_EXPECT_LIVE_DIALECT_MATRIX: '1'
14201440
run: |
1421-
pnpm --filter @objectstack/metadata-protocol exec vitest run live-mysql
1441+
pnpm --filter @objectstack/metadata-protocol exec vitest run live-mysql live-postgres
14221442
14231443
dogfood:
14241444
# Sharded 3-way: the suite is ~60 independent test files, each booting its

packages/metadata-protocol/package.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,9 @@
4949
},
5050
"devDependencies": {
5151
"@types/node": "^26.2.0",
52+
"@types/pg": "^8.15.6",
5253
"mysql2": "^3.23.3",
54+
"pg": "^8.22.0",
5355
"tsup": "^8.5.1",
5456
"typescript": "^6.0.3",
5557
"vitest": "^4.1.11"

packages/metadata-protocol/src/migrations/read-probe.test.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,15 @@
2626
* one family does not silently fall through to the fallback probe.
2727
*
2828
* ⚠️ Running those statements against a live SERVER is a different claim, and
29-
* this file does not make it. The MySQL arm is executed against a real server in
29+
* this file still does not make it — it names where each arm is executed
30+
* instead. The MySQL arm runs against a real server in
3031
* \`seed-tenancy-backfill.live-mysql.test.ts\`; the SQLite arm end to end against
3132
* a real \`SqlDriver\` in \`packages/runtime\`'s
32-
* \`seed-tenancy-autonumber-split.integration.test.ts\`. ⛔ The POSTGRES arm is
33-
* NOT MEASURED against a live server anywhere — this package has no live-PG
34-
* harness, no \`pg\` dependency, and its CI leg supplies \`OS_TEST_MYSQL_URL\`
35-
* only while filtering to \`live-mysql\`. Recorded here rather than left to be
36-
* discovered.
33+
* \`seed-tenancy-autonumber-split.integration.test.ts\`; and [#17621] the POSTGRES
34+
* arm — recorded here as NOT MEASURED anywhere for as long as that was true —
35+
* now runs in \`seed-tenancy-backfill.live-postgres.test.ts\`, on the postgres
36+
* service the \`Temporal Conformance\` job already provisions. All three arms are
37+
* executed.
3738
*/
3839

3940
import { describe, it, expect } from 'vitest';

0 commit comments

Comments
 (0)