Dev - #2435
Dev#2435
Conversation
… matrix (Goal A G4) Flip GENIE_ROLE_CUTOVER to DEFAULT-ON behind the documented kill-switch (=0 forces legacy postgres/postgres). Centralize the gate literal in a single isRoleCutoverEnabled() so db.ts and role-cutover.ts cannot drift. All Wave 2 safety properties preserved: direct-postmaster rebind only, non-hard-fail fallback, per-fingerprint sentinel, test-mode/FORCE_TCP skip. - src/lib/role-cutover.ts: isRoleCutoverEnabled() (=0 kill-switch) + read-only inspectRoleCutover() snapshot (no DB, no FS writes) - src/lib/db.ts: both gate sites use isRoleCutoverEnabled() - src/genie-commands/doctor.ts + src/genie.ts: read-only 'genie doctor --connection-identity' (role, rolsuper, db, grants, fallback flag+reason, sentinel) — SELECT-only, zero mutations - src/lib/role-cutover.matrix.test.ts: hardening matrix — N=8 concurrent single-provision, fallback trio, multi-fingerprint, test-mode/FORCE_TCP skip, kill-switch + default-on contract Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two confirmed root causes in the embed TUI render path. Root cause 1 (ASCII mirror): paintXtermBufferToFrame walked IBuffer.getLine(y) from y=0, which is the OLDEST scrollback line, so the pane froze on ancient output while the live screen sat at viewportY. Offset the source line by xtermBuffer.viewportY (== baseY for a follow terminal, honours user scrollback otherwise). Root cause 2 (slow as fuck): TerminalPaneCore.paintInto ran a full O(rows x cols) setCell blit on every OpenTUI frame (30-60 fps), unconditionally. xterm-headless 5.5.0 exposes no parse/render event (verified at runtime: only onBell/onBinary/onCursorMove/onData/ onLineFeed/onResize/onScroll/onTitleChange -- neither onWriteParsed nor onRender exists), so dirty is armed in xterm's write(data, cb) parse-complete callback (all content flows through this core's own write calls) and directly on synchronous resize reflow. paintInto early-returns the cached painted-cell count without walking the buffer when not dirty; first paint is unconditional. Regression tests: viewport-tail paint (fails against getLine(y)) and the dirty gate (no-op frame issues zero setCell; repaints after a write). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(tui-host): embed render — live viewport + dirty-gate (real-use disaster)
The following vulnerabilities are fixed with an upgrade: - https://snyk.io/vuln/SNYK-JS-SYSTEMINFORMATION-16677388
The following vulnerabilities are fixed with an upgrade: - https://snyk.io/vuln/SNYK-JS-SYSTEMINFORMATION-16677388
… 3 G4) The pgserve v2 smoke runs genie against a TRULY fresh pgserve postgres DB. With default-on, the first runMigrations executes as the scoped NOSUPERUSER/NOCREATEROLE role; migrations 039/041 CREATE EXTENSION pgcrypto (untrusted ⇒ superuser-only) and 041/043 CREATE ROLE events_*/executors_reader (NOCREATEROLE) — all denied to the scoped role on a cold DB. Warm DBs (local + CI PG-Tests template clones) already have these so IF-NOT-EXISTS guards skip them; Group 3's replay test pre-seeded them and missed the gap. Fix (least-privilege preserved — scoped role unchanged): on the cutover path, run ensurePrivilegedBootstrapObjects() on the BOOTSTRAP SUPERUSER connection BEFORE the rebind — idempotent CREATE EXTENSION pgcrypto + cluster RBAC/executor roles + ALTER SCHEMA public OWNER TO <scoped role> (the proven Group-3 prerequisite set). The privileged one-time setup is done by the superuser who legitimately may; the scoped role never gains SUPERUSER/CREATEROLE. Defensive: any failure ⇒ existing role-cutover fallback (stay on bootstrap superuser pool, migrations run as superuser exactly like legacy, never hard-fail boot). Kill-switch path unchanged (early return before pre-ensure). Regression guard: role-cutover.migration-replay.test.ts gains a TRULY-fresh-DB block — negative (scoped role FAILS the cold replay without the bootstrap, the exact gap Group 3 missed) + positive (full set replays clean as the scoped role WITH ensurePrivilegedBootstrapObjects, rolsuper still false). Would have caught this before default-on. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…on ordering Records the Felipe-approved design implemented in 48ba9ab: migrations run on the privileged bootstrap connection first, scoped-role rebind after (runtime only). The original "migrations as scoped role / before runPostConnectSetup" wording was infeasible (genie migrations 039/041/043 issue CREATE EXTENSION + CREATE ROLE). Doc-accuracy only; no code change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ult-on feat(db): genie-dedicated-role-cutover Wave 3 — default-on + doctor + matrix (Goal A G4)
📝 WalkthroughWalkthroughThe PR advances the dedicated-role cutover feature to default-on Wave 3 behavior with centralized gating, privileged bootstrap setup before rebind, read-only inspection surface, and comprehensive hardening tests. It also optimizes terminal rendering via dirty-state tracking to avoid redundant buffer repaints. Version metadata is updated across manifests. ChangesDedicated-role cutover rollout
Terminal render optimization
Release metadata and version updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes The PR combines two substantial features (role-cutover rollout with hardened gating and privileged bootstrap, terminal render optimization with viewport fixes) plus comprehensive test coverage. The role-cutover work is intricate, touching enablement gates, error fallbacks, event emission, and sentinel management across multiple files. The terminal optimization is logic-dense but narrower in scope. Together, heterogeneous changes across DB connection paths, CLI commands, TUI rendering, and test suites demand careful review of gating semantics, fallback behavior, and cross-component interactions. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the dedicated-role cutover to be enabled by default and introduces a --connection-identity flag to the doctor command for read-only inspection of the database identity. It also includes performance optimizations for the TUI rendering via a dirty-checking gate and ensures that privileged database objects are created by a superuser before rebinding to a restricted role. Feedback from the reviewer focuses on improving database query safety and idiomatic consistency by replacing sql.unsafe calls with tagged template literals.
| // postgres.js Sql bleed-through — getConnection() returns `any` by design. | ||
| // biome-ignore lint/suspicious/noExplicitAny: postgres.js Sql type bleed-through | ||
| const sql = (await getConnection()) as any; | ||
| const [whoRow] = await sql.unsafe('SELECT current_user AS who, current_database() AS db'); |
There was a problem hiding this comment.
It is more idiomatic to use tagged template literals with postgres.js instead of sql.unsafe for static queries.
| const [whoRow] = await sql.unsafe('SELECT current_user AS who, current_database() AS db'); | |
| const [whoRow] = await sql`SELECT current_user AS who, current_database() AS db`; |
| const roleRows = await sql.unsafe(`SELECT rolsuper, rolcanlogin FROM pg_roles WHERE rolname = '${roleName}'`); | ||
| if (roleRows.length === 0) return { ...db, roleExists: false }; | ||
| const [g] = await sql.unsafe( | ||
| `SELECT has_database_privilege('${roleName}', current_database(), 'CONNECT') AS db_connect, | ||
| has_schema_privilege('${roleName}', 'public', 'USAGE') AS sch_usage, | ||
| has_schema_privilege('${roleName}', 'public', 'CREATE') AS sch_create`, | ||
| ); | ||
| const [t] = await sql.unsafe( | ||
| `SELECT count(*)::int AS total, | ||
| count(*) FILTER ( | ||
| WHERE has_table_privilege('${roleName}', format('%I.%I', schemaname, tablename), 'SELECT') | ||
| )::int AS selectable | ||
| FROM pg_tables WHERE schemaname = 'public'`, | ||
| ); |
There was a problem hiding this comment.
Using sql.unsafe with string interpolation for variables like roleName is generally discouraged. While roleName is validated against a regex here, it is more idiomatic and robust to use postgres.js tagged template literals for parameterization, which handles escaping and type conversion automatically.
| const roleRows = await sql.unsafe(`SELECT rolsuper, rolcanlogin FROM pg_roles WHERE rolname = '${roleName}'`); | |
| if (roleRows.length === 0) return { ...db, roleExists: false }; | |
| const [g] = await sql.unsafe( | |
| `SELECT has_database_privilege('${roleName}', current_database(), 'CONNECT') AS db_connect, | |
| has_schema_privilege('${roleName}', 'public', 'USAGE') AS sch_usage, | |
| has_schema_privilege('${roleName}', 'public', 'CREATE') AS sch_create`, | |
| ); | |
| const [t] = await sql.unsafe( | |
| `SELECT count(*)::int AS total, | |
| count(*) FILTER ( | |
| WHERE has_table_privilege('${roleName}', format('%I.%I', schemaname, tablename), 'SELECT') | |
| )::int AS selectable | |
| FROM pg_tables WHERE schemaname = 'public'`, | |
| ); | |
| const roleRows = await sql`SELECT rolsuper, rolcanlogin FROM pg_roles WHERE rolname = ${roleName}`; | |
| if (roleRows.length === 0) return { ...db, roleExists: false }; | |
| const [g] = await sql` | |
| SELECT has_database_privilege(${roleName}, current_database(), 'CONNECT') AS db_connect, | |
| has_schema_privilege(${roleName}, 'public', 'USAGE') AS sch_usage, | |
| has_schema_privilege(${roleName}, 'public', 'CREATE') AS sch_create | |
| `; | |
| const [t] = await sql` | |
| SELECT count(*)::int AS total, | |
| count(*) FILTER ( | |
| WHERE has_table_privilege(${roleName}, format('%I.%I', schemaname, tablename), 'SELECT') | |
| )::int AS selectable | |
| FROM pg_tables WHERE schemaname = 'public' | |
| `; |
| // no-op when the scoped role later replays them. | ||
| await sql.unsafe('CREATE EXTENSION IF NOT EXISTS pgcrypto'); | ||
| for (const role of PRIVILEGED_BOOTSTRAP_ROLES) { | ||
| const exists = await sql.unsafe(`SELECT 1 FROM pg_roles WHERE rolname = '${role}'`); |
There was a problem hiding this comment.
Prefer using tagged template literals for queries with parameters instead of sql.unsafe with string interpolation.
| const exists = await sql.unsafe(`SELECT 1 FROM pg_roles WHERE rolname = '${role}'`); | |
| const exists = await sql`SELECT 1 FROM pg_roles WHERE rolname = ${role}`; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aaab5ab13
ℹ️ 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".
| function deriveFallbackReason(inspection: RoleCutoverInspection, db: ConnectionIdentityDb): string | undefined { | ||
| if (!inspection.enabled) return 'kill-switch engaged (GENIE_ROLE_CUTOVER=0)'; | ||
| if (!inspection.roleName) return 'fingerprint-unstable (cannot derive scoped role)'; | ||
| if (!db.reachable) return `db-unreachable: ${db.error ?? 'unknown'}`; | ||
| if (db.roleExists === false) return 'role not provisioned in pg_roles'; | ||
| if (db.currentUser !== inspection.roleName) return `connected as ${db.currentUser} (legacy postgres path)`; | ||
| return undefined; |
There was a problem hiding this comment.
deriveFallbackReason returns undefined as soon as current_user matches the resolved role, even if that role is still rolsuper=true. In that state cutoverActive is false, but the command reports no fallback reason (and text mode can show both cutover active: no and fallback active: no), which hides a failed least-privilege cutover from operators. Add an explicit rolsuper !== false fallback reason so JSON/text output consistently flags this degraded state.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/role-cutover.migration-replay.test.ts (1)
185-200:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake this test self-contained.
This case depends on the previous test leaving the shared session in
SET ROLE "${ROLE}". If it runs alone, after a failure, or under randomized ordering,current_userispostgresand the assertion becomes flaky. Re-enter the role in this test andRESET ROLEin afinally.🤖 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/role-cutover.migration-replay.test.ts` around lines 185 - 200, This test is flaky because it assumes the session is already in SET ROLE "${ROLE}"; fix it by explicitly setting the role at the start of the test with replay.unsafe(`SET ROLE "${ROLE}"`) and wrap the test DML and assertions in a try/finally where the finally block always calls replay.unsafe('RESET ROLE') to restore the session; update the existing current_user checks (using replay.unsafe and ROLE) to run inside the try after setting the role. Use the existing symbols replay.unsafe, ROLE and the same SELECT/EXPECT logic—only add the SET ROLE before those steps and ensure RESET ROLE is executed in finally.
🤖 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/genie-commands/doctor.ts`:
- Around line 897-902: queryConnectionIdentityDb currently calls
getConnection(), which triggers genie's full bootstrap (migrations, sentinel
writes, role provisioning) and can mutate state; change it to obtain a truly
read-only DB handle instead. Replace the getConnection() call inside
queryConnectionIdentityDb with a direct, minimal Postgres client created
specifically for read-only inspection (or call a new/alternative helper like
getReadOnlyConnection or getConnection({ readOnly: true }) if available) that
uses the connection string/config but does not run bootstrap logic, then run the
SELECTs against that client and close it; ensure no code paths trigger
migrations, sentinel writes or role provisioning from inside
queryConnectionIdentityDb.
In `@src/lib/role-cutover.matrix.test.ts`:
- Around line 365-376: Replace the Promise.all usage in the concurrent boot
checks with Promise.allSettled so the test captures every resolve/reject from
the fan-out; specifically change the call that currently assigns results from
Promise.all(Array.from({ length: N }, () => resolveScopedConnectionIdentity({
sql, database, roleName: ROLE, fingerprintHex: FP, enabled: true, sink: () =>
{}, }))) to use Promise.allSettled(Array.from(...)) and then assert that every
settled result has status "fulfilled" (and optionally map to their .value for
further inspection), applying the same change to the second occurrence that fans
out resolveScopedConnectionIdentity in the block around the other test. Ensure
you still use N, ROLE, FP and the same argument object when building the
Array.from iterator.
- Around line 50-58: The withEnv helper currently tries to "unset" environment
variables by assigning undefined to process.env[key], which results in the
string "undefined" instead of removing the variable; update the withEnv function
(the helper named withEnv) to use delete process.env[key] when setting/unsetting
both the temporary value and when restoring the previous value so the
environment is truly removed in the unset branches.
In `@src/lib/role-cutover.ts`:
- Around line 367-370: The SELECT-then-CREATE_ROLE loop is racy; replace the
two-statement pattern around PRIVILEGED_BOOTSTRAP_ROLES with an atomic
server-side operation (either a single "CREATE ROLE IF NOT EXISTS ..." if your
Postgres version supports it, or a DO $$ BEGIN IF NOT EXISTS (SELECT 1 FROM
pg_roles WHERE rolname = role) THEN CREATE ROLE role NOINHERIT; END IF; END $$;
) so role creation cannot race between processes; alternatively wrap the CREATE
ROLE in a try/catch and explicitly ignore the duplicate-object SQLSTATE
('42710') error instead of falling back to legacy behavior. Ensure changes touch
the loop that references PRIVILEGED_BOOTSTRAP_ROLES and the code path that
currently calls sql.unsafe for the SELECT and CREATE ROLE.
---
Outside diff comments:
In `@src/lib/role-cutover.migration-replay.test.ts`:
- Around line 185-200: This test is flaky because it assumes the session is
already in SET ROLE "${ROLE}"; fix it by explicitly setting the role at the
start of the test with replay.unsafe(`SET ROLE "${ROLE}"`) and wrap the test DML
and assertions in a try/finally where the finally block always calls
replay.unsafe('RESET ROLE') to restore the session; update the existing
current_user checks (using replay.unsafe and ROLE) to run inside the try after
setting the role. Use the existing symbols replay.unsafe, ROLE and the same
SELECT/EXPECT logic—only add the SET ROLE before those steps and ensure RESET
ROLE is executed in finally.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ebae31ac-eae5-4735-a371-4aa8d2dd51e6
⛔ Files ignored due to path filters (5)
.genie/assets/commits-30d.svgis excluded by!**/*.svg.genie/assets/loc-30d.svgis excluded by!**/*.svg.genie/assets/releases-30d.svgis excluded by!**/*.svgREADME.mdis excluded by!*.mdVELOCITY.mdis excluded by!*.md
📒 Files selected for processing (18)
.claude-plugin/marketplace.json.genie/agents/metrics-updater/daily-stats.jsonl.genie/agents/metrics-updater/runs.jsonl.genie/agents/metrics-updater/state.json.genie/wishes/genie-dedicated-role-cutover/WISH.md.well-known/dev.jsonpackage.jsonplugins/genie/.claude-plugin/plugin.jsonplugins/genie/package.jsonsrc/genie-commands/doctor.tssrc/genie.tssrc/lib/db.tssrc/lib/role-cutover.matrix.test.tssrc/lib/role-cutover.migration-replay.test.tssrc/lib/role-cutover.tssrc/tui/widgets/TerminalPane.tsxsrc/tui/widgets/__tests__/TerminalPane.test.tsxsrc/tui/widgets/xterm-cell-paint.ts
| async function queryConnectionIdentityDb(roleName: string | null): Promise<ConnectionIdentityDb> { | ||
| try { | ||
| // postgres.js Sql bleed-through — getConnection() returns `any` by design. | ||
| // biome-ignore lint/suspicious/noExplicitAny: postgres.js Sql type bleed-through | ||
| const sql = (await getConnection()) as any; | ||
| const [whoRow] = await sql.unsafe('SELECT current_user AS who, current_database() AS db'); |
There was a problem hiding this comment.
--connection-identity is not actually read-only.
getConnection() runs genie's normal bootstrap path before these SELECTs, so this doctor command can provision the role-cutover, write the sentinel, and run migrations on first use. That lets the inspection mutate the state it is supposed to diagnose and can hide the very bootstrap failures this surface is meant to expose.
🤖 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/genie-commands/doctor.ts` around lines 897 - 902,
queryConnectionIdentityDb currently calls getConnection(), which triggers
genie's full bootstrap (migrations, sentinel writes, role provisioning) and can
mutate state; change it to obtain a truly read-only DB handle instead. Replace
the getConnection() call inside queryConnectionIdentityDb with a direct, minimal
Postgres client created specifically for read-only inspection (or call a
new/alternative helper like getReadOnlyConnection or getConnection({ readOnly:
true }) if available) that uses the connection string/config but does not run
bootstrap logic, then run the SELECTs against that client and close it; ensure
no code paths trigger migrations, sentinel writes or role provisioning from
inside queryConnectionIdentityDb.
| function withEnv<T>(key: string, value: string | undefined, fn: () => T): T { | ||
| const prev = process.env[key]; | ||
| if (value === undefined) process.env[key] = undefined; | ||
| else process.env[key] = value; | ||
| try { | ||
| return fn(); | ||
| } finally { | ||
| if (prev === undefined) process.env[key] = undefined; | ||
| else process.env[key] = prev; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Bun/Node.js, does assigning process.env.FOO = undefined remove the environment variable, or coerce it to a string value? What is the correct way to unset it?
💡 Result:
In both Node.js and Bun, assigning undefined to a property on process.env does not remove the environment variable [1][2][3]. Instead, the value is coerced into the string "undefined" [1][2][3]. The correct and only way to remove an environment variable from process.env is to use the delete operator [1][2][3]: delete process.env.FOO; Using delete removes the property entirely from the process.env object, which effectively unsets the environment variable [2][3]. Assigning null or undefined will simply result in the environment variable being set to the string "null" or "undefined", respectively [1][2].
Citations:
- 1: Setting
undefinedtoprocess.env.*sets a"undefined"string value nodejs/node#48875 - 2: https://stackoverflow.com/questions/42170365/how-do-i-remove-a-value-in-process-env/42170366
- 3: https://stackoverflow.com/questions/43314209/node-js-unset-environment-variable
Unset env vars with delete, not = undefined.
Assigning undefined to process.env does not unset the variable—it coerces the value to the string "undefined". This means the "unset" branch of this matrix test silently exercises a different code path than intended. Use the delete operator instead:
delete process.env[key];
This applies to lines 52, 55, and 57 in the withEnv helper.
🤖 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/role-cutover.matrix.test.ts` around lines 50 - 58, The withEnv helper
currently tries to "unset" environment variables by assigning undefined to
process.env[key], which results in the string "undefined" instead of removing
the variable; update the withEnv function (the helper named withEnv) to use
delete process.env[key] when setting/unsetting both the temporary value and when
restoring the previous value so the environment is truly removed in the unset
branches.
| const results = await Promise.all( | ||
| Array.from({ length: N }, () => | ||
| resolveScopedConnectionIdentity({ | ||
| sql, | ||
| database, | ||
| roleName: ROLE, | ||
| fingerprintHex: FP, | ||
| enabled: true, | ||
| sink: () => {}, | ||
| }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use Promise.allSettled() in the concurrent-boot checks.
With Promise.all(), the first rejection aborts the whole fan-out, so these tests never prove the stated property that all 8 boots completed and none threw. allSettled() lets you assert zero rejections and still inspect every result. As per coding guidelines, "Use Promise.allSettled() pattern for concurrency tests".
Also applies to: 407-418
🤖 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/role-cutover.matrix.test.ts` around lines 365 - 376, Replace the
Promise.all usage in the concurrent boot checks with Promise.allSettled so the
test captures every resolve/reject from the fan-out; specifically change the
call that currently assigns results from Promise.all(Array.from({ length: N },
() => resolveScopedConnectionIdentity({ sql, database, roleName: ROLE,
fingerprintHex: FP, enabled: true, sink: () => {}, }))) to use
Promise.allSettled(Array.from(...)) and then assert that every settled result
has status "fulfilled" (and optionally map to their .value for further
inspection), applying the same change to the second occurrence that fans out
resolveScopedConnectionIdentity in the block around the other test. Ensure you
still use N, ROLE, FP and the same argument object when building the Array.from
iterator.
| for (const role of PRIVILEGED_BOOTSTRAP_ROLES) { | ||
| const exists = await sql.unsafe(`SELECT 1 FROM pg_roles WHERE rolname = '${role}'`); | ||
| if (exists.length === 0) { | ||
| await sql.unsafe(`CREATE ROLE "${role}" NOINHERIT`); |
There was a problem hiding this comment.
Make bootstrap role creation atomic.
This SELECT-then-CREATE ROLE pair races under concurrent cold boots: two processes can both observe the role as missing, then one CREATE ROLE throws duplicate_object and falls back to the legacy superuser path. That breaks the helper's idempotent/convergent contract exactly when the rollout is under contention.
Suggested fix
for (const role of PRIVILEGED_BOOTSTRAP_ROLES) {
- const exists = await sql.unsafe(`SELECT 1 FROM pg_roles WHERE rolname = '${role}'`);
- if (exists.length === 0) {
- await sql.unsafe(`CREATE ROLE "${role}" NOINHERIT`);
- }
+ await sql.unsafe(`
+ DO $$
+ BEGIN
+ CREATE ROLE "${role}" NOINHERIT;
+ EXCEPTION
+ WHEN duplicate_object THEN NULL;
+ END
+ $$;
+ `);
}📝 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.
| for (const role of PRIVILEGED_BOOTSTRAP_ROLES) { | |
| const exists = await sql.unsafe(`SELECT 1 FROM pg_roles WHERE rolname = '${role}'`); | |
| if (exists.length === 0) { | |
| await sql.unsafe(`CREATE ROLE "${role}" NOINHERIT`); | |
| for (const role of PRIVILEGED_BOOTSTRAP_ROLES) { | |
| await sql.unsafe(` | |
| DO $$ | |
| BEGIN | |
| CREATE ROLE "${role}" NOINHERIT; | |
| EXCEPTION | |
| WHEN duplicate_object THEN NULL; | |
| END | |
| $$; | |
| `); | |
| } |
🤖 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/role-cutover.ts` around lines 367 - 370, The SELECT-then-CREATE_ROLE
loop is racy; replace the two-statement pattern around
PRIVILEGED_BOOTSTRAP_ROLES with an atomic server-side operation (either a single
"CREATE ROLE IF NOT EXISTS ..." if your Postgres version supports it, or a DO $$
BEGIN IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = role) THEN CREATE
ROLE role NOINHERIT; END IF; END $$; ) so role creation cannot race between
processes; alternatively wrap the CREATE ROLE in a try/catch and explicitly
ignore the duplicate-object SQLSTATE ('42710') error instead of falling back to
legacy behavior. Ensure changes touch the loop that references
PRIVILEGED_BOOTSTRAP_ROLES and the code path that currently calls sql.unsafe for
the SELECT and CREATE ROLE.
3 participants
Summary by CodeRabbit
New Features
genie doctor --connection-identityflag providing read-only diagnostics of role cutover configuration and identity statusGENIE_ROLE_CUTOVER=0as kill-switchImprovements
Chores