fix(cli): improve branch provisioning recovery and reconciliation - #203
fix(cli): improve branch provisioning recovery and reconciliation#203shobhitsahani wants to merge 9 commits into
Conversation
Three defects that share one theme: the command reports success on signals that do not mean what a caller needs them to mean. Found while automating branch creation against ap-southeast; details and measurements in InsForge/InsForge#1790. 1. `ready` is a control-plane state, not readiness. `pollUntilReady` returns the moment `branch_state === 'ready'` and never contacts the branch's own host. But `branch_state` flips when the provisioning job returns, while the instance is still coming up — every request to https://<appkey>.<region>.insforge.app resets until it does. Measured on ap-southeast: the host started serving at t+2m in one case and t+11.5m in another, `branch_state` reading 'ready' the whole time. Because create then auto-switches the directory onto that host, the failure surfaces on the user's NEXT command rather than here, as {"error":"fetch failed","code":"UNKNOWN_ERROR"} from whatever they ran. Now: after the control plane says ready, poll GET /api/health on the branch itself until it answers, and say so in the spinner. If it never answers within the budget the command reports that honestly instead of claiming success — the branch still exists and its name and id are still printed, because it is real and it is billing. 2. The 5-minute poll ceiling was below the observed provisioning time. POLL_TIMEOUT_MS was 5 minutes, so the 11.5-minute branch was reported as "still in 'creating' state" when it was simply not finished. Raised to 15, with a separate 10-minute budget for the data-plane wait. 3. A failed create can leave a live branch behind. `createBranchApi` carries no idempotency key. A transport failure on the RESPONSE leg — the POST arrived and the branch was created — throws before `created` is bound, so the CLI exits non-zero with no id and no name while a branch exists and bills. We hit exactly this: $ insforge branch create <name> --mode schema-only --no-switch {"error":"Connection to api.insforge.dev was reset. A proxy, VPN, or firewall may be interfering."} # exit 1 $ insforge branch list # the branch is there, state "creating" Now: on a create failure, ask `branch list` — a control-plane call that still works while the branch's own host is unreachable — whether the branch exists under that name, and adopt it if so. The original error is rethrown unchanged when nothing was created. Also: `ossFetch` called `fetch` unguarded, unlike `platformFetch`, so a dead data plane surfaced as the generic UNKNOWN_ERROR instead of naming the host. The new `probeBackendHealth` routes its errors through `formatFetchError`, so "Connection to <host> was reset" is what a caller sees. Tests cover the not-serving path, the adopt path, and that a genuine failure with nothing created still exits non-zero. The existing create tests needed a mock for the new probe — an unmocked one makes a real request to a fake host and then polls, so beforeEach resets it explicitly (clearAllMocks keeps implementations, which would otherwise leak an unreachable branch into every later test). Agent skills: `insforge-cli/references/branch/overview.md` says a branch takes 30-120s and that `ready` means "usable — can be switched, modified, merged, or reset", and its post-create checklist goes straight to `functions deploy`. Both are worth updating in InsForge/agent-skills once the timing here is confirmed; happy to open that PR alongside if you want it.
…host never serves
Both review findings are real; fixing rather than arguing.
1. Adoption was too broad (greptile P1/security, cubic P1).
The catch adopted a same-name branch after ANY create failure, so a
duplicate-name rejection — a refusal, not a lost response — could switch the
caller into a pre-existing branch with a different mode and different data.
Two guards now, either of which is sufficient:
- only a TAGGED transport failure is eligible. platformFetch's fetch-level
catch now throws CLIError with code NETWORK_ERROR, so "the server said no"
and "we never heard back" are finally distinguishable by callers. Every
HTTP/API rejection rethrows untouched, and the test asserts that
listBranchesApi is not even called in that case.
- the candidate must have been created at or after the moment we sent the
request, so a pre-existing same-name branch can never be adopted. A 60s
skew allowance keeps a genuinely-just-created branch eligible when the
local clock differs from the control plane's; being slightly wide risks
adopting a branch someone made seconds ago under the same name, while
being too narrow re-opens the orphaned-billing bug this exists to fix.
2. A branch that never serves exited 0 (greptile P1, cubic P1).
I stopped the spinner with an error frame and left it there: the process
still exited 0, --json still emitted a ready-looking branch with no serving
field, and the non-JSON path still advised re-sourcing the env. That is
exactly the "exit 0 does not mean ready" defect this PR is meant to remove,
reproduced inside the fix for it.
Now: `serving` is tracked separately from `provisioned`, --json emits
{ branch, serving }, the env hint only prints when the host actually answers,
and the command throws afterwards so the exit is non-zero. The identity is
emitted BEFORE the throw on purpose — the branch exists and is billing, so a
caller has to be able to find and delete it even as the command fails.
Three tests added: an API rejection is not adopted, a branch predating the
request is not adopted, and the never-serving path exits 1 while still printing
the branch id and serving:false.
- Fix network error detection so create reconciliation is reachable - Add tests for readiness, reconciliation, and delete retry - Improve provisioning documentation - Preserve existing CLI behavior while improving recovery from transient failures
|
Thanks for the PR, @shobhitsahani! A quick note on our workflow: we ask contributors to open an issue first, get it assigned, then submit a PR that links it (e.g. "Closes #123"). This PR isn't linked to any issue. It'll still be reviewed, but please open an issue and claim it (comment that you'd like it assigned to you) so the work is tracked. |
agent-zhang-beihai
Bot
added
the
needs-issue
WalkthroughBranch creation now waits for data-plane health and reconciles interrupted requests. Branch deletion retries busy-state failures. Database commands identify branch provisioning errors and emit branch-specific output. ChangesBranch lifecycle resilience
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 improves recovery and readiness handling for branch provisioning. The main changes are:
Confidence Score: 5/5No additional qualifying issues were found for this follow-up review.
Reviews (2): Last reviewed commit: "fix(cli): rebase onto #201 base and reso..." | 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: 3
🧹 Nitpick comments (3)
src/commands/branch/delete.test.ts (1)
176-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name is misleading and overlaps test at Lines 197-214.
This case asserts
deleteBranchApiis called twice — i.e. the retry flow — but is named as if it directly verifiesisBusyError.isBusyErroris not exported, so it's never unit-tested in isolation. Either rename this to reflect the retry behavior it actually covers (and drop the near-duplicate of the "busy then ready" test below), or exportisBusyErrorand add a true unit test over its message cases.🤖 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/branch/delete.test.ts` around lines 176 - 195, Rename the test describing the `deleteBranchApi` call count to reflect that it verifies retry behavior after a provisioning-busy error, and remove the overlapping “busy then ready” test below. Keep the existing retry assertions and message-case coverage intact; do not treat this test as direct `isBusyError` unit coverage unless you explicitly export that symbol and add isolated tests.src/commands/branch/delete.ts (1)
15-22: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDrop the redundant
currently busyclausebusyalready covers it; this path only has message text to inspect, so there isn’t a stable code/statusCode to key off here.🤖 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/branch/delete.ts` around lines 15 - 22, Remove the redundant msg.includes('currently busy') condition from isBusyError, while preserving the existing CLIError check and the busy/creating/merging message checks.src/commands/db/migrations.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated branch-provisioning catch handler. The same block — load
getProjectConfig(), checkbranched_from != null, callisProvisioningError, emitBRANCH_PROVISIONINGJSON/text viabuildProvisioningErrorMessage, thenprocess.exit(1)— is copied five times. Extract a single helper (e.g.handleBranchProvisioningError(err, json): booleaninsrc/lib/api/oss.tsalongside the existing helpers) and call it at each site so message/format/exit behavior stays consistent.
src/commands/db/migrations.ts#L137-149: replace inline block inmigrations listcatch with the shared helper.src/commands/db/migrations.ts#L218-230: replace inline block inmigrations fetchcatch with the shared helper.src/commands/db/migrations.ts#L277-289: replace inline block inmigrations newcatch with the shared helper.src/commands/db/migrations.ts#L470-482: replace inline block inmigrations upcatch with the shared helper.src/commands/db/query.ts#L45-59: replace inline block indb querycatch with the shared helper.🤖 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/db/migrations.ts` at line 1, The branch-provisioning error handling is duplicated across five command catch blocks. Extract the shared getProjectConfig/branched_from/isProvisioningError and BRANCH_PROVISIONING output logic into handleBranchProvisioningError(err, json): boolean alongside the existing helpers in oss.ts, preserving message formatting and process.exit(1); then replace the inline handlers in the migrations list, fetch, new, and up catches plus the db query catch with calls to the helper.
🤖 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/branch/create.ts`:
- Around line 119-140: Update the branch creation error handling around
isNetworkError and the provisioned branch to handle BRANCH_DATA_PLANE_TIMEOUT
explicitly. Since this error occurs after provisioning, remove it from the
reconciliation-only network-error check and surface dedicated data-plane
readiness timeout guidance instead of the generic context-switch message.
- Around line 54-55: Change the wait-ready option declaration in the branch
creation command to support opting out, using a negated option or equivalent
paired flag so opts.waitReady can be false when requested. Preserve the existing
default behavior of waiting for the data plane when no opt-out is provided.
In `@src/lib/api/oss.ts`:
- Around line 62-66: Update buildProvisioningErrorMessage so its provisioning
duration guidance matches the 15-minute value used by HEALTH_CHECK_TIMEOUT_MS
and the timeout error in create.ts. Change only the user-facing duration text,
preserving the existing branch-specific wording and retry instructions.
---
Nitpick comments:
In `@src/commands/branch/delete.test.ts`:
- Around line 176-195: Rename the test describing the `deleteBranchApi` call
count to reflect that it verifies retry behavior after a provisioning-busy
error, and remove the overlapping “busy then ready” test below. Keep the
existing retry assertions and message-case coverage intact; do not treat this
test as direct `isBusyError` unit coverage unless you explicitly export that
symbol and add isolated tests.
In `@src/commands/branch/delete.ts`:
- Around line 15-22: Remove the redundant msg.includes('currently busy')
condition from isBusyError, while preserving the existing CLIError check and the
busy/creating/merging message checks.
In `@src/commands/db/migrations.ts`:
- Line 1: The branch-provisioning error handling is duplicated across five
command catch blocks. Extract the shared
getProjectConfig/branched_from/isProvisioningError and BRANCH_PROVISIONING
output logic into handleBranchProvisioningError(err, json): boolean alongside
the existing helpers in oss.ts, preserving message formatting and
process.exit(1); then replace the inline handlers in the migrations list, fetch,
new, and up catches plus the db query catch with calls to the helper.
🪄 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: 517306a1-786d-46fc-9272-89ecfec19f22
📒 Files selected for processing (8)
src/commands/branch/create.test.tssrc/commands/branch/create.tssrc/commands/branch/delete.test.tssrc/commands/branch/delete.tssrc/commands/db/migrations.tssrc/commands/db/query.test.tssrc/commands/db/query.tssrc/lib/api/oss.ts
| // Check if this is a network error (fetch failed, ECONNRESET, etc.) | ||
| // Match both raw undici error messages AND the formatted output of | ||
| // formatFetchError (used by platformFetch), so reconciliation is | ||
| // reachable regardless of which layer surfaces the error. | ||
| // If so, attempt to reconcile by checking if the branch was actually created | ||
| const isNetworkError = err instanceof CLIError && | ||
| (err.message.includes('fetch failed') || | ||
| err.message.includes('ECONNRESET') || | ||
| err.message.includes('ETIMEDOUT') || | ||
| err.message.includes('ENOTFOUND') || | ||
| err.message.includes('ECONNREFUSED') || | ||
| err.message.includes('UND_ERR_CONNECT_TIMEOUT') || | ||
| err.message.includes('UND_ERR_SOCKET') || | ||
| err.message.includes('timeout') || | ||
| // Formatted messages from formatFetchError (used by platformFetch) | ||
| err.message.includes('was reset') || | ||
| err.message.includes('was refused') || | ||
| err.message.includes('timed out') || | ||
| err.message.includes('Cannot resolve') || | ||
| err.message.includes('Network error contacting') || | ||
| err.message.includes('TLS certificate error') || | ||
| err.code === 'BRANCH_DATA_PLANE_TIMEOUT'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
BRANCH_DATA_PLANE_TIMEOUT in isNetworkError is unreachable here, and the timeout produces a misleading message.
waitForDataPlaneReady only runs after provisioned becomes true (Line 100), so its BRANCH_DATA_PLANE_TIMEOUT error can only surface with provisioned === true. But reconciliation is gated on !provisioned (Line 142), so this err.code check never contributes. Worse, that timeout then falls through to the if (provisioned) branch (Line 167), telling the user "switching context failed — run insforge branch switch to retry", which misdescribes a data-plane readiness timeout. Consider handling the timeout case explicitly so the surfaced guidance matches the actual failure.
🤖 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/branch/create.ts` around lines 119 - 140, Update the branch
creation error handling around isNetworkError and the provisioned branch to
handle BRANCH_DATA_PLANE_TIMEOUT explicitly. Since this error occurs after
provisioning, remove it from the reconciliation-only network-error check and
surface dedicated data-plane readiness timeout guidance instead of the generic
context-switch message.
| export function buildProvisioningErrorMessage(branchName?: string): string { | ||
| const base = 'Branch is still provisioning (this can take up to ~12 minutes).'; | ||
| const branchPart = branchName ? ` Branch: ${branchName}.` : ''; | ||
| return `${base}${branchPart} Retry shortly, or create the branch with \`--wait-ready\` to block until it's usable.`; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Minor: message says "~12 minutes" while the health-check timeout in create.ts is 15 minutes.
HEALTH_CHECK_TIMEOUT_MS and the timeout error both reference 15 minutes; this guidance says ~12. Align the numbers to avoid confusing users.
🤖 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/api/oss.ts` around lines 62 - 66, Update
buildProvisioningErrorMessage so its provisioning duration guidance matches the
15-minute value used by HEALTH_CHECK_TIMEOUT_MS and the timeout error in
create.ts. Change only the user-facing duration text, preserving the existing
branch-specific wording and retry instructions.
![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 8 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/branch/delete.test.ts">
<violation number="1" location="src/commands/branch/delete.test.ts:176">
P3: Test name claims to cover 'busy, creating, and merging' message patterns but only tests a 'busy' message. Either rename to match what's actually tested (e.g., 'retries deletion on busy error') or add test cases for 'creating' and 'merging' messages to match the name.</violation>
</file>
<file name="src/lib/api/oss.ts">
<violation number="1" location="src/lib/api/oss.ts:23">
P3: The new classifier can regress without a test catching which network causes are accepted versus ordinary or TLS failures. Direct unit cases for supported cause codes, unsupported causes, and HTTP `CLIError` messages would make this recovery behavior verifiable.</violation>
<violation number="2" location="src/lib/api/oss.ts:32">
P3: Network failure classification is duplicated between `isProvisioningError` and `formatFetchError`, so adding or changing a supported cause requires edits in two places and can produce inconsistent CLI behavior. Sharing the codes or classifier would keep branch recovery aligned with the existing fetch-error handling.</violation>
</file>
<file name="src/commands/branch/create.ts">
<violation number="1" location="src/commands/branch/create.ts:54">
P2: Every `branch create` now performs the data-plane health wait by default, potentially blocking for 15 minutes and changing the existing command behavior; the declared positive flag also provides no way to disable that default. The option should default to false/undefined so the health wait only runs when `--wait-ready` is supplied.</violation>
</file>
<file name="src/commands/db/query.test.ts">
<violation number="1" location="src/commands/db/query.test.ts:60">
P2: Heavy boilerplate duplication between the two test cases (~20 lines repeated). Extract the console.error capture, process.exit mock, and try/finally teardown into a shared helper or beforeEach/afterEach so new tests don't need to copy the same pattern.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| * This detects network-level failures (ECONNRESET, fetch failed, timeout) | ||
| * that occur when the branch's data plane isn't ready yet. | ||
| */ | ||
| export function isProvisioningError(err: unknown): boolean { |
There was a problem hiding this comment.
P3: The new classifier can regress without a test catching which network causes are accepted versus ordinary or TLS failures. Direct unit cases for supported cause codes, unsupported causes, and HTTP CLIError messages would make this recovery behavior verifiable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/api/oss.ts, line 23:
<comment>The new classifier can regress without a test catching which network causes are accepted versus ordinary or TLS failures. Direct unit cases for supported cause codes, unsupported causes, and HTTP `CLIError` messages would make this recovery behavior verifiable.</comment>
<file context>
@@ -15,6 +15,56 @@ function requireProjectConfig(): ProjectConfig {
+ * This detects network-level failures (ECONNRESET, fetch failed, timeout)
+ * that occur when the branch's data plane isn't ready yet.
+ */
+export function isProvisioningError(err: unknown): boolean {
+ if (!(err instanceof Error)) return false;
+ const msg = err.message.toLowerCase();
</file context>
| : ''; | ||
|
|
||
| // Network errors that indicate the data plane isn't ready | ||
| const provisioningCodes = [ |
There was a problem hiding this comment.
P3: Network failure classification is duplicated between isProvisioningError and formatFetchError, so adding or changing a supported cause requires edits in two places and can produce inconsistent CLI behavior. Sharing the codes or classifier would keep branch recovery aligned with the existing fetch-error handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/api/oss.ts, line 32:
<comment>Network failure classification is duplicated between `isProvisioningError` and `formatFetchError`, so adding or changing a supported cause requires edits in two places and can produce inconsistent CLI behavior. Sharing the codes or classifier would keep branch recovery aligned with the existing fetch-error handling.</comment>
<file context>
@@ -15,6 +15,56 @@ function requireProjectConfig(): ProjectConfig {
+ : '';
+
+ // Network errors that indicate the data plane isn't ready
+ const provisioningCodes = [
+ 'econnreset',
+ 'etimedout',
</file context>
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Review — fix(cli): improve branch provisioning recovery and reconciliation
Summary: Solid, well-commented resiliency work (data-plane readiness poll, create reconciliation, delete retry, provisioning-aware DB errors), but the new --wait-ready default-on behavior ships without a working way to turn it off, which blocks merge.
Requirements context
No matching spec/plan for branch provisioning exists under docs/specs/ (this repo uses docs/specs/, not docs/superpowers/). The closest doc — docs/specs/2026-04-17-db-migrations-command-design.md — covers the db migrations surface and states user-facing failures should use CLIError (relevant to a Suggestion below). Otherwise assessed against the PR description and the surrounding code.
Critical
functionality — --wait-ready is default-on with no working way to disable it (src/commands/branch/create.ts:54)
The flag is registered as a positive boolean defaulting to true:
.option('--wait-ready', 'Wait for the branch data plane to be fully ready (up to 15 min)', true)The PR description says the wait can be disabled "with --no-wait-ready" / --wait-ready=false, but neither works with commander (v13). Commander only creates a negation when a --no-<name> option is explicitly defined — a positive-only flag has no auto-generated negation (the docs example shows --sauce → error: unknown option '--sauce' when only --no-sauce is declared; the reverse is symmetric). So:
insforge branch create x --no-wait-ready→error: unknown option '--no-wait-ready'insforge branch create x --wait-ready=false→ commander rejects a value on a boolean flag
Net effect: branch create now always blocks on the data-plane health poll (up to 15 min, on top of the existing ~5 min control-plane poll) with no escape hatch. That breaks existing non-blocking/scripted create behavior and directly contradicts the documented opt-out. No test covers the disable path, so it went unnoticed.
Fix: declare the negatable form, e.g. .option('--no-wait-ready', 'Do not wait for the branch data plane to be ready') (this alone yields opts.waitReady defaulting to true), and add a test that passes --no-wait-ready and asserts the health poll is skipped.
Suggestion
functionality — data-plane timeout is reported as a context-switch failure (src/commands/branch/create.ts:100-104, :140, :167-171)
When waitForDataPlaneReady throws BRANCH_DATA_PLANE_TIMEOUT, provisioned is already true, so the catch skips reconciliation (if (!provisioned && isNetworkError)) and falls into the if (provisioned) branch, printing "Branch '<name>' is ready, but switching context failed — run insforge branch switch" — which is misleading (the switch never ran; the health wait timed out). Consequently the err.code === 'BRANCH_DATA_PLANE_TIMEOUT' clause in the isNetworkError list is effectively unreachable. Consider handling the timeout with its own message.
software engineering (tests) — coverage gaps around the new behavior
- No test for the
--no-wait-readydisable path (would have caught the Critical above). - No test for
waitForDataPlaneReadyon an unhealthy/timeout response. - No test for the delete "still busy after 6 min" give-up path (
delete.ts:44-54). - The PR says tests were added for "DB provisioning messaging," but only
db queryis covered (query.test.ts) — the identical block added to all fourdb migrationssubcommands (migrations.ts) has no test.
software engineering — duplicated provisioning-error block (src/commands/db/migrations.ts:137-149, 218-230, 277-289, 470-482)
The same ~13-line isBranch && isProvisioningError(err) → console.error + process.exit(1) block is copy-pasted four times (and a fifth time in query.ts:47-59). Extract a shared helper (e.g. handleBranchProvisioningError(err, json, config)). Related: this path bypasses the existing handleError convention with a direct console.error/process.exit, which diverges from how the rest of the CLI (and the db-migrations spec's "use CLIError") reports failures.
functionality — isBusyError matches error text too loosely (src/commands/branch/delete.ts:15-22)
msg.includes('creating') || msg.includes('merging') || msg.includes('busy') will treat any error message that merely contains those substrings (e.g. a server error mentioning "creating") as a busy state and enter the 6-minute retry loop. Prefer matching a server-provided error code/statusCode over free-text.
functionality — 'was refused' never matches the formatted error (src/commands/branch/create.ts:135)
formatFetchError emits "Connection to <host> refused." for ECONNREFUSED (no "was"), so err.message.includes('was refused') never matches the formatted message ('was reset', 'timed out', 'Cannot resolve' do match their counterparts). Low impact — a refused connection generally means the branch was never created, so reconciliation isn't needed — but the clause is misleading; align it with the actual formatFetchError output.
Information
src/commands/branch/delete.ts:37—elapsedSecis computed but never used. Not CI-blocking (@typescript-eslint/no-unused-varsis configured aswarnandlintdoesn't set--max-warnings 0), but it's dead code.src/commands/branch/create.ts:159,194— the reconciliation success path callsawait shutdownAnalytics()beforereturn, and the outerfinallycalls it again. Harmless if idempotent, but redundant.src/commands/branch/create.ts:18— the health URL hardcodeshttps://<appkey>.<region>.insforge.app, ignoring--api-url. Acceptable since branches are cloud-only, but worth a comment.- Reconciliation non-JSON path stops the spinner with error code
1(red frame) but the command exits0; minor UX inconsistency for a branch that did get created. - The PR body lists "Improve provisioning documentation," but the diff changes only
.tsfiles (inline comments + error-message hints); no doc files were touched.
Security: No security-relevant concerns — no new secrets logged, the health endpoint is unauthenticated by design, and no new user input reaches SQL/shell.
Performance: No concerns beyond the behavioral one above — both new poll loops are bounded (health ≤ ~180 sequential requests over 15 min; delete ≤ 12 over 6 min) with no N+1 or hot-path work.
Verdict: request_changes
One Critical (no working way to disable the new default-on 15-minute wait). The reconciliation, delete-retry, and provisioning-error mechanics are otherwise sound; addressing the negation flag (plus a test for it) is the main blocker.
…OR_CODE export Addresses the review on InsForge#201 (jwfing, approved): - Functionality: `branch create` now exits non-zero when the branch never reaches 'ready' within the poll budget, matching the existing ready-but- not-serving exit. Both outcomes are "not usable", and this PR's goal is that success means usable — so automation reading the exit code must not see 0 for a branch stuck provisioning. Test added (getBranchApi stuck in 'creating' → exit 1), with the shared mock impl restored so it can't leak the full poll budget into later tests. - Software engineering: moved `export const NETWORK_ERROR_CODE` below the import block in platform.ts (it was interleaved between two imports — legal via hoisting, but conventionally declarations sit under the imports). The `/api/health` "listening ≠ usable" note is acknowledged as the best client-side signal available today and left as the PR's documented open question, to key off a data-plane-scoped signal if one lands cloud-side. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…it-capture test helper Addresses the two follow-up reviews on InsForge#201: - cubic (P2): a transport failure whose response leg is lost could, in the 60s skew window, adopt a collaborator's SAME-NAME branch — and a default --switch would then move local context onto it. createBranchOrAdopt now also requires the candidate's mode to match the requested mode, narrowing the collision to an even more specific coincidence (same name AND same mode AND the same ~60s AND our transport failure). The real fix is a server-issued idempotency token (InsForge/InsForge#1790); this is the tightest client-side guard until then. Test added: a same-name, different-mode branch in the window is NOT adopted. - CodeRabbit (nitpick): extracted `withCapturedExit(fn)` for the repeated process.exit/stderr override-restore boilerplate, and applied it to the tests this PR added (incl. the fake-timer provisioning-timeout one). The older pre-existing blocks are left as-is to keep this diff scoped to the PR's surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…readiness fix(branch): make `branch create` success mean the branch is usable
- Fix network error detection so create reconciliation is reachable - Add tests for readiness, reconciliation, and delete retry - Improve provisioning documentation - Preserve existing CLI behavior while improving recovery from transient failures
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/commands/db/query.ts (1)
53-65: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve the healthy-branch check before reporting provisioning.
These inline replacements remove
handleBranchProvisioningError’s/api/healthcheck. SinceisProvisioningErrorclassifies genericfetch failederrors, a transient failure against a healthy branch is now incorrectly emitted asBRANCH_PROVISIONINGinstead of the real network error. Extract/reuse a branch-aware helper that retains both the branch guard and health verification.
src/commands/db/query.ts#L53-L65: route provisioning classification through the health-verified path.src/commands/db/migrations.ts#L145-L154: do the same formigrations list.src/commands/db/migrations.ts#L230-L239: do the same formigrations fetch.src/commands/db/migrations.ts#L293-L302: do the same formigrations new.src/commands/db/migrations.ts#L490-L499: do the same formigrations up.🤖 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/db/query.ts` around lines 53 - 65, Preserve the health-verified branch provisioning classification by extracting or reusing a branch-aware helper that checks both the branch guard and /api/health before reporting BRANCH_PROVISIONING. Update the provisioning error paths in src/commands/db/query.ts:53-65, src/commands/db/migrations.ts:145-154, src/commands/db/migrations.ts:230-239, src/commands/db/migrations.ts:293-302, and src/commands/db/migrations.ts:490-499 to use this helper, while leaving healthy-branch transient fetch failures as their original network errors.src/commands/branch/delete.ts (1)
11-41: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winResolve the unfinished merge before merging this PR.
The committed
<<<<<<<,=======, and>>>>>>>markers make both production code and tests unparsable.
src/commands/branch/delete.ts#L11-L41: select and retain one busy-error implementation.src/commands/branch/delete.ts#L54-L79: resolve the polling-loop implementation.src/commands/branch/delete.ts#L94-L133: resolve the retry helper implementation.src/commands/branch/delete.ts#L163-L174: resolve the command action implementation.src/commands/db/migrations.ts#L4-L8: retain one import declaration.src/commands/db/migrations.ts#L142-L157: resolve themigrations listerror handler.src/commands/db/migrations.ts#L226-L242: resolve themigrations fetcherror handler.src/commands/db/migrations.ts#L289-L305: resolve themigrations newerror handler.src/commands/db/migrations.ts#L486-L502: resolve themigrations uperror handler.src/commands/db/query.test.ts#L5-L33: resolve the OSS API mock.src/commands/db/query.test.ts#L66-L129: resolve the first test’s capture/assertion setup.src/commands/db/query.test.ts#L149-L193: resolve the second test’s capture/assertion setup.src/commands/db/query.ts#L2-L6: retain one import declaration.src/commands/db/query.ts#L49-L68: resolve the query error handler.🤖 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/branch/delete.ts` around lines 11 - 41, Resolve the unfinished merge by removing every conflict marker and selecting one coherent implementation at all affected sites: retain one busy-error implementation and reconcile the polling loop, retry helper, and command action in src/commands/branch/delete.ts at lines 11-41, 54-79, 94-133, and 163-174; retain one import and reconcile the migrations list, fetch, new, and up error handlers in src/commands/db/migrations.ts at lines 4-8, 142-157, 226-242, 289-305, and 486-502; reconcile the OSS API mock and both test capture/assertion setups in src/commands/db/query.test.ts at lines 5-33, 66-129, and 149-193; retain one import and reconcile the query error handler in src/commands/db/query.ts at lines 2-6 and 49-68. Ensure all resulting production code and tests parse and preserve the intended behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/branch/create.ts`:
- Around line 3-13: Resolve all remaining merge conflicts by retaining the HEAD
implementation and removing conflict markers and incoming-side code. In
src/commands/branch/create.ts at lines 3-13, 23-74, 82-86, and 141-158, preserve
NETWORK_ERROR_CODE, probeBackendHealth, --no-wait-ready, and the
waitUntilServing branch; in src/lib/api/oss.ts lines 69-120, retain
handleBranchProvisioningError; in src/commands/branch/delete.test.ts lines
216-276, retain the “still busy after max retry time” test; and in
src/commands/branch/create.test.ts lines 33-57, 345-384, 390-391, 690, and
695-846, retain the HEAD readiness/adoption tests. Ensure no conflict markers
remain and all files parse and compile.
---
Outside diff comments:
In `@src/commands/branch/delete.ts`:
- Around line 11-41: Resolve the unfinished merge by removing every conflict
marker and selecting one coherent implementation at all affected sites: retain
one busy-error implementation and reconcile the polling loop, retry helper, and
command action in src/commands/branch/delete.ts at lines 11-41, 54-79, 94-133,
and 163-174; retain one import and reconcile the migrations list, fetch, new,
and up error handlers in src/commands/db/migrations.ts at lines 4-8, 142-157,
226-242, 289-305, and 486-502; reconcile the OSS API mock and both test
capture/assertion setups in src/commands/db/query.test.ts at lines 5-33, 66-129,
and 149-193; retain one import and reconcile the query error handler in
src/commands/db/query.ts at lines 2-6 and 49-68. Ensure all resulting production
code and tests parse and preserve the intended behavior.
In `@src/commands/db/query.ts`:
- Around line 53-65: Preserve the health-verified branch provisioning
classification by extracting or reusing a branch-aware helper that checks both
the branch guard and /api/health before reporting BRANCH_PROVISIONING. Update
the provisioning error paths in src/commands/db/query.ts:53-65,
src/commands/db/migrations.ts:145-154, src/commands/db/migrations.ts:230-239,
src/commands/db/migrations.ts:293-302, and src/commands/db/migrations.ts:490-499
to use this helper, while leaving healthy-branch transient fetch failures as
their original network errors.
🪄 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 Plus
Run ID: 8456e85d-6211-45bc-a5b0-023c5ce3083e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
README.mdsrc/commands/branch/create.test.tssrc/commands/branch/create.tssrc/commands/branch/delete.test.tssrc/commands/branch/delete.tssrc/commands/db/migrations.tssrc/commands/db/query.test.tssrc/commands/db/query.tssrc/lib/api/oss.tssrc/lib/api/platform.ts
| <<<<<<< HEAD | ||
| import { | ||
| createBranchApi, | ||
| getBranchApi, | ||
| listBranchesApi, | ||
| NETWORK_ERROR_CODE, | ||
| } from '../../lib/api/platform.js'; | ||
| import { probeBackendHealth } from '../../lib/api/oss.js'; | ||
| ======= | ||
| import { createBranchApi, getBranchApi, listBranchesApi } from '../../lib/api/platform.js'; | ||
| >>>>>>> 34b302ebc5c301be89edb5a9c7e75ac702eb55ca |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unresolved merge conflicts across four files (incomplete rebase). Each file still carries <<<<<<< HEAD / ======= / >>>>>>> 34b302eb… markers, so none of them parse or compile. In every case the HEAD side is the intended resolution (it embodies the --no-wait-ready, waitUntilServing/probeBackendHealth, and handleBranchProvisioningError behavior this PR describes). Resolve by keeping HEAD and deleting the markers plus the incoming side.
src/commands/branch/create.ts#L3-L13: also resolve the further conflict blocks at L23-74, L82-86, and L141-158; keep the--no-wait-readyoption andwaitUntilServingbranch.src/lib/api/oss.ts#L69-L120: keep the HEADhandleBranchProvisioningError(L74-116), drop markers.src/commands/branch/delete.test.ts#L216-L276: keep the HEAD "still busy after max retry time" test, drop the empty incoming side.src/commands/branch/create.test.ts#L33-L57: also resolve the additional blocks at L345-384, L390-391, L690, and L695-846; keep the HEAD readiness/adoption tests.
🧰 Tools
🪛 Biome (2.5.3)
[error] 3-3: Expected a statement but instead found '<<<<<<< HEAD'.
(parse)
[error] 11-11: Expected a statement but instead found '======='.
(parse)
[error] 13-13: Expected a statement but instead found '>>>>>>> 34b302e'.
(parse)
[error] 13-13: numbers cannot be followed by identifiers directly after
(parse)
📍 Affects 4 files
src/commands/branch/create.ts#L3-L13(this comment)src/lib/api/oss.ts#L69-L120src/commands/branch/delete.test.ts#L216-L276src/commands/branch/create.test.ts#L33-L57
🤖 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/branch/create.ts` around lines 3 - 13, Resolve all remaining
merge conflicts by retaining the HEAD implementation and removing conflict
markers and incoming-side code. In src/commands/branch/create.ts at lines 3-13,
23-74, 82-86, and 141-158, preserve NETWORK_ERROR_CODE, probeBackendHealth,
--no-wait-ready, and the waitUntilServing branch; in src/lib/api/oss.ts lines
69-120, retain handleBranchProvisioningError; in
src/commands/branch/delete.test.ts lines 216-276, retain the “still busy after
max retry time” test; and in src/commands/branch/create.test.ts lines 33-57,
345-384, 390-391, 690, and 695-846, retain the HEAD readiness/adoption tests.
Ensure no conflict markers remain and all files parse and compile.
Source: Linters/SAST tools
2 participants
Summary by cubic
Makes branch provisioning predictable and safer.
branch createnow waits until the branch actually serves traffic by default and exits non‑zero if provisioning stalls or never serves, with tighter recovery from network interruptions.New Features
--wait-ready(default on) waits for the data plane/api/healthto be healthy before returning; use--no-wait-readyto skip.creatingor never starts serving.Bug Fixes
{ reconciled: true }in--json.db queryanddb migrations, with a--wait-readyhint.branch deleteretries when busy (creating/merging) for up to ~6 minutes, then reports a clear timeout if still busy.Written for commit 40acf52. Summary will update on new commits.
Note
Add data plane health polling, delete retry logic, and provisioning error handling to branch commands
--wait-readyflag (defaulttrue) tobranch createthat polls the branch/api/healthendpoint every 5s for up to 15 minutes, waiting for ahealthy/okstatus before returning.branch create: ifcreateBranchApifails with a network-style error, the CLI callslistBranchesApito check if the branch was actually created and returns{ branch, reconciled: true }in JSON mode.branch delete: if the branch is busy (creating/merging), the CLI pollsgetBranchApievery 30s for up to 6 minutes until the branch is deletable, then retries the delete.db queryanddb migrationssubcommands: when running against a branch, network errors indicative of an unready data plane now emit a friendly message with a--wait-readyhint and exit with code 1.branch createnow waits for data plane readiness by default; callers relying on fast non-blocking creation must pass--wait-ready=false.Macroscope summarized 34b302e.
Summary by CodeRabbit
--no-wait-readyoption, and supports adopting/reconciling a branch after connectivity interruptions.