fix(selfhost): make PgStatement.first() coalesce to null like the D1 adapter#8411
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
…adapter PgStatement.first(colName) returned row[colName] uncoalesced, so an absent column (or a driver representing SQL NULL as undefined) leaked undefined through the documented Promise<T | null> contract, diverging from d1-adapter.ts's first() -- the reference implementation callers rely on when treating the two backends interchangeably via backend-contracts.ts. Applies the same ?? null coalesce the D1 sibling already uses. Adds the NULL-value and absent-column parity tests the pg suite was missing (the D1 suite has had the equivalent NULL test all along). Closes JSONbored#8361
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-24 12:11:23 UTC
Review summary Nits — 2 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Summary
PgStatement.first(colName)(src/selfhost/pg-adapter.ts) returned(colName ? row[colName] : row) as Twith no coalesce, so when the requested key is absent from the row (or a driver represents SQLNULLasundefined) it returnedundefinedcast asT— breaking its own documentedPromise<T | null>contract and diverging from the D1 sibling that callers rely on when treating the two backends interchangeably viabackend-contracts.ts.d1-adapter.ts:57-61already uses:return ((colName != null ? row[colName] : row) ?? null) as T | null;.d1-adapter.tsis untouched (it is the correct reference), and no otherPgStatementmethod is changed.selfhost-d1-adapter.test.tshas had a "returns null when the row exists but the column value is NULL" test all along, while the pg suite never exercised a NULL/absent-column case. Adds both parity cases.Closes #8361
Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
src/selfhost/pg-adapter.ts(one return statement) andtest/unit/selfhost-pg-adapter.test.ts, soactionlint(no workflow change),build:mcp/test:mcp-pack(no MCP change),ui:*(no UI/OpenAPI change),test:workers(no worker change), andnpm audit(no dependency change) are not exercised by it.vitest run --coverage --coverage.all=false): the changed line is hit and every branch on it is taken — thecolName != nulltrue arm (first("name")), its false arm (the existing no-argfirst()test), the?? nulldefined arm (a real value), and its nullish arm (the two new tests).test/unit/selfhost-pg-adapter.test.tspasses in full (11 tests) and roottsc --noEmitis clean.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.UI Evidence
Not applicable — backend adapter contract fix in
src/selfhost/; no visible UI, frontend, docs, or extension change.Notes
colName != nullrather than a bare truthiness check specifically to match the D1 implementation character-for-character, as the issue requires — the two adapters'first()bodies are now identical in behavior.NULLone passes either way with the currentpgdriver and is kept as an explicit contract/parity assertion mirroring the D1 suite's existing test.