INS-356: PoC: Full-stack preview for e2e verify#166
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces a complete preview environment provisioning system with manifest persistence, environment file mutation utilities, hidden ChangesPreview Environment Provisioning System
Test Agents Installation Extension
Deployment File Exclusion Patterns
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
Greptile SummaryAdds hidden
Confidence Score: 4/5Safe to merge after fixing the URL encoding issue in the RLS probe. The RLS probe builds a PostgREST filter by interpolating the user-supplied src/commands/verify/rls.ts — the filter construction on line 42 needs Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Agent as AI Agent
participant CLI as insforge verify rls/truth
participant Backend as oss_host API
participant PH as PostHog
Agent->>CLI: insforge verify rls --table t --owner user_id
CLI->>Backend: POST /api/auth/sessions (user A + B login)
Backend-->>CLI: JWT tokens A, B
CLI->>Backend: POST /api/auth/tokens/anon
Backend-->>CLI: anon key
CLI->>Backend: POST /api/database/advance/rawsql (get user A UUID)
Backend-->>CLI: "[{id: uuid-a}]"
CLI->>Backend: "GET /api/database/records/t?user_id=eq.uuid-a (as user B)"
Backend-->>CLI: rows (RLS check)
CLI->>Backend: "GET /api/database/records/t?user_id=eq.uuid-a (as user A)"
Backend-->>CLI: rows (positive control)
CLI->>Backend: GET /api/database/records/t (as anon)
Backend-->>CLI: rows (anon check)
CLI->>PH: "trackVerifyFinding(rls_leak|rls_overrestrict|none)"
CLI-->>Agent: exit 0/1 + JSON finding
%%{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 Agent as AI Agent
participant CLI as insforge verify rls/truth
participant Backend as oss_host API
participant PH as PostHog
Agent->>CLI: insforge verify rls --table t --owner user_id
CLI->>Backend: POST /api/auth/sessions (user A + B login)
Backend-->>CLI: JWT tokens A, B
CLI->>Backend: POST /api/auth/tokens/anon
Backend-->>CLI: anon key
CLI->>Backend: POST /api/database/advance/rawsql (get user A UUID)
Backend-->>CLI: "[{id: uuid-a}]"
CLI->>Backend: "GET /api/database/records/t?user_id=eq.uuid-a (as user B)"
Backend-->>CLI: rows (RLS check)
CLI->>Backend: "GET /api/database/records/t?user_id=eq.uuid-a (as user A)"
Backend-->>CLI: rows (positive control)
CLI->>Backend: GET /api/database/records/t (as anon)
Backend-->>CLI: rows (anon check)
CLI->>PH: "trackVerifyFinding(rls_leak|rls_overrestrict|none)"
CLI-->>Agent: exit 0/1 + JSON finding
Reviews (12): Last reviewed commit: "fix(verify): block destructive DML hidde..." | Re-trigger Greptile |
![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: 6
🧹 Nitpick comments (3)
src/lib/env-writer.overwrite.test.ts (1)
42-60: 📐 Maintainability & Code Quality | ⚡ Quick winAdd a duplicate-key regression test for overwrite semantics.
Please add a case with two
NEXT_PUBLIC_INSFORGE_URL=lines and assert both are rewritten (or only one remains with the new value). That locks in the intended overwrite behavior.🤖 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 `@src/lib/env-writer.overwrite.test.ts` around lines 42 - 60, Add a regression test that ensures duplicate keys are handled by overwriteEnvFile: create a new case (or extend the existing "leaves unrelated lines and comments intact" test) where the original .env content contains two lines starting with NEXT_PUBLIC_INSFORGE_URL= (different values), call overwriteEnvFile(file, { NEXT_PUBLIC_INSFORGE_URL: 'https://branch.insforge.app' }), then read the file with readFile and assert that either both original lines were replaced with the new value or only one remains with the new value depending on intended semantics (but assert the chosen behavior); reference the NEXT_PUBLIC_INSFORGE_URL key, overwriteEnvFile, writeFile, readFile and the file variable to locate and implement the test.src/commands/preview/create.test.ts (1)
50-90: 📐 Maintainability & Code Quality | ⚡ Quick winAdd a failure-path test for post-create wiring errors.
Please add a case where branch creation succeeds but env wiring fails (e.g., mock
overwriteEnvFileto throw), and assert the manifest still exists withbranchId/appkey. This guards against orphaned-preview regressions.🤖 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 `@src/commands/preview/create.test.ts` around lines 50 - 90, Add a test that simulates a failure in the env wiring step while branch creation succeeds: use jest to spyOn/mock the overwriteEnvFile function (or the module export used by the command) to throw an error, then run the CLI command via registerPreviewCreateCommand (call program.parseAsync with ['preview','create','feat-likes','--wire-env', envFile] like other tests), and finally assert the preview manifest (readPreviewManifest) still exists and contains branchId ('branch-123') and appkey ('p1ky-x9p'); also assert the original env file was not overwritten (or backup created) if relevant. Ensure you restore the mock after the test.src/commands/preview/teardown.test.ts (1)
42-71: 📐 Maintainability & Code Quality | ⚡ Quick winAdd a restore-failure regression test to lock cleanup behavior.
Add a case where backup exists but restore throws (e.g., mock copy failure), then assert manifest cleanup still happens after
deleteBranchApi. This protects against stale local manifests.🤖 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 `@src/commands/preview/teardown.test.ts` around lines 42 - 71, Add a new test that creates a wired env backup (like existing test using writePreviewManifest and envName), then mock the file-restore operation (mock fs.copyFile or fs.rename used by the teardown logic) to throw so restore fails; call the teardown via registerPreviewTeardownCommand and program.parseAsync as in the existing test; assert that deleteBranchApi (mock) is still invoked and that readPreviewManifest(tmpBase, 'feat-wired') returns null (manifest cleaned up) even though the restore threw, and also assert the backup file still exists to prove restore failed but cleanup ran. Ensure you reference the same helpers/register function names: registerPreviewTeardownCommand, writePreviewManifest, readPreviewManifest, and the API function deleteBranchApi when adding the test and mocks.
🤖 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 `@src/commands/preview/create.ts`:
- Around line 73-74: The next-step message currently tells users to run the
non-existent subcommand "insforge preview test ${name}" (see the two outputInfo
calls); update the second outputInfo to reference an actual registered preview
subcommand (replace "preview test ${name}" with the correct one, e.g. "preview
teardown ${name}" if you intend the user to stop the preview) so the guidance
matches the registered preview flow (adjust the string passed to outputInfo in
create.ts accordingly).
- Around line 39-61: Write the preview manifest immediately after
createBranchApi returns (use the created object) before calling pollUntilReady
or mutating local files so we never leave a remote branch without a local
manifest; specifically, move or add a writePreviewManifest call that uses
created.id/appkey/created metadata (instead of waiting for
ready.branch_created_at) right after createBranchApi (referencing
createBranchApi and writePreviewManifest), then proceed to pollUntilReady
(pollUntilReady) and only afterward perform env wiring (overwriteEnvFile,
copyFileSync) and update the manifest with any runtime fields from ready if
needed.
In `@src/commands/preview/index.test.ts`:
- Line 12: The test currently asserts the internal Commander field `_hidden` on
the `found` object; instead, change the assertion to verify public behavior:
call `program.helpInformation()` (or similar help output method used in this
test) and assert that the returned string does not include the preview
command/group name (e.g., "preview"), and optionally add a separate assertion
that parsing/execution of the preview command behaves as expected; update the
assertion referencing `found` to use `program.helpInformation()` and a negative
match for "preview" rather than checking `found._hidden`.
In `@src/commands/preview/teardown.ts`:
- Around line 23-35: The teardown can leave a stale local manifest if the
env-file restore (uses manifest.wiredEnvFile, existsSync, copyFileSync, rmSync)
throws after deleteBranchApi succeeded; change the control flow so
deletePreviewManifest(process.cwd(), name) always runs regardless of restore
errors by moving the restore into its own try/catch (or wrapping the restore
block in try and calling deletePreviewManifest in a finally) and log/propagate
restore errors without skipping deletePreviewManifest; keep deleteBranchApi
as-is and ensure deletePreviewManifest is invoked even when the wired-env
restore fails.
In `@src/lib/env-writer.ts`:
- Around line 99-101: The overwrite logic in overwriteEnvFile only replaces the
first match because content.replace(re, ...) is used; update overwriteEnvFile to
replace all occurrences of the key in content (or compute the effective one) by
using a global replace (ensure the RegExp re has the 'g' flag) or run a loop to
replace every match, then push the key into changed; reference the
variables/functions re, content, changed and the function overwriteEnvFile to
locate and update the replacement so duplicate KEY= lines are all rewritten.
In `@src/lib/preview-manifest.ts`:
- Around line 16-17: manifestPath currently concatenates the raw name into a
filesystem path allowing path traversal; update manifestPath to validate and
sanitize the name token before joining: normalize and reject any input that
contains path separators or resolves outside previewDir (e.g. use
path.basename(name) or a strict regex like /^[A-Za-z0-9._-]+$/ to allow only
safe filename characters), optionally throw a clear error if validation fails,
and then join the sanitized token with previewDir(baseDir) so read/write/delete
operations cannot escape .insforge/previews; keep references to manifestPath and
previewDir when locating the change.
---
Nitpick comments:
In `@src/commands/preview/create.test.ts`:
- Around line 50-90: Add a test that simulates a failure in the env wiring step
while branch creation succeeds: use jest to spyOn/mock the overwriteEnvFile
function (or the module export used by the command) to throw an error, then run
the CLI command via registerPreviewCreateCommand (call program.parseAsync with
['preview','create','feat-likes','--wire-env', envFile] like other tests), and
finally assert the preview manifest (readPreviewManifest) still exists and
contains branchId ('branch-123') and appkey ('p1ky-x9p'); also assert the
original env file was not overwritten (or backup created) if relevant. Ensure
you restore the mock after the test.
In `@src/commands/preview/teardown.test.ts`:
- Around line 42-71: Add a new test that creates a wired env backup (like
existing test using writePreviewManifest and envName), then mock the
file-restore operation (mock fs.copyFile or fs.rename used by the teardown
logic) to throw so restore fails; call the teardown via
registerPreviewTeardownCommand and program.parseAsync as in the existing test;
assert that deleteBranchApi (mock) is still invoked and that
readPreviewManifest(tmpBase, 'feat-wired') returns null (manifest cleaned up)
even though the restore threw, and also assert the backup file still exists to
prove restore failed but cleanup ran. Ensure you reference the same
helpers/register function names: registerPreviewTeardownCommand,
writePreviewManifest, readPreviewManifest, and the API function deleteBranchApi
when adding the test and mocks.
In `@src/lib/env-writer.overwrite.test.ts`:
- Around line 42-60: Add a regression test that ensures duplicate keys are
handled by overwriteEnvFile: create a new case (or extend the existing "leaves
unrelated lines and comments intact" test) where the original .env content
contains two lines starting with NEXT_PUBLIC_INSFORGE_URL= (different values),
call overwriteEnvFile(file, { NEXT_PUBLIC_INSFORGE_URL:
'https://branch.insforge.app' }), then read the file with readFile and assert
that either both original lines were replaced with the new value or only one
remains with the new value depending on intended semantics (but assert the
chosen behavior); reference the NEXT_PUBLIC_INSFORGE_URL key, overwriteEnvFile,
writeFile, readFile and the file variable to locate and implement the test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3976334c-5a3e-47ae-a4e3-5874ab856adb
📒 Files selected for processing (11)
src/commands/preview/create.test.tssrc/commands/preview/create.tssrc/commands/preview/index.test.tssrc/commands/preview/index.tssrc/commands/preview/teardown.test.tssrc/commands/preview/teardown.tssrc/index.tssrc/lib/env-writer.overwrite.test.tssrc/lib/env-writer.tssrc/lib/preview-manifest.test.tssrc/lib/preview-manifest.ts
| const created = await createBranchApi(project.project_id, { mode: 'full', name }, apiUrl); | ||
| captureEvent(project.project_id, 'cli_preview_create', { name }); | ||
| const ready = await pollUntilReady(created.id, apiUrl); | ||
|
|
||
| const previewUrl = `https://${ready.appkey}.${ready.region}.insforge.app`; | ||
|
|
||
| let wiredEnvFile: string | undefined; | ||
| if (opts.wireEnv) { | ||
| wiredEnvFile = typeof opts.wireEnv === 'string' ? opts.wireEnv : '.env.local'; | ||
| const envPath = path.resolve(process.cwd(), wiredEnvFile); | ||
| if (existsSync(envPath)) { | ||
| copyFileSync(envPath, envPath + '.preview-bak'); | ||
| } | ||
| overwriteEnvFile(envPath, { NEXT_PUBLIC_INSFORGE_URL: previewUrl }); | ||
| } | ||
|
|
||
| await writePreviewManifest(process.cwd(), { | ||
| name, | ||
| branchId: ready.id, | ||
| appkey: ready.appkey, | ||
| createdAt: ready.branch_created_at, | ||
| ...(wiredEnvFile ? { wiredEnvFile } : {}), | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist preview metadata before post-create local mutations to avoid orphaned environments.
Line 39 creates the remote branch, but the manifest is only written at Line 55 after polling and optional env-file mutation. If wiring/polling fails in between, the preview can exist remotely without a local manifest, and preview teardown won’t be able to discover it by name.
Suggested sequencing change
const created = await createBranchApi(project.project_id, { mode: 'full', name }, apiUrl);
captureEvent(project.project_id, 'cli_preview_create', { name });
const ready = await pollUntilReady(created.id, apiUrl);
const previewUrl = `https://${ready.appkey}.${ready.region}.insforge.app`;
+ // Persist baseline manifest first so teardown can always find the branch.
+ await writePreviewManifest(process.cwd(), {
+ name,
+ branchId: ready.id,
+ appkey: ready.appkey,
+ createdAt: ready.branch_created_at,
+ });
+
let wiredEnvFile: string | undefined;
if (opts.wireEnv) {
wiredEnvFile = typeof opts.wireEnv === 'string' ? opts.wireEnv : '.env.local';
const envPath = path.resolve(process.cwd(), wiredEnvFile);
if (existsSync(envPath)) {
copyFileSync(envPath, envPath + '.preview-bak');
}
overwriteEnvFile(envPath, { NEXT_PUBLIC_INSFORGE_URL: previewUrl });
+
+ // Persist wired file only after successful rewrite.
+ await writePreviewManifest(process.cwd(), {
+ name,
+ branchId: ready.id,
+ appkey: ready.appkey,
+ createdAt: ready.branch_created_at,
+ wiredEnvFile,
+ });
}
-
- await writePreviewManifest(process.cwd(), {
- name,
- branchId: ready.id,
- appkey: ready.appkey,
- createdAt: ready.branch_created_at,
- ...(wiredEnvFile ? { wiredEnvFile } : {}),
- });📝 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.
| const created = await createBranchApi(project.project_id, { mode: 'full', name }, apiUrl); | |
| captureEvent(project.project_id, 'cli_preview_create', { name }); | |
| const ready = await pollUntilReady(created.id, apiUrl); | |
| const previewUrl = `https://${ready.appkey}.${ready.region}.insforge.app`; | |
| let wiredEnvFile: string | undefined; | |
| if (opts.wireEnv) { | |
| wiredEnvFile = typeof opts.wireEnv === 'string' ? opts.wireEnv : '.env.local'; | |
| const envPath = path.resolve(process.cwd(), wiredEnvFile); | |
| if (existsSync(envPath)) { | |
| copyFileSync(envPath, envPath + '.preview-bak'); | |
| } | |
| overwriteEnvFile(envPath, { NEXT_PUBLIC_INSFORGE_URL: previewUrl }); | |
| } | |
| await writePreviewManifest(process.cwd(), { | |
| name, | |
| branchId: ready.id, | |
| appkey: ready.appkey, | |
| createdAt: ready.branch_created_at, | |
| ...(wiredEnvFile ? { wiredEnvFile } : {}), | |
| }); | |
| const created = await createBranchApi(project.project_id, { mode: 'full', name }, apiUrl); | |
| captureEvent(project.project_id, 'cli_preview_create', { name }); | |
| const ready = await pollUntilReady(created.id, apiUrl); | |
| const previewUrl = `https://${ready.appkey}.${ready.region}.insforge.app`; | |
| // Persist baseline manifest first so teardown can always find the branch. | |
| await writePreviewManifest(process.cwd(), { | |
| name, | |
| branchId: ready.id, | |
| appkey: ready.appkey, | |
| createdAt: ready.branch_created_at, | |
| }); | |
| let wiredEnvFile: string | undefined; | |
| if (opts.wireEnv) { | |
| wiredEnvFile = typeof opts.wireEnv === 'string' ? opts.wireEnv : '.env.local'; | |
| const envPath = path.resolve(process.cwd(), wiredEnvFile); | |
| if (existsSync(envPath)) { | |
| copyFileSync(envPath, envPath + '.preview-bak'); | |
| } | |
| overwriteEnvFile(envPath, { NEXT_PUBLIC_INSFORGE_URL: previewUrl }); | |
| // Persist wired file only after successful rewrite. | |
| await writePreviewManifest(process.cwd(), { | |
| name, | |
| branchId: ready.id, | |
| appkey: ready.appkey, | |
| createdAt: ready.branch_created_at, | |
| wiredEnvFile, | |
| }); | |
| } |
🤖 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 `@src/commands/preview/create.ts` around lines 39 - 61, Write the preview
manifest immediately after createBranchApi returns (use the created object)
before calling pollUntilReady or mutating local files so we never leave a remote
branch without a local manifest; specifically, move or add a
writePreviewManifest call that uses created.id/appkey/created metadata (instead
of waiting for ready.branch_created_at) right after createBranchApi (referencing
createBranchApi and writePreviewManifest), then proceed to pollUntilReady
(pollUntilReady) and only afterward perform env wiring (overwriteEnvFile,
copyFileSync) and update the manifest with any runtime fields from ready if
needed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
CarmenDou
changed the title
…fe names, resilient teardown
… keys, assert hidden via help
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Solid, well-tested PoC for hidden preview create/teardown commands, but preview-name validation runs only at manifest-write time — after the remote preview is fully provisioned — so a common git-style name like feat/likes orphans a live full-stack preview with no rollback and no teardown path.
Requirements context
No /docs/superpowers/ directory exists in this repo. The closest equivalent, docs/specs/, contains specs for diagnose and db migrations only — no spec/plan matches INS-356 / preview, so this review assesses against the PR description, DEVELOPMENT.md, and the repo's cli-development skill.
Verification performed in an isolated workspace at head f64f000: npm ci + npx vitest run → 466 passed, 13 skipped (pre-existing); eslint clean on all new/changed files. (Raw tsc --noEmit errors exist in payments/transactions.test.ts, config-capabilities.ts, prompts.ts — all untouched by this PR, pre-existing.)
Findings
Critical
-
[Functionality] Name validation happens after provisioning, orphaning the remote preview on realistic input.
assertSafeName(src/lib/preview-manifest.ts:17-21) rejects/, but it is only reached viawritePreviewManifest→manifestPath(src/commands/preview/create.ts:55-60) — aftercreateBranchApisucceeds andpollUntilReadycompletes (up to 5 minutes). The rollbacktry/catchwraps only the poll (create.ts:39-52), so for a git-style name likefeat/likes:- the full-stack preview branch is created and provisioned remotely,
writePreviewManifestthen throws a plainError("Invalid preview name …"),- no
deleteBranchApirollback runs, no manifest exists, sopreview teardowncan never find it, - the error message doesn't tell the user a live branch was left behind.
The PR description advertises "validates safe names", but the validation fires too late to protect anything except the manifest path itself. Fix: validate the name at the top of the action, before
createBranchApi(exportassertSafeNameor a shared validator frompreview-manifest.ts), and add a test for the rejected-name path.
Suggestion
-
[Functionality]
teardowndeadlocks if the remote branch is already gone.deleteBranchApiis awaited before any local cleanup (src/commands/preview/teardown.ts:20-23), andplatformFetchthrowsCLIErroron any non-2xx including 404 (src/lib/api/platform.ts:83-87;deleteBranchApipasses nopassThroughStatuses). If the branch was deleted by other means, teardown aborts every time and the manifest + wired env file can never be cleaned up via the CLI. Notably,create's own poll-failure message tells the user to runinsforge branch delete <name>(create.ts:46-50) — following that advice sets this exact trap. Tolerate 404/not-found from the delete and continue local cleanup. -
[Functionality] Re-running
preview create <same name>silently orphans the previous preview and can destroy the env backup. There is no guard against an existing manifest: a second create overwrites.insforge/previews/<name>.json(losing the firstbranchId, orphaning that branch), and with--wire-envthecopyFileSyncatcreate.ts:67-69overwrites.preview-bakwith the already-wired content — the user's original (e.g. prod) value is permanently lost, and teardown "restores" the previous preview's URL. Refuse to create when a manifest for the name exists, and don't overwrite an existing.preview-bak. -
[Software engineering] The rollback-on-poll-failure path — listed as a bug fix in the PR description — has no test. Moreover, the
vi.mockfactory insrc/commands/preview/create.test.ts:9-31doesn't stubdeleteBranchApiat all, so any future test exercising that path will fail with Vitest's "No 'deleteBranchApi' export is defined on the mock". Add the stub plus a test assertingdeleteBranchApiis called when polling fails. A create-side test for the--wire-envdefault (.env.local, no value) /wiredEnvCreated: truecase would also be welcome (the teardown side is covered). -
[Security/conventions] Analytics event sends user-entered free text.
captureEvent(project.project_id, 'cli_preview_create', { name })(create.ts:37) sends the user-chosen preview name to PostHog.DEVELOPMENT.md§2 says properties must include only non-sensitive metadata and "Never send SQL, file contents, credentials, or user-entered free text";branch createdeliberately sends only{ mode, parent_project_id }(src/commands/branch/create.ts:57-60). Dropnamefrom the properties (or send a derived non-identifying flag). -
[Software engineering]
pollUntilReadyis duplicated fromsrc/commands/branch/create.ts(same constants, near-identical loop). The branch version also re-checks terminal state once after timeout (branch/create.ts:129-135); the new copy doesn't. Consider extracting a shared helper — for a PoC, at minimum a comment noting the divergence.
Information
-
[Software engineering] No progress output during the up-to-5-minute poll.
branch createruns aclackspinner through the slow POST + provisioning;preview createis silent until ready in non-JSON mode. Acceptable for a hidden experimental command, but worth aligning before un-hiding. -
[Functionality] If
overwriteEnvFilethrows mid-wire (e.g. permissions), a.preview-bakis left orphaned and the manifest lackswiredEnvFile— teardown then leaves both behind. Low blast radius. -
[Security] Otherwise no security-relevant concerns:
assertSafeNamecorrectly blocks path traversal in manifest paths, both commands callrequireAuth, no secrets are logged,overwriteEnvFileuses a replacer function so$-patterns in values are written literally (tested), and no new dependencies are added. -
[Performance] No performance concerns: the poll loop is bounded (5 min / 3 s interval) and the sync file I/O matches existing CLI patterns; nothing runs per-request.
Verdict
request_changes — one Critical finding (late name validation orphaning provisioned previews). Everything else is non-blocking. The fix is small: validate the name before calling createBranchApi, plus a regression test.
…tes, tolerate 404 on teardown, drop name from analytics
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Solid, well-tested PoC adding hidden preview create/preview teardown commands that closely follow the existing branch create patterns; no blocking issues found.
Requirements context
No matching spec/plan found under docs/specs/ (only diagnose and db-migrations specs exist) — assessing against the PR description (INS-356 PoC) alone.
Verification run in a clean workspace at aa43a5b: npm run lint → 0 errors; npx vitest run → 471 passed, 0 failed (all 20 new tests pass; the deleteBranchApi signature change is backward compatible with existing callers).
Findings
Critical
(none)
Suggestion
Functionality / conventions
--jsonoutput is polluted in teardown —src/commands/preview/teardown.ts:37-48: the env-restore messages (outputInfo(' Removed …'),' Restored …',' ⚠ Could not restore …') are emitted unconditionally.outputInfoisconsole.logto stdout (src/lib/output.ts:22-24), so with--jsonthese plain-text lines precede theoutputJson({ teardown: … })payload and break strict JSON consumers parsing stdout.create.tsgates alloutputInfocalls behind!json— teardown should do the same (DEVELOPMENT.md §1: "Respect the--jsonflag"). Low blast radius since the command is hidden/experimental, hence non-blocking.
Functionality
2. Two previews wiring the same env file interact poorly — src/commands/preview/create.ts:90-93: creating preview A with --wire-env backs up prod; creating preview B reuses the same file but skips the backup (it already exists, intentionally protected). Tearing down A then restores the prod values and deletes the backup while B is still active — the env file silently stops pointing at B. Consider refusing --wire-env when the target file's .preview-bak belongs to another live preview (e.g. record the backup owner in the manifest), or at least warn.
Software engineering
3. No progress feedback during the up-to-5-minute provisioning poll — src/commands/preview/create.ts:55-60: branch create shows a @clack/prompts spinner across the identical POST + poll flow (src/commands/branch/create.ts:48-55); preview create hangs silently. Fine for a PoC, but worth spinner parity before unhiding the command.
Information
- Rollback deletes the branch on any poll error —
src/commands/preview/create.ts:57-69: a transient network failure ingetBranchApimid-poll triggersdeleteBranchApion a possibly healthy branch. The error message ("If the branch still exists, remove it…") mitigates this, and aggressive cleanup is arguably right for ephemeral previews — just noting the trade-off. - Teardown doesn't wrap
assertSafeName—insforge preview teardown 'feat/likes'throws the rawErrorfrommanifestPath(src/lib/preview-manifest.ts:24-26), rendered asUNKNOWN_ERRORin--json, whereas create wraps it inCLIError(src/commands/preview/create.ts:37-41). Cosmetic inconsistency. - Untyped
optsin create's action —src/commands/preview/create.ts:25leavesoptsimplicitlyany;branch/create.ts:21types it. Minor. - Manifest parse is unvalidated —
src/lib/preview-manifest.ts:42-50: a corrupt/hand-edited manifest JSON throws a raw parse error. Acceptable for a PoC. - DEVELOPMENT.md §3 (agent-skills sync): presumably intentionally skipped since the command is hidden — worth a conscious decision when it graduates from experimental.
Security — no security-relevant concerns. Preview names are validated against ^[A-Za-z0-9._-]+$ before any path use (blocks traversal); the name was correctly removed from the analytics payload per DEVELOPMENT.md §2 ("never send user-entered free text"); no new dependencies; no secrets logged. passThroughStatuses is a pre-existing, documented platformFetch option used correctly.
Performance — no findings. Poll interval/timeout match branch create; file operations are small and one-shot.
Verdict
approved — zero Critical findings (informational; explicit GitHub approval remains a human action via the approve flow). Test coverage is genuinely good: rollback on failed provisioning, duplicate-name guard, default vs explicit --wire-env, $-literal env values, duplicated-key rewrite, and help-hiding are all exercised.
…validation, type create opts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/commands/preview/create.test.ts`:
- Around line 47-51: The tests create a temporary directory in beforeEach
(tmpBase via fs.mkdtemp) but never delete it; add an afterEach that
asynchronously removes tmpBase (use fs.rm(tmpBase, { recursive: true, force:
true }) or fs.rmdir/tmpdir removal equivalent) and restores any mocked cwd if
needed; place the afterEach alongside beforeEach in the test file so tmpBase is
cleaned up after each test and avoid leaking temp directories.
- Line 92: The manifest stores manifest.wiredEnvFile as the raw --wire-env
string which can be relative, causing teardown.ts (which uses
path.resolve(process.cwd(), manifest.wiredEnvFile)) to resolve against the
current working directory instead of the original create-time path; change
create.ts to resolve the provided env path to an absolute path (e.g., compute
envPath = path.resolve(providedWireEnv || '.env.local')) and store that absolute
path in the manifest (replace wiredEnvFile with envPath or set
manifest.wiredEnvFile to the absolute path), update teardown.ts to use the
absolute manifest value directly, and update create.test.ts expectations to
assert the manifest contains the resolved absolute env path instead of the raw
relative value.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0e60b73-796f-4bc0-bd07-8e816650d1a7
📒 Files selected for processing (7)
src/commands/preview/create.test.tssrc/commands/preview/create.tssrc/commands/preview/teardown.test.tssrc/commands/preview/teardown.tssrc/lib/api/platform.tssrc/lib/preview-manifest.test.tssrc/lib/preview-manifest.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/preview-manifest.ts
- src/commands/preview/teardown.test.ts
- src/commands/preview/teardown.ts
- src/commands/preview/create.ts
…sforge-verify at link time
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Code review — INS-356: PoC full-stack preview for e2e verify
Summary: A well-scoped, well-tested experimental PoC that adds hidden preview create/teardown commands plus an opt-in link --with-test-agents flag; no blocking issues found.
Requirements context
No matching spec/plan found under docs/specs/ for INS‑356 (only diagnose and db-migrations designs exist there) — assessed against the PR description/title alone. Behavior was cross-checked against repo conventions in DEVELOPMENT.md §2 (PostHog analytics) and §3 (agent-skills sync), and the Playwright init-agents API was verified against current docs (real command, v1.56+).
Findings
Critical
(none)
Suggestion
-
Functionality — partial provisioning has no rollback on the env-wiring path (
src/commands/preview/create.ts:90-118). The polling-timeout path correctly rolls back the branch (deleteBranchApi) and writes no manifest. But once the branch isready, the manifest is written and thencopyFileSync/overwriteEnvFilerun unguarded. If backup or write fails (e.g. EACCES/EROFS), the outercatchcallshandleError, leaving a live preview branch and a manifest withoutwiredEnvFile— the env wiring is reported as failed but the branch is silently orphaned. Consider wrapping the wiring in atry/catchthat at minimum warns the user the preview exists and must be torn down. Low blast radius (teardownstill works), hence non-blocking. -
Software engineering —
preview teardownemits no analytics event (src/commands/preview/teardown.ts:14-58).createfirescaptureEvent('cli_preview_create', …)perDEVELOPMENT.md§2, and teardown imports/awaitsshutdownAnalytics()infinally, but never captures acli_preview_teardownevent. For symmetry and feature-usage tracking, add one (distinct ID =branchId's parent project ormanifest.appkeyas available).
Information
-
overwriteEnvFilenormalizes line formatting (src/lib/env-writer.ts:97-107). The replace callback rewrites a matched line to${key}=${value}, dropping any surrounding quotes andKEY = valspacing. Fine for repointing a URL, but worth a doc note that quote/spacing style isn't preserved (contrast withupsertEnvFile, which leaves mismatched values untouched). The() => …callback correctly avoids$-pattern interpretation, and the global-regexlastIndexreset before.replaceis handled — both well covered by tests. -
Duplicate-name detection is cwd-scoped (
src/commands/preview/create.ts:46-50).readPreviewManifest(process.cwd(), name)only detects a clash for previews created from the same directory. This is consistent with the local-manifest design (.insforge/previews/<name>.json), so it's intentional — just noting the limitation. -
agent-skills sync (
DEVELOPMENT.md§3). The new visiblelink --with-test-agentsflag (and, eventually, thepreviewcommands if they graduate from hidden) will need theinsforge-cliskill inInsForge/agent-skillsupdated. Low urgency whilepreviewis hidden and this is a PoC. -
assertSafeNamepath-traversal check is sound.^[A-Za-z0-9._-]+$rejects/, and pure-dot names (..) are neutralized by the${name}.jsonsuffix atmanifestPath, so no traversal escape — confirmed by tests.
Verdict
approved (informational; human approval via the approve flow still required). Strong test coverage across all new units (manifest round-trip, env overwrite incl. $/duplicate-key/comment-preservation, rollback, duplicate-refusal, --json stdout hygiene), correct use of the existing passThroughStatuses plumbing for the 404-tolerant delete, analytics convention followed (no preview name leaked), and the link signature change threaded through all call sites with updated tests. Security: no untrusted input reaches shell/SQL — init-agents is a fixed command string, env paths are user-local, and previewUrl is built from trusted API fields. Performance: no concerns — polling is await-based with bounded 5‑min timeout / 3s interval.
…eview-poc # Conflicts: # src/commands/projects/link.ts
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 (1)
src/commands/projects/link.ts (1)
118-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--guardis accidentally ignored by the skills-only fast path.On Line 118–Line 124,
isSkillsOnlydoes not account foropts.guard.
Solink --guard/link --guard offfalls into the skills-only branch and returns before any project config is saved, making the guard option a no-op in that path.Suggested fix
const isSkillsOnly = opts.projectId === undefined && opts.orgId === undefined && opts.template === undefined && opts.auth === undefined && opts.apiBaseUrl === undefined && - opts.apiKey === undefined; + opts.apiKey === undefined && + opts.guard === undefined;🤖 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 `@src/commands/projects/link.ts` around lines 118 - 124, The isSkillsOnly condition on lines 118-124 does not account for the opts.guard option, causing the link --guard and link --guard off commands to incorrectly fall into the skills-only fast path and return before the guard configuration is saved. Fix this by adding opts.guard === undefined to the list of conditions in the isSkillsOnly check, alongside the existing checks for opts.projectId, opts.orgId, opts.template, opts.auth, opts.apiBaseUrl, and opts.apiKey.
🤖 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 `@src/commands/projects/link.ts`:
- Around line 118-124: The isSkillsOnly condition on lines 118-124 does not
account for the opts.guard option, causing the link --guard and link --guard off
commands to incorrectly fall into the skills-only fast path and return before
the guard configuration is saved. Fix this by adding opts.guard === undefined to
the list of conditions in the isSkillsOnly check, alongside the existing checks
for opts.projectId, opts.orgId, opts.template, opts.auth, opts.apiBaseUrl, and
opts.apiKey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5d0d665-85a4-4fb8-a0dd-d37873288aa4
📒 Files selected for processing (4)
src/commands/projects/link.test.tssrc/commands/projects/link.tssrc/index.tssrc/lib/skills.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
…decisions, gotchas
…aywright-report, .playwright-mcp) from deploy uploads
There was a problem hiding this comment.
2 issues found across 24 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…fix finding props
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
| const aId = (rows[0] as { id?: string })?.id; | ||
| if (!aId) throw new CLIError(`Could not find user A (${opts.userA}) — seed it first.`); | ||
|
|
||
| const filter = `${opts.owner}=eq.${aId}`; |
There was a problem hiding this comment.
--owner column name is interpolated directly into the URL query string without encoding. PostgREST parses everything after ? as key=op.value pairs, so an agent-generated value like user_id&limit=0 would produce ?user_id&limit=0=eq.<uuid>, forcing the PostgREST endpoint to return 0 rows regardless of RLS state. That silences a real rls_leak and produces a false "isolation holds" verdict — the worst possible outcome for a security probe.
| const filter = `${opts.owner}=eq.${aId}`; | |
| const filter = `${encodeURIComponent(opts.owner)}=eq.${encodeURIComponent(aId)}`; |
2 participants
Summary by cubic
Adds experimental, hidden
insforge verifyprobes (RLS isolation, backend‑truth, and loud‑error capture) and auto‑configures the Playwright browser MCP (@playwright/mcp) duringinsforge linksoinsforge-verifycan drive the UI for INS‑356. Tightens safety by enforcing read‑only SQL (now also blocks DML in CTEs) and treating non‑2xx responses as failures.New Features
verify rlschecks cross‑user isolation (B can’t read A; A can read own), sets exit codes, and records evidence.verify truthcompares a DB read to what the UI showed; flagsfalse_passand records evidence.verify findingrecords loud errors (HTTP 4xx/5xx, console exceptions) seen during drives.linkconfigures a globalplaywrightMCP server for supported agents (Claude Code viaclaude mcp add, Cursor~/.cursor/mcp.json, Codex~/.codex/config.toml); prints manual steps if not detected. Added tests for MCP config and probe classifiers; deploys now excludetest-results,playwright-report, and.playwright-mcp.Bug Fixes
verify truthruns only read‑only SQL (starts with SELECT/WITH, single statement) and blocks destructive DML hidden inside a CTE; all probe fetches must return 2xx, preventing destructive queries and false passes.Written for commit 1452c86. Summary will update on new commits.
Note
Add hidden
preview createandpreview teardownCLI commands for full-stack e2e preview branchespreview create <name>which provisions a full-stack preview branch via the platform API, polls until ready, and writes a local manifest under.insforge/previews/.--wire-env [file]option topreview createthat rewrites a frontend env file (default.env.local) to pointNEXT_PUBLIC_INSFORGE_URLat the preview backend, backing up any existing file.preview teardown <name>which deletes the remote branch and restores or removes any wired env file and the local manifest.overwriteEnvFileinenv-writer.tsto deterministically overwrite or append env vars in.env-style files.--help) viaregisterPreviewCommands.Changes since #166 opened
deleteBranchApito optionally tolerate HTTP 404 responses and updated teardown command to ignore missing remote branches [aa43a5b]assertSafeNamevalidation function and integrated it into preview creation flow [aa43a5b]preview teardowncommand [3e1323f]preview createcommand options parameter [3e1323f]installSkillsfunction [21c8689]--with-test-agentsflag to thelinkcommand and threaded it through all installation code paths [21c8689]installSkillscalls to match the new function signature [21c8689]insforge previewcommands [3dd19ac]overwriteEnvFilefunction fromenv-writermodule and deleted associated test file [26d56bf]installSkillsfunction signature to accept single argument inprojects.linkcommand [26d56bf]isReadOnlyQueryvalidation toverify truthcommand and introducedassertOkHTTP error handling across authentication, database, and data API clients [29acf6c]mergeJsonMcpto handle valid JSON that is not an object [29acf6c]deleteBranchApi[29acf6c]trackVerifyFindingfor field precedence [29acf6c]Macroscope summarized 48d0d38.
Summary by CodeRabbit
previewCLI withcreateandteardownfor isolated full-stack previews, including safe naming, preview manifest tracking, and optional--wire-envenv wiring.linknow supports--with-test-agentsto install Playwright Test Agents during skill installation.$values literally.insforge previewcommand documentation.