refactor(cli): enhance IaC workflow and automation capabilities#72
Open
weroperking wants to merge 2 commits into
Open
refactor(cli): enhance IaC workflow and automation capabilities#72weroperking wants to merge 2 commits into
weroperking wants to merge 2 commits into
Conversation
Refactor the CLI to provide more robust Infrastructure as Code (IaC) management, specifically targeting automation for AI agents and seamless server synchronization. Key improvements: - Implement headless mode and auto-registration for `iac sync` to support non-interactive environments. - Add `migrate-legacy` command to facilitate the transition from legacy project structures to the IaC model. - Introduce `validate-project` command to ensure local project compliance with IaC standards. - Enhance `init` command with `AGENTS.md` templates and stricter project name validation. - Expand `api-client` with dedicated methods for project registration, schema synchronization, and environment syncing. - Add `runHeadlessLogin` to support API-key based authentication for automated workflows. - Implement server-side routing for project-scoped IaC synchronization. - Clean up configuration and remove obsolete log files.
|
Warning Review limit reached
More reviews will be available in 25 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughThis PR completes the migration of BetterBase from optional IaC to IaC-by-default. It adds environment detection, legacy project conversion, headless server synchronization, and project validation. The init command is rewritten as a template copier. Server gains IaC sync endpoints. Tests and config are updated accordingly. ChangesBetterBase IaC-by-Default Migration
Sequence DiagramsequenceDiagram
participant CLI as CLI: bb iac sync --headless
participant Auth as isAuthenticated
participant EnvDet as detectEnvironmentConfig
participant Sync as syncWithServer
participant ApiClient as ApiClient
participant Server as Server
CLI->>Auth: Check credentials
Auth-->>CLI: Token valid?
CLI->>EnvDet: Scan .env files
EnvDet-->>CLI: ProjectEnvironment
CLI->>Sync: projectRoot, schema, envConfig
Sync->>ApiClient: registerProject(name, slug)
ApiClient->>Server: POST /api/projects
Server-->>ApiClient: projectId
Sync->>ApiClient: syncSchema(projectId, schema)
ApiClient->>Server: POST /projects/:slug/schema
Server-->>ApiClient: Success
Sync->>ApiClient: syncEnvironment(projectId, envConfig)
ApiClient->>Server: POST /projects/:slug/environment
Server-->>ApiClient: Success
Sync-->>CLI: Completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Multiple architectural changes across CLI (environment detection, legacy migration, API client, headless sync, init refactor) and server (IaC routes). Includes new public APIs ( Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/cli/src/commands/init.ts (2)
59-72: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDead code:
copyDirhelper is defined but never called.The recursive
copyDirfunction is unused—templateFilesloop at lines 89-109 handles file copying. RemovecopyDiror use it consistently.🤖 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/init.ts` around lines 59 - 72, The helper function copyDir is dead code (defined but never invoked) and should be removed or used consistently; either delete the copyDir declaration or replace the manual templateFiles loop with a single call to copyDir(templateSrcPath, targetPath) where templateFiles is currently iterated. Locate the copyDir function and the templateFiles handling code and choose one approach: remove copyDir and keep the existing loop, or remove the loop and call copyDir for recursive copying to ensure consistent behavior.
116-117:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSilent catch hides real errors.
Empty catch block swallows all exceptions when updating
package.json. A missing file is expected (handled elsewhere), but parse errors or permission issues should surface.Proposed fix
try { const pkgJson = JSON.parse(await readFile(pkgPath, "utf-8")); pkgJson.name = projectName; await writeFile(pkgPath, `${JSON.stringify(pkgJson, null, 2)}\n`); - } catch { - } + } catch (err) { + const code = (err as NodeJS.ErrnoException | undefined)?.code; + if (code !== "ENOENT") { + throw err; + } + }🤖 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/init.ts` around lines 116 - 117, The empty catch in the init command swallows all errors when updating package.json; change the block to catch the error object (e.g., catch (err)) and only ignore expected "file not found" errors (err.code === 'ENOENT' or equivalent), but for all other errors (JSON parse errors, permission issues) rethrow or surface them (throw err or processLogger.error(...); throw err). Update the catch in the package.json update section of packages/cli/src/commands/init.ts (the try that writes/reads package.json) so unexpected exceptions are not silently ignored.packages/cli/test/integration/init.test.ts (1)
29-54: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueTests for removed/unused helpers.
providerTypeSchema,getDatabaseLabel, andgetAuthDialectare tested here but no longer used in the refactoredinit.ts. These appear to be legacy helpers from the removed provider selection flow. Either remove these tests or move the helpers/tests to where they're actually used.Also applies to: 129-230
🤖 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/test/integration/init.test.ts` around lines 29 - 54, The test file contains legacy helpers providerTypeSchema, getDatabaseLabel, and getAuthDialect that are no longer used by the refactored init flow; either remove these helper definitions and the tests that assert them from the init.test.ts file, or move the helper implementations and their tests into the module that still needs provider selection (e.g., the provider selection/CLI module) and update imports so the tests reference the actual exported symbols (providerTypeSchema, getDatabaseLabel, getAuthDialect). Ensure you only keep tests for helpers that are exported and used by production code.packages/cli/src/index.ts (1)
302-328:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate
iac importcommand registration.
iac importis defined twice (lines 302-314 and 316-328). Commander will use the second definition, making the first dead code. Remove one.Proposed fix
- iac - .command("import") - .description("Import data into the project database") - .argument("<input>", "Input file path to import") - .option("-t, --table <name>", "Table name to import into") - .option("-d, --dry-run", "Preview changes without applying them") - .action(async (input: string, options: { table?: string; dryRun?: boolean }) => { - await runIacImport(process.cwd(), { - input, - table: options.table, - dryRun: options.dryRun, - }); - }); - iac .command("migrate-legacy")🤖 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/index.ts` around lines 302 - 328, There are two duplicate registrations of the CLI command via iac.command("import") that both call runIacImport; remove the redundant block so the command is only registered once (keep one of the iac.command("import") ... .action(async (input, options) => { await runIacImport(process.cwd(), { input, table: options.table, dryRun: options.dryRun }); }) blocks), ensuring any options and arguments present in the kept block (e.g., .argument, .option, .action) are preserved.packages/cli/src/commands/iac/sync.ts (1)
50-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not skip headless sync when there is no schema diff.
The early return on
diff.isEmptybypasses the entire headless branch, sobb iac sync --headlessnever auto-registers the project or syncs environment config unless a local schema change also exists. That breaks the advertised headless workflow. As per coding guidelines, "bb iac sync --headlessmust sync schema+env without prompts."Also applies to: 101-123
🤖 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/iac/sync.ts` around lines 50 - 53, The early-return on diff.isEmpty prevents the headless path from running; change the logic in the iac sync handler so that you only return early when diff.isEmpty AND the headless flag (opts.headless or similar) is false — i.e., if diff.isEmpty and not opts.headless then log success (unless opts.silent) and return, otherwise continue into the headless/autoregistration and env-sync flow; make the same adjustment in the other identical block referenced (lines ~101-123) so headless always proceeds to auto-register and sync env config even when there is no schema diff.
🤖 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 @.coderabbit.yaml:
- Around line 23-122: The path_instructions block is missing a rule to validate
spec markdowns; add a new path entry under path_instructions that matches glob
patterns for "BetterBase_*_Spec.md" and related charter/spec docs (e.g.,
"**/BetterBase_*_Spec.md", "**/*_charter.md", "**/*.md" for spec dirs) and
include the required guardrail text: verify task dependencies, monorepo file
paths, and verifiable acceptance criteria (the same checklist style used for
other path entries). Update the existing path_instructions sequence to include
this new markdown spec rule so spec-document checks run during IaC/orchestration
reviews.
In `@packages/cli/src/commands/iac/env-detector.ts`:
- Around line 96-100: The code uses an unchecked cast for providerMatch[1] which
can assign arbitrary strings to envConfig.database.provider; change this to
validate the runtime value against an explicit allowlist (e.g. const allowed =
['postgresql','turso','planetscale','supabase','neon']), normalize the captured
string (trim().toLowerCase()), check allowed.includes(captured), and only then
set envConfig.database.provider = captured as the validated union; if it’s not
allowed, skip assignment or log/handle the unexpected value. Reference:
providerMatch, provider, envConfig.database.provider and the
configContent.match(...) usage.
- Around line 58-88: The detector currently only assigns known keys to envConfig
(database, auth, storage, ai, monitoring) and never fills envConfig.custom,
which loses unknown env vars; after the existing if-blocks that handle
DATABASE_URL, TURSO_*, AUTH_*, STORAGE_*, OPENAI_API_KEY, SENTRY_DSN, LOG_LEVEL,
add logic that if key is non-empty and not matched by any whitelist fields then
assign envConfig.custom[key] = value (ensure envConfig.custom is initialized as
an object), and skip adding duplicate empty keys; this preserves all
unrecognized env variables for headless sync while leaving the existing
envConfig.database/auth/storage/ai/monitoring assignments intact.
In `@packages/cli/src/commands/iac/migrate-legacy.ts`:
- Around line 160-203: The AGENTS.md content is hardcoded in
generateAgentsConstraintFile; instead read and copy the canonical template file
(templates/iac/AGENTS.md) into the project root so the migration uses the
authoritative IaC constraints; update generateAgentsConstraintFile to locate the
canonical template, read its contents (or throw/log a clear error if missing),
and write that content to path.join(projectRoot, "AGENTS.md") using the existing
writeFile call (preserve the function name generateAgentsConstraintFile and the
target "AGENTS.md"), optionally adding a safe fallback or explicit error to
surface template-missing cases.
- Around line 56-59: The current write of converted routes uses route.path as a
filename which causes collisions (e.g., multiple index.ts files) — update the
target path generation so it preserves the original route directory structure
(or otherwise makes filenames unique) instead of flattening to a single
filename; specifically, change the code that builds targetPath (which uses
route.method, betterbaseDir, route.path and writeFile with functionCode) to join
the equivalent source subpath (or a sanitized route.entry path/name) into the
target (e.g., path.join(betterbaseDir, "queries"|"mutations",
...routeSourceSubdirs, `${routeFileName}.ts`)), ensure any intermediate
directories are created before calling writeFile, and sanitize/normalize names
to avoid collisions.
- Around line 91-98: The current guard in migrate-legacy.ts only enters the
Hono-route detection block when content.includes("Hono") &&
(content.includes(".get(") || content.includes(".post(")), which skips files
that only use .put( or .delete(. Update that conditional to check for any HTTP
method (e.g., content.includes("Hono") && (content.includes(".get(") ||
content.includes(".post(") || content.includes(".put(") ||
content.includes(".delete("))) ) and keep the subsequent methods extraction
logic (the methods array population) as-is so PUT/DELETE-only files are detected
and migrated.
In `@packages/cli/src/commands/iac/server-sync.ts`:
- Around line 35-47: The function syncWithServer currently returns syncResult
(from apiClient.syncSchema) but is declared as Promise<void>; either remove the
returned value (drop "return syncResult") or change the function signature to
return the actual result type: update syncWithServer's return type from
Promise<void> to the type returned by apiClient.syncSchema (e.g., import the API
type or use ReturnType<typeof apiClient.syncSchema>) and adjust any callers
accordingly; locate syncWithServer, the syncResult variable, and the
apiClient.syncSchema / apiClient.syncEnvironment calls to apply the fix.
In `@packages/cli/src/commands/iac/sync.ts`:
- Around line 108-116: The code is loading the previous snapshot into the
variable `schema` (via loadSerializedSchema(prevFile)) and then passing that
stale `schema` into syncWithServer, but the up-to-date serialized schema is in
the variable `current`; change the sync call to pass the current serialized
schema (use `current` instead of `schema`) so the server receives the newly
generated schema during headless sync, leaving the prevFile load and its later
update intact.
In `@packages/cli/src/commands/init.ts`:
- Around line 28-33: The hardcoded ascending relative paths in
possibleTemplatePaths (using import.meta.dir) are brittle; replace them with a
deterministic package-relative resolution strategy: derive the current file path
from import.meta.url (using fileURLToPath) and compute a single well-formed
templates path, and/or attempt to resolve the templates directory via Node
package resolution (e.g., require.resolve or import resolution) for the
"betterbase" package as a fallback; update the logic that builds
possibleTemplatePaths to try the package-resolved template location first and
only fall back to a single relative path computed from import.meta.url, ensuring
you update references to possibleTemplatePaths and the code that iterates it.
- Around line 189-205: The check for an existing project directory uses
Bun.file(projectPath).exists(), which returns false for directories; replace
that call with the synchronous path existence check existsSync(projectPath) to
detect directories correctly, keep the existing overwrite prompt and the try
block that calls rm(projectPath, { recursive: true, force: true }) and
mkdir(projectPath, { recursive: true }), and ensure error paths still call
logger.error with the caught error; update the variable existingDir to be set
from existsSync(projectPath) instead of Bun.file(...).exists().
In `@packages/cli/src/commands/login.ts`:
- Around line 193-197: The saved credentials lack the required admin_email so
loadCredentials() fails later; update the saveCredentials() call in
runHeadlessLogin() to include admin_email (e.g., admin_email: opts.adminEmail)
and validate its presence before saving (or throw/return an error if
opts.adminEmail is missing) so the object saved matches CredentialsSchema and
can be successfully loaded by loadCredentials().
- Line 184: The call in runHeadlessLogin is passing an object to createApiClient
but createApiClient expects a string; update the invocation in runHeadlessLogin
to pass the base URL string instead of an object (i.e., call createApiClient
with opts.serverUrl or a safe string/coerced value) so createApiClient(baseUrl?:
string) receives the correct type and subsequent API wiring works; ensure any
undefined fallback is handled if opts.serverUrl can be missing.
In `@packages/cli/src/commands/validate.ts`:
- Around line 12-16: forbiddenPatterns is defined but never used so the
validator isn't blocking IaC-prohibited patterns; update the validation flow in
validate.ts to iterate files and apply forbiddenPatterns (use the constant name
forbiddenPatterns) to each file's path and contents, fail the validation when a
pattern matches and surface its message; additionally add explicit checks in the
same validation routine (e.g., in whichever function handles project validation
such as validateProject/validateFiles) to detect unauthorized package.json
changes (compare current package.json to expected or disallow dependency/key
edits) and to detect `_generated` mutation files (check for files or imports
with `_generated` in mutation contexts) so bb validate-project rejects projects
with Hono routes, direct db imports, package.json modifications, or `_generated`
mutation usage.
- Line 30: The empty catch after the readdir() call in the validate command
silently swallows I/O errors and can falsely report success; replace the empty
catch with handling that surfaces the error (e.g., catch (err) {
console.error('Failed to read routes directory:', err); process.exit(1); } or
rethrow the error) so the command fails closed when readdir() fails—update the
try/catch around readdir() in validate.ts (the validate command handler)
accordingly.
In `@packages/cli/src/index.ts`:
- Around line 635-640: When handling the headless path in index.ts (the block
that checks opts.headless and calls runHeadlessLogin), add an explicit
precondition check for opts.apiKey and fail fast with a clear error if it's
missing; before importing/calling runHeadlessLogin, validate that opts.apiKey is
a non-empty string and, if not, call the same CLI error/exit flow used elsewhere
(or throw a user-friendly Error) with a message like "Missing --api-key:
--api-key is required when using --headless" so runHeadlessLogin is never
invoked with undefined.
In `@packages/cli/src/utils/api-client.ts`:
- Around line 104-106: The request() method currently constructs the URL using
this.baseUrl and ignores the authenticated serverUrl; update request() to prefer
an explicit client base URL but fall back to the stored serverUrl from
requireAuth() (e.g. use const base = this.baseUrl || serverUrl) when building
the url so createApiClient() calls without args target the logged-in server;
ensure you handle missing slashes/undefined serverUrl and throw a clear error if
neither base is available.
- Around line 42-64: The client-side API contract in registerProject,
syncSchema, and syncEnvironment doesn't match the server: registerProject should
send { name, slug } (not name/environment/config) and the sync methods must
target the slug-based routes the server exposes (use slug path param instead of
projectId). Update the registerProject signature and payload in registerProject
to accept and post { name: string; slug: string }, and change syncSchema and
syncEnvironment to accept { slug: string; ... } and call
this.post(`/api/projects/${data.slug}/schema`, ...) and
this.post(`/api/projects/${data.slug}/environment`, ...) respectively so the
client and server contracts align (refer to registerProject, syncSchema,
syncEnvironment).
- Around line 72-102: The ApiClient class contains a duplicated method
implementation for async validateApiKey(apiKey: string): Promise<boolean>, which
violates TypeScript class rules; remove one of the two identical validateApiKey
implementations (keep a single copy) and ensure the remaining method
(validateApiKey) uses the fetch to POST to `${this.baseUrl}/api/auth/validate`
with the same headers and return res.ok behavior so callers still work as
expected.
In `@packages/server/src/routes/admin/project-scoped/iac-sync.ts`:
- Around line 57-68: The route currently allows creating a project with a
fallback adminId ("default-admin") and does not call
betterbase_meta.provision_project_schema(), which leaves metadata and
per-project schema out of sync; change the logic in the handler around
getProjectBySlug/createProject so it requires a real adminId (reject the request
or return an error if c.get("adminId") is missing) and after successfully
creating the project via createProject(...) call
betterbase_meta.provision_project_schema(pool, project.id or project.slug) (and
handle/propagate any errors) so the project schema is provisioned on creation;
update the same pattern for the duplicate creation branch referenced at lines
83-86.
- Around line 22-31: The handlers that currently short-circuit with "return
c.json({ success: true... })" must perform the real work: replace the
placeholder success return in the schema route handler and the environment route
handler with awaited calls to the actual sync logic (e.g., call
provisionProjectSchema and applySchemaChanges in the schema route and call the
environment persistence/upsert function for the environment route), handle and
log errors, and only return success if those operations complete; keep the
existing response shape but include validation of results and propagate failure
status/messages on exception so that "bb iac sync --headless" truly syncs schema
and env instead of always reporting success.
- Around line 8-13: The three route handlers
(iacSyncRoutes.post("/:slug/schema", ...), and the two other handlers flagged)
are calling c.req.json() directly instead of validating request bodies first;
define appropriate Zod schemas (e.g., SchemaUploadSchema, ReconcileSchema,
DeleteSchema) for each handler and attach `@hono/zod-validator` middleware to
those routes so the body is validated before the handler runs, then remove
c.req.json() from the handlers and consume the validated payload supplied by the
validator middleware (instead of raw c.req.json()) to ensure controlled error
responses.
In `@specs/migrating-bitterbase-to-iac-charter.md`:
- Around line 100-105: Replace the vague acceptance criterion "Migration tool
successfully converts legacy Hono projects" with a verifiable test: specify the
exact command to run (e.g. `bb migrate-hono --source <legacy_proj> --dest
<output_dir> --yes`), list the required generated artifact paths (e.g.
`<output_dir>/iac/main.tf`, `<output_dir>/iac/variables.tf`,
`<output_dir>/config/environment.json`, and a migration-manifest.json), and the
expected outcome (process exits with code 0 and migration-manifest.json contains
a "status":"success" and an entries array matching converted resources). Ensure
the new criterion names the command, required files, and exact exit/status
checks so the migration result can be automatically verified.
In `@templates/iac/AGENTS.md`:
- Around line 85-90: The "NEVER EDIT FILES IN `_generated/`" rule is
inconsistent because it lists `src/db/schema.generated.ts` which is outside
`_generated/`; update the section to split into two clear rules: one stating
that files under `betterbase/_generated/*` (e.g.,
`betterbase/_generated/api.d.ts`, `betterbase/_generated/schema.json`) are
overwritten and must never be edited, and a separate statement for other
generated artifacts like `src/db/schema.generated.ts` describing their generated
nature and editing policy (e.g., "auto-generated — do not manually edit; changes
should come from the generator"). Ensure both rules are adjacent and use the
exact filenames shown (`betterbase/_generated/api.d.ts`,
`betterbase/_generated/schema.json`, `src/db/schema.generated.ts`) so readers
can locate the files.
---
Outside diff comments:
In `@packages/cli/src/commands/iac/sync.ts`:
- Around line 50-53: The early-return on diff.isEmpty prevents the headless path
from running; change the logic in the iac sync handler so that you only return
early when diff.isEmpty AND the headless flag (opts.headless or similar) is
false — i.e., if diff.isEmpty and not opts.headless then log success (unless
opts.silent) and return, otherwise continue into the headless/autoregistration
and env-sync flow; make the same adjustment in the other identical block
referenced (lines ~101-123) so headless always proceeds to auto-register and
sync env config even when there is no schema diff.
In `@packages/cli/src/commands/init.ts`:
- Around line 59-72: The helper function copyDir is dead code (defined but never
invoked) and should be removed or used consistently; either delete the copyDir
declaration or replace the manual templateFiles loop with a single call to
copyDir(templateSrcPath, targetPath) where templateFiles is currently iterated.
Locate the copyDir function and the templateFiles handling code and choose one
approach: remove copyDir and keep the existing loop, or remove the loop and call
copyDir for recursive copying to ensure consistent behavior.
- Around line 116-117: The empty catch in the init command swallows all errors
when updating package.json; change the block to catch the error object (e.g.,
catch (err)) and only ignore expected "file not found" errors (err.code ===
'ENOENT' or equivalent), but for all other errors (JSON parse errors, permission
issues) rethrow or surface them (throw err or processLogger.error(...); throw
err). Update the catch in the package.json update section of
packages/cli/src/commands/init.ts (the try that writes/reads package.json) so
unexpected exceptions are not silently ignored.
In `@packages/cli/src/index.ts`:
- Around line 302-328: There are two duplicate registrations of the CLI command
via iac.command("import") that both call runIacImport; remove the redundant
block so the command is only registered once (keep one of the
iac.command("import") ... .action(async (input, options) => { await
runIacImport(process.cwd(), { input, table: options.table, dryRun:
options.dryRun }); }) blocks), ensuring any options and arguments present in the
kept block (e.g., .argument, .option, .action) are preserved.
In `@packages/cli/test/integration/init.test.ts`:
- Around line 29-54: The test file contains legacy helpers providerTypeSchema,
getDatabaseLabel, and getAuthDialect that are no longer used by the refactored
init flow; either remove these helper definitions and the tests that assert them
from the init.test.ts file, or move the helper implementations and their tests
into the module that still needs provider selection (e.g., the provider
selection/CLI module) and update imports so the tests reference the actual
exported symbols (providerTypeSchema, getDatabaseLabel, getAuthDialect). Ensure
you only keep tests for helpers that are exported and used by production code.
🪄 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: abffb4a7-497d-418d-8cc6-ca4dc1486805
📒 Files selected for processing (20)
.coderabbit-review-trigger.coderabbit.yamllogs.txtpackages/cli/src/commands/iac/analyze.tspackages/cli/src/commands/iac/env-detector.tspackages/cli/src/commands/iac/migrate-legacy.tspackages/cli/src/commands/iac/server-sync.tspackages/cli/src/commands/iac/sync.tspackages/cli/src/commands/init.tspackages/cli/src/commands/login.tspackages/cli/src/commands/validate.tspackages/cli/src/index.tspackages/cli/src/utils/api-client.tspackages/cli/src/utils/credentials.tspackages/cli/test/cli/cli-parsing.test.tspackages/cli/test/integration/init.test.tspackages/server/src/routes/admin/project-scoped/iac-sync.tsspecs/migrating-bitterbase-to-iac-charter.mdtemplates/iac/AGENTS.mdtest_logs.txt
💤 Files with no reviewable changes (1)
- packages/cli/test/cli/cli-parsing.test.ts
Comment on lines
+58
to
+88
| const [key, ...valueParts] = trimmed.split('='); | ||
| const value = valueParts.join('=').trim(); | ||
|
|
||
| // Database detection | ||
| if (key === 'DATABASE_URL') envConfig.database.connectionString = value; | ||
| if (key === 'TURSO_URL') { | ||
| envConfig.database.provider = 'turso'; | ||
| envConfig.database.url = value; | ||
| } | ||
| if (key === 'TURSO_AUTH_TOKEN') envConfig.database.authToken = value; | ||
| if (key === 'DATABASE_URL' && value.includes('neon')) envConfig.database.provider = 'neon'; | ||
| if (key === 'DATABASE_URL' && value.includes('planetscale')) envConfig.database.provider = 'planetscale'; | ||
|
|
||
| // Auth | ||
| if (key === 'AUTH_SECRET') envConfig.auth.secret = value; | ||
| if (key === 'AUTH_URL') envConfig.auth.url = value; | ||
|
|
||
| // Storage | ||
| if (key === 'STORAGE_PROVIDER') envConfig.storage.provider = value; | ||
| if (key === 'STORAGE_BUCKET') envConfig.storage.bucket = value; | ||
| if (key === 'STORAGE_ACCESS_KEY') envConfig.storage.accessKey = value; | ||
| if (key === 'STORAGE_SECRET_KEY') envConfig.storage.secretKey = value; | ||
| if (key === 'STORAGE_ENDPOINT') envConfig.storage.endpoint = value; | ||
|
|
||
| // AI | ||
| if (key === 'OPENAI_API_KEY') envConfig.ai.openaiKey = value; | ||
|
|
||
| // Monitoring | ||
| if (key === 'SENTRY_DSN') envConfig.monitoring.sentryDsn = value; | ||
| if (key === 'LOG_LEVEL') envConfig.monitoring.logLevel = value; | ||
| } |
There was a problem hiding this comment.
Environment detector drops non-whitelisted variables.
custom is never populated, so unknown env keys are discarded. That breaks full environment sync in headless mode.
Proposed fix
if (key === 'SENTRY_DSN') envConfig.monitoring.sentryDsn = value;
if (key === 'LOG_LEVEL') envConfig.monitoring.logLevel = value;
+
+ // Preserve non-whitelisted variables for server sync
+ const knownKeys = new Set([
+ 'DATABASE_URL','TURSO_URL','TURSO_AUTH_TOKEN',
+ 'AUTH_SECRET','AUTH_URL',
+ 'STORAGE_PROVIDER','STORAGE_BUCKET','STORAGE_ACCESS_KEY','STORAGE_SECRET_KEY','STORAGE_ENDPOINT',
+ 'OPENAI_API_KEY','SENTRY_DSN','LOG_LEVEL'
+ ]);
+ if (!knownKeys.has(key)) envConfig.custom[key] = value;Based on learnings: "Run bb validate-project to detect and reject non-IaC patterns..." and charter requirement that headless sync must auto-parse env config and sync environment during bb iac sync.
📝 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
| const [key, ...valueParts] = trimmed.split('='); | |
| const value = valueParts.join('=').trim(); | |
| // Database detection | |
| if (key === 'DATABASE_URL') envConfig.database.connectionString = value; | |
| if (key === 'TURSO_URL') { | |
| envConfig.database.provider = 'turso'; | |
| envConfig.database.url = value; | |
| } | |
| if (key === 'TURSO_AUTH_TOKEN') envConfig.database.authToken = value; | |
| if (key === 'DATABASE_URL' && value.includes('neon')) envConfig.database.provider = 'neon'; | |
| if (key === 'DATABASE_URL' && value.includes('planetscale')) envConfig.database.provider = 'planetscale'; | |
| // Auth | |
| if (key === 'AUTH_SECRET') envConfig.auth.secret = value; | |
| if (key === 'AUTH_URL') envConfig.auth.url = value; | |
| // Storage | |
| if (key === 'STORAGE_PROVIDER') envConfig.storage.provider = value; | |
| if (key === 'STORAGE_BUCKET') envConfig.storage.bucket = value; | |
| if (key === 'STORAGE_ACCESS_KEY') envConfig.storage.accessKey = value; | |
| if (key === 'STORAGE_SECRET_KEY') envConfig.storage.secretKey = value; | |
| if (key === 'STORAGE_ENDPOINT') envConfig.storage.endpoint = value; | |
| // AI | |
| if (key === 'OPENAI_API_KEY') envConfig.ai.openaiKey = value; | |
| // Monitoring | |
| if (key === 'SENTRY_DSN') envConfig.monitoring.sentryDsn = value; | |
| if (key === 'LOG_LEVEL') envConfig.monitoring.logLevel = value; | |
| } | |
| const [key, ...valueParts] = trimmed.split('='); | |
| const value = valueParts.join('=').trim(); | |
| // Database detection | |
| if (key === 'DATABASE_URL') envConfig.database.connectionString = value; | |
| if (key === 'TURSO_URL') { | |
| envConfig.database.provider = 'turso'; | |
| envConfig.database.url = value; | |
| } | |
| if (key === 'TURSO_AUTH_TOKEN') envConfig.database.authToken = value; | |
| if (key === 'DATABASE_URL' && value.includes('neon')) envConfig.database.provider = 'neon'; | |
| if (key === 'DATABASE_URL' && value.includes('planetscale')) envConfig.database.provider = 'planetscale'; | |
| // Auth | |
| if (key === 'AUTH_SECRET') envConfig.auth.secret = value; | |
| if (key === 'AUTH_URL') envConfig.auth.url = value; | |
| // Storage | |
| if (key === 'STORAGE_PROVIDER') envConfig.storage.provider = value; | |
| if (key === 'STORAGE_BUCKET') envConfig.storage.bucket = value; | |
| if (key === 'STORAGE_ACCESS_KEY') envConfig.storage.accessKey = value; | |
| if (key === 'STORAGE_SECRET_KEY') envConfig.storage.secretKey = value; | |
| if (key === 'STORAGE_ENDPOINT') envConfig.storage.endpoint = value; | |
| // AI | |
| if (key === 'OPENAI_API_KEY') envConfig.ai.openaiKey = value; | |
| // Monitoring | |
| if (key === 'SENTRY_DSN') envConfig.monitoring.sentryDsn = value; | |
| if (key === 'LOG_LEVEL') envConfig.monitoring.logLevel = value; | |
| // Preserve non-whitelisted variables for server sync | |
| const knownKeys = new Set([ | |
| 'DATABASE_URL','TURSO_URL','TURSO_AUTH_TOKEN', | |
| 'AUTH_SECRET','AUTH_URL', | |
| 'STORAGE_PROVIDER','STORAGE_BUCKET','STORAGE_ACCESS_KEY','STORAGE_SECRET_KEY','STORAGE_ENDPOINT', | |
| 'OPENAI_API_KEY','SENTRY_DSN','LOG_LEVEL' | |
| ]); | |
| if (!knownKeys.has(key)) envConfig.custom[key] = value; |
🤖 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/iac/env-detector.ts` around lines 58 - 88, The
detector currently only assigns known keys to envConfig (database, auth,
storage, ai, monitoring) and never fills envConfig.custom, which loses unknown
env vars; after the existing if-blocks that handle DATABASE_URL, TURSO_*,
AUTH_*, STORAGE_*, OPENAI_API_KEY, SENTRY_DSN, LOG_LEVEL, add logic that if key
is non-empty and not matched by any whitelist fields then assign
envConfig.custom[key] = value (ensure envConfig.custom is initialized as an
object), and skip adding duplicate empty keys; this preserves all unrecognized
env variables for headless sync while leaving the existing
envConfig.database/auth/storage/ai/monitoring assignments intact.
Comment on lines
+56
to
+59
| const targetPath = route.method === "GET" | ||
| ? path.join(betterbaseDir, "queries", `${route.path}.ts`) | ||
| : path.join(betterbaseDir, "mutations", `${route.path}.ts`); | ||
| await writeFile(targetPath, functionCode); |
There was a problem hiding this comment.
Converted route files can overwrite each other.
route.path is based on entry.name only, so src/routes/admin/index.ts and src/routes/user/index.ts both emit index.ts in the same target directory.
Also applies to: 100-101
🤖 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/iac/migrate-legacy.ts` around lines 56 - 59, The
current write of converted routes uses route.path as a filename which causes
collisions (e.g., multiple index.ts files) — update the target path generation
so it preserves the original route directory structure (or otherwise makes
filenames unique) instead of flattening to a single filename; specifically,
change the code that builds targetPath (which uses route.method, betterbaseDir,
route.path and writeFile with functionCode) to join the equivalent source
subpath (or a sanitized route.entry path/name) into the target (e.g.,
path.join(betterbaseDir, "queries"|"mutations", ...routeSourceSubdirs,
`${routeFileName}.ts`)), ensure any intermediate directories are created before
calling writeFile, and sanitize/normalize names to avoid collisions.
Comment on lines
+8
to
+13
| iacSyncRoutes.post( | ||
| "/:slug/schema", | ||
| requireAdminAuth, | ||
| async (c) => { | ||
| const slug = c.req.param("slug"); | ||
| const { schema, force } = await c.req.json(); |
There was a problem hiding this comment.
Validate these request bodies before handler logic.
All three handlers call c.req.json() directly. That violates the server contract for @hono/zod-validator on request bodies and leaves malformed payloads to throw before the route can return a controlled c.json({ error }) response. As per coding guidelines, "All request bodies validated with @hono/zod-validator before handler logic. Never access c.req.json() directly in a handler that should be validated."
Also applies to: 36-41, 55-56
🤖 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/server/src/routes/admin/project-scoped/iac-sync.ts` around lines 8 -
13, The three route handlers (iacSyncRoutes.post("/:slug/schema", ...), and the
two other handlers flagged) are calling c.req.json() directly instead of
validating request bodies first; define appropriate Zod schemas (e.g.,
SchemaUploadSchema, ReconcileSchema, DeleteSchema) for each handler and attach
`@hono/zod-validator` middleware to those routes so the body is validated before
the handler runs, then remove c.req.json() from the handlers and consume the
validated payload supplied by the validator middleware (instead of raw
c.req.json()) to ensure controlled error responses.
Comment on lines
+22
to
+31
| // For now, we'll just return success since the actual provisioning | ||
| // would depend on the specific database implementation | ||
| // In a real implementation, this would call provisionProjectSchema | ||
| // and applySchemaChanges functions | ||
|
|
||
| return c.json({ | ||
| success: true, | ||
| message: `Schema synced for project ${slug}`, | ||
| schemaName | ||
| }); |
There was a problem hiding this comment.
These sync endpoints acknowledge work they never perform.
Both /:slug/schema and /:slug/environment return success without applying schema changes or persisting environment data. That makes bb iac sync report a successful server sync while the server state remains unchanged. As per coding guidelines, bb iac sync --headless must "sync schema+env without prompts."
Also applies to: 44-50
🤖 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/server/src/routes/admin/project-scoped/iac-sync.ts` around lines 22
- 31, The handlers that currently short-circuit with "return c.json({ success:
true... })" must perform the real work: replace the placeholder success return
in the schema route handler and the environment route handler with awaited calls
to the actual sync logic (e.g., call provisionProjectSchema and
applySchemaChanges in the schema route and call the environment
persistence/upsert function for the environment route), handle and log errors,
and only return success if those operations complete; keep the existing response
shape but include validation of results and propagate failure status/messages on
exception so that "bb iac sync --headless" truly syncs schema and env instead of
always reporting success.
Comment on lines
+57
to
+68
| const adminId = c.get("adminId"); | ||
|
|
||
| const pool = getPool(); | ||
|
|
||
| // Check if project exists | ||
| let project = await getProjectBySlug(pool, slug); | ||
| if (!project) { | ||
| project = await createProject(pool, { | ||
| name, | ||
| slug, | ||
| adminId: adminId ?? "default-admin", | ||
| }); |
There was a problem hiding this comment.
Project creation must require a real admin and provision the schema.
Falling back to "default-admin" allows this route to create a project without a verified owner, and the creation path never calls betterbase_meta.provision_project_schema(). That leaves the project metadata and per-project schema out of sync from the first write. As per coding guidelines, "betterbase_meta.provision_project_schema() must be called on project creation. Flag any project POST handler that omits it."
Also applies to: 83-86
🤖 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/server/src/routes/admin/project-scoped/iac-sync.ts` around lines 57
- 68, The route currently allows creating a project with a fallback adminId
("default-admin") and does not call betterbase_meta.provision_project_schema(),
which leaves metadata and per-project schema out of sync; change the logic in
the handler around getProjectBySlug/createProject so it requires a real adminId
(reject the request or return an error if c.get("adminId") is missing) and after
successfully creating the project via createProject(...) call
betterbase_meta.provision_project_schema(pool, project.id or project.slug) (and
handle/propagate any errors) so the project schema is provisioned on creation;
update the same pattern for the duplicate creation branch referenced at lines
83-86.
Comment on lines
+100
to
+105
| ### 4.3 Acceptance Criteria | ||
| - `bb init` produces IaC-compliant project structure | ||
| - `bb iac sync --headless` syncs schema and environment without prompts | ||
| - `bb validate-project` detects and reports constraint violations | ||
| - Migration tool successfully converts legacy Hono projects | ||
|
|
There was a problem hiding this comment.
Acceptance criterion for migration is non-verifiable.
Line 104 is vague. Define a concrete test condition and expected artifacts, e.g. command + required generated paths + exit status.
Proposed patch
-- Migration tool successfully converts legacy Hono projects
+- `bb iac migrate-legacy` exits 0 on a legacy Hono project and generates:
+ - `betterbase/queries/`, `betterbase/mutations/`, `betterbase/actions/`
+ - `AGENTS.md`
+ - no remaining actionable files under `src/routes/` without an explicit migration warningAs per coding guidelines: “Acceptance criteria must be verifiable (not vague like ‘works correctly’).”
📝 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
| ### 4.3 Acceptance Criteria | |
| - `bb init` produces IaC-compliant project structure | |
| - `bb iac sync --headless` syncs schema and environment without prompts | |
| - `bb validate-project` detects and reports constraint violations | |
| - Migration tool successfully converts legacy Hono projects | |
| ### 4.3 Acceptance Criteria | |
| - `bb init` produces IaC-compliant project structure | |
| - `bb iac sync --headless` syncs schema and environment without prompts | |
| - `bb validate-project` detects and reports constraint violations | |
| - `bb iac migrate-legacy` exits 0 on a legacy Hono project and generates: | |
| - `betterbase/queries/`, `betterbase/mutations/`, `betterbase/actions/` | |
| - `AGENTS.md` | |
| - no remaining actionable files under `src/routes/` without an explicit migration warning | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 100-100: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 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 `@specs/migrating-bitterbase-to-iac-charter.md` around lines 100 - 105, Replace
the vague acceptance criterion "Migration tool successfully converts legacy Hono
projects" with a verifiable test: specify the exact command to run (e.g. `bb
migrate-hono --source <legacy_proj> --dest <output_dir> --yes`), list the
required generated artifact paths (e.g. `<output_dir>/iac/main.tf`,
`<output_dir>/iac/variables.tf`, `<output_dir>/config/environment.json`, and a
migration-manifest.json), and the expected outcome (process exits with code 0
and migration-manifest.json contains a "status":"success" and an entries array
matching converted resources). Ensure the new criterion names the command,
required files, and exact exit/status checks so the migration result can be
automatically verified.
Refactor IaC-related commands and utilities to improve reliability, security, and developer experience during the migration to IaC. - Refactor `iac sync` to use the current serialized schema instead of re-loading from file. - Update `migrate-legacy` to use a centralized `AGENTS.md` template instead of a hardcoded string. - Improve `env-detector` with stricter, case-insensitive provider validation. - Enhance `login` command with mandatory API key validation for headless mode and improved credential storage. - Add path instructions to `.coderabbit.yaml` for better automated review of spec and charter documents. - Clean up unused code and improve error handling in `init`, `validate`, and `api-client`. - Update `AGENTS.md` template for clearer documentation on generated files.
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.
Refactor the CLI to provide more robust Infrastructure as Code (IaC) management, specifically targeting automation for AI agents and seamless server synchronization.
Key improvements:
iac syncto support non-interactive environments.migrate-legacycommand to facilitate the transition from legacy project structures to the IaC model.validate-projectcommand to ensure local project compliance with IaC standards.initcommand withAGENTS.mdtemplates and stricter project name validation.api-clientwith dedicated methods for project registration, schema synchronization, and environment syncing.runHeadlessLoginto support API-key based authentication for automated workflows.Summary by CodeRabbit
Release Notes
New Features
Documentation