Fix SQLite/Postgres schema column parity checks - #2346
Conversation
The schema parity suite compared table names only, allowing missing SQLite columns to ship undetected. Changes: - Parse Postgres CREATE TABLE and ALTER TABLE column declarations - Compare every shared Postgres column against fresh SQLite schema columns - Keep the #2318 allow_env_keys exception exact and retain parent_run_id index coverage Fixes #2319
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR extends schema parity tests to compare columns, not just table names, between the PostgreSQL migration and SQLite schema. New parsing helpers handle SQL comments, quotes, and nested parentheses to extract table and column definitions reliably. Tests validate bidirectional column parity and allowlist accuracy. ChangesSchema Parity Column Checks
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
✨ 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 install failed: dependency version conflict. Check your lock file or package.json. 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 |
Consolidated Review: PR #2346Date: 2026-07-30T14:23:00Z Executive SummaryThis is a focused, test-only schema-parity change with no production, configuration, or user-facing documentation impact. The PostgreSQL-to-SQLite comparison is deterministic, keeps the known Three requested reviewer artifacts were not available at synthesis time ( Overall Verdict: REQUEST_CHANGES Auto-fix Candidates: 0 CRITICAL + HIGH issues can be auto-fixed Statistics
* Artifact unavailable at synthesis time. CRITICAL Issues (Must Fix)None identified in the available artifacts. HIGH Issues (Should Fix)None identified in the available artifacts. MEDIUM Issues (Options for User)Issue 1: Upgrade-parser anti-vacuity assertion is not independentSource Agent: code-review Problem: The asserted Options:
Recommendation: Fix now. It is a minimal test-only correction that directly validates the behavior this PR adds. const codebaseColumns = postgresColumns.get('remote_agent_codebases');
const userColumns = postgresColumns.get('remote_agent_users');
// Anti-vacuity checks cover a CREATE declaration and a genuinely ALTER-only declaration.
expect(codebaseColumns?.has('allow_env_keys')).toBe(true);
expect(userColumns?.has('role')).toBe(true);LOW Issues (For Consideration)
Recommended wording: * Better Auth's remote_agent_auth_* tables are intentionally Postgres-only
* (web auth never runs on SQLite — see migrateColumns() and CLAUDE.md), so
* the table-parity check excludes that prefix. The separate, exact
* `remote_agent_codebases.allow_env_keys` column exception is tracked by
* #2318; keep it column-specific. A genuinely new Postgres-only table must
* be added to the table allowlist with a justifying comment.Positive Observations
Suggested Follow-up Issues
Next Steps
Agent Artifacts
Metadata
|
Fixed:\n- cover ALTER-only PostgreSQL migration columns in the schema parity test\n- document the table and column parity exceptions\n\nTests added:\n- strengthened existing SQLite schema parity coverage\n\nSkipped:\n- none
⚡ Self-Fix Report (Aggressive)Status: COMPLETE Fixes Applied (2 total)
View all fixes
Tests Added(none — strengthened the existing SQLite schema-parity test) Skipped (0)(none — all findings addressed) Suggested Follow-up Issues(none) Validation✅ Type check | ✅ Lint | ✅ Tests Self-fix by Archon · aggressive mode · fixes pushed to |
pr: 2346
|
| # | Experiment | Result |
|---|---|---|
| 0 | Baseline, unmodified PR branch | 3 pass / 0 fail |
| 1 | Added drift_probe_create TEXT to remote_agent_workflow_events CREATE body |
FAIL — + "remote_agent_workflow_events.drift_probe_create" |
| 2 | Appended ALTER TABLE remote_agent_messages ADD COLUMN IF NOT EXISTS drift_probe_alter TEXT; |
FAIL — + "remote_agent_messages.drift_probe_alter" |
| 3 | Emptied POSTGRES_ONLY_COLUMNS (live-drift acid test) |
FAIL — + "remote_agent_codebases.allow_env_keys" |
| 4 | Added sqlite_only_probe TEXT to remote_agent_messages in sqlite.ts (reverse direction) |
3 pass / 0 fail — not detected |
| 5 | Added in-body SQL comment -- see remote_agent_workflow_runs(id); |
parsed columns 7 → 3; 3 pass / 0 fail — not detected |
| 6 | Wrapped column definition across two lines inside a CREATE body | phantom column references produced |
| 7 | ALTER TABLE … ADD COLUMN probe_no_ine TEXT; (no IF NOT EXISTS) |
silently not parsed |
| 8 | CREATE body terminated )\n; |
following table vanished from the parsed map entirely |
Answers to the three questions posed:
- Does it catch injected drift? Yes, in the PG→SQLite direction, via both declaration routes (Model stucked at response stream text #1, updated code to use locally hosted llama LLM, nomic-embed-text model. #2). No in the reverse direction ([FEATURE] How about bootstrapping the agent builder? #4).
- What does it do against the live
allow_env_keysdivergence? It detects it. The check passes ondevbecause of an allowlist entry, and removing that entry produces exactly that one failure (doesn't know what o1-mini is, or how to route to openrouter.ai #3). This is a satisfied exception, not a silenced check — see the allowlist assessment below. - Names only, or names plus types? Names only, as the issue recommended. That was the right call — see below.
Issues Found
Critical
None.
Important
I1 — Table discovery is now coupled to CREATE-body parsing; a body-parse failure silently drops tables from the table-parity check too
packages/core/src/db/adapters/sqlite.test.ts:431
The old postgresArchonTables() matched CREATE TABLE <name> alone — body-independent. The new expected list derives from postgresArchonColumns().keys() (sqlite.test.ts:431), and a table is only registered there if the full \(([\s\S]*?)\); body match succeeded.
Confirmed (experiment #8): given two tables where the first closes with )\n;, the parser returned only remote_agent_alpha — remote_agent_beta was gone from the map entirely, along with a phantom create column absorbed from the swallowed text.
Failure scenario: this is the PR-2033 drift class — a whole table present in Postgres and missing from SQLite. That is the drift the table-parity test in this file was originally written for. A single body-parse hiccup anywhere in 000_combined.sql re-opens it, and the expect(expected.length).toBeGreaterThan(10) floor at sqlite.test.ts:437 does not catch losing one or two tables out of nineteen.
Fix: keep the original body-independent table-name regex as the source of the table list, and use it to seed the column map:
const tableNameRe = /CREATE TABLE(?:\s+IF NOT EXISTS)?\s+"?([a-z0-9_]+)"?/gi;
for (const m of sql.matchAll(tableNameRe)) {
const t = m[1].toLowerCase();
if (t.startsWith('remote_agent_')) columnsByTable.set(t, new Set());
}
// …then fill columns from the body + ALTER passes as today.Table discovery then cannot regress below what it caught before this PR.
I2 — ); anywhere inside a CREATE body silently truncates the column list, and the guard stays green
packages/core/src/db/adapters/sqlite.test.ts:393
/…\s*\(([\s\S]*?)\);/gi is non-greedy, so the body ends at the first ); — not at the matching close paren.
Confirmed (experiment #5): inserting the ordinary comment -- see remote_agent_workflow_runs(id); into the remote_agent_workflow_events body cut the parsed column set from 7 to 3. step_index, step_name, data and created_at became invisible to the parity check, and the suite still reported 3 pass / 0 fail.
Failure scenario: someone documents an FK in a comment (-- FK to remote_agent_users(id);) — 000_combined.sql already carries in-body comments at lines 179–194, so this is ordinary house style, not a contrived input. Every column below that comment loses parity coverage, permanently and silently. The guard reports success while blind, which is strictly worse than the pre-PR state where nobody believed there was column coverage.
Fix: strip -- comments before matching, and split the body on top-level commas with paren-depth tracking rather than on \n. Depth tracking also fixes I3 and S3 as a side effect. Pair it with the count floor in I3 so a future collapse fails loudly instead of quietly.
I3 — Anti-vacuity guards prove two columns survive, not that the parse is intact
packages/core/src/db/adapters/sqlite.test.ts:450-452
The two assertions are well chosen, and the self-fix that introduced users.role is correct — I verified it is genuinely ALTER-only (the remote_agent_users CREATE body declares 5 columns; the parser reports 6, so the upgrade path is demonstrably contributing).
But they are point checks. Experiment #5 dropped four columns from a different table with both assertions still true.
Fix: add a total-column floor alongside them. The parser currently yields 136 columns across the 15 non-auth tables, so a conservative floor is cheap and catches partial collapse:
const totalColumns = [...postgresColumns]
.filter(([t]) => !t.startsWith(POSTGRES_ONLY_PREFIX))
.reduce((n, [, cols]) => n + cols.size, 0);
expect(totalColumns).toBeGreaterThan(120); // 136 todaySuggestions
S1 — Reverse direction is uncovered, and there is a live instance of it in the repo right now
The check is one-way (PG → SQLite). Experiment #4 confirmed a SQLite-only column passes unnoticed.
Scoping to one direction was defensible — all three drifts named in #2319 point the same way. But while probing I found a fourth, pointing the other way: remote_agent_isolation_environments.updated_at exists in sqlite.ts and not in 000_combined.sql. It is dead (no reader in packages/core/src/db/isolation-environments.ts), which makes it an exact mirror of the allow_env_keys situation.
That matters for cost: I enumerated the residue, and that column is the only reverse-direction difference in the whole schema. There are no SQLite-only tables. So the reverse check is one allowlist entry away from being enforceable today.
Worth weighing: SQLite-only drift is the works-in-dev, breaks-in-production direction — local dev defaults to SQLite, the VPS runs Postgres. Recommend adding the reverse pass in this PR, or filing it explicitly rather than leaving it implied by "out of scope".
S2 — ADD COLUMN without IF NOT EXISTS is invisible to the parser
packages/core/src/db/adapters/sqlite.test.ts:411 requires the literal ADD COLUMN IF NOT EXISTS. Experiment #7 confirmed a plain ADD COLUMN is not parsed.
Low probability — all 20 existing ALTERs use IF NOT EXISTS, since the migration must be idempotent. But a contributor who forgets it breaks the idempotency convention and silently loses parity coverage for that column, with nothing flagging either. Making the clause optional (ADD COLUMN(?:\s+IF NOT EXISTS)?) costs nothing and closes the hole.
S3 — Wrapped column definitions produce a phantom references column
Experiment #6: a column definition wrapped across two lines yields a bogus references entry, because the line-split heuristic reads the continuation line as a new declaration and references is not in TABLE_CONSTRAINTS (sqlite.test.ts:386).
Lower severity than I2 — it produces a false failure, which is loud and fail-safe rather than silent. Same root cause; the depth-tracking fix in I2 resolves it. Note the wrapped style is exactly how every ALTER … ADD COLUMN … REFERENCES in this migration is already written, so it is a natural style for someone to carry into a CREATE body.
S4 — Make the POSTGRES_ONLY_COLUMNS allowlist self-expiring
packages/core/src/db/adapters/sqlite.test.ts:385
The entry is correct today. But when #2318 is resolved by adding the column to SQLite, the entry becomes dead and nothing forces its removal — and a stale allowlist entry is precisely the "place to bury future drift" the issue warns about. Assert that each exempted column is still genuinely absent:
// A resolved exception must be deleted, not left to rot.
for (const q of POSTGRES_ONLY_COLUMNS) {
const [t, c] = q.split('.');
expect(new Set(raw_pragma(currentDbPath, t)).has(c)).toBe(false);
}S5 — Nothing in 000_combined.sql points at the guard
Placement in sqlite.test.ts is correct — the existing table-parity test lives in the same describe block at sqlite.test.ts:363, so this is the right home and the property is discoverable from the file that already owns it. But the person most likely to trip the check is someone editing the migration, and that file says nothing about the guard. A one-line comment near the ALTER block ("columns here are parity-checked against sqlite.ts in sqlite.test.ts") would shorten the trip from red CI to understanding.
Assessment: the allowlist entry for the live drift
Established definitively (experiment #3): the check works; the allowlist is what makes it pass. Emptying POSTGRES_ONLY_COLUMNS produces exactly one failure, remote_agent_codebases.allow_env_keys.
I judge this a satisfied exception rather than a silenced check, for four reasons: it is scoped to one exact table.column rather than a table or prefix; it carries an inline justification and an issue reference (sqlite.test.ts:383-385); it follows the precedent already set by the remote_agent_auth_ table exclusion; and it is the option #2318 itself sanctions ("the new check needs an allowlist entry").
The reservation worth stating: it is uncomfortable for a guard's first act to exempt the only live instance of the class it guards. But the alternative — propagating a dead column into a second dialect purely for symmetry — is worse, and the decision belongs to #2318, which is open. Recommend keeping it, adding S4 so the entry cannot outlive its reason, and noting on #2318 that closing it means deleting this line.
Assessment: names-only was the right recommendation
The issue's call to compare names only, not types, holds up. Every known drift — the three in #2319 plus the reverse-direction isolation_environments.updated_at I found — is a presence drift, not a mistyped one. And a cross-dialect type map would be asserting something SQLite does not enforce anyway: SQLite uses type affinity, so a declared TEXT against a Postgres UUID constrains nothing at runtime. The map would be pure maintenance cost for no additional caught bugs. No change needed here.
Validation Results
| Check | Status | Details |
|---|---|---|
| Type Check | PASS | all 9 packages exit 0 |
| Lint | PASS | 0 warnings (--max-warnings 0) |
| Format | PASS | prettier clean |
Tests (@archon/core, full) |
PASS | all batches exit 0 |
Targeted (sqlite.test.ts) |
PASS | 18 tests, 3 in the parity block |
| CI | PASS | ubuntu, windows, docker-build all SUCCESS |
Strengths
- The core mechanism works. Both declaration routes are genuinely handled — verified independently, not just asserted.
- Failure output is precise and actionable: sorted
table.columnentries, so a red CI run names the exact drift. - The allowlist is column-scoped and issue-referenced, not a broad table or prefix exemption.
- The self-review's
users.roleanti-vacuity fix is substantively correct, not cosmetic — it is genuinely the ALTER-only path. - The constraint-keyword filter works on today's SQL: I dumped all 19 parsed tables and their columns and found no false positives.
- Removing the bespoke
parent_run_idcolumn assertion was right — the generic check subsumes it — while correctly keeping the index assertion, which the column check does not cover. - Scope discipline: one test file, no production changes, no premature shared schema abstraction.
Recommendation
REQUEST CHANGES (posted as a comment — PR is a draft).
I1 and I2 are the blockers, and they are the same class of problem the PR exists to fix: a check that reports success while unable to see. Both are fixed by one change — parse bodies with paren-depth tracking after stripping comments, and keep table discovery on a body-independent regex — plus the count floor in I3 so the next collapse is loud.
S1 is a judgment call worth making explicitly rather than inheriting: the reverse direction costs one allowlist entry today, and there is already a real instance sitting in the repo.
Reviewed by Claude · empirical verification in an isolated worktree, since removed
Follow-up to the column-level parity check. Three ways it could pass while seeing nothing: 1. Table discovery had become a by-product of column-body parsing (the list came from postgresArchonColumns().keys()), so a table only registered if its full `(...);` body matched. A `)\n;` terminator made the *following* table vanish from the map entirely — re-opening the PR #2033 drift class (whole table in the migration, missing from sqlite.ts) that the original table-parity test exists to catch. Table discovery is back on a body-independent `CREATE TABLE <name>` regex. 2. The non-greedy body match ended at the first `);`, including one inside a comment. `migrations/000_combined.sql` already writes `);` in prose (lines 447, 467, 469), so this is house style, not a contrived input: an ordinary `-- see remote_agent_workflow_runs(id);` cut remote_agent_workflow_events from 7 parsed columns to 3 with the suite still green. Comments are now stripped first, and bodies are read with paren-depth tracking and split on top-level commas, so nested parens in REFERENCES / CHECK / DEFAULT clauses cannot end a body early. 3. The anti-vacuity pair only proved two named columns survived parsing — both stayed true while four others silently disappeared. Added a floor on the number of columns actually compared (136 today, verified by execution; the truncating parser scores 132). Also adds the reverse pass. isolation_environments.updated_at exists in sqlite.ts and not in the migration, and is the only reverse-direction difference in the schema with no SQLite-only tables, so the check costs one allowlist entry. Reverse drift is the works-locally / breaks-on-the-Postgres-VPS direction, which is worth a guard. Both allowlists are now self-expiring: each entry must still describe real drift, so fixing #2318 fails the suite until the entry is deleted rather than quietly leaving a live column unprotected. Comparison stays names-only — SQLite type affinity means a declared TEXT against a Postgres UUID constrains nothing, so a type map would add cost without catching a bug.
pr: 2346
|
| # | Injection | Observed |
|---|---|---|
| a | Table body terminated )\n;, bogus column on the following table |
RED — remote_agent_workflow_node_sessions.zz_injected_after_terminator reported |
| b | Comment containing ); inside remote_agent_workflow_events body |
GREEN (correct) — and provably non-lossy: compared count stayed exactly 136 |
| c | Postgres-only column added via a CREATE TABLE body |
RED — remote_agent_user_provider_keys.zz_create_only |
| d | Postgres-only column added via trailing ALTER TABLE … ADD COLUMN |
RED — remote_agent_users.zz_alter_only |
| e | SQLite-only column (reverse pass) | RED — remote_agent_workflow_node_sessions.zz_sqlite_only |
Supplementary, for the strong form of B1 — a table whose body is genuinely
unparseable (unbalanced paren) and absent from sqlite.ts:
every non-auth Postgres table is created by the SQLite schema→ RED, naming
remote_agent_zz_unparseable. Table discovery is body-independent as claimed.- The column parser did not degrade silently: it threw
Unbalanced parentheses in schema SQL at index 9221. Fail-fast, per CLAUDE.md.
Injection (b) deserves a note on method: "green" alone would not prove the comment
was harmless. Re-probing the compared count with the comment in place returned
136, identical to baseline, so no column was dropped. The exact-valued floor is
what makes this verifiable — a happy side effect of the choice criticised below.
Item 2 — Is 136 right, and is it a floor or a trap?
The value is correct: the parser compares exactly 136 non-auth columns today
(verified by forcing the assertion to report the actual count). Per-table audit
confirms all 15 non-auth tables contribute non-zero counts summing to 136, with
remote_agent_workflow_events back at its full 7 — the truncation victim, restored.
But it has zero headroom, and it is asserted first. Two demonstrated consequences:
- Any legitimate column removal fails a test named "every non-auth Postgres column
exists in a fresh SQLite schema" with the messageExpected: >= 136 / Received: 135.
Nothing is missing; the test name misdescribes the failure. - Worse — it can mask real drift. With two legitimate PG-side removals plus one
genuine new drift (zz_real_drift), the floor fired first and the drift list was
never printed. Observed output was onlyExpected: >= 136 / Received: 135. The
actionable finding was invisible in that run.
On the value itself: I recommend keeping 136, not adding headroom. The collapse
this guards against cost only 4 columns (7→3), so a floor of e.g. 130 would miss it.
Exact-count is the right sensitivity. The problem is ordering and messaging, not the
number.
Recommended fix (Important)
packages/core/src/db/adapters/sqlite.test.ts — move the anti-vacuity guard into its
own test so both signals surface independently under truthful names:
test('every non-auth Postgres column exists in a fresh SQLite schema', async () => {
// …
expect(missing.sort()).toEqual([]); // drift first — the actionable list
});
test('the migration parser still sees the whole schema (anti-vacuity)', async () => {
db = createTestDb();
const postgresColumns = postgresArchonColumns();
let compared = 0;
for (const table of postgresArchonTables()) {
if (table.startsWith(POSTGRES_ONLY_PREFIX)) continue;
compared += (postgresColumns.get(table) ?? new Set()).size;
}
if (compared < MIN_NON_AUTH_COLUMNS) {
throw new Error(
`Parsed only ${compared} non-auth columns (floor ${MIN_NON_AUTH_COLUMNS}). ` +
`If columns were intentionally removed, lower MIN_NON_AUTH_COLUMNS; ` +
`otherwise the migration parser is silently dropping columns.`
);
}
});The minimum acceptable alternative is simply swapping the two expect lines so
missing is asserted first — that alone removes the masking. The explicit throw
is what makes the failure self-explaining to the next person.
Item 3 — Quote-awareness
Extracted the four helpers verbatim and exercised them directly. 12/12 semantic
checks pass; 8/8 malformed inputs terminate in 0 ms (no hang, no mis-index, no crash).
Confirmed correct:
- Doubled single quotes (
'it''s fine') — literal does not end early, and a--
inside such a literal is not treated as a comment. - Doubled double quotes inside a quoted identifier (
"we""ird"). - Quoted identifiers containing
,or)neither split a declaration nor skew depth. - String literal containing
)does not close a table body. REFERENCES t(id)/CHECK (b = 1)/DEFAULT NOW()all survive.- Apostrophe in a prose comment (
-- don't do this) does not open a literal — the
comment branch consumes it first. - Line comment terminated by EOF; block comment containing
);.
Malformed-input behaviour is safe in every case: unterminated literal clamps to
sql.length; unterminated block comment consumes to EOF; unbalanced parens throws.
Two theoretical residues, neither reachable from the current file (verified: the
migration has no /* */ blocks, no $$ dollar-quoting, and no line with
an odd apostrophe count outside comments):
- A stray apostrophe outside a literal would swallow the remainder as a string. Both
downstream paths are loud (throw, or the floor), never silent. splitTopLevelCommasletsdepthgo negative on an unmatched). Unreachable —
its only input comes fromreadBalancedParens, which is balanced by construction.
No change requested here. The quote-awareness was not asked for and is correct.
Item 4 — SQLITE_ONLY_COLUMNS self-expiry
Works, in both directions.
- Added
updated_attoremote_agent_isolation_environmentsin the migration →
RED:remote_agent_isolation_environments.updated_at (now exists in the Postgres migration). - Simulated the fix(db): codebases.allow_env_keys exists in Postgres and not SQLite — dead column, live drift #2318 fix (dropped
allow_env_keysfrom the migration) → RED:
remote_agent_codebases.allow_env_keys (no longer in the Postgres migration).
Both messages state the reason, so the required action is obvious. The allowlists are
also minimal and accurate: the per-table audit found exactly two asymmetries across
all 15 non-auth tables, and both are the allowlisted ones.
Note the #2318 case also trips the floor as collateral (Item 2, consequence 1) —
the test's own comment promises this scenario fails "until the allowlist entry is
deleted", and it does, but with one extra confusing failure alongside the clear one.
Item 5 — Did the fix break the pre-existing table-parity test?
No. A well-formed Postgres-only table absent from sqlite.ts produces RED on
every non-auth Postgres table is created by the SQLite schema, naming
remote_agent_zz_missing_table. The B1 regression is genuinely repaired, and the
unparseable-body variant above shows the guard now holds even in the case that
originally broke it.
Item 6 — Anything neither review has examined
Nothing further of substance. Checked and clear: stripSqlComments output indices
stay consistent with the readBalancedParens calls that consume them; the reverse
test's exclusion of remote_agent_auth_* is correct by construction (auth tables
never exist in SQLite) though only implicitly so; TABLE_CONSTRAINTS covers the only
two top-level constraint keywords the migration actually uses (PRIMARY, UNIQUE);
over-parsing is self-catching because a phantom name would surface in missing; and
all 34 quoted identifiers in the file live in excluded Better Auth tables.
Suggestions only:
- S1 — The aggregate floor could in principle be netted out by a per-table collapse
coinciding with growth elsewhere. A per-table minimum map would close it; probably
not worth the maintenance. Note only. - S2 — No
$$dollar-quoting support. None exists today, and adding one would fail
loud rather than silent. Note only. - S3 —
18a1a1da's commit message contains literal\nescape sequences instead of
newlines (template bug in whatever generated it). - S4 — The PR body's validation evidence says
18 passed; the suite is now 20 tests.
Worth refreshing before undrafting.
Validation
| Check | Status | Details |
|---|---|---|
| Type check | PASS | all 6 packages exit 0 |
| Lint | PASS | 0 warnings |
| Format | PASS | prettier clean on the changed file |
| Tests (file) | PASS | 20 pass / 0 fail |
Tests (@archon/core) |
PASS | full batched suite exits 0 |
| CI on PR head | PASS | ubuntu, windows, docker-build |
Strengths
- Body-independent table discovery genuinely restores the PR fix(db): create remote_agent_user_ai_prefs in the SQLite schema #2033 guard — verified
against an unparseable body, the hardest form of the case. readBalancedParensthrows instead of returning a truncated body. Exactly the
fail-fast posture CLAUDE.md asks for, and the reason the silent-truncation class
cannot recur.- Self-expiring allowlists work in all four directions tested, with messages that
name the reason rather than just the column. 91c9d34bcaught and fixed a flaw in18a1a1da: the anti-vacuity probe had been
pointed at an allowlisted column (allow_env_keys), which would have coupled the
probe to the exception. Moving it todefault_cwdwas the right call.- Commit message and inline documentation are unusually good — they explain the
failure mode, cite the real line numbers, and state what was deliberately left out
(type comparison) and why.
Recommendation
NEEDS FIXES — resolve the MIN_NON_AUTH_COLUMNS ordering/messaging issue
(Item 2). Everything else verified correct; no other changes requested.
Reviewed by Claude — second pass, scoped to 18a1a1da+91c9d34b
The MIN_NON_AUTH_COLUMNS floor was asserted before the missing-column check in the same test, so it could mask the drift it exists to protect. Demonstrated: two legitimate column removals plus one genuine drift made the floor fire first and the drift list never printed. Drift is now asserted first. The floor follows as an explicit throw rather than an expect(), because a bare "Expected: >= 136 / Received: 135" under a test named "every non-auth Postgres column exists in a fresh SQLite schema" reads as drift when it is either a parser regression or a legitimate removal -- the message now says which to check and how to respond. 136 kept with no headroom, deliberately: the collapse this guards cost only 4 columns (workflow_events 7 -> 3), so a lower floor would miss it. Verified: 20/20 on the clean tree; with 2 removals + 1 injected drift the run now names remote_agent_workflow_events.zz_real_drift, which the previous ordering swallowed. Raised in second-pass review of #2346.
1 participant
Summary
CREATE TABLEandALTER TABLE ... ADD COLUMNdeclarations, compare expected column names against fresh SQLitePRAGMA table_inforesults, and retain the workflow-run index assertion.remote_agent_codebases.allow_env_keysPostgreSQL-only residue remains an explicit, narrowly scoped fix(db): codebases.allow_env_keys exists in Postgres and not SQLite — dead column, live drift #2318 exception.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
migrations/000_combined.sqlpackages/core/src/db/bundled-schema.tspackages/core/src/db/bundled-schema.tspackages/core/src/db/adapters/sqlite.test.tspackages/core/src/db/adapters/sqlite.tspackages/core/src/db/adapters/sqlite.test.tsPRAGMA table_infofor column parity.Label Snapshot
risk: lowsize: Mtestscore:sqlite-adapterChange Metadata
bugcoreLinked Issue
Validation Evidence (required)
Commands and result summary:
.archon/workflow edits; the validation run regenerated their corresponding bundled artifact and subsequently recorded all commands as passing. Those unrelated files are intentionally excluded from this PR.Security Impact (required)
No)No)No)No)Yes, describe risk and mitigation: Not applicable.Compatibility / Migration
Yes)No)No)Human Verification (required)
What was personally validated beyond CI:
parent_run_idindex remains covered.remote_agent_user_ai_prefs.default_model; PostgreSQL auth tables stay excluded; onlyremote_agent_codebases.allow_env_keysis excepted for fix(db): codebases.allow_env_keys exists in Postgres and not SQLite — dead column, live drift #2318.Side Effects / Blast Radius (required)
Rollback Plan (required)
e929b01afrom this PR branch.Risks and Mitigations
Summary by CodeRabbit