fix(deploy): surface API status code in latest-revision failure#21
Conversation
Mixpanel showed a steady "Could not fetch latest revision" bucket on v0.18.x (6 hits last week) with no way to tell 404 (repo not synced) from 5xx (transient API) — telemetry's SanitizeErrorMessage keeps only the first line of err.Error(), so the status code that was buried in the Hint never reached the dashboard. - Embed (api: %d) in the Message itself when the primary API error is *sdk.APIError so the status code survives sanitization. - Tailor the Hint by status: 404 points at repo configuration/sync; 5xx points at a transient API issue. - Add regression tests that mirror SanitizeErrorMessage's first-line behaviour to lock down where the status code must appear. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR improves error reporting in the deployment command's ChangesError Handling Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 122336dfe8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if errors.As(primaryErr, &apiErr) { | ||
| message = fmt.Sprintf("Could not fetch latest revision (api: %d)", apiErr.StatusCode) | ||
| switch { |
There was a problem hiding this comment.
When /repository/latest_revision returns an API error other than 404 or 5xx (for example 401/403 auth, 422 validation, or 429 rate limiting) and the deployments fallback has no usable revision, this branch now consumes all *sdk.APIErrors but leaves the generic Specify a revision hint unchanged. That regresses the previous output, which included primaryErr and told the user the actual cause; e.g. invalid credentials now show only Could not fetch latest revision (api: 401), and suggesting --revision will not fix the auth failure. Add a default case that preserves the API error text or tailor the remaining statuses.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/commands/deploy.go (1)
137-157: ⚡ Quick winStatus code correctly embedded in Message for telemetry.
The implementation properly addresses the telemetry requirement by placing the API status code in the
Messagefield (line 144), ensuring it survivesSanitizeErrorMessage's first-line extraction. The use oferrors.Asand status-specific hints (404 → config, 5xx → transient) is correct.Minor: Consider adding an explicit `default:` case
While the current logic is correct (hint is already initialized on line 141), an explicit default case would make the intent clearer for other status codes (e.g., 400, 403):
switch { case apiErr.StatusCode == http.StatusNotFound: hint = "The project's repository may not be configured or synced yet. Check `dhq repos show -p <project>`, or specify --revision <sha>." case apiErr.StatusCode >= 500: hint = "DeployHQ API is having trouble. Try again in a moment, or specify --revision <sha>." + default: + // Use the generic hint initialized above (line 141) }🤖 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 `@internal/commands/deploy.go` around lines 137 - 157, Add an explicit default branch to the switch on apiErr.StatusCode within the block that sets message/hint (the variables message, hint, and apiErr in deploy.go) so that for non-404/non-5xx status codes you clearly preserve or reassign the generic hint (e.g., "Specify a revision with --revision <sha>") instead of relying on the initialized value; update the switch in the code handling primaryErr to include case apiErr.StatusCode == http.StatusNotFound, case apiErr.StatusCode >= 500, and a default: that sets hint to the generic guidance.internal/commands/deploy_test.go (1)
508-574: ⚡ Quick winExcellent test coverage for telemetry-aware error messaging.
The table-driven test correctly validates that status codes appear on the first line of
err.Error()(mirroringSanitizeErrorMessagebehavior on line 567) and that hints are tailored by failure mode. The use ofhttptest.NewServerfollows coding guidelines.Recommended: Add test case for non-APIError path
The current tests cover
*sdk.APIErrorresponses (404, 500, 503), but lines 151-153 indeploy.gohandle the case whenprimaryErris not anAPIError(e.g., network timeout, DNS failure). Consider adding a test case that triggers this path to ensure the fallback hint formatting is correct:{ name: "non-API error → generic hint", // Simulate network-level error by closing server before request wantInMessage: "Could not fetch latest revision", wantInHintPart: "API error:", }This would strengthen confidence that all error paths produce the expected telemetry output. Based on learnings: Use
httptest.NewServerfor recorded response shapes.🤖 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 `@internal/commands/deploy_test.go` around lines 508 - 574, Add a table-driven test case to TestResolveLatestRevision_EmbedsStatusCodeInMessage that exercises the non-*sdk.APIError* branch in resolveLatestRevision (the primaryErr != APIError path handled in deploy.go around the primaryErr checks) by causing a network-level failure (e.g., create an httptest.NewServer and close it before calling newTestSDKClient/resolveLatestRevision or use an unreachable server URL), then assert err.Error() contains the generic leading message ("Could not fetch latest revision" or whatever the first-line text from resolveLatestRevision is) and that the hint contains "API error:" (or the exact hint text produced in that branch) so the test verifies both first-line message and hint formatting for non-API errors.Sources: Coding guidelines, Learnings
🤖 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.
Nitpick comments:
In `@internal/commands/deploy_test.go`:
- Around line 508-574: Add a table-driven test case to
TestResolveLatestRevision_EmbedsStatusCodeInMessage that exercises the
non-*sdk.APIError* branch in resolveLatestRevision (the primaryErr != APIError
path handled in deploy.go around the primaryErr checks) by causing a
network-level failure (e.g., create an httptest.NewServer and close it before
calling newTestSDKClient/resolveLatestRevision or use an unreachable server
URL), then assert err.Error() contains the generic leading message ("Could not
fetch latest revision" or whatever the first-line text from
resolveLatestRevision is) and that the hint contains "API error:" (or the exact
hint text produced in that branch) so the test verifies both first-line message
and hint formatting for non-API errors.
In `@internal/commands/deploy.go`:
- Around line 137-157: Add an explicit default branch to the switch on
apiErr.StatusCode within the block that sets message/hint (the variables
message, hint, and apiErr in deploy.go) so that for non-404/non-5xx status codes
you clearly preserve or reassign the generic hint (e.g., "Specify a revision
with --revision <sha>") instead of relying on the initialized value; update the
switch in the code handling primaryErr to include case apiErr.StatusCode ==
http.StatusNotFound, case apiErr.StatusCode >= 500, and a default: that sets
hint to the generic guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e736157c-532e-42ec-824d-64091be6a3f0
📒 Files selected for processing (2)
internal/commands/deploy.gointernal/commands/deploy_test.go
The previous version of the switch on apiErr.StatusCode handled 404 and 5xx with tailored hints but fell through to the generic "Specify a revision with --revision <sha>" for everything else. That regresses the pre-PR-#21 behaviour for 401/403/422/429: callers now see e.g. "Could not fetch latest revision (api: 401)" with a "specify a revision" hint, when the real fix is to re-auth. - Add a default branch that embeds primaryErr's text in the hint, matching the non-APIError fallback below it. - Extend the table test with 401 and 422 cases that assert the API error text survives into the hint (e.g. "deployhq api: 401", "validation failed"). Codex + CodeRabbit both flagged this on the PR review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Mixpanel keeps showing a steady "Could not fetch latest revision" bucket on
dhq deploy(6 hits in the last 7d on v0.18.x). Today we can't tell from telemetry whether those failures are 404 (project repo not synced yet), 5xx (transient API), or something else —telemetry.SanitizeErrorMessagekeeps only the first line oferr.Error(), so the status code that lived in the Hint was getting stripped before it reached the dashboard.internal/commands/deploy.go— when the primary/repository/latest_revisioncall returns an*sdk.APIError, embed(api: <code>)in theUserError.Messageitself so it survives sanitization. Tailor the Hint by status: 404 → "repository may not be configured or synced; checkdhq repos show"; 5xx → "DeployHQ API is having trouble, try again or specify--revision".internal/commands/deploy_test.go— table-driven test for 404 / 500 / 503 that asserts on the first line oferr.Error()(mirroring what telemetry does), so a regression putting the status code back into the Hint fails the test. Also covers the existing deployment-list fallback path so the new branch doesn't silently bypass it.Why the assertion is shaped this way
Naively asserting
err.Error()contains(api: 404)would also pass if the status code were only in the Hint, becauseUserError.Error()concatenates Message and Hint. The first-line assertion is what telemetry actually consumes.Test plan
go test ./...— all packages greengolangci-lint run ./...— 0 issuesgit stashondeploy.goproduces"Could not fetch latest revision" does not contain "(api: 503)")Follow-ups (deliberately out of scope)
SanitizeErrorMessagestill drops the Hint entirely; the broader fix needs a redaction-policy decision (see the existing memory note on hostname/account-name handling). This PR is the minimum to make the existing bucket actionable.🤖 Generated with Claude Code
Summary by CodeRabbit