Repository navigation
Conversation
The Polylane API removed the Conductor integration. The generated client no longer accepts "conductor" as an integration type, so typecheck failed on a fresh codegen. This change removes the Conductor option, its key instructions and its test fixtures. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`polylane cloud connect --provider azure` starts an Azure connection request, prints the Azure portal deployment URL and opens it in the browser. The person selects the subscription and creates the deployment. The command then reads the request every 5 seconds for up to 15 minutes and prints the connected account. Ctrl+C during the wait cancels the request. `--no-alert-rules` stops Polylane from creating Azure Monitor alert rules. `--no-wait` and JSON output print the request ID and the command that checks it later (`--request azureconn_...`), then exit 7. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Polylane could not verify the production impact of this pull request. No cloud resource in the workspace executes coreplanelabs/cli. It is an npm-distributed client CLI, and the repository has no deploy target and no attached issue. The delta at d8f62f9 only makes one transient Azure poll read (timeout) retry on the unchanged 5 s interval, bounded by the 15-minute deadline. Also considered · 2 refuted
Analysed against 8 cloud accounts and 1 repository
Polylane could not find the cloud resources this repository manages, so this review looked at the entire cloud account. Connect this repository to its resources and the next review will focus on exactly what this code deploys to. Polylane analysed |
boristane
left a comment
boristane
left a comment
There was a problem hiding this comment.
One Important finding blocks this PR: the check command that the CLI prints (--request azureconn_...) fails with exit 4 when the new Azure account used the last plan slot. The rest of the flow agrees with the server contract.
Checked against the server (nominal-azure-identity at cdfc445016, routers/azure-connections/index.ts, connects/azure.ts, durable-workspaces/operations/azure-connections.ts):
POST /v1/cloud_accountswithprovider: "azure"returnsrequest,templateUrlanddeployUrl. The CLI readsrequest.requestId,request.expiresAtanddeployUrl, and it narrows onresult.provider. Correct.- The request ID is
azureconn+ 24 characters (packages/util/random/index.ts,len: 24, lowercase base36).^azureconn_[0-9a-z]{24}$agrees. - The GET drives activation (
activatewhenpendingwith a subscription, oractivewithoutcompletedAt) and swallows the 404/409 pending states. Thusstatus === 'active' && completedAt !== nullis the correct ready test, andcancelled/expiredare the correct terminal states. - The DELETE cancels only a
pendingrequest. On anactiverequest it returns the request unchanged, so Ctrl+C after the account connects does not disconnect it. - The dry run:
requestJsonreturns{}, soresumetakes the pending branch andconnectAzurereturns before it reads the result. No crash. - The Conductor removal: no
conductorstays insrc/or in the regenerated client, and typecheck passes on a fresh codegen. This agrees with the PR body.
Fresh local evidence at a04a386 (Node v22, not CI): npm ci, npm run codegen (live spec), npm run typecheck, npm run lint all exit 0. npm test: 632 pass, 0 fail. The diff touches skill/SKILL.md, so I also ran npm run build in a clean env -i: ./dist/polylane.mjs --version prints polylane 0.2.53, and cloud connect --help shows the Azure provider, --no-alert-rules, --no-wait and the three examples. I did not run Node 20 or 24 locally. The diff uses no new Node API.
Findings, by severity:
- Important (
azure-connect.tsresume, inline):cloud connectrunsensureCloudAccountCapacitybefore the provider dispatch, and--requestdoes not skip it. An Azure request can become active on the server without the CLI (the Event Grid handshake activates it). When this account fills the plan, the gate counts it and the check of that same request fails with QUOTA. The class: every path that tells the user to runazureResumeCommand. These are thenextfield of JSON output (JSON runs always exit 7 and hand over this command), the--no-waithint, the timeout hint, the failed-cancel hint, and the SKILL.md text. All of them reach the gate. The GCP--requestpath is not in the class: its resume creates the account, so the gate is correct there. - Nit (
pollAzureRequest, inline): one failed GET (network error, 5xx) during the 15-minute wait stops the wait with a generic error. The request stays pending and the error does not name the check command. - Nit (
cancelAzureRequest, inline): the CLI writes "Cancelled." without reading the returned status. If the account connected between two polls, the DELETE returnsactiveand the message is wrong. - Nit: in
README.md, line 126, the#comment does not align with the column of the lines around it.
| workspaceId: string, | ||
| requestId: string | ||
| ): Promise<AzureOutcome> { | ||
| const request = await api.cloudAccountsAzureRequestsGet(workspaceId, requestId); |
There was a problem hiding this comment.
Important. This read is the check that every Azure hint tells the user to run. But cloudConnectCommand.execute runs ensureCloudAccountCapacity first (connect.ts, the if (getArgBoolean(args, 'reconnect') !== true) block), and --request does not skip it. The Event Grid validation handshake activates the request on the server (azureConnectionsRouter events route, activate in waitUntil), so the account can exist before this command runs. When that account fills the plan (for example, the second account on a 2-account plan), cloudAccountCapacity(plan).atLimit is true:
- In JSON or non-interactive mode, the gate throws
CLIError(limitLine(plan), ExitCode.QUOTA, ...)and the command exits 4. - In interactive mode, it offers an upgrade.
In both cases the request already connected. An agent that follows the next field of the JSON output sees this failure each time the connection was the last slot.
Recommendation: skip the gate when --provider azure and --request are both set. The server already enforces the limit when it activates the request (the capacity callback in registerAzureConnection), so the gate adds nothing for this path. Add a test that runs --request with a plan at its limit and an active request, and expects exit 0 and the connected account.
There was a problem hiding this comment.
Fixed in f51a602. cloud connect now calls needsCloudAccountCapacity(args). It skips the plan gate for --reconnect and for --provider azure --request <id>, because the server checks capacity when it activates the request. GCP's --request still goes through the gate. Test: "checks a started Azure request without the plan capacity gate".
| const now = deps.now ?? Date.now; | ||
| const deadline = now() + timeoutMs; | ||
| for (;;) { | ||
| const request = await api.cloudAccountsAzureRequestsGet(workspaceId, requestId); |
There was a problem hiding this comment.
Nit. One transient failure here (a network error, or a 5xx from the API) stops a 15-minute wait. The error goes to handleError, and it does not name azureResumeCommand. The request stays pending. Recommendation: retry a network or 5xx error on the next interval until the deadline, or catch it in waitForConnection and print the check command, as the timeout path does.
There was a problem hiding this comment.
Fixed in f51a602. pollAzureRequest now reads again on the next interval after a transient error, until the deadline. A transient error is a network error (TypeError from fetch or a CLIError with ExitCode.NETWORK), an ApiError with status 5xx, or a 429. Any other error, such as 403 or 404, still stops the wait. When the deadline passes, the wait prints the check command, as before. Tests: "keeps waiting through a network error or a server error" and "stops on an error that a retry cannot fix".
| process.stderr.write('Cancelling the Azure connection request…\n'); | ||
| try { | ||
| await api.cloudAccountsAzureRequestsCancel(workspaceId, requestId); | ||
| process.stderr.write(`Cancelled. To connect Azure later, run:\n${startAgainCommand(workspaceId)}\n`); |
There was a problem hiding this comment.
Nit. The DELETE cancels only a pending request. For any other status it returns the request unchanged (AzureConnectionsOperations.cancel). If the deployment finished between two polls, the request is active, but this line writes "Cancelled.". Recommendation: read the returned status. If it is active with completedAt, report the connected account (or tell the user to run the check command) instead.
There was a problem hiding this comment.
Fixed in f51a602. cancelAzureRequest now reads the status that the DELETE returns. If the request is active, the CLI says that the deployment finished before the cancel and prints the check command. It no longer prints "Cancelled." in that case. Test: "reports a request that connected before the cancel instead of cancelling it".
…nd keep waiting through transient errors Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review 5835936022 is addressed at f51a602. The Important finding about the capacity gate and the request check, and both inline nits, are fixed with tests. The README comment column is aligned again. Typecheck, lint and 636/636 tests pass on Node 22. |
boristane
left a comment
boristane
left a comment
There was a problem hiding this comment.
LGTM: the four findings of the previous review (5835936022 on a04a386) are fixed, and the delta adds no blocking issue.
Verification of each finding in a04a386..f51a602:
- Important, capacity gate:
needsCloudAccountCapacityreturns false for--provider azure --request <id>and for--reconnect. It returns true for a new Azure connect and for every other provider. The server checks capacity when it activates the request (markActiveinazure-connections.ts, andassertWithinLimitinconnects/azure.ts). Fixed. - Nit, transient poll failure:
pollAzureRequestnow reads again after a 5xx, a 429, aNETWORKCLIError or aTypeError. A 4xx stops the wait. Fixed, with one remaining gap (inline Nit). - Nit, Ctrl+C message:
cancelAzureRequestnow reads the returned status. The servercancelreturns the stored record without change when the request is not pending, so anactiveresult prints the resume command and not "Cancelled.". Fixed. - Nit, README column: aligned. Fixed.
Local checks at f51a602 on Node v22.23.4 (fresh, not cached): npm ci, npm run typecheck (codegen against the live spec), npm run lint and npm test all pass: 636 tests, 0 failures.
Nit (inline): a request timeout still ends the 15-minute wait.
| // wait reads again on the next interval instead of giving up. | ||
| export function isTransientAzurePollError(err: unknown): boolean { | ||
| if (err instanceof ApiError) return err.status >= 500 || err.status === 429; | ||
| if (err instanceof CLIError) return err.exitCode === ExitCode.NETWORK; |
There was a problem hiding this comment.
Nit: src/client/http.ts:110 converts a fetch timeout (TimeoutError) into CLIError('Request timed out', ExitCode.TIMEOUT), and an abort into ExitCode.TIMEOUT too. This check accepts only ExitCode.NETWORK, so one slow read still ends the 15-minute wait. That is the same failure as the previous Nit. Suggestion: also accept ExitCode.TIMEOUT here. Also, http.ts converts every fetch TypeError into a NETWORK CLIError, so the TypeError branch does not occur through the generated client. It is harmless.
There was a problem hiding this comment.
Fixed in d8f62f9. isTransientAzurePollError now also accepts ExitCode.TIMEOUT, so one slow read does not end the wait. The transient-error test now includes a timeout. Typecheck, lint and 636/636 tests pass.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
boristane
left a comment
boristane
left a comment
There was a problem hiding this comment.
LGTM: the delta since f51a602 fixes the earlier nit. A fetch timeout (ExitCode.TIMEOUT) is now a transient poll error, and the test proves it.
Delta reviewed: git diff f51a602..d8f62f9 (2 files, 4 lines).
isTransientAzurePollErrornow acceptsExitCode.TIMEOUTas well asExitCode.NETWORK.src/client/http.tsthrowsExitCode.TIMEOUTforTimeoutError("Request timed out") and forAbortError("Request aborted"). The only signal on that fetch isAbortSignal.timeout, so anAbortErrorthere also comes from the timeout and not from the user. A retry is correct for both.- The retry cannot loop forever:
pollAzureRequestchecks its deadline after each read, failed or not, and returns{ kind: 'timeout' }. - The only caller of
isTransientAzurePollErrorispollAzureRequest(azure-connect.ts:88). No other behavior changes. - The test adds
new CLIError('Request timed out', ExitCode.TIMEOUT)to the failure sequence and expects 5 reads. This matches the new error.
Local checks at d8f62f9 on Node 22.23.4, in a clean worktree: npm ci, npm run codegen (through typecheck), npm run typecheck, npm run lint and npm test (636/636 pass) all pass. The build/smoke leg does not apply because the delta does not touch build.ts, codegen/, skill/SKILL.md or packaging.
No findings.
TL;DR
polylane cloud connect --provider azureconnects an Azure subscription. It starts a connection request, opens the Azure portal deployment, waits until the subscription connects, and prints the account.Why
The Polylane API can connect Azure (
provider: "azure"oncloud_accounts.connect, pluscloud_accounts.azure.requests.getand.cancel). The CLI and the installer had no way to start it.Where to look
connectAzure: the flow, the alert-rules prompt,--no-waitand JSON output, and--requestto check a started request.pollAzureRequest: reads the request every 5 s for up to 15 min. It stops when the status isactiveandcompletedAtis set, and it stops with an error oncancelledorexpired. Ctrl+C cancels the request throughDELETE.connect.ts: provider option, dispatch, flags (--no-alert-rules,--no-wait) and examples.Feedback wanted
Ctrl+C cancels the request, as the brief asked. If the person already created the deployment, the cancel also releases the registration. Tell me if Ctrl+C should only stop the wait.
Risk
Low. The change adds a new provider branch and does not change other providers. The Conductor removal drops an option the API already refuses.
Verified
npm run typecheck,npm run lint,npm test(632 pass),npm run build.polylane cloud connect --helpshows the Azure provider, flags and examples.🤖 Generated with Claude Code