chore(gastown): migrate DO SQLite to Drizzle ORM#704
Closed
Conversation
Replace raw SQL query() calls and Zod-based table definitions with Drizzle ORM query builder across all 3 DOs (TownDO, GastownUserDO, AgentDO) and 5 sub-modules (~110 query sites total). - Add drizzle-orm/drizzle-kit deps and sqlite-schema.ts with 11 tables - Wire drizzle() + migrate() in each DO constructor - Convert all queries to type-safe query builder (select/insert/update/delete) - Delete 19 legacy table files, query.util.ts, and table.ts - Update AGENTS.md SQL conventions for Drizzle patterns - Migration SQL uses IF NOT EXISTS for backward compatibility
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge This is a thorough, well-structured migration from raw SQL (
All previously flagged issues from earlier review rounds have been addressed in follow-up commits ( Remaining Minor Items (already flagged in prior review)
These are all pre-existing suggestions from earlier review rounds and don't block merge. Files Reviewed (18 files)
|
- Derive AgentJoinRow, ReviewJoinRow, EscalationJoinRow, ConvoyJoinRow from query builder ReturnType instead of hand-writing them; eliminates all 'as' casts (AgentJoinRow x4, ReviewJoinRow x2, Agent['role'], Agent['status'], EscalationEntry['severity'], status as 'idle') - Narrow updateAgentStatus param to AgentMetadataSelect['status']; add AgentStatus.parse() at the TownDO public boundary (matching the BeadStatus.parse() pattern used by updateBeadStatus) - Export parseBead from beads.ts and import it in agents.ts, mail.ts, review-queue.ts, Town.do.ts instead of duplicating the function - Delete dead init stubs: initAgentTables, initMailTables, initReviewQueueTables (never called, stale comments) - Add comment on self-ref FK any in sqlite-schema.ts (drizzle limitation) - Clarify docs/do-sqlite-drizzle.md reference as repo-root path in AGENTS.md - Add comment on db.run(sql`...`) prune in Agent.do.ts explaining why the sql template tag is used (NOT IN subquery)
# Conflicts: # cloudflare-gastown/package.json # cloudflare-gastown/src/db/tables/agent-metadata.table.ts # cloudflare-gastown/src/db/tables/beads.table.ts # pnpm-lock.yaml
…C types Cloudflare's Rpc.Serializable<T> resolves DO RPC return types to never when they contain `unknown`. This was fixed on main (1da0cb3) for the old table-based types, but the drizzle migration reintroduced `unknown` in the new Omit-based type overrides. Changed to `any` with eslint-disable comments: - Bead.metadata, CreateBeadInput.metadata, BeadEventRecord.metadata - Molecule.formula - AgentMetadataRecord.checkpoint
toAgent() in agents.ts correctly does JSON.parse(row.checkpoint), but the inline agent construction in schedulePendingWork() was passing the raw SQLite text string through. Matches the toAgent() pattern.
| // Exclude beads.status from the join columns since agent_metadata.status | ||
| // shadows it and we need the agent status, not the bead status. | ||
| const { status: _beadStatus, ...beadCols } = getTableColumns(beads); | ||
| const pendingAgentColumns = { |
Contributor
There was a problem hiding this comment.
[SUGGESTION]: Duplicated agent join columns + manual toAgent mapping
pendingAgentColumns (lines 1181-1191) and the rows.map(row => ({...})) block (lines 1209-1221) duplicate agentJoinColumns and toAgent() from agents.ts. This duplication was the root cause of the checkpoint JSON.parse regression caught in the previous review round.
Consider exporting agentJoinColumns, agentJoinQuery, and toAgent from agents.ts and reusing them here. The additional .where() clause for cooldown filtering can be appended via .$dynamic(), the same pattern already used in listAgents().
Contributor
Author
|
Closing per agreement with @jrf0110 |
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
query()calls and Zod-based table definitions with Drizzle ORM query builder across all 3 DOs (TownDO,GastownUserDO,AgentDO) and 5 sub-modules (~110 query sites)drizzle-orm/drizzle-kitdependencies, createsqlite-schema.tswith all 11 active tables, and wiredrizzle()+migrate()in each DO constructorquery.util.ts,table.ts, and updateAGENTS.mdSQL conventionsDetails
Schema & Config
src/db/sqlite-schema.ts— 11 tables defined withsqliteTable, matching existing DDL exactly (columns, types, defaults, CHECK constraints, indexes). Exports$inferSelect/$inferInserttypes.drizzle.config.ts— sqlite dialect,durable-sqlitedriverdrizzle/0000_mushy_elektra.sql— Generated migration withIF NOT EXISTSfor backward compatibility with existing DO instanceswrangler.jsonc— Added.sqlimport rules for migration bundlepnpm-workspace.yaml— Addeddrizzle-kit: ^0.31.9to catalogQuery Rewrites
Town.do.tssqltemplate for arithmeticbeads.tsreview-queue.tsjson_set()via sql tag, molecule step chainagents.tsgetTableColumnsspread, name allocationGastownUser.do.tsAgent.do.ts.returning()replaceslast_insert_rowid(), NOT IN subquery prunerigs.ts.onConflictDoUpdate()for upsertmail.tsCleanup
src/db/tables/(19 files),src/util/table.ts,src/util/query.util.tsAGENTS.mdSQL section for Drizzle patternsVerification
client.test.tsconfirmed on main)Backward Compatibility
Migration SQL uses
CREATE TABLE IF NOT EXISTS/CREATE INDEX IF NOT EXISTS, so existing DO instances (which already have tables but lack__drizzle_migrations) will not fail when drizzle runs the initial migration.