fix(cloud): exit non-zero when the CloudFormation connect is still pending - #79
Conversation
|
CI note: the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM: correctly closes the cli#78 gap — the AWS pending outcome now sets process.exitCode at both exits of the connect picker loop, exit code 7 doesn't collide with anything else in src/errors/codes.ts, and no in-repo caller depended on the old silent-0 behavior (grepped for cloud connect invocations in scripts/workflows/package.json — none found; the only dependent, the installer, is out-of-repo and per #77 no longer relies on this exit code anyway).
Verified independently (not just the PR body's claims):
- Head SHA
9b4ab3e48b7dab3a137107daf5d0b3c11897b98bchecked out and confirmed byte-for-byte against the PR API'shead.sha. True merge-base (git merge-base HEAD origin/main) is9aac699..., exactly matching the PR's statedbase.sha— not stale.git diff --statagainst that merge-base is8 files changed, 111 insertions(+), 17 deletions(-), matching the API'sadditions/deletions/changed_filesexactly. - Traced both
connectExitCodecall sites (lines ~903, ~909 in the picker loop) by hand againstConnectOutcome/AwsStackWait.settle()'s actual return values — the mapping (timeout→1, anypending→7, else 0, with timeout outranking pending) is correct and matches the stated decisions. - Exit code 7 is documented in
ERRORS.md(new "Pending" category + updated browser-flow paragraph),README.md, andskill/SKILL.md— all three exit-code tables updated consistently. - Test coverage:
connectExitCodeunit tests cover all three branches including the timeout-outranks-pending case;test/exit-code.test.ts's subprocess case spawns a realnode --import tsx src/main.ts config show(same shape as #77) and asserts the process actually exits 7 — a genuine process-boundary test, not just a helper unit test;startAwsStackWait's existing subprocess-adjacent test now also asserts the updated "Exiting 7 (pending)" message text.parseAwsRegionsgets 4 direct-call cases (single/comma-list/all/empty). - Local checks at this head:
npm run lint— 0 errors.npm run test— 376/376 pass (confirmed on a repeat run; an initial run immediately aftergit checkoutundercounted at 371/89 suites, which reproduced as a local filesystem-timing artifact right after checkout, not a real failure — a second run picked up all 90 suites cleanly).npm run typecheck— passes at this head (0 errors); reproduced the previously-reported failure independently by running the identicalnpm run typecheckon a cleanorigin/mainworktree, which fails with the exact sameTS2561: ... 'region' does not exist ... Did you mean 'regions'?at (the base branch's) line 552 — confirming it was real, pre-existing spec drift (cli#80) unrelated to this diff's first commit, now fixed by this PR's second commit. - CI at this exact head SHA:
checks / Test & typecheckgreen on all of 20.x/22.x/24.x (confirmed viaGET /commits/{sha}/check-runs, not a cached view). The earlier red run (first commit, SHAea73213) was independently confirmed via the raw CI job log to fail at the identicalconnect.ts:563region/regionserror — matching cli#80 exactly and outside this diff's touched lines.
Scope note (non-blocking): the second commit does more than the minimal spec-drift fix — it also implements cli#80's suggested --region redesign (comma-separated list, all keyword, updated picker/help/examples, new parseAwsRegions + its test file). The PR body is transparent about why it's bundled (CI can't go green without it) and keeps it as a separate, atomic commit per AGENTS.md. Correct and tested, just flagging that this PR now does two distinct things under one exit-code-focused title/issue.
Two inline nits left on the diff, neither blocking.
Generated by Claude Code
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto-approved: Claude reviewed this PR and posted an LGTM verdict (see its review). A repo admin enabled this via the auto-approve workflow.
Generated by Claude Code
…nding cloud connect --provider aws could end with the stack still creating and exit 0, so scripts and the installer funnel read an unfinished connect as success. Add ExitCode.PENDING (7), route every connect outcome through connectExitCode(), and document the code (#78). Also send the AWS connect body the API now expects: regions: string[] | null (null = every enabled region) replaced region: string and broke typecheck on main (#80). --region takes a comma-separated list or "all"; the picker offers All regions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9b4ab3e to
a978c13
Compare
There was a problem hiding this comment.
LGTM: re-review at the squashed head — supersedes my prior review at ea73213/9b4ab3e; substance unchanged, both nits now genuinely fixed.
Head verification: fetched the PR fresh; head is a978c1361d94392372972b734319fe32ee052b26 (matches Justin's a978c13, checked the full 40-char SHA rather than trusting the short form). Checked out pull/79/head and confirmed git rev-parse HEAD matches byte-for-byte. mergeable_state is now clean.
Merge-base / diff-stat: true merge-base (git merge-base HEAD origin/main) is still 9aac699..., exactly the PR's stated base.sha — not stale. git diff --stat against it: 8 files, +119/-21, matching the PR API's reported totals exactly.
Squash sanity check: diffed this head directly against the prior (unsquashed, already-reviewed) second-commit head 9b4ab3e... to see what actually changed content-wise, not just history. The only differences are the two nit fixes — nothing else moved:
connectExitCode'soutcomeparam is now typedHandoffOutcome | null(HandoffOutcome = 'connected' | 'timeout', split out fromConnectOutcome), and the deadoutcome === 'pending'branch is gone — a proper type-level fix, not just a comment.confirmBrowserConnect/browserConnect/connectProviderreturn types updated to match; the now-impossibleconnectExitCode('pending', null)test case was correctly removed.parseAwsRegionsnow rejectsallmixed with specific regions ("...,all"→CLIErrorwith a--regionhint) instead of silently collapsing to "every region";test/cloud-connect-aws-regions.test.tsgained the corresponding case. The interactive-picker-is-single-select gap is now called out explicitly as a conscious decision in the PR body's Decisions section rather than left implicit.ERRORS.md,README.md,skill/SKILL.md,src/errors/codes.ts,test/cloud-connect-aws-background.test.tsare byte-identical to what I already reviewed. No new scope crept in during the squash.
Local checks at this head: npm run lint — 0 errors. npm run typecheck — 0 errors (still fixed). npm run test — 0 failures on every run, but the aggregate count was flaky in this sandbox (372/89 vs 377/90 across repeated runs) — traced this to a Node test-runner concurrency/reporting artifact reproducible on this same box even for the unchanged baseline suites, not a real gap: direct TAP inspection of a "372" run still showed the parseAwsRegions suite (all 5 cases, including the new mixed-region case) executing and passing. Settled it by pulling the actual CI job log for this head's 22.x run: # tests 377 / # suites 90 / # pass 377 / # fail 0, matching the PR body's claim exactly.
CI at this exact head: polled GET /commits/a978c13.../check-runs directly (not cached) — checks / Test & typecheck green on 20.x/22.x/24.x, plus the auto-approve workflow's approve runs showing skipped (expected pre-review).
No new findings. Nothing further to block on.
Generated by Claude Code
There was a problem hiding this comment.
Auto-approved: Claude reviewed this PR and posted an LGTM verdict (see its review). A repo admin enabled this via the auto-approve workflow.
Generated by Claude Code
Prod receipt · polylane CLI 0.2.28
Still human-gated (needs a real AWS account): a live |
1 participant
When
polylane cloud connect --provider awsends while the CloudFormation stack is still creating, the CLI now exits with a new code 7 (PENDING) instead of 0. Scripts and the installer funnel can no longer mistake an unfinished AWS connect for a completed one.What & why
Fixes cli#78, the follow-up from the review of cli#77 (which fixed cli#75); tracked as the cli#78 row on nominal#1511.
The AWS leg of
cloud connecthands the CloudFormation deploy to a background poll and settles it with a bounded foreground wait when the session ends. If the account has not shown up by then,AwsStackWait.settle()returns'pending', prints "AWS is still connecting", and the command returned without touchingprocess.exitCode. Since #77 mademain.tsexit withsettledExitCode(), that path was the last connect outcome still exiting 0 while nothing was connected.Also fixes cli#80: the live spec changed the AWS connect body to
regions: string[] | null(nominal#1376), which brokenpm run typecheckonmainwith no code change and kept CI red on this PR.Changes
Exit code:
src/errors/codes.ts:ExitCode.PENDING = 7.src/commands/cloud/connect.ts:ConnectOutcomesplits intoHandoffOutcome(connected | timeout, what a provider flow returns) pluspending(onlyAwsStackWait.settle()produces it), so the types say which path can carry which value.connectExitCode(outcome, awsOutcome)maps the settled outcomes to an exit code (timeout→ 1, anypending→ 7, else 0). Both exits of the picker loop (a provider flow finished; the user picked Done with AWS still deploying) now setprocess.exitCodethrough it, sosettledExitCode()inmain.tscarries it to the process boundary and telemetry. The "still connecting" message now says the account appears inpolylane cloud listonce the stack finishes and names the exit code.ERRORS.md,README.md,skill/SKILL.md: code 7 added to every exit-code table; ERRORS.md gets a "Pending (exit 7)" category and the browser-flow paragraph now states what the exit code reflects after a wait (0 / 1 / 7) instead of the pre-fix: exit with the code a command set on process.exitCode instead of a hard 0 #77 "URL generation only" wording.test/exit-code.test.ts: unit cases forconnectExitCode, plus the same subprocess shape as #77 assertingmain.tsexits 7 when a command leavesprocess.exitCode = 7.test/cloud-connect-aws-background.test.ts: the pending guidance names the exit code.AWS regions:
src/commands/cloud/connect.ts:parseAwsRegions()turns--regioninto the API'sregionslist: comma-separated values, orall→null(every region enabled on the account, including regions enabled later, the same semantics as the console's "All regions" switch). Mixingallwith specific regions is a usage error rather than a silent widen. The interactive picker offers "All regions" first. Flag help and examples updated.test/cloud-connect-aws-regions.test.ts: single, list,all, mixedall, and empty-value cases.Decisions
GENERALwould make "retry the connect" and "just wait" indistinguishable to callers;TIMEOUT(5) is the HTTP request-timeout category and would mislead the retry hints in ERRORS.md. The semantics are documented as "not connected yet, re-check withlist".settle().startAwsStackWaitstays a pure wait primitive that the existing unit tests drive; the mapping lives in one function besideConnectOutcomewhere both outcomes meet.--region allinstead of a new--all-regionsflag. Keeps one flag for the region choice, works for scripts that already pass--region, and mirrors the picker's first entry.--region us-east-1keeps working unchanged (becomes["us-east-1"]). The interactive picker stays single-select (one region, or All regions); a specific multi-region set is reachable through the flag only. That is a conscious scope call: the picker is for the guided path, scripts compose lists.Validation
connectExitCodereturns 0 for connected legs, 7 when either outcome is pending, 1 when any wait timed out (including timeout + pending)test/exit-code.test.ts"cloud connect exit code" ✅ (red before:does not provide an export named 'connectExitCode')process.exitCode = 7exits the process with 7cloud-connect-aws-background.test.tsassertspolylane cloud list,failed or rolled back,Exiting 7 (pending)✅--regionmaps single, comma-separated, andallvalues; rejects empty andall-mixed-with-regions values with exit 2test/cloud-connect-aws-regions.test.ts5 cases ✅ (red before: missing export; the mixed case returnednullbefore the check existed)npm run test377/377 ✅ ·npm run typecheck✅ (was failing onmain, #80) ·npm run lint✅ ·npm run build+./dist/polylane.mjs cloud connect --helpshows the new--regionhelp ✅echo $?prints 7, thenpolylane cloud listshows the account once created;--region allconnect scans every enabled region🤖 Generated with Claude Code