feat: implement documented-but-unbuilt features (CLI lifecycle, guardrail engine, templates) - #74
Merged
Merged
Conversation
…rail engine, templates) Implement features specced in docs but previously unbuilt: - CLI Stage 3/4 commands (packages/cli): pipeline (generate/validate/run/status), infra (init/plan/apply/destroy/validate), deploy (deploy/canary/rollback/preview), monitor (logs/metrics/alerts/status), scale (status/config/events/now), patch (check/apply/schedule/rollback), maintain (backup/restore/cleanup/optimize/migrate) plus shared utils/project-state.ts; wired into index.ts. - Provider multi-tenant guardrail engine (packages/core): GuardrailEngine enforcing tenant-scoped writes/strict reads with app-layer fallback, integrated into DbContext via asTenant(); ManagedProviderAdapter stub; tenant-mode Phase C contract types. - New project templates (templates/): saas, api, realtime, blog, ecommerce mirroring the iac template structure. - Fix pre-existing broken relative imports in server-sync.ts that blocked CLI build. Verified: core + cli typecheck/build pass; 71 guardrail/iac tests pass; new CLI commands smoke-tested. No regression vs HEAD baseline.
|
Caution Review failedThe pull request is closed. Note
|
| Layer / File(s) | Summary |
|---|---|
CLI operations and local state packages/cli/src/commands/*, packages/cli/src/utils/project-state.ts |
Adds pipeline, infrastructure, deployment, monitoring, scaling, patching, and maintenance workflows backed by local state, validation, subprocess execution, and structured output. |
CLI registration and synchronization packages/cli/src/index.ts, packages/cli/src/commands/iac/server-sync.ts |
Registers the new command trees, expands the public-command authentication bypass list, and corrects server-sync utility import paths. |
Tenant guardrails and provider contracts packages/core/src/providers/*, packages/core/src/iac/* |
Adds tenant-mode schemas, guardrail enforcement, tenant-aware DbContext, managed-provider placeholders, and public exports. |
Guardrail and context validation packages/core/test/* |
Tests strict and enabled tenant enforcement, bypass and exempt-table behavior, tenant binding, status mapping, and passthrough behavior without a guardrail. |
Application templates templates/api/*, templates/blog/*, templates/ecommerce/*, templates/realtime/*, templates/saas/* |
Adds domain schemas, typed queries and mutations, Hono entrypoints, configurations, package metadata, TypeScript settings, cron examples, module guidance, and documentation for five templates. |
Estimated code review effort: 5 (Critical) | ~120 minutes
Possibly related PRs
- weroperking/Betterbase#69: Overlaps with the CLI public-command authentication bypass changes.
- weroperking/Betterbase#72: Overlaps with the IaC server-sync workflow and utility imports.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 36.73% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately summarizes the main changes: CLI lifecycle commands, guardrail engine work, and new templates. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feat/unbuilt-documented-features
Warning
There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
🔧 Checkov (3.3.8)
templates/api/package.json
Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'
templates/api/tsconfig.json
Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'
templates/blog/package.json
Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'
- 7 others
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 help to get the list of available commands.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/iac/db-context.ts (1)
162-184: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftTenant scoping needs to reach the SQL path —
get/patch/deleteonly filter by_id,query()drops tenant context, andinsert()doesn’t force the tenant column. A tenant-bound caller can still read or mutate another tenant’s row if they know its ID.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/iac/db-context.ts` around lines 162 - 184, Propagate tenant scoping through the database operations: update get, patch, and delete to constrain SQL by both _id and the active tenant; make query pass the tenant context into IaCQueryBuilder; and ensure insert always writes the active tenant column rather than trusting caller data. Reuse _tenantId and the existing tenant configuration symbols, preserving unscoped behavior when no tenant is bound.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/deploy.ts`:
- Line 83: Replace the hardcoded "bun" executable in the build subprocess
invocation with process.execPath, while preserving the existing "run", "build"
arguments and runSubprocess options.
In `@packages/cli/src/commands/infra.ts`:
- Around line 87-93: Update loadConfig to handle schema validation failures
without propagating a raw ZodError: use the safe validation path and return null
(or the existing missing/invalid-config result) when parsing fails. Ensure
runInfraPlan and runInfraApply then produce the same clean invalid-config
guidance used by runInfraValidate, while preserving successful parsing and
missing-file behavior.
In `@packages/cli/src/commands/monitor.ts`:
- Around line 113-126: Update fetchServerMetrics to use the shared apiRequest()
client from utils/api-client.ts instead of loading credentials and calling raw
fetch. Preserve the existing metrics extraction and null-on-failure behavior,
while relying on apiRequest() for the centralized authenticated request
handling, base URL, headers, errors, and timeout.
In `@packages/cli/src/commands/pipeline.ts`:
- Around line 286-289: Replace the `"bun"` executable token with
`process.execPath` for every subprocess invocation: in
`packages/cli/src/commands/pipeline.ts` lines 286-289, update the argv derived
from `step.command` before calling `runSubprocess`; in
`packages/cli/src/commands/patch.ts` lines 60-67, construct the audit argv with
`process.execPath`; and in `packages/cli/src/commands/patch.ts` line 198,
construct the update argv with `process.execPath`. Ensure no spawned command
uses the literal `"bun"` executable.
In `@packages/cli/src/commands/scale.ts`:
- Around line 17-23: The scaleConfigSchema allows an invalid range where min
exceeds max. Add a cross-field refinement to scaleConfigSchema that validates
config.min <= config.max, while preserving the existing field-level validation
and reporting the refinement as a schema validation error.
In `@packages/core/src/providers/guardrail.ts`:
- Around line 164-170: Update DbContext.asTenant() to delegate tenant binding
through the Guardrail.withTenant() method instead of reimplementing it with
unsafe any casts, and make withTenant() apply the binding to the provided
context rather than ignoring _ctx. Preserve the returned tenantId scoping
behavior while ensuring the shared binding path is actually used.
- Around line 65-73: Update enforceWrite and enforceRead to validate that
bypassReason is provided whenever bypass is true, rejecting the operation before
auditBypass runs; preserve normal behavior when bypass is false and ensure valid
bypasses pass the supplied reason through unchanged instead of relying on the
"no reason supplied" fallback.
- Around line 7-13: Reuse TenantModeProviderSchema and its inferred type in
tenant-mode.ts for SetTenantModeRequestSchema.provider and
TenantModeStatus.provider instead of redefining the provider list inline. Import
the existing schema/type from guardrail.ts and remove the duplicate provider
enum and union definitions so both APIs share one source of truth.
In `@packages/core/src/providers/managed.ts`:
- Around line 1-8: Break the circular dependency by relocating
ManagedProviderNotSupportedError from providers/index.ts into a dedicated error
module, then update managed.ts and the providers/index.ts barrel to import or
re-export it from that module. Ensure managed.ts no longer imports through the
providers/index.ts barrel.
In `@packages/core/src/providers/tenant-mode.ts`:
- Around line 28-40: Update the TenantModeStatus interface documentation to
describe enforcedAt as the enforcement location: "database" when database-level
enforcement is active, "application" when application-level enforcement is used,
and null when tenant mode is disabled. Remove the contradictory boolean-style
wording while preserving the existing type and function behavior.
In `@templates/api/betterbase/mutations/items.ts`:
- Around line 28-34: Update the updateItem mutation around the patch
construction to detect when the filtered patch has no fields before calling
ctx.db.patch. Return the existing item unchanged or throw a clear validation
error, ensuring ctx.db.patch is never invoked with an empty object.
In `@templates/api/src/index.ts`:
- Line 3: Add `@betterbase/server` to the dependencies of
templates/api/package.json and templates/blog/package.json so the
betterbaseRouter imports in both src/index.ts files resolve during installation
and builds.
In `@templates/api/tsconfig.json`:
- Line 11: Both template TypeScript configurations include the incorrect
“bbf/**/*.ts” path, so update the include arrays in templates/api/tsconfig.json
and templates/blog/tsconfig.json to use “betterbase/**/*.ts” instead, preserving
the other entries.
In `@templates/blog/betterbase/schema.ts`:
- Around line 15-25: Remove the redundant regular slug indexes from the posts
and tags table definitions, and rename each corresponding unique index from
by_slug_unique to by_slug. Preserve the existing by_slug query references while
retaining uniqueness enforcement.
In `@templates/ecommerce/betterbase/schema.ts`:
- Around line 22-25: Require a string userId for all cart and order records and
related operations, preventing anonymous users from sharing a null-keyed cart.
Update templates/ecommerce/betterbase/schema.ts lines 22-25 and 35-41,
templates/ecommerce/betterbase/mutations/store.ts lines 20-24 and 67-70, and
templates/ecommerce/betterbase/queries/store.ts lines 26-28 to use v.string()
for userId; clients must provide a stable locally persisted session ID for
anonymous users.
- Around line 17-18: Remove the redundant regular "by_sku" index from the schema
definition, keeping the unique "by_sku_unique" index on the sku column unchanged
so sku lookups remain indexed.
In `@templates/ecommerce/package.json`:
- Around line 11-15: Add `@betterbase/server` to the dependencies object in
package.json alongside the existing workspace dependencies, using the same
workspace version specifier so the src/index.ts import of betterbaseRouter
resolves at runtime.
In `@templates/ecommerce/tsconfig.json`:
- Line 11: Update the include array in tsconfig.json to replace the non-existent
bbf/**/*.ts pattern with betterbase/**/*.ts, while preserving the existing
src/**/*.ts and betterbase.config.ts entries so database queries, mutations, and
schemas are typechecked.
In `@templates/realtime/betterbase/mutations/rooms.ts`:
- Around line 41-53: Update the lastSeen values in the existing-record patch and
new-record insert paths of the presence mutation to remove the unsafe as unknown
as Date cast. Match the value to the schema’s expected type: pass a Date
instance when the field uses v.datetime(), or retain the ISO string without
casting when the schema expects a string.
In `@templates/realtime/betterbase/schema.ts`:
- Around line 14-21: Remove the duplicate index declaration from the messages
table definition, keeping only one index for ["roomId", "_createdAt"] and
preserving the existing index name used by the intended queries.
In `@templates/realtime/package.json`:
- Around line 11-15: Add `@betterbase/server` to the dependencies object in the
template package configuration, using the same workspace version convention as
`@betterbase/core` and `@betterbase/client`, so the betterbaseRouter import in
src/index.ts resolves during installation and builds.
In `@templates/realtime/tsconfig.json`:
- Around line 11-12: Update the include array in the template tsconfig to
replace the incorrect bbf/**/*.ts entry with betterbase/**/*.ts, preserving the
existing src and betterbase.config.ts entries.
In `@templates/saas/package.json`:
- Around line 11-16: Add `@betterbase/server` to the dependencies object in
templates/saas/package.json using the workspace version convention already used
by `@betterbase/core` and `@betterbase/client`, so the import in src/index.ts
resolves during installation and builds.
In `@templates/saas/tsconfig.json`:
- Around line 11-12: Update the `include` array in `tsconfig.json` to replace
the incorrect `bbf/**/*.ts` pattern with `betterbase/**/*.ts`, while preserving
the existing `src/**/*.ts` and `betterbase.config.ts` entries.
---
Outside diff comments:
In `@packages/core/src/iac/db-context.ts`:
- Around line 162-184: Propagate tenant scoping through the database operations:
update get, patch, and delete to constrain SQL by both _id and the active
tenant; make query pass the tenant context into IaCQueryBuilder; and ensure
insert always writes the active tenant column rather than trusting caller data.
Reuse _tenantId and the existing tenant configuration symbols, preserving
unscoped behavior when no tenant is bound.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52a1b728-be1b-4cd0-bceb-054eac4bc503
📒 Files selected for processing (69)
.gitignorepackages/cli/src/commands/deploy.tspackages/cli/src/commands/iac/server-sync.tspackages/cli/src/commands/infra.tspackages/cli/src/commands/maintain.tspackages/cli/src/commands/monitor.tspackages/cli/src/commands/patch.tspackages/cli/src/commands/pipeline.tspackages/cli/src/commands/scale.tspackages/cli/src/index.tspackages/cli/src/utils/project-state.tspackages/core/src/iac/db-context.tspackages/core/src/iac/index.tspackages/core/src/providers/guardrail.tspackages/core/src/providers/index.tspackages/core/src/providers/managed.tspackages/core/src/providers/tenant-mode.tspackages/core/test/guardrail.test.tspackages/core/test/iac.test.tstemplates/api/README.mdtemplates/api/betterbase.config.tstemplates/api/betterbase/cron.tstemplates/api/betterbase/mutations/items.tstemplates/api/betterbase/queries/items.tstemplates/api/betterbase/schema.tstemplates/api/package.jsontemplates/api/src/index.tstemplates/api/src/modules/README.mdtemplates/api/tsconfig.jsontemplates/blog/README.mdtemplates/blog/betterbase.config.tstemplates/blog/betterbase/cron.tstemplates/blog/betterbase/mutations/posts.tstemplates/blog/betterbase/queries/posts.tstemplates/blog/betterbase/schema.tstemplates/blog/package.jsontemplates/blog/src/index.tstemplates/blog/src/modules/README.mdtemplates/blog/tsconfig.jsontemplates/ecommerce/README.mdtemplates/ecommerce/betterbase.config.tstemplates/ecommerce/betterbase/cron.tstemplates/ecommerce/betterbase/mutations/store.tstemplates/ecommerce/betterbase/queries/store.tstemplates/ecommerce/betterbase/schema.tstemplates/ecommerce/package.jsontemplates/ecommerce/src/index.tstemplates/ecommerce/src/modules/README.mdtemplates/ecommerce/tsconfig.jsontemplates/realtime/README.mdtemplates/realtime/betterbase.config.tstemplates/realtime/betterbase/cron.tstemplates/realtime/betterbase/mutations/rooms.tstemplates/realtime/betterbase/queries/rooms.tstemplates/realtime/betterbase/schema.tstemplates/realtime/package.jsontemplates/realtime/src/index.tstemplates/realtime/src/modules/README.mdtemplates/realtime/tsconfig.jsontemplates/saas/README.mdtemplates/saas/betterbase.config.tstemplates/saas/betterbase/cron.tstemplates/saas/betterbase/mutations/tenants.tstemplates/saas/betterbase/queries/tenants.tstemplates/saas/betterbase/schema.tstemplates/saas/package.jsontemplates/saas/src/index.tstemplates/saas/src/modules/README.mdtemplates/saas/tsconfig.json
Comment on lines
+87
to
+93
| async function loadConfig(projectRoot: string): Promise<InfraConfig | null> { | ||
| const file = configPath(projectRoot); | ||
| if (!existsSync(file)) return null; | ||
| const raw = await readJson<unknown>(file, null); | ||
| if (raw === null) return null; | ||
| return infraConfigSchema.parse(raw); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
loadConfig throws on malformed config; plan/apply surface a raw ZodError.
runInfraValidate uses safeParse and prints friendly issues, but runInfraPlan/runInfraApply go through loadConfig, which calls .parse(). A config that exists but fails schema validation throws, escaping to the global unhandledRejection handler with a stack instead of a clean "invalid config — run bb infra validate" message.
Proposed fix
-async function loadConfig(projectRoot: string): Promise<InfraConfig | null> {
- const file = configPath(projectRoot);
- if (!existsSync(file)) return null;
- const raw = await readJson<unknown>(file, null);
- if (raw === null) return null;
- return infraConfigSchema.parse(raw);
-}
+async function loadConfig(projectRoot: string): Promise<InfraConfig | null> {
+ const file = configPath(projectRoot);
+ if (!existsSync(file)) return null;
+ const raw = await readJson<unknown>(file, null);
+ if (raw === null) return null;
+ const result = infraConfigSchema.safeParse(raw);
+ if (!result.success) {
+ throw new Error(
+ `Invalid infrastructure.config.json. Run \`bb infra validate\` for details.`,
+ );
+ }
+ return result.data;
+}📝 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 function loadConfig(projectRoot: string): Promise<InfraConfig | null> { | |
| const file = configPath(projectRoot); | |
| if (!existsSync(file)) return null; | |
| const raw = await readJson<unknown>(file, null); | |
| if (raw === null) return null; | |
| return infraConfigSchema.parse(raw); | |
| } | |
| async function loadConfig(projectRoot: string): Promise<InfraConfig | null> { | |
| const file = configPath(projectRoot); | |
| if (!existsSync(file)) return null; | |
| const raw = await readJson<unknown>(file, null); | |
| if (raw === null) return null; | |
| const result = infraConfigSchema.safeParse(raw); | |
| if (!result.success) { | |
| throw new Error( | |
| `Invalid infrastructure.config.json. Run \`bb infra validate\` for details.`, | |
| ); | |
| } | |
| return result.data; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/infra.ts` around lines 87 - 93, Update loadConfig
to handle schema validation failures without propagating a raw ZodError: use the
safe validation path and return null (or the existing missing/invalid-config
result) when parsing fails. Ensure runInfraPlan and runInfraApply then produce
the same clean invalid-config guidance used by runInfraValidate, while
preserving successful parsing and missing-file behavior.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/iac/db-context.ts (2)
289-292: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winCritical tenant isolation bypass: update the
DatabaseWriterconstructor signature.
DbContextinvokesnew DatabaseWriterwith 5 arguments (passingguardrail,tenantId, andtenantExemptTables), but this constructor only accepts 3 arguments(pool, schema, options?). Consequently:
- The
guardrailengine instance is incorrectly passed into theoptionsparameter.- The
super(pool, schema)call entirely omits the guardrail and tenant arguments.As a result,
this._guardrail,this._tenantId, andthis._tenantExemptTablesevaluate toundefinedon the writer, completely bypassing all tenant-scoping enforcement for every database write.🔒️ Proposed fix to preserve tenant guardrails
- constructor(pool: Pool, schema: string, options?: { onChange?: ChangeHook }) { - super(pool, schema); - this._onChange = options?.onChange; - } + constructor( + pool: Pool, + schema: string, + guardrail?: GuardrailEngine | null, + tenantId?: string, + tenantExemptTables?: string[], + options?: { onChange?: ChangeHook } + ) { + super(pool, schema, guardrail, tenantId, tenantExemptTables); + this._onChange = options?.onChange; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/iac/db-context.ts` around lines 289 - 292, Update the DatabaseWriter constructor to accept and forward the guardrail, tenantId, and tenantExemptTables arguments expected by DbContext, while preserving the optional onChange options parameter. Pass all guardrail and tenant-scoping values to the superclass so DatabaseWriter initializes _guardrail, _tenantId, and _tenantExemptTables correctly.
70-74: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix SQL syntax error when appending
LIMIT 1.Appending
LIMIT 1directly to the generatedsqlstring will result in a syntax error (e.g.,LIMIT 5 LIMIT 1) ifthis._limitwas previously set via the.take()method. Set the limit state explicitly before building the SQL instead.🐛 Proposed fix
async first(): Promise<T | null> { + const originalLimit = this._limit; + this._limit = 1; const { sql, params } = this._buildSQL(); - const { rows } = await this._pool.query(`${sql} LIMIT 1`, params as any[]); + this._limit = originalLimit; + const { rows } = await this._pool.query(sql, params as any[]); return (rows[0] as T) ?? null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/iac/db-context.ts` around lines 70 - 74, Update the first method to set the query limit state to 1 before calling _buildSQL(), then execute the generated SQL without appending another LIMIT clause. Preserve the existing first-row return behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/iac/db-context.ts`:
- Around line 289-292: Update the DatabaseWriter constructor to accept and
forward the guardrail, tenantId, and tenantExemptTables arguments expected by
DbContext, while preserving the optional onChange options parameter. Pass all
guardrail and tenant-scoping values to the superclass so DatabaseWriter
initializes _guardrail, _tenantId, and _tenantExemptTables correctly.
- Around line 70-74: Update the first method to set the query limit state to 1
before calling _buildSQL(), then execute the generated SQL without appending
another LIMIT clause. Preserve the existing first-row return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c0c9042-6acc-462b-a682-9747b790932e
📒 Files selected for processing (2)
packages/cli/src/commands/iac/server-sync.tspackages/core/src/iac/db-context.ts
- guardrail: remove warnAppEnforced from success paths (log spam)
- guardrail: require bypassReason when bypass is true
- guardrail: reuse TenantModeProviderSchema in tenant-mode.ts
- guardrail: update withTenant to accept context parameter
- managed: break circular dependency via new errors.ts
- db-context: add _contextParams() accessor replacing as any casts
- db-context: propagate tenant scoping through get/patch/delete/query/insert
- project-state: replace require('node:fs') with proper import
- project-state: make hasBinary cross-platform (where on win32)
- deploy/pipeline/patch: replace hardcoded 'bun' with process.execPath
- infra: use safeParse in loadConfig returning null on failure
- monitor: use shared apiRequest for fetchServerMetrics
- scale: add cross-field refinement min <= max
- templates: add @betterbase/server to all 5 package.json files
- templates: fix bbf/**/*.ts -> betterbase/**/*.ts in tsconfig
- templates/blog: remove redundant by_slug indexes
- templates/ecommerce: require userId as string; remove redundant by_sku index
- templates/realtime: remove duplicate by_room_created index; fix Date cast
- templates/api: guard against empty patch in updateItem
- docs: update bbf/ references to betterbase/ in READMEs
Comment on lines
338
to
+350
| /** Partial update — merges provided fields, updates `_updatedAt` */ | ||
| async patch(table: string, id: string, fields: Record<string, unknown>): Promise<void> { | ||
| this._guardrail?.enforceWrite(table, this._enforceOpts(table)); | ||
| const updates = Object.entries(fields) | ||
| .map(([k], i) => `"${k}" = $${i + 2}`) | ||
| .join(", "); | ||
| const values = [id, ...Object.values(fields)]; | ||
| await this._pool.query( | ||
| `UPDATE "${this._schema}"."${table}" SET ${updates}, "_updatedAt" = NOW() WHERE _id = $1`, | ||
| values as any[], | ||
| ); | ||
| const values = this._tenantId | ||
| ? [id, this._tenantId, ...Object.values(fields)] | ||
| : [id, ...Object.values(fields)]; | ||
| const query = this._tenantId | ||
| ? `UPDATE "${this._schema}"."${table}" SET ${updates}, "_updatedAt" = NOW() WHERE _id = $1 AND tenant_id = $2` | ||
| : `UPDATE "${this._schema}"."${table}" SET ${updates}, "_updatedAt" = NOW() WHERE _id = $1`; | ||
| await this._pool.query(query, values as any[]); |
There was a problem hiding this comment.
patch writes every field to the wrong value. When this._tenantId is set, the values array is [id, tenantId, ...fieldValues], making $2 = tenantId. The WHERE clause correctly uses tenant_id = $2, but the SET clause also starts at $2 — so the first field is set to tenantId, the second field gets the value intended for the first, and so on. All updated field values are shifted by one and the last value is silently dropped. For example, patching { email: "x@y.com" } in a tenant context would execute SET "email" = $2 where $2 is the tenant ID string, corrupting the row.
Suggested change
| /** Partial update — merges provided fields, updates `_updatedAt` */ | |
| async patch(table: string, id: string, fields: Record<string, unknown>): Promise<void> { | |
| this._guardrail?.enforceWrite(table, this._enforceOpts(table)); | |
| const updates = Object.entries(fields) | |
| .map(([k], i) => `"${k}" = $${i + 2}`) | |
| .join(", "); | |
| const values = [id, ...Object.values(fields)]; | |
| await this._pool.query( | |
| `UPDATE "${this._schema}"."${table}" SET ${updates}, "_updatedAt" = NOW() WHERE _id = $1`, | |
| values as any[], | |
| ); | |
| const values = this._tenantId | |
| ? [id, this._tenantId, ...Object.values(fields)] | |
| : [id, ...Object.values(fields)]; | |
| const query = this._tenantId | |
| ? `UPDATE "${this._schema}"."${table}" SET ${updates}, "_updatedAt" = NOW() WHERE _id = $1 AND tenant_id = $2` | |
| : `UPDATE "${this._schema}"."${table}" SET ${updates}, "_updatedAt" = NOW() WHERE _id = $1`; | |
| await this._pool.query(query, values as any[]); | |
| /** Partial update — merges provided fields, updates `_updatedAt` */ | |
| async patch(table: string, id: string, fields: Record<string, unknown>): Promise<void> { | |
| this._guardrail?.enforceWrite(table, this._enforceOpts(table)); | |
| const fieldOffset = this._tenantId ? 3 : 2; | |
| const updates = Object.entries(fields) | |
| .map(([k], i) => `"${k}" = $${i + fieldOffset}`) | |
| .join(", "); | |
| const tenantParamIdx = this._tenantId ? Object.keys(fields).length + fieldOffset : undefined; | |
| const values = this._tenantId | |
| ? [id, ...Object.values(fields), this._tenantId] | |
| : [id, ...Object.values(fields)]; | |
| const query = this._tenantId | |
| ? `UPDATE "${this._schema}"."${table}" SET ${updates}, "_updatedAt" = NOW() WHERE _id = $1 AND tenant_id = $${tenantParamIdx}` | |
| : `UPDATE "${this._schema}"."${table}" SET ${updates}, "_updatedAt" = NOW() WHERE _id = $1`; | |
| await this._pool.query(query, values as any[]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/iac/db-context.ts
Line: 338-350
Comment:
**Tenant `patch` writes every field to the wrong value.** When `this._tenantId` is set, the values array is `[id, tenantId, ...fieldValues]`, making `$2 = tenantId`. The WHERE clause correctly uses `tenant_id = $2`, but the SET clause also starts at `$2` — so the first field is set to `tenantId`, the second field gets the value intended for the first, and so on. All updated field values are shifted by one and the last value is silently dropped. For example, patching `{ email: "x@y.com" }` in a tenant context would execute `SET "email" = $2` where `$2` is the tenant ID string, corrupting the row.
```suggestion
/** Partial update — merges provided fields, updates `_updatedAt` */
async patch(table: string, id: string, fields: Record<string, unknown>): Promise<void> {
this._guardrail?.enforceWrite(table, this._enforceOpts(table));
const fieldOffset = this._tenantId ? 3 : 2;
const updates = Object.entries(fields)
.map(([k], i) => `"${k}" = $${i + fieldOffset}`)
.join(", ");
const tenantParamIdx = this._tenantId ? Object.keys(fields).length + fieldOffset : undefined;
const values = this._tenantId
? [id, ...Object.values(fields), this._tenantId]
: [id, ...Object.values(fields)];
const query = this._tenantId
? `UPDATE "${this._schema}"."${table}" SET ${updates}, "_updatedAt" = NOW() WHERE _id = $1 AND tenant_id = $${tenantParamIdx}`
: `UPDATE "${this._schema}"."${table}" SET ${updates}, "_updatedAt" = NOW() WHERE _id = $1`;
await this._pool.query(query, values as any[]);
```
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements features specced in docs/specs but previously unbuilt (per audit in
.kilo/plans/1784539654788-unbuilt-documented-features.md).CLI Stage 3 & 4 commands (
packages/cli/src/commands/)pipeline— generate/validate/run/status (CI config generation + local runs)infra— init/plan/apply/destroy/validate (infrastructure config state)deploy— deploy/canary/rollback/preview (manifest-based,--env,--dry-run)monitor— logs/metrics/alerts/status (local state +--json)scale— status/config/events/now (scaling config state)patch— check/apply/schedule/rollback (wrapsbun audit, safe patching)maintain— backup/restore/cleanup/optimize/migrateutils/project-state.tsshared helper; all wired intoindex.ts.Provider multi-tenant guardrail engine (
packages/core/src/providers/)guardrail.ts—GuardrailEngineenforcing tenant-scoped writes / strict reads with app-layer fallback for non-RLS providers; integrated intoDbContextviaasTenant().managed.ts—ManagedProviderAdapterfills theProviderAdapterinterface (methods throwManagedProviderNotSupportedError).tenant-mode.ts— Phase C contract types (no invented server endpoints).New project templates (
templates/)saas/,api/,realtime/,blog/,ecommerce/mirroring theiactemplate structure with real schemas/functions per domain.Fix
server-sync.ts(../utils->../../utils) that blocked the entire CLI build/run.Verification
packages/core: typecheck pass, build pass, 71 guardrail/iac tests passpackages/cli: new files typecheck clean,bun run buildpass, all 7 command groups smoke-testediac-commands/context-generatorissues unrelated to this PR)Note
coderabbit review --agentcould not be run: the CodeRabbit CLI is rate-limited on the free tier. Manual structured review performed instead (no stubs/TODOs in core engine or CLI commands; templates use the samev.*API astemplates/iac).🤖 Generated with Kilo
Summary by CodeRabbit
Greptile Summary
This PR implements six CLI command groups (pipeline, infra, deploy, monitor, scale, patch, maintain), a multi-tenant guardrail engine in
packages/core, and five new project templates. It also fixes a pre-existing broken relative import inserver-sync.tsthat blocked the CLI build.guardrail.ts,managed.ts,tenant-mode.ts):GuardrailEngineenforces tenant-scoped writes and, in strict mode, reads — with bypass auditing and app-layer fallback for non-RLS providers. Integrated intoDbContextviaasTenant()..betterbase/state (no server dependency); all wired intoindex.ts. A sharedproject-state.tshelper handles subprocess spawning, JSON state I/O, and CI/PM detection.saas,api,realtime,blog,ecommerce): Mirror the existingiactemplate structure with domain-appropriate schemas and mutations.Confidence Score: 3/5
The CLI and template additions are clean, but the tenant-aware DatabaseWriter.patch method has a parameter index mismatch that would silently corrupt every field on a tenant-scoped update.
In DatabaseWriter.patch, the SET clause generates placeholders starting at $2 (
$${i + 2}), while the tenant query prepends tenantId as $2 in the values array. The WHERE clause'stenant_id = $2and the first SET assignment both bind to the same tenantId value — so every patched field receives the wrong bound value and the actual field values are shifted or dropped entirely.packages/core/src/iac/db-context.ts — specifically DatabaseWriter.patch
Important Files Changed
DatabaseWriter.patchthat corrupts data on any tenant-scoped update.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant App participant DbContext participant GuardrailEngine participant DatabaseWriter participant Postgres App->>DbContext: asTenant("t1") DbContext-->>App: "new DbContext(tenantId="t1")" App->>DbContext: "writer.patch("users", id, {email})" DbContext->>DatabaseWriter: "patch("users", id, {email})" DatabaseWriter->>GuardrailEngine: "enforceWrite("users", {tenantId:"t1"})" GuardrailEngine-->>DatabaseWriter: ok (scoped) Note over DatabaseWriter: Bug: values=[id,"t1",email]<br/>SET "email"=$2 binds tenantId<br/>tenant_id=$2 also binds tenantId DatabaseWriter->>Postgres: "UPDATE SET "email"=$2 WHERE _id=$1 AND tenant_id=$2" Postgres-->>DatabaseWriter: corrupt row (email set to tenantId)%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant App participant DbContext participant GuardrailEngine participant DatabaseWriter participant Postgres App->>DbContext: asTenant("t1") DbContext-->>App: "new DbContext(tenantId="t1")" App->>DbContext: "writer.patch("users", id, {email})" DbContext->>DatabaseWriter: "patch("users", id, {email})" DatabaseWriter->>GuardrailEngine: "enforceWrite("users", {tenantId:"t1"})" GuardrailEngine-->>DatabaseWriter: ok (scoped) Note over DatabaseWriter: Bug: values=[id,"t1",email]<br/>SET "email"=$2 binds tenantId<br/>tenant_id=$2 also binds tenantId DatabaseWriter->>Postgres: "UPDATE SET "email"=$2 WHERE _id=$1 AND tenant_id=$2" Postgres-->>DatabaseWriter: corrupt row (email set to tenantId)Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix: address all CodeRabbit review comme..." | Re-trigger Greptile