Fix CLI/template issues; harden auth, migrate/generate flows and realtime#1
Merged
Helal-maker merged 3 commits intomainfrom Feb 20, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds defensive validation and safer parsing across the CLI, restructures migrate commands and signal handling, and modernizes the base template with WebSocket token extraction, a token-aware realtime server, pagination validation, and dependency bumps. Changes
Sequence DiagramssequenceDiagram
participant Client
participant WS as WS Upgrade Handler
participant Realtime as RealtimeServer
participant DB as Database
Client->>WS: Open WS connection (Authorization header or ?token)
WS->>WS: extract token from header or query
WS->>Realtime: handleConnection(ws, token)
Realtime->>Realtime: authenticate(token)
alt auth fails
Realtime-->>Client: close with auth error
else auth succeeds
Realtime-->>Client: open/ack
Client->>Realtime: send subscribe/unsubscribe (validated by zod)
Realtime->>Realtime: authorize(userId, claims, table)
alt unauthorized or limits hit
Realtime-->>Client: error message
else accepted
Realtime->>DB: (optional) fetch initial data
Realtime-->>Client: subscribed confirmation
end
end
sequenceDiagram
participant Client
participant CLI as Auth Command
participant DB as Database
Client->>CLI: POST /signup or /login (JSON)
CLI->>CLI: try parse JSON, safeParse with zod
alt invalid payload
CLI-->>Client: 400 with validation errors
else valid payload
CLI->>DB: create or verify user
alt DB failure or no created user
CLI-->>Client: 500
else success
CLI-->>Client: 200 with user fields (id, email, name)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
betterbase/packages/cli/src/commands/migrate.ts (1)
289-310: Missing escape handling for doubled double-quotes and backticks.The function correctly handles
''as an escape sequence inside single-quoted strings (lines 291-295), but doesn't apply the same logic to double-quoted identifiers ("") or backtick identifiers (``). Standard SQL uses doubled quotes to escape the quote character itself within identifiers.For example,
"table""name"or`col``name`would be incorrectly parsed as exiting and re-entering the quoted context.♻️ Proposed fix to handle doubled escape sequences consistently
if (!inSingle && !inBacktick && ch === '"') { + current += ch; + if (inDouble && next === '"') { + current += next; + i += 1; + continue; + } inDouble = !inDouble; - current += ch; continue; } if (!inSingle && !inDouble && ch === '`') { + current += ch; + if (inBacktick && next === '`') { + current += next; + i += 1; + continue; + } inBacktick = !inBacktick; - current += ch; continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@betterbase/packages/cli/src/commands/migrate.ts` around lines 289 - 310, The parser currently only treats doubled single quotes as escapes; update the double-quote and backtick branches to mirror that logic: when encountering ch === '"' or ch === '`' (and not in the other quote types) append ch, and if already inDouble/inBacktick and next equals the same quote char then append next, increment i, and continue; otherwise toggle inDouble/inBacktick and continue. Use the existing variables (inDouble, inSingle, inBacktick, current, ch, next, i) and add the doubled-quote/backtick checks inside the blocks handling double quotes and backticks.betterbase/templates/base/src/lib/realtime.ts (3)
76-79: Placeholder authentication returns token as userId.The
authenticatemethod simply returns the trimmed token as theuserId. While this may be intentional as a stub, it conflates authentication tokens with user identity. In production, this should validate the token (e.g., JWT verification) and extract the actual user ID.If this is a placeholder, consider adding a
TODOcomment or documenting the expected production implementation.📝 Suggested documentation
authenticate(token: string | undefined): { userId: string; claims: string[] } | null { + // TODO: Replace with actual token validation (e.g., JWT verification) + // This stub returns the token as userId for development purposes only if (!token || !token.trim()) return null; return { userId: token.trim(), claims: ['realtime:*'] }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@betterbase/templates/base/src/lib/realtime.ts` around lines 76 - 79, The authenticate function currently returns the raw trimmed token as userId, which conflates auth tokens with identity; update authenticate(token) to validate the token (e.g., verify JWT or call your auth service), extract and return the real user id and claims instead of the token, and if you intend this to remain a stub, add a clear TODO comment explaining it is a placeholder and what production behavior should be (verify signature, check expiry, map claims) so callers don't rely on token-as-userId; refer to the authenticate method for changes.
171-183: Consider defensive copy of subscribers set during iteration.The spread
[...subscribers]is good, but modifications tosubscribersviahandleCloseduring iteration could cause issues ifhandleClosetriggers other side effects. The current implementation looks safe, but consider documenting this invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@betterbase/templates/base/src/lib/realtime.ts` around lines 171 - 183, Make the iteration over the subscribers set explicitly defensive by materializing a copy before the loop (e.g., const subs = Array.from(subscribers)) and iterate over that copy instead of using [...subscribers] inline; this ensures that concurrent removals triggered by this.handleClose(ws) or other side-effects (and uses of this.safeSend, this.clients.get, subscription.filter checks via this.matchesFilter) cannot affect the active iterator. Alternatively, if you prefer to keep the current behavior, add a clear comment above the loop documenting the invariant that handleClose may mutate subscribers but iteration uses a snapshot to avoid concurrent-modification issues.
30-33: Consider adding max length constraints to table names in the schema.The
tablefield uses.min(1)but has no upper bound, which could allow excessively long table names that consume memory in the subscription maps.🛡️ Suggested constraint
const messageSchema = z.union([ - z.object({ type: z.literal('subscribe'), table: z.string().min(1), filter: z.record(z.string(), z.unknown()).optional() }), - z.object({ type: z.literal('unsubscribe'), table: z.string().min(1) }), + z.object({ type: z.literal('subscribe'), table: z.string().min(1).max(128), filter: z.record(z.string(), z.unknown()).optional() }), + z.object({ type: z.literal('unsubscribe'), table: z.string().min(1).max(128) }), ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@betterbase/templates/base/src/lib/realtime.ts` around lines 30 - 33, The schema for messageSchema allows unbounded table names (z.string().min(1)) which can enable extremely long names; update both branch schemas for the 'subscribe' and 'unsubscribe' objects to add a reasonable upper bound (e.g., .max(255) or .max(100)) on the table field so the Zod validators reject excessively long table names and protect subscription maps; modify the z.string().min(1) occurrences inside messageSchema to z.string().min(1).max(<chosenLimit>) for both the subscribe and unsubscribe variants.betterbase/packages/cli/src/utils/scanner.ts (2)
96-99: Clarify table key fallback logic.The code uses
tableObj.name || declaration.name.textas the key. This means if the parsed table name is an empty string, it falls back to the declaration name. Consider whether this is the intended behavior or if onlyundefined/missing names should trigger the fallback.📝 More explicit fallback
const tableObj = this.parseTable(initializer); - const tableKey = tableObj.name || declaration.name.text; + const tableKey = tableObj.name.trim() || declaration.name.text; tables[tableKey] = tableObj;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@betterbase/packages/cli/src/utils/scanner.ts` around lines 96 - 99, The current key uses a falsy check so an empty string tableObj.name falls back to declaration.name.text; update the fallback so only undefined or null trigger it: when handling sqliteTable/pgTable/mysqlTable (the block that calls this.parseTable and sets tables[tableKey] = tableObj), compute tableKey by treating tableObj.name as missing only if it is nullish (use a null/undefined check or nullish coalescing) rather than any falsy value, or explicitly prefer non-empty strings (e.g., prefer tableObj.name when !== ''), to preserve an intentionally empty name if that is valid.
169-188: Potential infinite loop in parseIndexes if AST is malformed.The
while (ts.isCallExpression(value))loop relies on breaking conditions that may not be met for all AST shapes. Ifvalue.expressionis a CallExpression but not a PropertyAccessExpression, and the call name doesn't match index/uniqueIndex, the loop breaks correctly. However, consider adding a max iteration guard for defensive coding.🛡️ Defensive iteration limit
let value = unwrapExpression(property.initializer); + let iterations = 0; + const maxIterations = 100; - while (ts.isCallExpression(value)) { + while (ts.isCallExpression(value) && iterations++ < maxIterations) { const callName = getCallName(value); // ... rest of loop }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@betterbase/packages/cli/src/utils/scanner.ts` around lines 169 - 188, The while loop in parseIndexes that traverses call expressions using unwrapExpression/getCallName can spin indefinitely on malformed AST; add a defensive max-iteration guard (e.g., const MAX_ITER = 50) and an iteration counter that increments each loop iteration and forces a break (and optionally logs a warning) if exceeded, placing the counter check at the top or bottom of the while block so functions unwrapExpression, getCallName and the existing break/continue logic remain unchanged except for the added counter and forced exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@betterbase/packages/cli/src/commands/auth.ts`:
- Around line 46-52: Wrap the await c.req.json() call in a try/catch before
calling signupSchema.safeParse so malformed JSON returns a 400 instead of
throwing; inside the catch return c.json({ error: 'Invalid JSON', details:
err.message }, 400). Apply the same pattern to the other handler(s) (e.g., the
handler around lines 78-85) where c.req.json() is used before a safeParse,
ensuring you only call signupSchema.safeParse (or the appropriate schema) with a
successfully parsed body.
---
Nitpick comments:
In `@betterbase/packages/cli/src/commands/migrate.ts`:
- Around line 289-310: The parser currently only treats doubled single quotes as
escapes; update the double-quote and backtick branches to mirror that logic:
when encountering ch === '"' or ch === '`' (and not in the other quote types)
append ch, and if already inDouble/inBacktick and next equals the same quote
char then append next, increment i, and continue; otherwise toggle
inDouble/inBacktick and continue. Use the existing variables (inDouble,
inSingle, inBacktick, current, ch, next, i) and add the doubled-quote/backtick
checks inside the blocks handling double quotes and backticks.
In `@betterbase/packages/cli/src/utils/scanner.ts`:
- Around line 96-99: The current key uses a falsy check so an empty string
tableObj.name falls back to declaration.name.text; update the fallback so only
undefined or null trigger it: when handling sqliteTable/pgTable/mysqlTable (the
block that calls this.parseTable and sets tables[tableKey] = tableObj), compute
tableKey by treating tableObj.name as missing only if it is nullish (use a
null/undefined check or nullish coalescing) rather than any falsy value, or
explicitly prefer non-empty strings (e.g., prefer tableObj.name when !== ''), to
preserve an intentionally empty name if that is valid.
- Around line 169-188: The while loop in parseIndexes that traverses call
expressions using unwrapExpression/getCallName can spin indefinitely on
malformed AST; add a defensive max-iteration guard (e.g., const MAX_ITER = 50)
and an iteration counter that increments each loop iteration and forces a break
(and optionally logs a warning) if exceeded, placing the counter check at the
top or bottom of the while block so functions unwrapExpression, getCallName and
the existing break/continue logic remain unchanged except for the added counter
and forced exit.
In `@betterbase/templates/base/src/lib/realtime.ts`:
- Around line 76-79: The authenticate function currently returns the raw trimmed
token as userId, which conflates auth tokens with identity; update
authenticate(token) to validate the token (e.g., verify JWT or call your auth
service), extract and return the real user id and claims instead of the token,
and if you intend this to remain a stub, add a clear TODO comment explaining it
is a placeholder and what production behavior should be (verify signature, check
expiry, map claims) so callers don't rely on token-as-userId; refer to the
authenticate method for changes.
- Around line 171-183: Make the iteration over the subscribers set explicitly
defensive by materializing a copy before the loop (e.g., const subs =
Array.from(subscribers)) and iterate over that copy instead of using
[...subscribers] inline; this ensures that concurrent removals triggered by
this.handleClose(ws) or other side-effects (and uses of this.safeSend,
this.clients.get, subscription.filter checks via this.matchesFilter) cannot
affect the active iterator. Alternatively, if you prefer to keep the current
behavior, add a clear comment above the loop documenting the invariant that
handleClose may mutate subscribers but iteration uses a snapshot to avoid
concurrent-modification issues.
- Around line 30-33: The schema for messageSchema allows unbounded table names
(z.string().min(1)) which can enable extremely long names; update both branch
schemas for the 'subscribe' and 'unsubscribe' objects to add a reasonable upper
bound (e.g., .max(255) or .max(100)) on the table field so the Zod validators
reject excessively long table names and protect subscription maps; modify the
z.string().min(1) occurrences inside messageSchema to
z.string().min(1).max(<chosenLimit>) for both the subscribe and unsubscribe
variants.
![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: 0d1506487b
ℹ️ 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".
| if (existsSync(realtimePath)) return; | ||
|
|
||
| const canonicalRealtimePath = path.resolve(import.meta.dir, '../../../templates/base/src/lib/realtime.ts'); | ||
| const canonicalRealtimePath = path.resolve(import.meta.dir, '../../../../templates/base/src/lib/realtime.ts'); |
There was a problem hiding this comment.
ensureRealtimeUtility resolves the canonical template from import.meta.dir using four .. segments, which only works when running from src in dev mode. The shipped bb binary runs from packages/cli/dist (bin points to ./dist/index.js), so this path resolves outside the repo and bb generate crud ... throws Canonical realtime template not found whenever src/lib/realtime.ts is missing.
Useful? React with 👍 / 👎.
Comment on lines
+78
to
+82
| return { userId: token.trim(), claims: ['realtime:*'] }; | ||
| } | ||
|
|
||
| authorize(userId: string, claims: string[], table: string): boolean { | ||
| return Boolean(userId) && (claims.includes('realtime:*') || claims.includes(`realtime:${table}`)); |
There was a problem hiding this comment.
The new auth path accepts any non-empty token and immediately assigns realtime:*, and authorize trusts that claim for every table. In practice, an unauthenticated client can connect with any arbitrary ?token= or Authorization value and subscribe to all realtime streams, so this does not enforce actual authentication/authorization despite the new checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
betterbase/packages/cli/src/commands/migrate.ts (1)
396-397: Consider simplifying the diagnostic message.The note is helpful but verbose. A more concise version improves readability:
♻️ Suggested wording
logger.info('Applying migrations with drizzle-kit push...'); - logger.info('Note: drizzle-kit generate produced files in drizzle/ for preview/diff analysis only; push is what applied changes in this run.'); + logger.info('Note: generated files in drizzle/ are for preview only; push applies the actual changes.');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@betterbase/packages/cli/src/commands/migrate.ts` around lines 396 - 397, Replace the verbose diagnostic logged in migrate.ts with a more concise message: update the second logger.info call (the one after logger.info('Applying migrations with drizzle-kit push...')) to a shorter string such as "drizzle/ files are for preview; push applied changes." so the intent remains but the output is more readable.betterbase/packages/cli/src/utils/scanner.ts (2)
109-114: Consider usingparse()instead ofsafeParse()when throwing on failure.Since invalid output always throws,
TablesRecordSchema.parse(tables)would be simpler and produce the same behavior with built-in Zod error formatting.♻️ Suggested simplification
- const validated = TablesRecordSchema.safeParse(tables); - if (!validated.success) { - throw new Error(`Schema scanner produced invalid output: ${JSON.stringify(validated.error.format())}`); - } - - return validated.data; + return TablesRecordSchema.parse(tables);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@betterbase/packages/cli/src/utils/scanner.ts` around lines 109 - 114, Replace the safeParse + manual throw pattern with a direct parse call on TablesRecordSchema: instead of calling TablesRecordSchema.safeParse(tables), inspecting validated.success and throwing with JSON.stringify(validated.error.format()), call TablesRecordSchema.parse(tables) and return its result; this preserves the throw-on-failure behavior and uses Zod's built-in error formatting (update the return to return the parse result and remove the validated variable and custom throw logic).
170-177: Silent break on MAX_ITER may hide parsing issues.When
MAX_ITERis exceeded, the loop breaks without any indication. For complex schemas, this could silently skip indexes. Consider logging a warning for debuggability.♻️ Add warning when limit is reached
if (iter > MAX_ITER) { + console.warn(`parseIndexes: exceeded MAX_ITER (${MAX_ITER}) for property, skipping further traversal`); break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@betterbase/packages/cli/src/utils/scanner.ts` around lines 170 - 177, The loop that walks nested call expressions (while (ts.isCallExpression(value)) using MAX_ITER/iter) silently breaks when iter > MAX_ITER; change this to emit a visible warning instead of silently breaking: after detecting iter > MAX_ITER, call the project's logger (or console.warn if no logger) with context (mention MAX_ITER was hit, include the node's text or identifier such as value.getText() and any parent/index info) so callers can debug skipped indexes; keep the existing break/return behavior but ensure the warning message is informative and uses the same logging facility as the surrounding code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@betterbase/templates/base/src/lib/realtime.ts`:
- Around line 207-222: The cap checks in the subscription flow incorrectly block
re-subscribes/updates by the same client when caps are reached; change the
checks in the subscription handler to bypass the per-client and per-table limits
if the client is already subscribed to that table: before enforcing
this.config.maxSubscriptionsPerClient, check client.subscriptions.has(table) and
skip the client-cap error if true; before enforcing
this.config.maxSubscribersPerTable, check tableSet.has(ws) and skip the
table-cap error if true; then update the existing entry in client.subscriptions
(and ensure tableSubscribers still contains ws) instead of treating it as a new
subscription.
- Around line 76-90: The authenticate method currently grants a wildcard claim
when rawClaims is missing which gives full access for tokens like "user" or
"user:"; change authenticate (function authenticate) to avoid defaulting to
['realtime:*'] — parse rawClaims into claims only when present and non-empty
(e.g., produce an empty array when rawClaims is undefined/empty) and ensure
authorize (method authorize) continues to check for 'realtime:*' or
`realtime:${table}` against the claims array so that tokens without explicit
claims have no privileges.
---
Nitpick comments:
In `@betterbase/packages/cli/src/commands/migrate.ts`:
- Around line 396-397: Replace the verbose diagnostic logged in migrate.ts with
a more concise message: update the second logger.info call (the one after
logger.info('Applying migrations with drizzle-kit push...')) to a shorter string
such as "drizzle/ files are for preview; push applied changes." so the intent
remains but the output is more readable.
In `@betterbase/packages/cli/src/utils/scanner.ts`:
- Around line 109-114: Replace the safeParse + manual throw pattern with a
direct parse call on TablesRecordSchema: instead of calling
TablesRecordSchema.safeParse(tables), inspecting validated.success and throwing
with JSON.stringify(validated.error.format()), call
TablesRecordSchema.parse(tables) and return its result; this preserves the
throw-on-failure behavior and uses Zod's built-in error formatting (update the
return to return the parse result and remove the validated variable and custom
throw logic).
- Around line 170-177: The loop that walks nested call expressions (while
(ts.isCallExpression(value)) using MAX_ITER/iter) silently breaks when iter >
MAX_ITER; change this to emit a visible warning instead of silently breaking:
after detecting iter > MAX_ITER, call the project's logger (or console.warn if
no logger) with context (mention MAX_ITER was hit, include the node's text or
identifier such as value.getText() and any parent/index info) so callers can
debug skipped indexes; keep the existing break/return behavior but ensure the
warning message is informative and uses the same logging facility as the
surrounding code.
Comment on lines
+239
to
+331
| const usersExportIdx = current.search(/export\s+const\s+users\s*=\s*sqliteTable\s*\(/); | ||
| if (usersExportIdx === -1) { | ||
| logger.warn('Could not find sqlite users table block; skipping passwordHash injection.'); | ||
| return; | ||
| } | ||
|
|
||
| const replacement = usersBlock[0].replace(/\n\}\);$/, "\n passwordHash: text('password_hash').notNull(),\n});"); | ||
| writeFileSync(schemaPath, current.replace(usersBlock[0], replacement)); | ||
| const callStart = current.indexOf('sqliteTable(', usersExportIdx); | ||
| if (callStart === -1) { | ||
| logger.warn('Could not locate sqliteTable call for users; skipping passwordHash injection.'); | ||
| return; | ||
| } | ||
|
|
||
| let i = callStart; | ||
| let parenDepth = 0; | ||
| let inSingle = false; | ||
| let inDouble = false; | ||
| let inBacktick = false; | ||
| let escaped = false; | ||
|
|
||
| while (i < current.length) { | ||
| const ch = current[i]; | ||
| const next = current[i + 1]; | ||
|
|
||
| if (escaped) { | ||
| escaped = false; | ||
| i += 1; | ||
| continue; | ||
| } | ||
|
|
||
| if ((inSingle || inDouble || inBacktick) && ch === '\\') { | ||
| escaped = true; | ||
| i += 1; | ||
| continue; | ||
| } | ||
|
|
||
| if (!inDouble && !inBacktick && ch === "'") { | ||
| if (inSingle && next === "'") { | ||
| i += 2; | ||
| continue; | ||
| } | ||
| inSingle = !inSingle; | ||
| i += 1; | ||
| continue; | ||
| } | ||
|
|
||
| if (!inSingle && !inBacktick && ch === '"') { | ||
| inDouble = !inDouble; | ||
| i += 1; | ||
| continue; | ||
| } | ||
|
|
||
| if (!inSingle && !inDouble && ch === '`') { | ||
| inBacktick = !inBacktick; | ||
| i += 1; | ||
| continue; | ||
| } | ||
|
|
||
| if (inSingle || inDouble || inBacktick) { | ||
| i += 1; | ||
| continue; | ||
| } | ||
|
|
||
| if (ch === '(') { | ||
| parenDepth += 1; | ||
| } else if (ch === ')') { | ||
| parenDepth -= 1; | ||
| if (parenDepth === 0) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| i += 1; | ||
| } | ||
|
|
||
| if (i >= current.length || parenDepth !== 0) { | ||
| logger.warn('Could not safely parse users sqliteTable block; skipping passwordHash injection.'); | ||
| return; | ||
| } | ||
|
|
||
| const statementEnd = current.indexOf(';', i); | ||
| if (statementEnd === -1) { | ||
| logger.warn('Could not locate end of users sqliteTable statement; skipping passwordHash injection.'); | ||
| return; | ||
| } | ||
|
|
||
| const usersBlock = current.slice(usersExportIdx, statementEnd + 1); | ||
| const replacement = usersBlock.replace(/\n\}\);\s*$/, "\n passwordHash: text('password_hash').notNull(),\n});"); | ||
| if (replacement === usersBlock) { | ||
| logger.warn('Could not inject passwordHash into users table; block layout was unexpected.'); | ||
| return; | ||
| } | ||
|
|
||
| writeFileSync(schemaPath, `${current.slice(0, usersExportIdx)}${replacement}${current.slice(statementEnd + 1)}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the auth.ts file and examine the context
find . -name "auth.ts" -path "*/cli/src/commands/*" 2>/dev/null | head -5Repository: Helal-maker/Betterbase
Length of output: 112
🏁 Script executed:
# Search for sqliteTable calls in the codebase to see actual usage patterns
rg "sqliteTable\s*\(" -A 3 -B 1 --max-count=20Repository: Helal-maker/Betterbase
Length of output: 5858
🏁 Script executed:
# More specifically, search for 3-argument sqliteTable patterns (with callbacks)
rg "sqliteTable\s*\(\s*['\"]" -A 5 | head -50Repository: Helal-maker/Betterbase
Length of output: 3631
🏁 Script executed:
# Read the exact lines 239-331 in auth.ts
sed -n '239,331p' betterbase/packages/cli/src/commands/auth.tsRepository: Helal-maker/Betterbase
Length of output: 2444
🏁 Script executed:
# Also search for all sqliteTable calls with 3 arguments to confirm they exist
rg "sqliteTable\s*\(['\"].*?['\"],\s*\{[^}]*\},\s*\(" -A 3 --max-count=10Repository: Helal-maker/Betterbase
Length of output: 48
🏁 Script executed:
# Let me verify the exact pattern when a 3-arg sqliteTable call is used
cat -n betterbase/packages/cli/test/scanner.test.ts | sed -n '20,35p'Repository: Helal-maker/Betterbase
Length of output: 822
🏁 Script executed:
# Extract a more complete view of the 3-arg sqliteTable usage in the test
rg -A 10 "export const users = sqliteTable\('users'" betterbase/packages/cli/test/scanner.test.ts | head -15Repository: Helal-maker/Betterbase
Length of output: 539
🏁 Script executed:
# Test the proposed regex patterns to see if they correctly detect callbacks
python3 << 'EOF'
import re
# Sample 3-arg sqliteTable code
code_3arg = """}), (table) => ({
usersEmailIdx: index('users_email_idx').on(table.email),
}));"""
code_2arg = """}
);"""
# Test the proposed patterns
pattern1 = r'/\},\s*\(\s*\w+\s*\)\s*=>/'
pattern1_fixed = r'\},\s*\(\s*\w+\s*\)\s*=>'
pattern2_fixed = r'\},\s*function\s*\('
print("Testing with 3-arg callback:")
print(f"Pattern 1 match: {bool(re.search(pattern1_fixed, code_3arg))}")
print(f"Pattern 2 match: {bool(re.search(pattern2_fixed, code_3arg))}")
print("\nTesting with 2-arg (no callback):")
print(f"Pattern 1 match: {bool(re.search(pattern1_fixed, code_2arg))}")
print(f"Pattern 2 match: {bool(re.search(pattern2_fixed, code_2arg))}")
# Test the replacement pattern
replacement_pattern = r'\n\}\);\s*$'
test_block = "export const users = sqliteTable('users', {\n id: text('id'),\n});"
print(f"\nReplacement pattern matches 2-arg: {bool(re.search(replacement_pattern, test_block))}")
test_block_3arg = "export const users = sqliteTable('users', {\n id: text('id'),\n}), (table) => ({\n idx: index('idx')\n}));"
print(f"Replacement pattern matches 3-arg: {bool(re.search(replacement_pattern, test_block_3arg))}")
if re.search(replacement_pattern, test_block_3arg):
print(f" Would match at: {repr(re.search(replacement_pattern, test_block_3arg).group())}")
EOFRepository: Helal-maker/Betterbase
Length of output: 303
🏁 Script executed:
# Test corrected patterns for the actual 3-arg sqliteTable format
python3 << 'EOF'
import re
# Real 3-arg sqliteTable code from the test file
code_3arg = """ }, (table) => ({
usersEmailIdx: index('users_email_idx').on(table.email),
}));"""
code_2arg = """ }
);"""
# The actual pattern in the code: }, (table) => ({
pattern_correct = r'\}\),\s*\(\s*\w+\s*\)\s*=>'
print("Testing with actual 3-arg callback pattern:")
print(f"Corrected pattern match: {bool(re.search(pattern_correct, code_3arg))}")
print(f"Match found: {re.search(pattern_correct, code_3arg).group() if re.search(pattern_correct, code_3arg) else 'None'}")
print("\nTesting with 2-arg (no callback):")
print(f"Corrected pattern match: {bool(re.search(pattern_correct, code_2arg))}")
# Test the replacement patterns
print("\n--- Testing replacement patterns ---")
block_2arg = """export const users = sqliteTable('users', {
id: text('id'),
});"""
block_3arg = """export const users = sqliteTable('users', {
id: text('id'),
}), (table) => ({
idx: index('idx')
}));"""
# The code uses this pattern:
replacement_pattern = r'\n\}\);\s*$'
print(f"\n2-arg block ends with: {repr(block_2arg[-15:])}")
print(f"Pattern matches 2-arg: {bool(re.search(replacement_pattern, block_2arg))}")
print(f"\n3-arg block ends with: {repr(block_3arg[-15:])}")
print(f"Pattern matches 3-arg: {bool(re.search(replacement_pattern, block_3arg))}")
# What would happen if we try to inject into 3-arg?
if re.search(replacement_pattern, block_2arg):
result_2arg = re.sub(replacement_pattern, "\n passwordHash: text('password_hash').notNull(),\n});", block_2arg)
print(f"\n2-arg injection result:\n{result_2arg}")
if re.search(replacement_pattern, block_3arg):
result_3arg = re.sub(replacement_pattern, "\n passwordHash: text('password_hash').notNull(),\n});", block_3arg)
print(f"\n3-arg injection result:\n{result_3arg}")
else:
print(f"\n3-arg block: Pattern does NOT match (good - injection won't happen incorrectly)")
EOFRepository: Helal-maker/Betterbase
Length of output: 626
🏁 Script executed:
# Let's verify exactly what happens with the actual usersBlock when we have a 3-arg call
python3 << 'EOF'
import re
# Simulate what the code extracts as usersBlock for a 3-arg call
usersBlock_3arg = """export const users = sqliteTable('users', {
id: text('id'),
email: text('email').notNull().unique(),
age: integer('age').default(18),
}, (table) => ({
usersEmailIdx: index('users_email_idx').on(table.email),
}));"""
usersBlock_2arg = """export const users = sqliteTable('users', {
id: text('id'),
email: text('email').notNull().unique(),
age: integer('age').default(18),
});"""
# The actual replacement pattern from the code
replacement_pattern = r'\n\}\);\s*$'
replacement_text = "\n passwordHash: text('password_hash').notNull(),\n});"
# Test with 2-arg
result_2arg = usersBlock_2arg.replace(replacement_pattern, replacement_text)
print("2-arg replacement:")
print(f"Changed: {result_2arg != usersBlock_2arg}")
print(f"Result ends with: {repr(result_2arg[-50:])}\n")
# Test with 3-arg
result_3arg = usersBlock_3arg.replace(replacement_pattern, replacement_text)
print("3-arg replacement:")
print(f"Changed: {result_3arg != usersBlock_3arg}")
print(f"Original ends with: {repr(usersBlock_3arg[-50:])}")
if result_3arg == usersBlock_3arg:
print("⚠️ REPLACEMENT FAILED - would trigger the warning and return early")
else:
print(f"Result ends with: {repr(result_3arg[-50:])}")
# Now test the proposed detection patterns from the review
print("\n--- Testing proposed detection patterns ---")
pattern1 = r'\},\s*\(\s*\w+\s*\)\s*=>'
pattern2 = r'\},\s*function\s*\('
print(f"Pattern 1 (arrow fn) matches 3-arg: {bool(re.search(pattern1, usersBlock_3arg))}")
print(f"Pattern 2 (function) matches 3-arg: {bool(re.search(pattern2, usersBlock_3arg))}")
EOFRepository: Helal-maker/Betterbase
Length of output: 477
Provide clearer diagnostics for unsupported 3-arg sqliteTable callbacks.
The code already prevents schema corruption when encountering 3-arg sqliteTable calls with callbacks—the replacement pattern /\n\}\);\s*$/ only matches the 2-arg syntax ending with });, so injection safely fails and logs a generic warning. However, a more specific warning would improve debugging. The suggested detection fix is partially correct: the arrow function pattern (\},\s*\(\s*\w+\s*\)\s*=>) does match the callback syntax, but the function pattern alone is incomplete. Consider adding an explicit callback detection to distinguish this case from other layout failures.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
betterbase/packages/cli/src/utils/scanner.ts (1)
98-101: The table-key keying concern is valid for edge cases, but they do not occur in this codebase.All table declarations in the codebase use string literals (e.g.,
sqliteTable('users', {...})), sotableObj.namealways contains the correct value. The edge case you've identified—where a non-string-literal argument likesqliteTable(tableName, {...})would causetableObj.nameto contain raw expression text instead of the intended table name—has no real-world instances here. The current fallback todeclaration.name.textwould only matter in such edge cases, and they don't exist. The suggested fix is a defensive improvement but adds complexity for a theoretical scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@betterbase/packages/cli/src/utils/scanner.ts` around lines 98 - 101, The current code uses tableObj.name ?? declaration.name.text to key the tables map, which can capture raw expression text for non-string-literal table initializers; update the logic in the block that calls parseTable (around where tableObj, tableKey and tables[...] are set) to only use tableObj.name if it is a string literal (or otherwise a safe string), otherwise skip adding the entry (or log a warning) instead of falling back to declaration.name.text; reference the parseTable result (tableObj.name), the declaration identifier (declaration.name.text) and the tables map when implementing the guarded check so the map only gets valid literal table names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@betterbase/packages/cli/src/utils/scanner.ts`:
- Around line 98-101: The current code uses tableObj.name ??
declaration.name.text to key the tables map, which can capture raw expression
text for non-string-literal table initializers; update the logic in the block
that calls parseTable (around where tableObj, tableKey and tables[...] are set)
to only use tableObj.name if it is a string literal (or otherwise a safe
string), otherwise skip adding the entry (or log a warning) instead of falling
back to declaration.name.text; reference the parseTable result (tableObj.name),
the declaration identifier (declaration.name.text) and the tables map when
implementing the guarded check so the map only gets valid literal table names.
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.
Motivation
Description
"jsx": "react-jsx"totsconfig.base.json, updated roottypecheckscript quoting for cross-platform use, added.gitignoreto the base template, and bumped templatezodto v4; generator now writesbuild/startscripts in producedpackage.jsonand uses validatedenv.DB_PATHfor generated DB scripts.safeParsewith structured 400 responses, guarded against empty.returning()results before dereference, changedvalidateSessionto select onlyid/email/name, replaced fragile regex injection with a safe parser-like injector, and madeensureRoutesIndexHookcheck anchors and only write when safe (logs warnings otherwise).limit+1/hasMoresemantics in generated CRUD routes (with a comment about filter allowlists), madeensureZodValidatorInstalleddetect an existing install before runningbun add, hoisted the HTTP methods set in the route scanner, and strengthened the schema scanner to return Zod-validated TableInfo records while detecting index calls in chained expressions and keying tables by SQL name.splitStatements, surfaced a CLI notice thatdrizzle-kit generateoutput is for preview whilepushapplies changes, and rewrote the realtime utility to accept authenticated websocket upgrades, validate messages with Zod, enforce subscription/clients caps, and use deep-equality for filter matching.Testing
git diff --checkwhich passed successfully.bun installin this environment but it failed due to external registry 403 responses, so dependency install and full typecheck could not be completed here.bun run typecheck/ turbo workflows but they were blocked by the failed install / missing tooling in this environment, so no full typecheck run was completed.Codex Task
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests