test: classify test backends and split backup inventory by backend - #81
Merged
Conversation
A reference-implementation test entry's backend requirement is implicit today: it lives in whichever fixtures the file happens to import, so the only way to learn whether a file needs Postgres, SQLite or no database at all is to run it and watch what it connects to. Scheduling cannot see that, which is why a Postgres case that silently skipped looks the same as one that ran. Add the two halves of a checkable answer, both inert until something wires them up: check-test-backends.ts declares the obligation in a manifest and independently enumerates the tracked test entries, requiring exact set equality -- missing, stale, duplicate and unknown-backend entries all fail. The manifest is an array rather than a path-keyed map because a map cannot represent a duplicate: a second entry for the same path overwrites the first during parsing, so the check would never see it. An entry claiming it needs no database is checked against its own imports and rejected if it reaches storage, with no override. test-unit-preload.mjs is the runtime counterpart, following the existing scripts/hermetic/preload.ts wiring. Static analysis cannot see a computed specifier or generated code, so this hook watches what the process actually resolves. It rejects at resolve, before any module evaluates, so an admissible test observes nothing different. Violations are recorded outside the thrown error and forced onto the exit code, which is what stops a file from swallowing its own denial in a try/catch and reporting green. Denial is per specifier form, not per file. A rule matching "pg/lib/client" but not the bare "pg" reports zero violations on a file that plainly imports Postgres -- a silent false pass, and the worst thing a guard like this could do. Bare package, subpath and both builtin spellings are matched, with a separator boundary so "pgvector" is not mistaken for "pg". The classifier also recognises createRequire(...)(...) because server/db.ts reaches better-sqlite3 that way and a scan for `require(` alone would miss it. No production manifest ships here. Classifying the full inventory is separate work, and set equality against a partial list would not be a meaningful check. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
backup-table-inventory.test.ts mixed three backend requirements in one file: five cases read only source files, docs and static policy exports, three bootstrap a real SQLite database, and three need a live Postgres server. Because the file imports server/db.ts and server/postgres-storage.ts at the top level, every case carried every requirement, and the eight that need no Postgres could not be scheduled apart from the three that do. Split it into three files along the backend each case actually uses. All eleven case names and their assertions are unchanged; the split moves bodies between files and nothing else. The database imports and fixtures travelled with the cases that use them, which is what lets the remaining file run with no database at all. The Postgres cases keep their existing skip-when-unconfigured guards. Turning those skips into failures belongs to whichever change makes Postgres a required backend, not to a split that is meant to preserve behaviour. Verified by running all three files: five plus three plus three cases pass with no skips, the Postgres file against a real pgvector server. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
The storage guard for database-free tests matched the raw import specifier
only, so a test could resolve a driver itself and import the resulting
location:
const url = pathToFileURL(createRequire(import.meta.url).resolve("pg")).href;
await import(url);
No denied package name is spelled as a specifier there, so the guard did not
fire. Executed against the real driver, that ran to completion and exited 0
with PostgreSQL loaded -- the guard's one unacceptable failure, a silent pass
for a test that reached a database.
A denied driver is now also matched on its resolved package identity: the
`node_modules/<pkg>/` segment its resolved path must contain. The trailing
separator keeps unrelated packages whose names merely begin with a denied
name, such as pgvector and pg-boss, resolving as before. Builtins have no
package directory and stay with the specifier rule, which is exact for them.
The static checker had the same hole from the other side. It read import,
require and createRequire calls but not `resolve`, so the file above appeared
to have no storage dependency at all. Naming a denied driver in `resolve` now
counts as reaching it, since resolving a driver has no other purpose.
Also make the checker fail closed when run as a command. It exported `main`
but never invoked it, so `node scripts/check-test-backends.ts <bad-manifest>`
exited 0 and printed nothing while the exported function rejected the same
manifest -- a checker that passes silently when asked to check. A missing
argument now exits 2 rather than doing nothing.
Tests cover each denied route as a real child process asserting the actual
exit code, because the defect was an exit-0 run that loaded the driver and no
in-process assertion would have caught it: pre-resolved file URL, pre-resolved
absolute path, createRequire, require by resolved path, and each of those with
the error swallowed in a catch. Controls prove the rules are not overbroad: a
clean unit test still passes, sqlite-vec still loads through its own
node_modules path, and the checker still accepts a manifest covering every
tracked test entry. Removing either fix turns the corresponding controls red.
Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
Assisted-by: AI
… access
A test could execute real SQL with the guard enabled:
const { DatabaseSync } = process.getBuiltinModule("node:sqlite");
new DatabaseSync(":memory:").prepare("select 42").get();
That printed a query result and exited 0. `process.getBuiltinModule` returns a
builtin without consulting module resolution, so the resolve hook never saw
it. Wrapping that function closes the route, and because the check runs on the
argument's runtime value it covers a computed name -- `"node:" + "sqlite"` --
which no scan of source text could.
The CJS `require("node:sqlite")` path needed nothing: `registerHooks`
intercepts CJS resolution too, so the resolve hook already denied it. Verified
by disabling the new function and re-probing that route, which still fails.
A second wrap of `Module._load` would have been dead code around a Node
internal, so it is not here.
The deeper problem was the shape of the rule rather than any single missing
form. Each round of review found one more spelling because the rule enumerated
spellings. It now states the invariant instead: a target is denied by WHAT IT
IS, whatever named it.
on disk the resolved real path, symlinks resolved, of the driver's own
installed package root or of a denied storage module
builtin the canonical `node:` name, normalised from the runtime value
This also replaces the `node_modules/<pkg>/` substring test, which was an
installation-layout guess rather than proof of identity: it would have called
any file under a directory of that name a driver, and missed a driver
installed elsewhere. Comparing against the root that resolution itself reports
removes the guesswork, and the boundary that keeps pgvector and pg-boss
loading is now a real directory boundary rather than a string coincidence.
The static checker reads the literal `getBuiltinModule` form for the same
reason it reads the other loader routes; a computed name remains outside what
source reading can recover, and is recorded as such next to the existing
computed-specifier note.
What the boundary does not cover is now stated in the source rather than
implied: a copy of a driver at a different real path is a different file, and
identifying it would need content fingerprinting. Storage reached over a
socket by hand-rolled protocol code, and any unexecuted branch, are likewise
outside it.
Controls per form, each a real child process asserting the actual exit code:
literal builtin name with a real query, computed builtin name, the denial
caught and the body completed, and the builtin via createRequire. Against
those, controls that must keep working: node:path and node:fs through both
routes, the real installed sqlite-vec package, a clean unit test, and a path
that merely contains a denied package name. Disabling the builtin guard turns
three of the new controls red; the identity tests run against the real
installed drivers rather than synthetic path strings.
Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
Assisted-by: AI
…se positives The Postgres/SQLite backup-inventory split left the new Postgres file unclassified in the template-eligibility registry and the old combined file stale there, failing both inventory gate tests. The zero- connector-knowledge conformance gate also flagged the classifier CLI's own manifest-path argument as an unresolvable data load; it is an operator-supplied test-backend manifest, never connector/provider identity, so it is allowlisted the same way deploy-canary.ts's --manifest argument already is. Also fixes a stale preload comment that claimed a CJS require wrap exists for builtins; it does not, and the doc explaining why (registerHooks already covers CJS resolution) was already documented in installBuiltinGuard's own comment. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
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
1 participant
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.
A test meant to run against Postgres can skip when no database is configured, and nothing notices. To schedule tests by the database they need, each file has to state that requirement, and the statement has to be checked.
This adds the checker and the runtime guard, and splits one file as the first real case. Nothing is wired into CI yet and no repository-wide manifest ships here.
scripts/check-test-backends.tscompares a manifest listing each test file with its backend (none,sqlite,postgres, orsqlite+postgres) against the files that exist, and fails on a missing, stale, duplicate, or unknown entry. An entry claiming no database is checked against the file's own imports and rejected if it imports one.scripts/test-unit-preload.mjsguards the unit lane. It rejects any load ofpg,better-sqlite3,node:sqlite, or the repository's storage modules, identified by resolved real path or canonical builtin name rather than by spelling, so every form is covered: bare name, subpath, relative or absolute path, file URL, dynamic import,createRequire, a pre-resolved path, andprocess.getBuiltinModuleunder a literal or computed name. A test that catches the error still fails the lane. Not covered: a copy of a driver at a different path, which is a different file.test/backup-table-inventory.test.tsheld eleven tests with three different requirements. It is now three files: five that read only source and config, three on SQLite, three on Postgres. Names and assertions are unchanged.Verify from
reference-implementation/:node --test --import tsx scripts/check-test-backends.test.ts scripts/test-unit-preload.test.ts test/backup-table-inventory*.test.ts. The Postgres file runs withPDPP_TEST_POSTGRES_URLset.Not covered: an import reached three modules deep without any denied form on the executed path. The guard fires on what actually loads, not on what a branch might load.
Assisted-by: AI