Pending changes exported from your codespace#41
Merged
weroperking merged 1 commit intoweroperking:mainfrom Mar 27, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThe PR introduces a comprehensive Infrastructure as Code (IaC) specification and foundational implementation. It adds validator primitives, schema definition utilities, schema serialization and diffing mechanics, typed function registrations (query/mutation/action), database context abstractions, code generators for Drizzle schemas and SQL migrations, a function discovery and registry system, and a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (12)
packages/core/src/iac/cron.ts (2)
1-1: Unused import.The
zimport fromzodis not used anywhere in this file.🧹 Proposed removal
-import { z } from "zod"; import type { MutationRegistration } from "./functions";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/iac/cron.ts` at line 1, The import statement "import { z } from \"zod\";" is unused in this file (packages/core/src/iac/cron.ts); remove that import line (or if z was intended to be used, replace its usage where appropriate) so there are no unused imports and the file compiles without lint errors referencing the symbol "z".
11-24: Consider validating cron schedule format and providing a reset mechanism.The
scheduleparameter accepts any string without validation—typos like"0 * * *"(missing field) won't be caught until runtime. Additionally, the module-scoped_jobsarray has no reset mechanism, which can cause issues during testing or hot-reload scenarios.✨ Proposed improvements
const _jobs: CronJob[] = []; +// Basic cron expression validation (5 or 6 fields) +const CRON_REGEX = /^(\*|[0-9,\-\/]+)\s+(\*|[0-9,\-\/]+)\s+(\*|[0-9,\-\/]+)\s+(\*|[0-9,\-\/]+)\s+(\*|[0-9,\-\/]+)(\s+(\*|[0-9,\-\/]+))?$/; + /** Register a cron job. Called in bbf/cron.ts. */ export function cron( name: string, schedule: string, fn: MutationRegistration<any, any>, args: Record<string, unknown> = {} ): void { + if (!CRON_REGEX.test(schedule.trim())) { + throw new Error(`Invalid cron schedule: "${schedule}"`); + } _jobs.push({ name, schedule, fn, args }); } export function getCronJobs(): CronJob[] { return _jobs; } + +/** Reset registry (for testing). */ +export function clearCronJobs(): void { + _jobs.length = 0; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/iac/cron.ts` around lines 11 - 24, The cron registration currently pushes any string into module-scoped _jobs without verifying the cron schedule or allowing reset; update cron(name, schedule, fn, args) to validate the schedule string (e.g., use a cron parsing/validation utility or a simple field-count check) and throw or return an error when the schedule is invalid, and add a new exported function resetCronJobs() that clears the _jobs array; also ensure getCronJobs() still returns the current _jobs so tests and hot-reload code can call resetCronJobs() to avoid state leakage.packages/core/src/iac/db-context.ts (1)
106-106: Unused_mutationsarray.The
_mutationsarray is declared but never populated or used anywhere in the class. If this was intended for batching/transaction support, consider removing it until implemented to avoid confusion.🧹 Proposed removal
export class DatabaseWriter extends DatabaseReader { - private _mutations: (() => Promise<void>)[] = [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/iac/db-context.ts` at line 106, The private field _mutations: (() => Promise<void>)[] is declared but never used; remove this unused declaration from the class (delete the line "private _mutations: (() => Promise<void>)[] = [];") or, if you intended to support batched/transactional mutations, implement the push/flush methods that populate and execute _mutations and update related methods (e.g., any commit/execute methods in the same class) to use it—prefer removing the unused _mutations declaration if no implementation is planned.packages/core/test/iac-edge-cases.test.ts (3)
536-546: Type inference tests have limited runtime value.Lines 537 and 543 test
Inferon the raw function types (typeof v.object,typeof v.array) rather than on actual schema instances. While these pass at runtime due to TypeScript's structural typing, they don't validate the intended use case of inferring types from constructed schemas. Consider testing with actual schema instances:const objSchema = v.object({ name: v.string() }); type Obj = Infer<typeof objSchema>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/iac-edge-cases.test.ts` around lines 536 - 546, The tests use Infer on raw function types (typeof v.object, typeof v.array) which doesn't validate inferring from constructed schemas; update the two tests to create actual schema instances (e.g. use v.object({...}) and v.array(...) to produce objSchema and arrSchema), then use type Obj = Infer<typeof objSchema> and type Arr = Infer<typeof arrSchema> and assert runtime values against those instances (referencing v.object, v.array, and Infer in the test names/functions) so the tests verify inference from real schema objects rather than the constructor types.
437-446: Migration filename allows potentially problematic characters.The test expects
"add users table!"to produce"0001_add_users_table!.sql", allowing the!character in the filename. While most modern filesystems handle this, shell escaping issues and cross-platform compatibility concerns suggest sanitizing filenames more strictly (e.g., allowing only alphanumeric, underscore, and hyphen).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/iac-edge-cases.test.ts` around lines 437 - 446, The test shows generateMigration currently includes unsafe characters (e.g., '!') in migration.filename; update generateMigration to sanitize the provided title before building the filename by: normalizing whitespace to underscores, removing any characters except ASCII letters, digits, underscores and hyphens, collapsing consecutive underscores, trimming leading/trailing underscores, and then prefixing with the zero-padded migration number as before; refer to the generateMigration function and the migration.filename construction to locate where to apply this sanitization logic and ensure existing behavior for numbering remains unchanged.
512-520: Misleading test name:v.optional()allowsundefined, notnull.The test title states "optional field can be null" but the assertions test
undefinedbehavior, which is correct for Zod's.optional(). Consider renaming to "optional field can be undefined" for clarity.📝 Suggested rename
- test("optional field can be null", () => { + test("optional field can be undefined", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/iac-edge-cases.test.ts` around lines 512 - 520, Rename the test title to accurately reflect what v.optional() allows: change the test description in the test using v.optional(v.string()) from "optional field can be null" to "optional field can be undefined" (or similar) so it matches the assertions that validate undefined behavior; locate the test block using the test(...) call and the schema variable that uses v.optional to update the title only.BetterBase_InfraAsCode_Spec.md (2)
27-38: Add language specifier to fenced code block.Same as above—add
textorplaintextlanguage specifier for the directory structure block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BetterBase_InfraAsCode_Spec.md` around lines 27 - 38, The fenced code block showing the directory tree (the block starting with ``` and the bbf/ structure) lacks a language specifier; update that fenced block by adding a language token like "text" or "plaintext" (e.g., change ``` to ```text) so the directory structure renders as plain text in docs; ensure you only modify the opening fence and leave the directory content unchanged.
16-21: Add language specifier to fenced code block.This code block showing the directory structure should have a language specifier (e.g.,
textorplaintext) for consistent rendering across Markdown parsers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BetterBase_InfraAsCode_Spec.md` around lines 16 - 21, The fenced code block that shows the directory tree (the block starting with ``` and the src/ tree lines) lacks a language specifier; update that opening fence to include a language like "text" or "plaintext" (e.g., change ``` to ```text) so Markdown parsers render it consistently.packages/core/src/iac/generators/api-typegen.ts (1)
36-54: Minor: Redundant|| "root"fallback.Line 39 has
const key = file.replace(/\//g, "_") || "root"butfileis already assigned"root"as fallback on line 18 when the path has no intermediate segments. The|| "root"is redundant.♻️ Simplify
- const key = file.replace(/\//g, "_") || "root"; + const key = file.replace(/\//g, "_");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/iac/generators/api-typegen.ts` around lines 36 - 54, The fallback `|| "root"` on the `key` assignment is redundant because `file` is already set to `"root"` earlier; update the loop in api-typegen.ts that declares `const key = file.replace(/\//g, "_") || "root"` to simply `const key = file.replace(/\//g, "_")`, keeping the rest of the logic that uses `key`, `file`, and the surrounding for-loops unchanged.packages/core/src/iac/generators/drizzle-schema-gen.ts (1)
46-50:searchIndextype is silently downgraded to a regular index.The
SerializedIndextype includes"searchIndex"(used for full-text search), but the generator treats it as a regular index. This loses the semantic distinction and won't create the correct full-text search index in the database.Consider either:
- Explicitly handling
searchIndexwith appropriate Drizzle/SQL constructs- Filtering out
searchIndexentries and logging a warning that they require manual setup- Throwing an error for unsupported index types
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/iac/generators/drizzle-schema-gen.ts` around lines 46 - 50, The generator currently treats any non-unique index as a regular index in the indexLines map, silently downgrading SerializedIndex entries with type "searchIndex"; update the logic that builds indexLines (iterating table.indexes and referencing idx.type and idx.name) to explicitly handle "searchIndex" — either generate the correct Drizzle/full-text construct (e.g., map "searchIndex" to the DB-specific full-text index helper if available) or, if not supported, filter those entries out and emit a clear warning or throw an error indicating that "searchIndex" requires manual/full-text setup; ensure the branch checks idx.type === "searchIndex" before falling back to regular index generation so you don't lose the semantic.packages/core/src/iac/function-registry.ts (1)
14-24: Silent error swallowing may hide configuration issues.The
.catch(() => [])on line 16 silently ignores directory read errors (permissions, symlink issues, etc.). Consider logging these failures at debug level for troubleshooting.Also, the extension filter excludes
.tsx,.jsx,.mts,.mjswhich may contain valid function exports in some projects.♻️ Suggested improvement
async function walk(dir: string): Promise<string[]> { - const entries = await readdir(dir, { withFileTypes: true }).catch(() => []); + const entries = await readdir(dir, { withFileTypes: true }).catch((err) => { + // Log at debug level - missing dirs are expected, other errors may indicate issues + if (err.code !== "ENOENT") console.debug(`[function-registry] Could not read ${dir}:`, err.message); + return []; + }); const files: string[] = []; + const validExts = [".ts", ".js", ".tsx", ".jsx", ".mts", ".mjs"]; for (const entry of entries) { const full = join(dir, entry.name); if (entry.isDirectory()) files.push(...await walk(full)); - else if ([".ts", ".js"].includes(extname(entry.name))) files.push(full); + else if (validExts.includes(extname(entry.name))) files.push(full); } return files; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/iac/function-registry.ts` around lines 14 - 24, The walk function currently swallows readdir errors with .catch(() => []) and only accepts .ts/.js files; update walk to surface or at least log readdir failures (use the existing logger at debug level or throw a wrapped error) instead of silently returning an empty list, and broaden the extension filter in walk to include .tsx, .jsx, .mts, .mjs (and any other expected module extensions) so valid function files aren’t skipped; look for the function named walk and the readdir(...) call and adjust error handling and the extname check accordingly.packages/core/src/iac/validators.ts (1)
3-4: Dead code:ID_BRANDsymbol is defined but never used.This symbol was likely intended for branding but
v.id()uses Zod's built-in.brand()instead.🧹 Remove unused symbol
-// Brand symbol for typed IDs -const ID_BRAND = Symbol("BetterBaseId"); - export type BrandedId<T extends string> = string & { __table: T };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/iac/validators.ts` around lines 3 - 4, Remove the unused symbol constant ID_BRAND from the file (it's dead code) and any related unused imports or exports; locate the definition "const ID_BRAND = Symbol(\"BetterBaseId\")" in validators.ts and delete it, then run a quick search for ID_BRAND to ensure there are no remaining references and update the module exports if it was exported accidentally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/iac/db-context.ts`:
- Around line 38-42: The order() method assigns the incoming field to _orderBy
and it's later interpolated into SQL without validation; update order(field) to
validate the field as a safe SQL identifier using the same identifier validation
routine/pattern already used elsewhere in this file (or central helper) and only
accept the validated/normalized value for _orderBy, rejecting or defaulting on
invalid input; also update _buildSQL() to use the validated _orderBy value (or
quote it properly via the same identifier sanitizer) instead of raw
interpolation so identifiers cannot be injected.
- Around line 141-144: The replace() method's behavior is incorrect: it calls
patch() (a partial update) while its docstring promises a full replace; either
implement true replace semantics in replace() or update the docstring to match
the current behavior. To implement replace semantics, have replace(table, id,
data) load the existing record, preserve system fields (e.g.,
createdAt/updatedBy or whatever your code treats as "system fields"), remove all
other user fields from the record, then set the record's user fields to exactly
the keys in data and persist; alternatively, if partial updates are intended,
change the docstring for replace() to state that it delegates to patch() and
performs a partial update. Ensure you reference and use the existing patch()
helper only if keeping partial behavior, and keep system-field preservation
logic consistent with other DB methods.
- Around line 30-36: The filter method directly interpolates the field param
into SQL causing injection risk; update the filter(field, op, value)
implementation to validate field before using it (e.g., check against an
allowlist of permitted column names or validate with a strict identifier regex
like /^[A-Za-z_][A-Za-z0-9_]*$/), and throw/return an error if validation fails;
keep the rest of the logic that maps op via opMap and pushes to this._filters
and this._params unchanged so only valid identifiers are ever inserted into the
constructed SQL.
- Around line 61-65: The first() method unconditionally appends " LIMIT 1"
causing double LIMIT when _buildSQL() already included a limit (e.g., after
take()). Update first() (and related logic around _buildSQL() and the _limit
field) so it does not append a second LIMIT: either set/override this._limit = 1
before calling _buildSQL(), or check whether this._limit is already set and only
append " LIMIT 1" when no limit was produced by _buildSQL(); ensure you still
return a single row (rows[0] or null) and reuse the existing _buildSQL()
behavior rather than duplicating LIMIT logic.
In `@packages/core/src/iac/function-registry.ts`:
- Around line 44-45: The lookup for the BetterBaseFunction symbol in
function-registry.ts is using Symbol.for("BetterBaseFunction") which doesn't
match the local Symbol("BetterBaseFunction") used elsewhere, so fnKind always
falls back and produces "querie" for "queries"; fix by making the symbols
consistent (prefer changing functions.ts to create the symbol with
Symbol.for("BetterBaseFunction") so the global lookup in function-registry.ts
finds it), or alternatively adjust the fallback in function-registry.ts (the
fnKind calculation) to correctly map "queries" → "query" (handle the "ies"→"y"
plural rule) instead of slicing the last char; reference fnKind and the symbol
names Symbol.for("BetterBaseFunction") / Symbol("BetterBaseFunction") when
applying the fix.
In `@packages/core/src/iac/functions.ts`:
- Line 77: The constant FUNCTION_KIND uses a local symbol
(Symbol("BetterBaseFunction")) which won't match lookups using the global
registry; replace its creation with the global symbol
(Symbol.for("BetterBaseFunction")) so cross-module interop works; update the
declaration of FUNCTION_KIND in functions.ts to use Symbol.for and ensure any
code in function-registry.ts that uses Symbol.for("BetterBaseFunction") will now
successfully match.
In `@packages/core/src/iac/generators/drizzle-schema-gen.ts`:
- Around line 38-40: The code unconditionally appends .default(sql`now()`) for
timestamp columns which breaks SQLite; update the generator logic that builds
column definitions (the block handling col.name === "_createdAt" || "_updatedAt"
where def is modified) to choose the SQL expression based on the target dialect:
use sql`CURRENT_TIMESTAMP` for SQLite and sql`now()` for Postgres (or the
existing default for other dialects). Detect the dialect from the
generator/config context (the same variable used elsewhere to branch SQL
generation) and apply the dialect-specific default when assigning to def.
In `@packages/core/src/iac/generators/migration-gen.ts`:
- Around line 47-49: The ADD_INDEX branch in migration-gen.ts builds SQL using
change.index as if it were a column name; instead use the index's field list
stored on change.after to generate the ON ("col1","col2",...) clause. Update the
ADD_INDEX case to read the index definition from change.after (e.g.,
change.after.fields or similar) and join the field names into the column list
for the CREATE INDEX IF NOT EXISTS statement, and also update schema-diff.ts to
ensure ADD_INDEX changes populate change.after with the index definition (fields
and name) so migration-gen can consume it.
In `@packages/core/src/iac/schema-diff.ts`:
- Around line 78-83: The index-diff currently only pushes the index name (idx)
for ADD_INDEX and DROP_INDEX, but migration-gen.ts needs the full index
definition; update the pushes in schema-diff.ts (inside the loops iterating over
toIdx and fromIdx) to include the index definition object from toIdx.get(idx)
for ADD_INDEX as an "after" property and fromIdx.get(idx) for DROP_INDEX as a
"before" property (keep existing keys table, index, type/destructive). Ensure
the pushed change objects match the shape migration-gen.ts expects (include
fields/type in the index definition).
In `@packages/core/src/iac/schema-serializer.ts`:
- Around line 109-116: The function loadSerializedSchema currently calls the
async Bun.file(path).text() without awaiting it, so JSON.parse receives a
Promise; change loadSerializedSchema to be async (declare async and return
Promise<SerializedSchema | null>) and await Bun.file(path).text() before
JSON.parse; update all call sites to await loadSerializedSchema. Alternatively,
if you must keep it synchronous, replace Bun.file(path).text() with a
synchronous read (e.g., Bun.file(path).textSync() or Node's fs.readFileSync) and
keep the return type as SerializedSchema | null. Ensure the function signature
and callers match the chosen approach.
In `@packages/core/src/iac/schema.ts`:
- Around line 10-22: The TableDefinition interface's _schema currently uses
z.ZodObject<TShape>, which omits the runtime system fields (_id, _createdAt,
_updatedAt) added by defineTable and causes InferDocument<T> to miss those
fields; update the _schema type to a ZodObject that extends TShape with the
system field definitions (e.g., TShape & { _id: ..., _createdAt: ...,
_updatedAt: ... } or a reusable SystemFields type) so the declared schema
matches the shape built in defineTable and InferDocument<T> will correctly
include the system fields; adjust any related generics (e.g., _shape,
TableDefinition<TShape>) as needed to preserve keyof TShape behavior for index
methods (index, uniqueIndex, searchIndex).
In `@packages/core/src/iac/validators.ts`:
- Around line 31-33: The union helper's generic signature currently allows a
single validator but z.union requires at least two; update the signature of the
exported union function (the union: <T ...> helper in validators.ts) to require
at least two items by using a tuple type like [z.ZodTypeAny, z.ZodTypeAny,
...z.ZodTypeAny[]] for the generic so callers cannot pass a single validator,
and remove the unsafe cast when calling z.union(validators) so the compiler
enforces the 2+ constraint.
In `@packages/core/test/iac.test.ts`:
- Around line 545-559: Tests rely on a module-scoped cron registry so asserting
getCronJobs()[0] is flaky; update the test that uses cron("test-job", ...) and
getCronJobs() to either clear/reset the registry in a beforeEach (call the
module's clear/reset helper if available) or locate the registered job by name
(e.g., use getCronJobs().find(j => j.name === "test-job")) and assert its
schedule equals "0 * * * *" instead of assuming index 0; reference getCronJobs,
cron, and mutation when making the change.
In `@packages/server/src/routes/bbf/index.ts`:
- Around line 61-64: The catch block in the BBF route currently returns
err.message to the client (see the catch handling that logs `[bbf] Error in
${path}:` and calls c.json), which can leak internal details; change it to
always log the full error server-side using the existing console.error call and
return a generic error message to clients in production (e.g., "Internal
function error"), while optionally exposing err.message only when NODE_ENV !==
"production" (use an isDev flag) before calling c.json with status 500.
- Around line 88-95: In buildActionCtx update runQuery and runMutation so they
validate incoming args with fn._args.safeParse() before calling fn._handler:
call fn._args.safeParse(args), if parsing fails throw or propagate a clear
validation error (including parse error details), and if it succeeds pass the
parsed.data to fn._handler (instead of the original args); reference
buildActionCtx, runQuery, runMutation, fn._handler and fn._args.safeParse to
locate and implement the change.
- Around line 40-42: The code is vulnerable because projectSlug from the
X-Project-Slug header is interpolated into dbSchema and later used by
DatabaseWriter (which formats `"${this._schema}"."${table}"`), enabling SQL
injection; fix it by validating and sanitizing projectSlug immediately after
reading it (in the getPool / route code) — accept only a strict whitelist (e.g.,
alphanumeric, dash/underscore) and reject or normalize anything else (fallback
to "default" or return a 400), and ensure dbSchema is built from that validated
value before any DatabaseWriter or schema interpolation occurs; reference the
variables projectSlug, dbSchema and the DatabaseWriter/_schema/table
interpolation when making the change.
---
Nitpick comments:
In `@BetterBase_InfraAsCode_Spec.md`:
- Around line 27-38: The fenced code block showing the directory tree (the block
starting with ``` and the bbf/ structure) lacks a language specifier; update
that fenced block by adding a language token like "text" or "plaintext" (e.g.,
change ``` to ```text) so the directory structure renders as plain text in docs;
ensure you only modify the opening fence and leave the directory content
unchanged.
- Around line 16-21: The fenced code block that shows the directory tree (the
block starting with ``` and the src/ tree lines) lacks a language specifier;
update that opening fence to include a language like "text" or "plaintext"
(e.g., change ``` to ```text) so Markdown parsers render it consistently.
In `@packages/core/src/iac/cron.ts`:
- Line 1: The import statement "import { z } from \"zod\";" is unused in this
file (packages/core/src/iac/cron.ts); remove that import line (or if z was
intended to be used, replace its usage where appropriate) so there are no unused
imports and the file compiles without lint errors referencing the symbol "z".
- Around line 11-24: The cron registration currently pushes any string into
module-scoped _jobs without verifying the cron schedule or allowing reset;
update cron(name, schedule, fn, args) to validate the schedule string (e.g., use
a cron parsing/validation utility or a simple field-count check) and throw or
return an error when the schedule is invalid, and add a new exported function
resetCronJobs() that clears the _jobs array; also ensure getCronJobs() still
returns the current _jobs so tests and hot-reload code can call resetCronJobs()
to avoid state leakage.
In `@packages/core/src/iac/db-context.ts`:
- Line 106: The private field _mutations: (() => Promise<void>)[] is declared
but never used; remove this unused declaration from the class (delete the line
"private _mutations: (() => Promise<void>)[] = [];") or, if you intended to
support batched/transactional mutations, implement the push/flush methods that
populate and execute _mutations and update related methods (e.g., any
commit/execute methods in the same class) to use it—prefer removing the unused
_mutations declaration if no implementation is planned.
In `@packages/core/src/iac/function-registry.ts`:
- Around line 14-24: The walk function currently swallows readdir errors with
.catch(() => []) and only accepts .ts/.js files; update walk to surface or at
least log readdir failures (use the existing logger at debug level or throw a
wrapped error) instead of silently returning an empty list, and broaden the
extension filter in walk to include .tsx, .jsx, .mts, .mjs (and any other
expected module extensions) so valid function files aren’t skipped; look for the
function named walk and the readdir(...) call and adjust error handling and the
extname check accordingly.
In `@packages/core/src/iac/generators/api-typegen.ts`:
- Around line 36-54: The fallback `|| "root"` on the `key` assignment is
redundant because `file` is already set to `"root"` earlier; update the loop in
api-typegen.ts that declares `const key = file.replace(/\//g, "_") || "root"` to
simply `const key = file.replace(/\//g, "_")`, keeping the rest of the logic
that uses `key`, `file`, and the surrounding for-loops unchanged.
In `@packages/core/src/iac/generators/drizzle-schema-gen.ts`:
- Around line 46-50: The generator currently treats any non-unique index as a
regular index in the indexLines map, silently downgrading SerializedIndex
entries with type "searchIndex"; update the logic that builds indexLines
(iterating table.indexes and referencing idx.type and idx.name) to explicitly
handle "searchIndex" — either generate the correct Drizzle/full-text construct
(e.g., map "searchIndex" to the DB-specific full-text index helper if available)
or, if not supported, filter those entries out and emit a clear warning or throw
an error indicating that "searchIndex" requires manual/full-text setup; ensure
the branch checks idx.type === "searchIndex" before falling back to regular
index generation so you don't lose the semantic.
In `@packages/core/src/iac/validators.ts`:
- Around line 3-4: Remove the unused symbol constant ID_BRAND from the file
(it's dead code) and any related unused imports or exports; locate the
definition "const ID_BRAND = Symbol(\"BetterBaseId\")" in validators.ts and
delete it, then run a quick search for ID_BRAND to ensure there are no remaining
references and update the module exports if it was exported accidentally.
In `@packages/core/test/iac-edge-cases.test.ts`:
- Around line 536-546: The tests use Infer on raw function types (typeof
v.object, typeof v.array) which doesn't validate inferring from constructed
schemas; update the two tests to create actual schema instances (e.g. use
v.object({...}) and v.array(...) to produce objSchema and arrSchema), then use
type Obj = Infer<typeof objSchema> and type Arr = Infer<typeof arrSchema> and
assert runtime values against those instances (referencing v.object, v.array,
and Infer in the test names/functions) so the tests verify inference from real
schema objects rather than the constructor types.
- Around line 437-446: The test shows generateMigration currently includes
unsafe characters (e.g., '!') in migration.filename; update generateMigration to
sanitize the provided title before building the filename by: normalizing
whitespace to underscores, removing any characters except ASCII letters, digits,
underscores and hyphens, collapsing consecutive underscores, trimming
leading/trailing underscores, and then prefixing with the zero-padded migration
number as before; refer to the generateMigration function and the
migration.filename construction to locate where to apply this sanitization logic
and ensure existing behavior for numbering remains unchanged.
- Around line 512-520: Rename the test title to accurately reflect what
v.optional() allows: change the test description in the test using
v.optional(v.string()) from "optional field can be null" to "optional field can
be undefined" (or similar) so it matches the assertions that validate undefined
behavior; locate the test block using the test(...) call and the schema variable
that uses v.optional to update the title only.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f154727a-f74e-4f0c-9827-32f3035efb50
⛔ Files ignored due to path filters (2)
apps/test-project/bun.lockis excluded by!**/*.lockbun.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
BetterBase_InfraAsCode_Spec.mdapps/test-project/README.mdapps/test-project/betterbase.config.tsapps/test-project/drizzle.config.tsapps/test-project/package.jsonapps/test-project/src/auth/index.tsapps/test-project/src/auth/types.tsapps/test-project/src/db/index.tsapps/test-project/src/db/migrate.tsapps/test-project/src/db/policies/.gitkeepapps/test-project/src/db/schema.tsapps/test-project/src/functions/.gitkeepapps/test-project/src/functions/hello/index.tsapps/test-project/src/index.tsapps/test-project/src/lib/env.tsapps/test-project/src/lib/realtime.tsapps/test-project/src/middleware/auth.tsapps/test-project/src/middleware/validation.tsapps/test-project/src/routes/graphql.d.tsapps/test-project/src/routes/health.tsapps/test-project/src/routes/index.tsapps/test-project/src/routes/storage.tsapps/test-project/src/routes/users.tsapps/test-project/src/routes/webhooks.tsapps/test-project/test/crud.test.tsapps/test-project/test/health.test.tsapps/test-project/tsconfig.jsonpackages/core/package.jsonpackages/core/src/iac/cron.tspackages/core/src/iac/db-context.tspackages/core/src/iac/function-registry.tspackages/core/src/iac/functions.tspackages/core/src/iac/generators/api-typegen.tspackages/core/src/iac/generators/drizzle-schema-gen.tspackages/core/src/iac/generators/migration-gen.tspackages/core/src/iac/index.tspackages/core/src/iac/schema-diff.tspackages/core/src/iac/schema-serializer.tspackages/core/src/iac/schema.tspackages/core/src/iac/validators.tspackages/core/test/iac-edge-cases.test.tspackages/core/test/iac.test.tspackages/server/package.jsonpackages/server/src/index.tspackages/server/src/routes/bbf/index.tspackages/server/src/types.d.tspackages/server/tsconfig.json
💤 Files with no reviewable changes (25)
- apps/test-project/drizzle.config.ts
- apps/test-project/README.md
- apps/test-project/src/db/migrate.ts
- apps/test-project/src/routes/graphql.d.ts
- apps/test-project/src/functions/hello/index.ts
- apps/test-project/src/functions/.gitkeep
- apps/test-project/tsconfig.json
- apps/test-project/src/db/index.ts
- apps/test-project/src/middleware/validation.ts
- apps/test-project/test/health.test.ts
- apps/test-project/src/routes/index.ts
- apps/test-project/src/routes/webhooks.ts
- apps/test-project/src/auth/types.ts
- apps/test-project/package.json
- apps/test-project/src/lib/env.ts
- apps/test-project/src/routes/health.ts
- apps/test-project/betterbase.config.ts
- apps/test-project/src/auth/index.ts
- apps/test-project/src/db/schema.ts
- apps/test-project/src/middleware/auth.ts
- apps/test-project/src/routes/users.ts
- apps/test-project/test/crud.test.ts
- apps/test-project/src/lib/realtime.ts
- apps/test-project/src/routes/storage.ts
- apps/test-project/src/index.ts
Comment on lines
+30
to
+36
| filter(field: string, op: "eq" | "neq" | "gt" | "gte" | "lt" | "lte", value: unknown): this { | ||
| const idx = this._params.length + 1; | ||
| const opMap = { eq: "=", neq: "!=", gt: ">", gte: ">=", lt: "<", lte: "<=" }; | ||
| this._filters.push(`"${field}" ${opMap[op]} $${idx}`); | ||
| this._params.push(value); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
SQL injection risk via unvalidated field names.
The field parameter is interpolated directly into the SQL string without sanitization. If field originates from user input, an attacker could inject arbitrary SQL (e.g., field = '"]; DROP TABLE users; --'). Consider validating that field matches an allowlist of known column names or at least matches a safe identifier pattern.
🛡️ Proposed fix: validate identifier pattern
filter(field: string, op: "eq" | "neq" | "gt" | "gte" | "lt" | "lte", value: unknown): this {
+ if (!/^[a-zA-Z_][a-zA-Z0-9_]*$/.test(field)) {
+ throw new Error(`Invalid field name: ${field}`);
+ }
const idx = this._params.length + 1;
const opMap = { eq: "=", neq: "!=", gt: ">", gte: ">=", lt: "<", lte: "<=" };
this._filters.push(`"${field}" ${opMap[op]} $${idx}`);📝 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.
Suggested change
| filter(field: string, op: "eq" | "neq" | "gt" | "gte" | "lt" | "lte", value: unknown): this { | |
| const idx = this._params.length + 1; | |
| const opMap = { eq: "=", neq: "!=", gt: ">", gte: ">=", lt: "<", lte: "<=" }; | |
| this._filters.push(`"${field}" ${opMap[op]} $${idx}`); | |
| this._params.push(value); | |
| return this; | |
| } | |
| filter(field: string, op: "eq" | "neq" | "gt" | "gte" | "lt" | "lte", value: unknown): this { | |
| if (!/^[a-zA-Z_][a-zA-Z0-9_]*$/.test(field)) { | |
| throw new Error(`Invalid field name: ${field}`); | |
| } | |
| const idx = this._params.length + 1; | |
| const opMap = { eq: "=", neq: "!=", gt: ">", gte: ">=", lt: "<", lte: "<=" }; | |
| this._filters.push(`"${field}" ${opMap[op]} $${idx}`); | |
| this._params.push(value); | |
| return this; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/iac/db-context.ts` around lines 30 - 36, The filter method
directly interpolates the field param into SQL causing injection risk; update
the filter(field, op, value) implementation to validate field before using it
(e.g., check against an allowlist of permitted column names or validate with a
strict identifier regex like /^[A-Za-z_][A-Za-z0-9_]*$/), and throw/return an
error if validation fails; keep the rest of the logic that maps op via opMap and
pushes to this._filters and this._params unchanged so only valid identifiers are
ever inserted into the constructed SQL.
Comment on lines
+38
to
+42
| order(direction: "asc" | "desc", field = "_createdAt"): this { | ||
| this._orderBy = field; | ||
| this._orderDir = direction === "asc" ? "ASC" : "DESC"; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Same SQL injection concern applies to order() field and _buildSQL() identifiers.
The field parameter in order() and the _orderBy value are interpolated into SQL without validation. Apply the same identifier validation pattern.
🛡️ Proposed fix for order()
order(direction: "asc" | "desc", field = "_createdAt"): this {
+ if (!/^[a-zA-Z_][a-zA-Z0-9_]*$/.test(field)) {
+ throw new Error(`Invalid field name: ${field}`);
+ }
this._orderBy = field;
this._orderDir = direction === "asc" ? "ASC" : "DESC";
return this;
}Also applies to: 50-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/iac/db-context.ts` around lines 38 - 42, The order() method
assigns the incoming field to _orderBy and it's later interpolated into SQL
without validation; update order(field) to validate the field as a safe SQL
identifier using the same identifier validation routine/pattern already used
elsewhere in this file (or central helper) and only accept the
validated/normalized value for _orderBy, rejecting or defaulting on invalid
input; also update _buildSQL() to use the validated _orderBy value (or quote it
properly via the same identifier sanitizer) instead of raw interpolation so
identifiers cannot be injected.
Comment on lines
+61
to
+65
| async first(): Promise<T | null> { | ||
| const { sql, params } = this._buildSQL(); | ||
| const { rows } = await this._pool.query(sql + " LIMIT 1", params as any[]); | ||
| return (rows[0] as T) ?? null; | ||
| } |
There was a problem hiding this comment.
Double LIMIT clause in first() method.
The _buildSQL() method already appends LIMIT ${this._limit} when _limit is set. Then first() unconditionally appends " LIMIT 1", resulting in invalid SQL like SELECT * FROM "schema"."table" LIMIT 5 LIMIT 1 if take() was called before first().
🐛 Proposed fix
async first(): Promise<T | null> {
- const { sql, params } = this._buildSQL();
- const { rows } = await this._pool.query(sql + " LIMIT 1", params as any[]);
+ this._limit = 1;
+ const { sql, params } = this._buildSQL();
+ const { rows } = await this._pool.query(sql, params as any[]);
return (rows[0] as T) ?? null;
}📝 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.
Suggested change
| async first(): Promise<T | null> { | |
| const { sql, params } = this._buildSQL(); | |
| const { rows } = await this._pool.query(sql + " LIMIT 1", params as any[]); | |
| return (rows[0] as T) ?? null; | |
| } | |
| async first(): Promise<T | null> { | |
| this._limit = 1; | |
| const { sql, params } = this._buildSQL(); | |
| const { rows } = await this._pool.query(sql, params as any[]); | |
| return (rows[0] as T) ?? null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/iac/db-context.ts` around lines 61 - 65, The first() method
unconditionally appends " LIMIT 1" causing double LIMIT when _buildSQL() already
included a limit (e.g., after take()). Update first() (and related logic around
_buildSQL() and the _limit field) so it does not append a second LIMIT: either
set/override this._limit = 1 before calling _buildSQL(), or check whether
this._limit is already set and only append " LIMIT 1" when no limit was produced
by _buildSQL(); ensure you still return a single row (rows[0] or null) and reuse
the existing _buildSQL() behavior rather than duplicating LIMIT logic.
Comment on lines
+141
to
+144
| /** Full replace — replaces all user fields (preserves system fields) */ | ||
| async replace(table: string, id: string, data: Record<string, unknown>): Promise<void> { | ||
| await this.patch(table, id, data); | ||
| } |
There was a problem hiding this comment.
replace() semantics mismatch: delegates to patch() instead of full replacement.
The docstring states "Full replace — replaces all user fields" but the implementation just calls patch(), which performs a partial update. A true replace should clear fields not present in data. If this is intentional for v1, update the docstring to reflect actual behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/iac/db-context.ts` around lines 141 - 144, The replace()
method's behavior is incorrect: it calls patch() (a partial update) while its
docstring promises a full replace; either implement true replace semantics in
replace() or update the docstring to match the current behavior. To implement
replace semantics, have replace(table, id, data) load the existing record,
preserve system fields (e.g., createdAt/updatedBy or whatever your code treats
as "system fields"), remove all other user fields from the record, then set the
record's user fields to exactly the keys in data and persist; alternatively, if
partial updates are intended, change the docstring for replace() to state that
it delegates to patch() and performs a partial update. Ensure you reference and
use the existing patch() helper only if keeping partial behavior, and keep
system-field preservation logic consistent with other DB methods.
Comment on lines
+44
to
+45
| const fnKind: "query" | "mutation" | "action" = | ||
| fn[Symbol.for("BetterBaseFunction")] ?? kind.slice(0, -1) as any; |
There was a problem hiding this comment.
Critical: Symbol mismatch causes kind detection to always fail; fallback produces "querie" for queries.
functions.ts uses Symbol("BetterBaseFunction") (local/unique symbol), but this file looks up Symbol.for("BetterBaseFunction") (global symbol). These are never equal, so the lookup always returns undefined.
The fallback kind.slice(0, -1) then converts:
"queries"→"querie"❌ (should be"query")"mutations"→"mutation"✓"actions"→"action"✓
This breaks query function registration.
🐛 Proposed fix in functions.ts (root cause)
Change functions.ts to use the global symbol:
-const FUNCTION_KIND = Symbol("BetterBaseFunction");
+const FUNCTION_KIND = Symbol.for("BetterBaseFunction");🐛 Alternative: fix the fallback logic here
const fnKind: "query" | "mutation" | "action" =
- fn[Symbol.for("BetterBaseFunction")] ?? kind.slice(0, -1) as any;
+ fn[Symbol.for("BetterBaseFunction")] ?? (kind === "queries" ? "query" : kind.slice(0, -1)) as any;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/iac/function-registry.ts` around lines 44 - 45, The lookup
for the BetterBaseFunction symbol in function-registry.ts is using
Symbol.for("BetterBaseFunction") which doesn't match the local
Symbol("BetterBaseFunction") used elsewhere, so fnKind always falls back and
produces "querie" for "queries"; fix by making the symbols consistent (prefer
changing functions.ts to create the symbol with Symbol.for("BetterBaseFunction")
so the global lookup in function-registry.ts finds it), or alternatively adjust
the fallback in function-registry.ts (the fnKind calculation) to correctly map
"queries" → "query" (handle the "ies"→"y" plural rule) instead of slicing the
last char; reference fnKind and the symbol names
Symbol.for("BetterBaseFunction") / Symbol("BetterBaseFunction") when applying
the fix.
Comment on lines
+31
to
+33
| /** Discriminated union */ | ||
| union: <T extends [z.ZodTypeAny, ...z.ZodTypeAny[]]>(...validators: T) => | ||
| z.union(validators as unknown as [z.ZodTypeAny, z.ZodTypeAny, ...z.ZodTypeAny[]]), |
There was a problem hiding this comment.
Type signature allows single-item union but Zod requires at least 2.
The signature [z.ZodTypeAny, ...z.ZodTypeAny[]] accepts 1+ validators, but z.union() throws at runtime if given fewer than 2. The cast masks this at compile time.
🔧 Proposed fix to enforce 2+ at compile time
/** Discriminated union */
- union: <T extends [z.ZodTypeAny, ...z.ZodTypeAny[]]>(...validators: T) =>
- z.union(validators as unknown as [z.ZodTypeAny, z.ZodTypeAny, ...z.ZodTypeAny[]]),
+ union: <T extends [z.ZodTypeAny, z.ZodTypeAny, ...z.ZodTypeAny[]]>(...validators: T) =>
+ z.union(validators),📝 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.
Suggested change
| /** Discriminated union */ | |
| union: <T extends [z.ZodTypeAny, ...z.ZodTypeAny[]]>(...validators: T) => | |
| z.union(validators as unknown as [z.ZodTypeAny, z.ZodTypeAny, ...z.ZodTypeAny[]]), | |
| /** Discriminated union */ | |
| union: <T extends [z.ZodTypeAny, z.ZodTypeAny, ...z.ZodTypeAny[]]>(...validators: T) => | |
| z.union(validators), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/iac/validators.ts` around lines 31 - 33, The union helper's
generic signature currently allows a single validator but z.union requires at
least two; update the signature of the exported union function (the union: <T
...> helper in validators.ts) to require at least two items by using a tuple
type like [z.ZodTypeAny, z.ZodTypeAny, ...z.ZodTypeAny[]] for the generic so
callers cannot pass a single validator, and remove the unsafe cast when calling
z.union(validators) so the compiler enforces the 2+ constraint.
Comment on lines
+545
to
+559
| describe("Cron Jobs", () => { | ||
| test("cron registers a job", () => { | ||
| const jobs = getCronJobs().length; | ||
|
|
||
| const mockMutation = mutation({ | ||
| args: {}, | ||
| handler: async () => {}, | ||
| }); | ||
|
|
||
| cron("test-job", "0 * * * *", mockMutation, {}); | ||
|
|
||
| expect(getCronJobs()).toHaveLength(jobs + 1); | ||
| expect(getCronJobs()[0].name).toBe("test-job"); | ||
| expect(getCronJobs()[0].schedule).toBe("0 * * * *"); | ||
| }); |
There was a problem hiding this comment.
Cron test relies on shared mutable state and may be flaky.
getCronJobs() returns a module-scoped array that persists across tests. The assertion getCronJobs()[0].name assumes the newly registered job is at index 0, but if other tests (or test runs) add jobs first, this will fail. Consider either:
- Clearing the cron registry in a
beforeEachhook - Asserting against the last element or finding by name
💚 Proposed fix using find
cron("test-job", "0 * * * *", mockMutation, {});
expect(getCronJobs()).toHaveLength(jobs + 1);
- expect(getCronJobs()[0].name).toBe("test-job");
- expect(getCronJobs()[0].schedule).toBe("0 * * * *");
+ const job = getCronJobs().find(j => j.name === "test-job");
+ expect(job).toBeDefined();
+ expect(job?.schedule).toBe("0 * * * *");📝 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.
Suggested change
| describe("Cron Jobs", () => { | |
| test("cron registers a job", () => { | |
| const jobs = getCronJobs().length; | |
| const mockMutation = mutation({ | |
| args: {}, | |
| handler: async () => {}, | |
| }); | |
| cron("test-job", "0 * * * *", mockMutation, {}); | |
| expect(getCronJobs()).toHaveLength(jobs + 1); | |
| expect(getCronJobs()[0].name).toBe("test-job"); | |
| expect(getCronJobs()[0].schedule).toBe("0 * * * *"); | |
| }); | |
| describe("Cron Jobs", () => { | |
| test("cron registers a job", () => { | |
| const jobs = getCronJobs().length; | |
| const mockMutation = mutation({ | |
| args: {}, | |
| handler: async () => {}, | |
| }); | |
| cron("test-job", "0 * * * *", mockMutation, {}); | |
| expect(getCronJobs()).toHaveLength(jobs + 1); | |
| const job = getCronJobs().find(j => j.name === "test-job"); | |
| expect(job).toBeDefined(); | |
| expect(job?.schedule).toBe("0 * * * *"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/test/iac.test.ts` around lines 545 - 559, Tests rely on a
module-scoped cron registry so asserting getCronJobs()[0] is flaky; update the
test that uses cron("test-job", ...) and getCronJobs() to either clear/reset the
registry in a beforeEach (call the module's clear/reset helper if available) or
locate the registered job by name (e.g., use getCronJobs().find(j => j.name ===
"test-job")) and assert its schedule equals "0 * * * *" instead of assuming
index 0; reference getCronJobs, cron, and mutation when making the change.
Comment on lines
+40
to
+42
| const pool = getPool(); | ||
| const projectSlug = c.req.header("X-Project-Slug") ?? "default"; | ||
| const dbSchema = `project_${projectSlug}`; |
There was a problem hiding this comment.
SQL injection risk via X-Project-Slug header.
The projectSlug from the request header is directly interpolated into the database schema name without validation. Per context snippet 1, DatabaseWriter methods use string interpolation: `"${this._schema}"."${table}"`. A malicious X-Project-Slug header like default"; DROP TABLE users; -- could execute arbitrary SQL.
🔐 Proposed fix: validate and sanitize project slug
+const PROJECT_SLUG_REGEX = /^[a-zA-Z0-9_-]+$/;
+
bbfRouter.post("/:kind/*", async (c) => {
// ... existing code ...
// Build DB context
const pool = getPool();
const projectSlug = c.req.header("X-Project-Slug") ?? "default";
+
+ if (!PROJECT_SLUG_REGEX.test(projectSlug)) {
+ return c.json({ error: "Invalid project slug" }, 400);
+ }
+
const dbSchema = `project_${projectSlug}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/routes/bbf/index.ts` around lines 40 - 42, The code is
vulnerable because projectSlug from the X-Project-Slug header is interpolated
into dbSchema and later used by DatabaseWriter (which formats
`"${this._schema}"."${table}"`), enabling SQL injection; fix it by validating
and sanitizing projectSlug immediately after reading it (in the getPool / route
code) — accept only a strict whitelist (e.g., alphanumeric, dash/underscore) and
reject or normalize anything else (fallback to "default" or return a 400), and
ensure dbSchema is built from that validated value before any DatabaseWriter or
schema interpolation occurs; reference the variables projectSlug, dbSchema and
the DatabaseWriter/_schema/table interpolation when making the change.
Comment on lines
+61
to
+64
| } catch (err: any) { | ||
| console.error(`[bbf] Error in ${path}:`, err); | ||
| return c.json({ error: err.message ?? "Function error" }, 500); | ||
| } |
There was a problem hiding this comment.
Internal error details exposed to client.
Line 63 returns err.message directly to the client. In production, this could leak sensitive information like file paths, stack traces, or internal implementation details. Consider returning a generic message while logging the full error server-side.
🔐 Proposed fix
} catch (err: any) {
console.error(`[bbf] Error in ${path}:`, err);
- return c.json({ error: err.message ?? "Function error" }, 500);
+ return c.json({ error: "Internal function error" }, 500);
}Alternatively, only expose messages in development:
const isDev = process.env.NODE_ENV !== "production";
return c.json({ error: isDev ? err.message : "Internal function error" }, 500);📝 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.
Suggested change
| } catch (err: any) { | |
| console.error(`[bbf] Error in ${path}:`, err); | |
| return c.json({ error: err.message ?? "Function error" }, 500); | |
| } | |
| } catch (err: any) { | |
| console.error(`[bbf] Error in ${path}:`, err); | |
| return c.json({ error: "Internal function error" }, 500); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/routes/bbf/index.ts` around lines 61 - 64, The catch
block in the BBF route currently returns err.message to the client (see the
catch handling that logs `[bbf] Error in ${path}:` and calls c.json), which can
leak internal details; change it to always log the full error server-side using
the existing console.error call and return a generic error message to clients in
production (e.g., "Internal function error"), while optionally exposing
err.message only when NODE_ENV !== "production" (use an isDev flag) before
calling c.json with status 500.
Comment on lines
+88
to
+95
| function buildActionCtx(pool: any, dbSchema: string, auth: any) { | ||
| return { | ||
| auth, | ||
| storage: buildStorageWriter(), | ||
| scheduler: buildScheduler(pool), | ||
| runQuery: async (fn: any, args: any) => (fn._handler({ db: new DatabaseReader(pool, dbSchema), auth, storage: buildStorageReader() }, args)), | ||
| runMutation: async (fn: any, args: any) => (fn._handler({ db: new DatabaseWriter(pool, dbSchema), auth, storage: buildStorageWriter(), scheduler: buildScheduler(pool) }, args)), | ||
| }; |
There was a problem hiding this comment.
runQuery/runMutation bypass argument validation.
The action context's runQuery and runMutation helpers directly call fn._handler without validating args through fn._args.safeParse(). This allows actions to pass invalid arguments to nested function calls, potentially causing runtime errors or unexpected behavior.
🐛 Proposed fix: add validation
function buildActionCtx(pool: any, dbSchema: string, auth: any) {
return {
auth,
storage: buildStorageWriter(),
scheduler: buildScheduler(pool),
- runQuery: async (fn: any, args: any) => (fn._handler({ db: new DatabaseReader(pool, dbSchema), auth, storage: buildStorageReader() }, args)),
- runMutation: async (fn: any, args: any) => (fn._handler({ db: new DatabaseWriter(pool, dbSchema), auth, storage: buildStorageWriter(), scheduler: buildScheduler(pool) }, args)),
+ runQuery: async (fn: any, args: any) => {
+ const parsed = fn._args.safeParse(args);
+ if (!parsed.success) throw new Error(`Invalid query args: ${parsed.error.message}`);
+ return fn._handler({ db: new DatabaseReader(pool, dbSchema), auth, storage: buildStorageReader() }, parsed.data);
+ },
+ runMutation: async (fn: any, args: any) => {
+ const parsed = fn._args.safeParse(args);
+ if (!parsed.success) throw new Error(`Invalid mutation args: ${parsed.error.message}`);
+ return fn._handler({ db: new DatabaseWriter(pool, dbSchema), auth, storage: buildStorageWriter(), scheduler: buildScheduler(pool) }, parsed.data);
+ },
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/routes/bbf/index.ts` around lines 88 - 95, In
buildActionCtx update runQuery and runMutation so they validate incoming args
with fn._args.safeParse() before calling fn._handler: call
fn._args.safeParse(args), if parsing fails throw or propagate a clear validation
error (including parse error details), and if it succeeds pass the parsed.data
to fn._handler (instead of the original args); reference buildActionCtx,
runQuery, runMutation, fn._handler and fn._args.safeParse to locate and
implement the change.
Closed
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
2 participants
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 by CodeRabbit