feat(provision): pgserve provision orchestration verb (singleton G3 verb 4 — closes G3)#92
Merged
Merged
Conversation
…erb 4)
Closes the singleton G3 group. Composes the merged G3 foundations into
an idempotent provisioner that maps a consumer fingerprint → CREATE
ROLE / CREATE DATABASE / GRANT / INSERT INTO pgserve_meta under a
per-fingerprint advisory lock.
Foundations consumed:
src/provision/fingerprint.js → resolveFingerprint
src/provision/advisory-lock.js → buildAdvisoryLockSql
src/provision/db-naming.js → deriveProvisionedNames
src/schema/pgserve-meta.js → bootstrapPgserveMeta
src/cosign/schema.js → applyVerifiedColumns
src/lib/pg-query.js → pgQuery / quoteIdent / quoteLiteral
(NEW shared primitive — see below)
src/lib/admin-json.js → readAdminJson (port discovery)
src/lib/pg-query.js (new — shared foundation):
Extracted the existing pgQuery shape from
src/upgrade/steps/cosign-meta-migration.js#pgQuery (PR #79) into a
shared module so gc (PR #91) and provision can compose it instead
of each inlining their own. Exports:
- pgQuery({ sql, db, port, host, user, password, captureStdout })
- quoteIdent(name)
- quoteLiteral(value)
- PG_QUERY_DEFAULTS (frozen)
src/commands/provision.js (new):
- runProvision(argv) — top-level dispatcher.
- Idempotent strategy:
1. ensurePgserveMetaSchema() — bootstrap + apply verified columns
(both modules use IF NOT EXISTS, safe to re-run).
2. Fast-path: SELECT existing meta row + check pg_database; if
both present, UPDATE last_used_at and exit 'touched'.
3. Else runCreateSequence() under xact-scoped advisory lock:
a. BEGIN; pg_advisory_xact_lock(<bigint>); COMMIT
(lock-only step — releases on commit).
b. CREATE ROLE IF NOT EXISTS via DO block (postgres has no
CREATE ROLE IF NOT EXISTS sugar pre-15).
c. CREATE DATABASE IF NOT (already SELECTed). Catches and
ignores 42P04 (database_already_exists) per wish spec.
d. GRANT CONNECT, CREATE on the DB to the role (idempotent).
e. INSERT INTO pgserve_meta ... ON CONFLICT (fingerprint)
DO UPDATE SET <fields>, last_used_at = now() — replays
converge to current values without operator surgery.
- --json emits structured summary; default is human-readable.
- --port / -p overrides the postgres port discovery; default is
read from admin.json then 5432.
- Positional arg = explicit fingerprint (operator pin); when
omitted, resolves from cwd's package.json.
- Exit codes:
0 provisioned (or no-op idempotent replay)
1 user error (bad flags, fingerprint validation)
2 pgserve_meta bootstrap failed (postmaster unreachable)
3 postgres error during create sequence (partial state safe
to resume; rerun is benign)
bin/pgserve-wrapper.cjs:
Add 'provision' to __installSubcommands. Pure node + child_process;
must skip the bun probe.
src/cli-install.cjs:
Add `case 'provision':` dispatch with the dynamic-import pattern
matching verify / trust / gc / doctor.
Tests (23 pass, 0 fail):
tests/cli/provision.test.js (new):
- parseFlags: positional, --port range check, --json, --help, +/-
flag mix, unknown-flag throw.
- resolvePort: explicit wins; fallback returns valid integer.
- bigintLiteral: ::bigint cast format; full int64 range.
- runProvision: --help → 0; bad flag → 1.
tests/lib/pg-query.test.js (new):
- pgQuery rejects empty/non-string sql before shellout.
- quoteIdent: shape, internal-quote escape, non-string coerce,
already-quoted symmetry.
- quoteLiteral: shape + apostrophe-escape (SQL injection guard).
- PG_QUERY_DEFAULTS: canonical surface + frozen.
Smoke (HOME=$(mktemp -d) node bin/pgserve-wrapper.cjs provision --help):
routes through wrapper → dispatch → runProvision and prints the
usage block.
Validation: bun run lint clean; bun run deadcode clean.
Cohort: Singleton G3 verb 4 of 4 — closes G3 entirely. Verbs 1
(doctor), 2 (trust), 3 (gc, PR #91) already shipped or in flight.
After this lands, G7 (roles + GRANTs schema, depends on G3) is
unblocked.
Note: full DB integration ("does it actually create a database") is
covered by tests/integration/ (deferred to a follow-up — needs a
running postmaster for both gc and provision).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces the ChangesProvision Command Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces the provision subcommand to pgserve, enabling idempotent creation of databases and roles based on fingerprints. It includes a new pgQuery utility that executes SQL via psql shell-outs. Review feedback identified several critical issues: the parsing of psql output uses an incorrect field separator, the transaction-scoped advisory locks provide no concurrency protection because each query runs in a separate process, and the error handling for existing databases incorrectly expects SQLSTATE codes that are not present in default psql output.
| captureStdout: true, | ||
| }); | ||
| if (!out) return null; | ||
| const [database_name, role_name] = out.split('\t'); |
There was a problem hiding this comment.
The default field separator for psql in unaligned mode (-A) is |, not \t. Since pgQuery uses the default settings, splitting by \t will fail to correctly parse the columns, resulting in database_name containing the full row string (e.g., "dbname|rolename") and role_name being undefined. This will break the fast-path idempotency check later in the process.
Suggested change
| const [database_name, role_name] = out.split('\t'); | |
| const [database_name, role_name] = out.split('|'); |
Comment on lines
+192
to
+199
| pgQuery({ | ||
| sql: [ | ||
| 'BEGIN;', | ||
| lockSql.replace('$1::bigint', lockBigint) + ';', | ||
| 'COMMIT;', | ||
| ].join('\n'), | ||
| port, | ||
| }); |
There was a problem hiding this comment.
The advisory lock acquired here is transaction-scoped (pg_advisory_xact_lock) and is immediately released upon COMMIT. This provides no concurrency protection for the subsequent provisioning steps (Steps 2-5). Furthermore, because each pgQuery call spawns a new psql process, they operate in different sessions, meaning even a session-level lock would not persist across these calls. To effectively serialize the provisioning sequence, you would need to execute the commands within a single psql session or rely entirely on the DDL idempotency and error handling in Step 3. Also, the "second SELECT inside the catch path" mentioned in the file header (line 36) does not appear to be implemented.
| port, | ||
| }); | ||
| } catch (err) { | ||
| if (err.stderr && err.stderr.includes('42P04')) { |
There was a problem hiding this comment.
Standard psql output does not include SQLSTATE error codes (like 42P04) by default. It typically outputs a human-readable message like ERROR: database "..." already exists. Consequently, this check will likely fail to identify the 'database already exists' condition during a race. Consider checking for the specific error message text instead.
Suggested change
| if (err.stderr && err.stderr.includes('42P04')) { | |
| if (err.stderr && err.stderr.includes('already exists')) { |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d96b2f2d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const env = { ...process.env, PGPASSWORD: password }; | ||
| const result = spawnSync( | ||
| 'psql', | ||
| ['-h', host, '-p', String(port), '-U', user, '-d', db, '-At', '-f', '-'], |
There was a problem hiding this comment.
Make pgQuery fail fast on SQL errors
pgQuery only throws when the psql process exits non-zero, but this invocation does not enable ON_ERROR_STOP; per PostgreSQL psql behavior, script processing continues after SQL errors by default, so many failed statements still exit with status 0. In this commit, runProvision depends on exceptions to detect create/grant failures, so a failed CREATE DATABASE/GRANT can be treated as success and still proceed to metadata writes. Add -v ON_ERROR_STOP=1 (or equivalent) to make SQL errors surface as non-zero exits.
Useful? React with 👍 / 👎.
| captureStdout: true, | ||
| }); | ||
| if (!out) return null; | ||
| const [database_name, role_name] = out.split('\t'); |
There was a problem hiding this comment.
Parse provision probe output with the correct delimiter
selectMetaRow splits psql -At output on a tab, but unaligned psql output defaults to | unless -F is set. That means a row like db|role is parsed as database_name='db|role' and role_name=undefined, so the idempotent fast-path check against pg_database misses existing databases and unnecessarily re-runs the create sequence on every replay.
Useful? React with 👍 / 👎.
namastex888
added a commit
that referenced
this pull request
May 9, 2026
… bool PR #91 bot reviews found three real correctness gaps and one consistency nit: [CRITICAL — Codex P1] psql -At unaligned output defaults to '|', not '\t'. selectMetaRows / selectActiveDbs / selectExistingDbs all split on '\t' — every multi-column SELECT silently produced one-token rows (database_name='db|role|...', everything else undefined). The fast- path classification was effectively non-functional. Fix: pass -F '\t' explicitly so the separator matches what callers parse against. [CRITICAL] psql in script mode (-f) by default continues past statement errors and still exits 0. A failed CREATE DATABASE / GRANT in the provision verb's sequence could be invisible. (Caught by Codex P1 on the sibling PR #92, but the bug applies to gc's pgQuery helper too — this PR's pg-queries.js is the same helper.) Fix: pass -v ON_ERROR_STOP=1 so SQL errors surface as non-zero. [HIGH — Gemini] Hardcoded password = 'postgres' fallback prevented .pgpass / peer-auth on hosts that use them. Fix: only set PGPASSWORD in the child env when the caller or the caller's environment supplies one; let psql fall through to its default authentication flow otherwise. [MEDIUM — Gemini] dryRun: 'true' (string) was inconsistent with other boolean fields in the audit-log payload. Fix: dryRun: true. Filtered out (low-value): - "Extract duplicate error-handling helper" (Gemini medium) — cosmetic; the two error paths handle different layers (meta-rows vs pg_state). Validation: 26/26 tests pass; lint clean. The integration round-trip now requires that callers strip empty trailing-tab tokens correctly when reading single-column queries — verified manually against the existing parsing in selectExistingDbs / selectActiveDbs which split on '\n' first, then on '\t' is benign because there's only one field. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rrency honesty PR #92 bot reviews found four real correctness gaps: [CRITICAL — Codex P2 + Gemini HIGH] psql -At unaligned output defaults to '|', not '\t'. selectMetaRow split on '\t' → database_name picked up the entire 'db|role' string and role_name was undefined, so the fast-path idempotency check missed every existing database and the verb re-ran the create sequence on every replay. Fix: pass -F '\t' explicitly in pgQuery so the separator matches what callers parse against. [CRITICAL — Codex P1] psql in script mode (-f) by default continues past statement errors and STILL exits 0. A failed CREATE DATABASE / GRANT could be invisible — the metadata write would still happen, producing a pgserve_meta row pointing at a database that doesn't exist. Fix: pass -v ON_ERROR_STOP=1 so SQL errors surface as non-zero exits. [HIGH — Gemini] Hardcoded password = 'postgres' fallback prevented .pgpass / peer-auth on hosts that use them. Fix: only set PGPASSWORD in the child env when the caller or the caller's environment supplies one. [HIGH — Gemini] The advisory-lock-only-tx step provided ZERO concurrency protection. Every pgQuery call starts a NEW psql session via spawnSync, so an xact-scoped lock releases on the lock-step's COMMIT and a session-scoped lock can't survive across separate spawnSync invocations either. The "second SELECT inside the catch path" claim in the file header was also wishful — never implemented. Fix: drop the misleading lock-only-tx step entirely. The honest serialization mechanism is **idempotency under racing replays**, documented now in the file header: - CREATE ROLE wrapped in DO + IF NOT EXISTS. - CREATE DATABASE catches "already exists" race. - GRANT is set-style (re-run adds nothing). - INSERT ... ON CONFLICT (fingerprint) DO UPDATE. 10 concurrent provisioners with the same fingerprint converge to one (database, role, meta-row) trio without any explicit lock. The advisory-lock helpers in src/provision/advisory-lock.js are retained on main for future single-session callers (daemon mode); removing the stale import here, plus the bigintLiteral helper that only existed to feed the now-deleted lock SQL. [MEDIUM — Gemini] psql does NOT emit SQLSTATE codes (42P04) by default. The catch-path text-match against '42P04' would never fire on a real race; psql emits the human-readable "ERROR: database "<name>" already exists" message instead. Fix: match against 'already exists' in stderr. src/lib/pg-query.js: Same -F '\t' + ON_ERROR_STOP + PGPASSWORD-only-when-set fixes (the shared primitive that gc + provision both use; mirrors the parallel fix-up commit on PR #91's src/gc/pg-queries.js). Validation: 21/21 tests pass; lint clean; knip clean. Filtered out (none — every bot finding addressed). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/lib/pg-query.test.js (1)
12-19: ⚡ Quick winAdd explicit tests for subprocess failure branches.
Current suite is good on validation/quoting, but it should also lock behavior when
spawnSyncreturnserrororstatus === nullto prevent regressions in CLI error mapping.Also applies to: 55-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/lib/pg-query.test.js` around lines 12 - 19, Add two explicit tests in the pgQuery input-validation test suite that simulate subprocess failures by mocking child_process.spawnSync for the pgQuery function: one test where spawnSync returns an { error: new Error(...) } and assert pgQuery throws or rejects with that error (or maps it to the expected CLI error), and another where spawnSync returns { status: null } and assert pgQuery throws the expected CLI-mapping error; use the same mocking approach already used in this file and add equivalent tests covering the other branch referenced around the 55-66 region to lock the behavior for both error and status===null cases.src/commands/provision.js (1)
24-33: 💤 Low valueStale "advisory-lock window" phrasing in idempotency strategy.
The numbered strategy still references performing the SELECT "inside the advisory-lock window," and Line 361's inline comment says "full create sequence under advisory lock." The wish-level note at lines 35–60 already documents that the lock was abandoned in favour of idempotent SQL; the earlier section and the inline comment should be brought in line so a future reader doesn't conclude the lock is still in effect. Same wording propagates to
src/cli-install.cjs(Line 1138) andbin/pgserve-wrapper.cjs(Line 60); consider updating those too.📝 Suggested doc tweak
- * Idempotency strategy: - * 1. SELECT existing pgserve_meta row by fingerprint inside the - * advisory-lock window. If present + database still exists, just - * `touch` last_used_at and exit success. + * Idempotency strategy (no advisory lock — see "Concurrency note" below): + * 1. SELECT existing pgserve_meta row by fingerprint. If present and + * the database still exists, just `touch` last_used_at and exit + * success.- // Step 3 — full create sequence under advisory lock. + // Step 3 — full create sequence (no lock; idempotent SQL handles races).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/provision.js` around lines 24 - 33, Update the outdated "advisory-lock" wording in the idempotency documentation and inline comments to reflect that the advisory lock was removed and idempotent SQL is used instead: search for the doc block starting with "Idempotency strategy:" in src/commands/provision.js and replace references like "inside the advisory-lock window" and the inline comment "full create sequence under advisory lock" with wording such as "using idempotent SQL (SELECT/IF NOT EXISTS/ON CONFLICT) without an advisory lock"; also make the same textual change in src/cli-install.cjs and bin/pgserve-wrapper.cjs where the phrase "advisory lock" appears so all places consistently state the lock was abandoned in favor of idempotent SQL.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/provision.js`:
- Around line 336-359: The fast-path calls databaseExists and touchMetaRow
inside the provision flow can throw Postgres errors that currently escape
runProvision and become exit code 1; wrap the databaseExists(...) and
touchMetaRow(...) calls in the same try/catch behavior used for
selectMetaRow(...) so any DB error is caught and returns exit code 3 (the
documented "postgres error during create sequence" path). Specifically, put
databaseExists({ port, database: existing.database_name }) and touchMetaRow({
port, fingerprint: resolved.fingerprint }) inside a try block (or separate try
blocks) that catches DB errors, logs or writes the error message like
selectMetaRow does, and returns 3 so the fast-path converges with the create
sequence error handling. Ensure you only call emit/return 0 after the guarded
operations succeed.
In `@src/lib/pg-query.js`:
- Around line 40-43: The JSDoc for the pg-query helper misstates the password
fallback; update the docstring for the args.password parameter in
src/lib/pg-query.js to match the implementation (remove the "'postgres'"
fallback and state that it defaults to process.env.PGPASSWORD or undefined/not
provided). Ensure the description for args.password and any related
example/comment references to a 'postgres' default are removed or corrected so
the JSDoc accurately reflects the function's behavior (see the args.password
param and its surrounding JSDoc block).
- Around line 64-90: The current spawnSync invocation for running psql doesn't
handle process spawn failures and allows psql to prompt interactively for a
password; update the spawnSync result handling to check for result.error (e.g.,
when the psql binary can't be spawned) and throw an Error that includes
result.error (and preserve result.status/stderr as before), and add the '-w'
flag to the psql argument list to disable interactive password prompts so the
process won't hang; make these changes around the existing spawnSync call and
the error-path that builds err (referencing spawnSync, result, result.error,
stderr, and err).
---
Nitpick comments:
In `@src/commands/provision.js`:
- Around line 24-33: Update the outdated "advisory-lock" wording in the
idempotency documentation and inline comments to reflect that the advisory lock
was removed and idempotent SQL is used instead: search for the doc block
starting with "Idempotency strategy:" in src/commands/provision.js and replace
references like "inside the advisory-lock window" and the inline comment "full
create sequence under advisory lock" with wording such as "using idempotent SQL
(SELECT/IF NOT EXISTS/ON CONFLICT) without an advisory lock"; also make the same
textual change in src/cli-install.cjs and bin/pgserve-wrapper.cjs where the
phrase "advisory lock" appears so all places consistently state the lock was
abandoned in favor of idempotent SQL.
In `@tests/lib/pg-query.test.js`:
- Around line 12-19: Add two explicit tests in the pgQuery input-validation test
suite that simulate subprocess failures by mocking child_process.spawnSync for
the pgQuery function: one test where spawnSync returns an { error: new
Error(...) } and assert pgQuery throws or rejects with that error (or maps it to
the expected CLI error), and another where spawnSync returns { status: null }
and assert pgQuery throws the expected CLI-mapping error; use the same mocking
approach already used in this file and add equivalent tests covering the other
branch referenced around the 55-66 region to lock the behavior for both error
and status===null cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e5a895f-4b77-4291-9631-a82a78f34d54
📒 Files selected for processing (6)
bin/pgserve-wrapper.cjssrc/cli-install.cjssrc/commands/provision.jssrc/lib/pg-query.jstests/cli/provision.test.jstests/lib/pg-query.test.js
Comment on lines
+336
to
+359
| let existing; | ||
| try { | ||
| existing = selectMetaRow({ port, fingerprint: resolved.fingerprint }); | ||
| } catch (err) { | ||
| process.stderr.write(`pgserve provision: ${err.message}\n`); | ||
| return 3; | ||
| } | ||
| if (existing && databaseExists({ port, database: existing.database_name })) { | ||
| touchMetaRow({ port, fingerprint: resolved.fingerprint }); | ||
| summary.action = 'touched'; | ||
| summary.databaseName = existing.database_name; | ||
| summary.roleName = existing.role_name; | ||
| emit({ | ||
| json: opts.json, | ||
| summary, | ||
| humanLines: [ | ||
| `pgserve provision: idempotent replay`, | ||
| ` fingerprint: ${resolved.fingerprint}`, | ||
| ` database: ${existing.database_name} (already exists, last_used_at touched)`, | ||
| ` role: ${existing.role_name}`, | ||
| ], | ||
| }); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Fast-path postgres errors bypass the documented exit-code 3.
selectMetaRow is wrapped in try/catch (returns 3 on failure), but databaseExists on Line 343 and touchMetaRow on Line 344 are unguarded. A transient postgres error during the fast-path (e.g. connectivity blip mid-run) will throw out of runProvision and the wrapper will surface it as a generic exit 1, breaking the contract documented at lines 62–67 ("3 = postgres error during create sequence"). The fast-path SQL is part of the create sequence's lifecycle for the operator's purposes — it should converge on the same exit code.
🛡️ Suggested fix
- if (existing && databaseExists({ port, database: existing.database_name })) {
- touchMetaRow({ port, fingerprint: resolved.fingerprint });
- summary.action = 'touched';
- summary.databaseName = existing.database_name;
- summary.roleName = existing.role_name;
- emit({
- json: opts.json,
- summary,
- humanLines: [
- `pgserve provision: idempotent replay`,
- ` fingerprint: ${resolved.fingerprint}`,
- ` database: ${existing.database_name} (already exists, last_used_at touched)`,
- ` role: ${existing.role_name}`,
- ],
- });
- return 0;
- }
+ try {
+ if (existing && databaseExists({ port, database: existing.database_name })) {
+ touchMetaRow({ port, fingerprint: resolved.fingerprint });
+ summary.action = 'touched';
+ summary.databaseName = existing.database_name;
+ summary.roleName = existing.role_name;
+ emit({
+ json: opts.json,
+ summary,
+ humanLines: [
+ `pgserve provision: idempotent replay`,
+ ` fingerprint: ${resolved.fingerprint}`,
+ ` database: ${existing.database_name} (already exists, last_used_at touched)`,
+ ` role: ${existing.role_name}`,
+ ],
+ });
+ return 0;
+ }
+ } catch (err) {
+ summary.action = 'error';
+ summary.error = err.message;
+ process.stderr.write(`pgserve provision: ${err.message}\n`);
+ if (opts.json) emit({ json: true, summary });
+ return 3;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/provision.js` around lines 336 - 359, The fast-path calls
databaseExists and touchMetaRow inside the provision flow can throw Postgres
errors that currently escape runProvision and become exit code 1; wrap the
databaseExists(...) and touchMetaRow(...) calls in the same try/catch behavior
used for selectMetaRow(...) so any DB error is caught and returns exit code 3
(the documented "postgres error during create sequence" path). Specifically, put
databaseExists({ port, database: existing.database_name }) and touchMetaRow({
port, fingerprint: resolved.fingerprint }) inside a try block (or separate try
blocks) that catches DB errors, logs or writes the error message like
selectMetaRow does, and returns 3 so the fast-path converges with the create
sequence error handling. Ensure you only call emit/return 0 after the guarded
operations succeed.
Comment on lines
+40
to
+43
| * @param {string} [args.password] PGPASSWORD; defaults to | ||
| * `process.env.PGPASSWORD` | ||
| * or 'postgres' | ||
| * @param {boolean} [args.captureStdout=false] trim + return stdout |
There was a problem hiding this comment.
JSDoc password fallback is outdated.
Lines 40-43 still document a 'postgres' fallback, but implementation no longer does that. Update the docstring to match current behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/pg-query.js` around lines 40 - 43, The JSDoc for the pg-query helper
misstates the password fallback; update the docstring for the args.password
parameter in src/lib/pg-query.js to match the implementation (remove the
"'postgres'" fallback and state that it defaults to process.env.PGPASSWORD or
undefined/not provided). Ensure the description for args.password and any
related example/comment references to a 'postgres' default are removed or
corrected so the JSDoc accurately reflects the function's behavior (see the
args.password param and its surrounding JSDoc block).
Comment on lines
+64
to
+90
| const result = spawnSync( | ||
| 'psql', | ||
| [ | ||
| '-h', host, | ||
| '-p', String(port), | ||
| '-U', user, | ||
| '-d', db, | ||
| // -At: tuples-only + unaligned. -F: explicit tab field separator | ||
| // (psql defaults to '|' for unaligned mode; callers split rows on | ||
| // '\t' so the separator MUST be '\t'). Bot review CRITICAL. | ||
| '-At', '-F', '\t', | ||
| // ON_ERROR_STOP=1: in script mode (-f), psql by default continues | ||
| // past statement errors and STILL exits 0. A failed CREATE | ||
| // DATABASE / GRANT could otherwise be invisible to the caller. | ||
| // Bot review CRITICAL P1. | ||
| '-v', 'ON_ERROR_STOP=1', | ||
| '-f', '-', | ||
| ], | ||
| { env, input: sql, stdio: ['pipe', 'pipe', 'pipe'] }, | ||
| ); | ||
| if (result.status !== 0) { | ||
| const stderr = (result.stderr || Buffer.from('')).toString(); | ||
| const err = new Error(`psql exited ${result.status}: ${stderr.trim()}`); | ||
| err.status = result.status; | ||
| err.stderr = stderr; | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js child_process.spawnSync, when process spawn fails (e.g., command not found), are status/signalnull anderrorpopulated? Also, does PostgreSQL psql-w disable password prompting and fail immediately?
💡 Result:
- Node.js child_process.spawnSync when spawn fails (e.g., command not found) - In the spawnSync result object, status is the exit code of the child process, or null if the subprocess terminated due to a signal, and signal is the signal used to kill the subprocess, or null if it did not terminate due to a signal [1]. - When the process could not be spawned, spawnSync provides an error object via the result.error field (“The error object if the child process failed or timed out.”) [1]. - Therefore, for the specific case of “command not found” (typically an ENOENT during spawn), the spawn did not start successfully, so you should expect result.error to be populated (with an ENOENT-style spawnSync error) [2], while status/signal are not meaningful as an executed child exit result (they’ll generally not represent a normal exit code) [1]. - Note: spawnSync throws only for timeouts or non-zero exit codes; but the docs also clarify that it includes the error object in the return result when the child process failed or timed out [1]. 2) PostgreSQL psql -w / --no-password: does it disable password prompting and fail immediately? - Yes: -w/--no-password “Never issue a password prompt.” [3][4]. - If the server requires password authentication and a password is not available via other means (notably a.pgpass file), “the connection attempt will fail.” [3][4]. - This means it will not prompt interactively; instead, it fails once psql attempts the connection without a password [3][4].
Citations:
- 1: http://nodejs.org/api/child_process.html
- 2: child_process.spawnSync() fails with ENOENT under Windows nodejs/node-v0.x-archive#25330
- 3: https://www.postgresql.org/docs/current/app-psql.html
- 4: https://manpages.debian.org/bookworm/postgresql-client-15/psql.1.en.html
Add error handling for failed process spawn and disable interactive password prompts.
spawnSync populates result.error when the process fails to spawn (e.g., psql binary not found). Additionally, add the -w flag to psql to prevent interactive password prompts that can block CLI execution; without it, psql will hang if a password is required but unavailable through other means.
Suggested patch
const result = spawnSync(
'psql',
[
'-h', host,
'-p', String(port),
'-U', user,
'-d', db,
+ '-w',
// -At: tuples-only + unaligned. -F: explicit tab field separator
// (psql defaults to '|' for unaligned mode; callers split rows on
// '\t' so the separator MUST be '\t'). Bot review CRITICAL.
'-At', '-F', '\t',
@@
{ env, input: sql, stdio: ['pipe', 'pipe', 'pipe'] },
);
+ if (result.error) {
+ const err = new Error(`psql spawn failed: ${result.error.message}`);
+ err.cause = result.error;
+ throw err;
+ }
if (result.status !== 0) {
const stderr = (result.stderr || Buffer.from('')).toString();
- const err = new Error(`psql exited ${result.status}: ${stderr.trim()}`);
+ const code = result.status ?? `signal ${result.signal ?? 'unknown'}`;
+ const err = new Error(`psql exited ${code}: ${stderr.trim()}`);
err.status = result.status;
err.stderr = stderr;
throw err;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const result = spawnSync( | |
| 'psql', | |
| [ | |
| '-h', host, | |
| '-p', String(port), | |
| '-U', user, | |
| '-d', db, | |
| // -At: tuples-only + unaligned. -F: explicit tab field separator | |
| // (psql defaults to '|' for unaligned mode; callers split rows on | |
| // '\t' so the separator MUST be '\t'). Bot review CRITICAL. | |
| '-At', '-F', '\t', | |
| // ON_ERROR_STOP=1: in script mode (-f), psql by default continues | |
| // past statement errors and STILL exits 0. A failed CREATE | |
| // DATABASE / GRANT could otherwise be invisible to the caller. | |
| // Bot review CRITICAL P1. | |
| '-v', 'ON_ERROR_STOP=1', | |
| '-f', '-', | |
| ], | |
| { env, input: sql, stdio: ['pipe', 'pipe', 'pipe'] }, | |
| ); | |
| if (result.status !== 0) { | |
| const stderr = (result.stderr || Buffer.from('')).toString(); | |
| const err = new Error(`psql exited ${result.status}: ${stderr.trim()}`); | |
| err.status = result.status; | |
| err.stderr = stderr; | |
| throw err; | |
| } | |
| const result = spawnSync( | |
| 'psql', | |
| [ | |
| '-h', host, | |
| '-p', String(port), | |
| '-U', user, | |
| '-d', db, | |
| '-w', | |
| // -At: tuples-only + unaligned. -F: explicit tab field separator | |
| // (psql defaults to '|' for unaligned mode; callers split rows on | |
| // '\t' so the separator MUST be '\t'). Bot review CRITICAL. | |
| '-At', '-F', '\t', | |
| // ON_ERROR_STOP=1: in script mode (-f), psql by default continues | |
| // past statement errors and STILL exits 0. A failed CREATE | |
| // DATABASE / GRANT could otherwise be invisible to the caller. | |
| // Bot review CRITICAL P1. | |
| '-v', 'ON_ERROR_STOP=1', | |
| '-f', '-', | |
| ], | |
| { env, input: sql, stdio: ['pipe', 'pipe', 'pipe'] }, | |
| ); | |
| if (result.error) { | |
| const err = new Error(`psql spawn failed: ${result.error.message}`); | |
| err.cause = result.error; | |
| throw err; | |
| } | |
| if (result.status !== 0) { | |
| const stderr = (result.stderr || Buffer.from('')).toString(); | |
| const code = result.status ?? `signal ${result.signal ?? 'unknown'}`; | |
| const err = new Error(`psql exited ${code}: ${stderr.trim()}`); | |
| err.status = result.status; | |
| err.stderr = stderr; | |
| throw err; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/pg-query.js` around lines 64 - 90, The current spawnSync invocation
for running psql doesn't handle process spawn failures and allows psql to prompt
interactively for a password; update the spawnSync result handling to check for
result.error (e.g., when the psql binary can't be spawned) and throw an Error
that includes result.error (and preserve result.status/stderr as before), and
add the '-w' flag to the psql argument list to disable interactive password
prompts so the process won't hang; make these changes around the existing
spawnSync call and the error-path that builds err (referencing spawnSync,
result, result.error, stderr, and err).
…vision-verb # Conflicts: # bin/pgserve-wrapper.cjs # src/cli-install.cjs
2 tasks
namastex888
added a commit
that referenced
this pull request
May 9, 2026
…5 more PR #93 bot reviews caught 7 findings (1 HIGH + 5 MEDIUM + 1 P2). All addressed: [HIGH — Gemini] Decision 7 contradicted reality. README + CHANGELOG + package.json all show `autopg` and `pgserve` as INTERCHANGEABLE CLI bins — both ship in package.json bin, both route to the same dispatcher. Earlier draft incorrectly claimed `autopg` was "never as a CLI verb." Flipped Decision 7 to match reality: both bins coexist; new verbs land for both; examples use `pgserve <verb>` for consistency with PRs #86–#92 but `autopg <verb>` is equivalent at runtime. No rename, no bin removal in this wish. [MEDIUM — Gemini] `~/.pgserve/` vs `~/.autopg/` inconsistency. Reality on main: both exist. autopg houses admin.json + settings.json (newer cohort schema); pgserve houses trust + audit (authored in PRs #87 + #90 before the split was rationalized). Added Decision 9 documenting the split + explicitly OUTING migration as scope of a future wish. G3's per-consumer state goes under `~/.autopg/<sanitized-slug>/...` to match autopg posture; trust/audit references stay at `~/.pgserve/` to match shipped code. [MEDIUM — Gemini] install.sh 404 doesn't help operators with bookmarked URLs. Replaced "let it 404" plan with a tiny ≤15-line shim at `install.sh` that prints a deprecation note + new URL on stderr and exits 0. Acceptance criterion added. [MEDIUM — Gemini] `<scope>` directory naming was ambiguous — `@demo/app` could be read as nested. Pinned the sanitization rule to `src/provision/db-naming.js#sanitizeSlug` (lowercase + non-alnum → '_' + collapse + trim), so `@demo/app` becomes `demo_app` (one flat dir). [MEDIUM — Gemini] State across 3 locations (autopg_meta + admin.json + manifest) without a stated source-of-truth invited desync. Pinned `autopg_meta` as authoritative; admin.json + manifest are derived caches; `pgserve doctor` reports divergences and `--fix` regenerates the caches from the table. Documented in the deliverable. [MEDIUM — Gemini] G5 smoke test used `npx pgserve@latest install` — that tests the PUBLISHED version, not the changes about to be released. Switched to local-build install (npm pack + npx local tarball, or worktree wrapper directly). [P2 — Codex] G4 validation `grep -E … docs/` passes a directory without `-r`, returning exit 2 ("Is a directory"). Added `-r` flag. `genie wish lint autopg-distribution-cutover-finalize`: clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced May 9, 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
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.
Summary
Closes Singleton G3. Idempotent provisioner — composes the merged G3 foundations into a working `pgserve provision`.
What ships
Decisions captured
Exit codes
Test plan
Cohort closure
After this lands:
Singleton G3 fully closed. Unblocks G7 (roles + GRANTs schema, depends on G3).
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
pgserve provisionCLI command for idempotent database and role provisioning with fingerprint tracking.servecommand now routes directly to postgres supervision (postmaster).Tests