INS-356: PoC: Full-stack preview for e2e verify#166
Conversation
WalkthroughThis pull request adds a preview environment provisioning system: manifest persistence, env-file overwrite utility, a hidden ChangesPreview Environment Provisioning System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 SummaryThis PR introduces two hidden experimental CLI commands —
Confidence Score: 5/5Safe to merge as a hidden experimental command; the core create/teardown flows are well-guarded and the only findings are non-blocking display and edge-case cleanup issues. The orphaned-branch rollback, duplicate-create guard, and 404-tolerant teardown are all correctly implemented and tested. The two findings are limited to a misleading success message when no backup was made and an orphaned .preview-bak in an unlikely failure path — neither affects the happy path or causes data loss. src/commands/preview/create.ts — the success-message branch and the error path between copyFileSync and overwriteEnvFile. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant C as Create or Teardown
participant A as Platform API
participant F as Local FS
U->>C: preview create name
C->>F: readPreviewManifest guard
C->>A: createBranchApi
A-->>C: Branch id appkey state creating
loop pollUntilReady 5 min
C->>A: getBranchApi branchId
A-->>C: Branch state
alt state ready
C-->>C: continue
else timeout or failed
C->>A: deleteBranchApi rollback
C-->>U: CLIError with hint
end
end
C->>F: writePreviewManifest
opt wire-env flag
C->>F: copyFileSync to preview-bak
C->>F: overwriteEnvFile
C->>F: writePreviewManifest with wiredEnvFile
end
C-->>U: Preview ready URL
U->>C: preview teardown name
C->>F: readPreviewManifest
F-->>C: manifest
C->>A: deleteBranchApi ignoreNotFound
opt wiredEnvFile set
alt wiredEnvCreated
C->>F: rmSync envFile
else backup exists
C->>F: restore from preview-bak
end
end
C->>F: deletePreviewManifest
C-->>U: Preview torn down
Reviews (5): Last reviewed commit: "fix(preview): gate teardown output behin..." | 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
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/preview/create.ts">
<violation number="1" location="src/commands/preview/create.ts:55">
P1: Data integrity issue: the remote branch is created at line 39, but the manifest isn't persisted until after env-file mutations complete. If `copyFileSync` or `overwriteEnvFile` throws, the catch handler runs without a manifest ever being written, so `preview teardown` cannot discover or clean up the orphaned remote branch. Write a baseline manifest immediately after `pollUntilReady` succeeds, then update it with `wiredEnvFile` after env operations complete.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| overwriteEnvFile(envPath, { NEXT_PUBLIC_INSFORGE_URL: previewUrl }); | ||
| } | ||
|
|
||
| await writePreviewManifest(process.cwd(), { |
There was a problem hiding this comment.
P1: Data integrity issue: the remote branch is created at line 39, but the manifest isn't persisted until after env-file mutations complete. If copyFileSync or overwriteEnvFile throws, the catch handler runs without a manifest ever being written, so preview teardown cannot discover or clean up the orphaned remote branch. Write a baseline manifest immediately after pollUntilReady succeeds, then update it with wiredEnvFile after env operations complete.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/preview/create.ts, line 55:
<comment>Data integrity issue: the remote branch is created at line 39, but the manifest isn't persisted until after env-file mutations complete. If `copyFileSync` or `overwriteEnvFile` throws, the catch handler runs without a manifest ever being written, so `preview teardown` cannot discover or clean up the orphaned remote branch. Write a baseline manifest immediately after `pollUntilReady` succeeds, then update it with `wiredEnvFile` after env operations complete.</comment>
<file context>
@@ -0,0 +1,95 @@
+ overwriteEnvFile(envPath, { NEXT_PUBLIC_INSFORGE_URL: previewUrl });
+ }
+
+ await writePreviewManifest(process.cwd(), {
+ name,
+ branchId: ready.id,
</file context>
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).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/preview/create.ts">
<violation number="1" location="src/commands/preview/create.ts:47">
P2: Automatic cleanup deletes the new branch for all polling errors, including transient API/network failures. This can destroy a preview that would otherwise become ready.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ready = await pollUntilReady(created.id, apiUrl); | ||
| } catch (pollErr) { | ||
| try { | ||
| await deleteBranchApi(created.id, apiUrl); |
There was a problem hiding this comment.
P2: Automatic cleanup deletes the new branch for all polling errors, including transient API/network failures. This can destroy a preview that would otherwise become ready.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/preview/create.ts, line 47:
<comment>Automatic cleanup deletes the new branch for all polling errors, including transient API/network failures. This can destroy a preview that would otherwise become ready.</comment>
<file context>
@@ -38,7 +38,22 @@ export function registerPreviewCreateCommand(preview: Command): void {
+ ready = await pollUntilReady(created.id, apiUrl);
+ } catch (pollErr) {
+ try {
+ await deleteBranchApi(created.id, apiUrl);
+ } catch {
+ // Best effort — fall through to the actionable error below.
</file context>
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
| beforeEach(async () => { | ||
| vi.clearAllMocks(); | ||
| tmpBase = await fs.mkdtemp(path.join(os.tmpdir(), 'preview-cmd-')); | ||
| vi.spyOn(process, 'cwd').mockReturnValue(tmpBase); | ||
| }); |
There was a problem hiding this comment.
Add afterEach to clean up temporary directories.
The test creates tmpBase in beforeEach but never removes it. This leaks temporary directories, especially if tests fail.
🧹 Proposed cleanup addition
describe('preview create', () => {
beforeEach(async () => {
vi.clearAllMocks();
tmpBase = await fs.mkdtemp(path.join(os.tmpdir(), 'preview-cmd-'));
vi.spyOn(process, 'cwd').mockReturnValue(tmpBase);
});
+ afterEach(async () => {
+ await fs.rm(tmpBase, { recursive: true, force: true });
+ });📝 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.
| beforeEach(async () => { | |
| vi.clearAllMocks(); | |
| tmpBase = await fs.mkdtemp(path.join(os.tmpdir(), 'preview-cmd-')); | |
| vi.spyOn(process, 'cwd').mockReturnValue(tmpBase); | |
| }); | |
| beforeEach(async () => { | |
| vi.clearAllMocks(); | |
| tmpBase = await fs.mkdtemp(path.join(os.tmpdir(), 'preview-cmd-')); | |
| vi.spyOn(process, 'cwd').mockReturnValue(tmpBase); | |
| }); | |
| afterEach(async () => { | |
| await fs.rm(tmpBase, { recursive: true, force: true }); | |
| }); |
🤖 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 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.
| expect(backup).toContain('NEXT_PUBLIC_INSFORGE_URL=https://prod.insforge.app'); | ||
|
|
||
| const manifest = await readPreviewManifest(tmpBase, 'feat-likes'); | ||
| expect(manifest?.wiredEnvFile).toBe(envFile); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how wiredEnvFile paths are stored in create.ts
# Search for wiredEnvFile assignment in create.ts
rg -A 3 -B 3 'wiredEnvFile' src/commands/preview/create.tsRepository: InsForge/CLI
Length of output: 1330
🏁 Script executed:
rg -n 'wiredEnvFile' src/commands/preview -g'*.ts' -C 3Repository: InsForge/CLI
Length of output: 7115
wiredEnvFile relative storage makes teardown depend on cwd
src/commands/preview/create.ts records manifest.wiredEnvFile as the raw --wire-env value (defaulting to '.env.local' when omitted), so it can be relative. src/commands/preview/teardown.ts then restores using path.resolve(process.cwd(), manifest.wiredEnvFile), meaning teardown resolves that relative path against the directory where teardown is run. If cwd differs from create time, teardown can restore/remove the wrong env file or fail. Store the absolute envPath in the manifest (and adjust create.test.ts expectations).
🤖 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` at 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.
2 participants
Summary by cubic
PoC for INS‑356: experimental full‑stack previews for e2e verify via
insforge previewcreate/teardown. Adds safe name validation, duplicate‑create guard, resilient teardown, and clean--jsonoutput.New Features
previewgroup withcreate <name>andteardown <name>.create: provisions a full preview, polls until ready, writes.insforge/previews/<name>.json, validates safe names, refuses duplicates, and supports--wire-env [file](default.env.local) to setNEXT_PUBLIC_INSFORGE_URL=https://{appkey}.{region}.insforge.appwith a.preview-bakbackup; tracks created files for teardown.teardown: deletes the preview (tolerates 404), restores the wired env from backup or removes a created file, and deletes the manifest; continues cleanup even if restore fails.Bug Fixes
overwriteEnvFilenow rewrites all duplicate keys and writes values literally (keeps$).previewis hidden from--help(behavior test).--jsonby suppressing non‑JSON logs; validate preview name on teardown.Written for commit 3e1323f. 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]Macroscope summarized f64f000.
Summary by CodeRabbit
New Features
Improvements
Tests