Repository navigation
fix(graphile-meta): count only generated values as system columns in junction detection - #1943
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟡 4 medium 💬 Inline comments (2)
📍 Findings outside the diff (2) — 🟡 2 medium — defects on lines GitHub can't attach comments to🟡 Medium — Unvalidated // agentic/db-tools/src/tools/provision-database.ts
14 database_name: z
15 .string()
16 .describe(
17 'Name for the app database (lowercase, alphanumeric + underscores). Doubles as the deterministic provisioning subdomain (api-<name>/auth-<name>/app-<name>.localhost), so keep it short and URL-safe.',
18 ),The 🟡 Medium — Account-mismatch check hides db-suspended state · context.ts:214–229 · // agentic/db-tools/src/context.ts
214 if (
215 options.plane !== 'data' &&
216 probe.ownerId &&
217 sessionUserId &&
218 probe.ownerId !== sessionUserId
219 ) {
220 return {
221 context: null,
222 reason: `The bound database (DATABASE_ID=${databaseId}) was provisioned under a different account than the one signed in. ${reprovisionAdvice(host)}`,
223 code: 'account-mismatch',
224 };
225 }A suspended database's probe result carries 🧹 Nitpicks (1) — 🟢 1 low
The change spans two areas: graphile-meta gains form-facing metadata (check-constraint allowed values, per-field writability/required-on-create, many-to-many junction exposure) with matching tests and snapshot updates, and agentic/db-tools hardens the provisioning pipeline (project lock, pending-ticket resume, suspended/account-mismatch classification, env-file writing) plus a new schema-model reader and composed-app codegen flow.
Reviewed commit: 126d36c |
There was a problem hiding this comment.
This PR adds form metadata to the _meta GraphQL schema (allowed values, required-on-create/writability flags, junction detection) and extends agentic/db-tools with provisioning guards, resume handling, an api-key/db-probe rework, and a schema-model reader.
Key findings
- 🟡 Per-project lock is process-local;
.envwrites non-atomic and world-readable — provision.ts:131 - 🟡 Unvalidated
database_namewritten unquoted into.env— provision-database.ts:14 - 🟡 Empty quoted env value swallows the following lines — env-file.ts:39
- 🟡 Account-mismatch check hides db-suspended state — context.ts:214
| await writeFile(envPath, edit(await readText(envPath))); | ||
| } |
There was a problem hiding this comment.
🟡 bug · medium
Per-project lock is process-local; .env writes non-atomic and world-readable
withProjectLock serializes callers through a module-local Map (agentic/db-tools/src/provision-database/provision.ts:166-180), so two OS processes (CLI + server, or two agent sessions) provisioning the same project each acquire their own "lock" and interleave their .env read-modify-writes via the non-atomic updateEnvFile (provision.ts:131). A crash mid-write also leaves a truncated .env, destroying unrelated user keys and the DATABASE_ID binding the resume logic depends on — prompting a second database, the exact accident the lock claims to prevent. Additionally, when .env does not yet exist the write creates it with default umask permissions, putting the ACCESS_TOKEN bearer in a typically world-readable file.
📋 Prompt for AI Agents
In agentic/db-tools/src/provision-database/provision.ts: (1) rewrite updateEnvFile (lines 130-132) to read the existing file's mode (default 0o600), write the edited content to ${envPath}.tmp-${process.pid}, and rename it over envPath so every .env transition is crash-atomic and the ACCESS_TOKEN bearer is not world-readable. (2) Extend withProjectLock (lines 166-180) to also acquire a cross-process advisory lock on a lock file under the project cwd (e.g. proper-lockfile or O_EXCL create with retry) before running fn, releasing it in the finally, because the module-level Map only serializes calls within one process.
| if (QUOTES.has(value[0]) && !value.slice(1).includes(value[0])) { | ||
| while (i + 1 < lines.length) { | ||
| span.push(lines[++i]); | ||
| if (lines[i].includes(value[0])) break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 bug · medium
Empty quoted env value swallows the following lines
In envEntries, the multiline-quoted-value detection treats KEY='' (and any value whose first char is a quote with no closing quote on the same line) as an open quoted span (agentic/db-tools/src/provision-database/env-file.ts:39-44): value.slice(1) of '' contains no quote, so the span loop consumes every following line until one happens to contain a quote character or EOF. mergeEnv, removeEnvKeys and archiveBindingKeys then rewrite or delete that whole span, clobbering unrelated user keys during any pending-ticket clear, binding write, or reprovision archive.
📋 Prompt for AI Agents
In agentic/db-tools/src/provision-database/env-file.ts envEntries lines 39-44, replace the multiline detection QUOTES.has(value[0]) && !value.slice(1).includes(value[0]) with a check that counts occurrences of value[0] in the whole value (an even count > 0 means closed on the same line) so KEY='' and KEY="" are single-line entries. Also stop the span loop at EOF instead of silently absorbing the rest of the file for an unterminated quote. Add unit cases for KEY='', KEY="", and unterminated quotes.
…junction detection
126d36c to
e980be6
Compare
Summary
Rebased onto
mainnow that #1919 has merged. The diff is this one commit.isSystemColumntreated anyhasDefaultcolumn as system, so a link table with a value column likequantity integer NOT NULL DEFAULT 1was reported asisJunction: true. Now a column counts as system only when the database generates its value:How
generatedValueis set. The introspection doesn't includepg_attrdef, so apgIntrospection_introspectiongather hook runs one query (adrelid, adnum, adbinfor tables with defaulted columns), andpgCodecs_attributesets the flag. Nothing new is exposed in_meta.Why this signal.
isConstantDefaultis an allow-list on theadbinnode tree:CONST, plus casts around constants (RELABELTYPE,COERCEVIAIO,COERCETODOMAIN,ARRAYCOERCEEXPR,ARRAYEXPR, andFUNCEXPRonly when it's an explicit or implicit cast, such asnumeric(10,2) DEFAULT 0).now(),gen_random_uuid()), SQL value functions (CURRENT_TIMESTAMP,current_user), operators and unknown nodes. Serial counts too, vianextval(...). Identity and generated columns come fromattidentity/attgenerated.Column names are never checked. Treating every function call as generated is the safe direction: a constant built from a function call (
DEFAULT lower('X')) means a junction is missed, never wrongly reported.Tests (
graphile/graphile-settings/__tests__/meta-form-fields.integration.test.ts,form-meta.test.ts):order_items(order_id, product_id, quantity DEFAULT 1)→isJunction: false, nomanyToMany. This fails before the fix.product_locationswithbigserial, identity, a stored generated column,CURRENT_TIMESTAMPandcurrent_user→ still a junction.default_kinds: the classifier runs against realadbinvalues for literals, casts and generated defaults.Publish after merge:
graphile-meta, andgraphile-settingsto pick up the new version.Link to Devin session: https://app.devin.ai/sessions/79f7fb78569b4a2cacec066edd47b62a
Open in Devin Desktop: https://app.devin.ai/desktop/session/79f7fb78569b4a2cacec066edd47b62a?variant=devin
Requested by: @pyramation