Repository navigation
Conversation
There was a problem hiding this comment.
AI Review Summary
Small, well-scoped change: bump the poll timeout to 18 minutes, move the deadline check to the top of the loop so the final GET actually runs after the deadline, keep the target error code through sanitization, and switch the timeout failure to InternalError. The logic reads correctly and the tests cover both terminal and non-terminal final polls. No blocking issues found.
- 🔵 1 suggestion(s)
Review Scope
- Mode: Full pull request
- Range:
Full pull request through 3979979063b6a9789a8edb54227930ff796449f3 - Current range coverage: Complete
- Cumulative pull request coverage: Complete
This review is complete for the current scope. Addressing all critical and warning findings above satisfies the review; suggestions are optional. Request another review only if subsequent changes introduce new behavior or materially expand the pull request's scope.
|
/ai-review |
There was a problem hiding this comment.
Incremental change swaps ?? for || in the target-error message fallback and adds a test for the empty-message case, exactly addressing the prior review's suggestion. No regressions spotted — the code check already guards against empty strings. Nothing further to flag.
Review Scope
- Mode: Incremental
- Range:
3979979063b6a9789a8edb54227930ff796449f3...eea6cbd6f0110bc6beb0641769e93f3c568bd60f - Current range coverage: Complete
- Cumulative pull request coverage: Complete
This review is complete for the current scope. Addressing all critical and warning findings above satisfies the review; suggestions are optional. Request another review only if subsequent changes introduce new behavior or materially expand the pull request's scope.
Description
Linear ticket: n/a
Part of hardening "stuck builds look like a hang" on the sdk-gen-api remote generation path. A companion sdk-gen-api PR makes server-side build timeouts return a target error
{ code: "FERN_BUILD_TIMEOUT", message, retryable? }. This PR makes the CLI wait long enough to receive that error and show its code.Changes Made
POLL_TIMEOUT_MSgoes from 15 to 18 minutes. The old value matched sdk-gen-api's 15-minute build timeout, so the CLI gave up at the same moment the server would have reported the real failure.Timed out waiting for sdk-gen-api build <id> after 18 minutesand codeCliError.Code.InternalErrorinstead ofNetworkError.error.code. Before this change,sanitizeFernBuildStatusdropped it. A failed target now shows<code>: <message>, then falls back to just the message, then tosdk-gen-api generation failed.NetworkError.packages/cli/cli/changes/unreleased/sdk-gen-api-poll-timeout.yml(fix).Instructions on how to review this PR
executeFernSdkGenApiBuildinpackages/cli/generation/remote-generation/remote-workspace-runner/src/fernSdkGenApi.ts). Review this closely. The deadline check moved from after the GET to the top of each iteration (isFinalPoll). This guarantees that the last GET starts at or after the deadline. That GET still goes through the missing-target check, log flushing, and the terminal check. We fail only if it is not terminal. I chose this over a separate post-loop GET helper because it does not duplicate the request, sanitizing, and log-handling code, and it does not send two GETs back to back. Things to check:isFinalPolliteration always returns, so there is never more than one GET after the deadline.NetworkErrorcatch.CliError.Codehas no timeout-specific code.InternalErroris the closest fit. With the 3-minute margin, hitting this path means sdk-gen-api did not enforce or report its own 15-minute timeout, which breaks the service contract. The same file already usesInternalErrorfor other contract violations ("response did not contain target", "build ended with status X while target remained Y"). It is also reported to Sentry, which we want for a real stuck build.ContainerErroris used for failures the server reports for generation or publication, and it is not reported to Sentry, so a hang that nobody explains would go unnoticed. If CLI-side Sentry noise for a backend fault is a concern, please say so.formatFernSdkGenApiTargetErrorandsanitizeFernBuildStatus).codeis kept only when it is a string, and it is redacted the same way asmessage. An empty code falls back to the message alone. TheFernBuildStatustype gainserror.code?: string.src/__test__/fernSdkGenApi.test.ts, the two newdescribeblocks at the end of the file). Tests are a secondary part of this PR.sdk-gen-api build status pollinguses fakesetTimeout/Datetimers and moves time forward 18 minutes:CliError.Code.InternalError.sdk-gen-api target failure messagecovers three cases: code with message, message only, and no error (default text).Testing
pnpm turbo run compile --filter @fern-api/remote-workspace-runnerpasses (50/50 tasks)pnpm turbo run test --filter @fern-api/remote-workspace-runnerpasses (39 files, 655 tests)biome check --error-on-warningson the changed files: clean.prettier --checkon the changelog file: clean.