Conversation
UploadsService.Download returned 401 for every upload because it fetched `upload.download_url` unauthenticated, assuming a pre-signed storage URL — but the API returns an authenticated endpoint. Extract the auth'd-hop + 302-follow flow from AccountClient.DownloadURL into a shared Client.fetchAPIDownload helper and route both callers through it. Four existing tests mocked download_url as a bare S3 URL — a shape the API never returns. _Success and _S3Error encoded the same wrong assumption as the bug. SecondLegNoAuth and SecondLegNoTimeout were silently neutralized by the refactor (URL rewriting sent the rewritten URL to the apiServer, skipping their s3Server handlers). Rewrite all four mocks to match the real API and add a secondLegHit assertion so the SecondLeg invariants bite.
The direct-2xx response branch — when bc3 serves blobs directly instead of redirecting to storage — was not exercised by any test through UploadsService.Download, even though the filename-precedence override at vaults.go:1044-1046 runs on that branch. Add TestUploadsService_Download_DirectBody: API-host download_url, download endpoint auth-gated and returns 200 + body (no 302). Uses a URL path filename that differs from the metadata filename so the assertion proves the override is in effect. TestDownloadURL_DirectDownload covers the helper's 2xx branch through AccountClient.DownloadURL, but that path has no filename override.
There was a problem hiding this comment.
Pull request overview
Fixes UploadsService.Download returning 401 by treating upload.download_url as an authenticated API endpoint (Bearer auth + 302 to a signed URL), and reusing the proven AccountClient.DownloadURL flow via a shared helper.
Changes:
- Extract shared authenticated-download flow into
Client.fetchAPIDownloadand routeAccountClient.DownloadURLthrough it. - Update
UploadsService.Downloadto use the authenticated hop + redirect flow and prefer metadata filename over URL-derived filename. - Update and extend tests to reflect real API behavior (auth-required first hop, unauthenticated signed second hop, plus direct-2xx body variant).
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| go/pkg/basecamp/download.go | Adds Client.fetchAPIDownload and routes AccountClient.DownloadURL through it to share the authenticated-download logic. |
| go/pkg/basecamp/vaults.go | Switches UploadsService.Download to use the shared authenticated-download flow and overrides filename from upload metadata. |
| go/pkg/basecamp/download_test.go | Updates regression tests to assert first-hop auth and second-hop behavior (no auth / no timeout) with the new API-shaped download_url. |
| go/pkg/basecamp/vaults_test.go | Updates upload download tests to model the authenticated 302 flow and adds coverage for the direct-2xx body branch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Audit of hand-rolled helpers in
The bug class is narrow. Outside
I reran the non-generated, non-test grep passes for Reference patterns going forward:
|
…xture The authenticated first hop in fetchAPIDownload was a bare apiClient.Do with no retry, so a transient 429/503 or network blip failed immediately. GETs elsewhere in the SDK retry with exponential backoff (see c.doWithRetry); uploads.Download(id) was quietly less resilient than uploads.Get(id). Wrap the hop-1 request in a loop that mirrors doWithRetry: MaxRetries attempts, exponential backoff via c.backoffDelay, Retry-After honored on 429, OnRetry hook fires each retry, re-authenticate per attempt (in case the token refreshed). Retry only on network error or 429/5xx — once the response enters 2xx/3xx dispatch the body belongs to the caller or is being handed to the signed-hop fetch, so retrying there would be unsafe. Not hoisting a shared retry helper yet; doWithRetry is tightly coupled to the JSON-response generated client path. New tests cover all four retry branches: 503 retry, 429 with Retry-After, synthetic network error via a flakyRoundTripper, and 404 no-retry. Also harden the hand-rolled Download tests so they can't drift from the real API response shape the way the original bug did. Add loadUploadFixture in test_helpers_test.go that parses spec/fixtures/uploads/get.json and substitutes the test server's host into download_url, and migrate the Success / S3Error / SecondLegNoAuth / SecondLegNoTimeout tests to consume it. DirectBody stays hand-rolled because it deliberately mismatches URL filename and metadata filename to prove the metadata-filename override. Trim 500 out of TestDownloadURL_APIError's table — the retry loop would burn default backoff (1s+2s+jitter) per entry and the 5xx retry path is already covered by TestDownloadURL_AuthHopRetriesOn503.
The original UploadsService.Download 401 bug survived CI because download had no conformance coverage — the Go convenience method shipped with hand-rolled tests that encoded a shape the API never returns. Add downloads.json codifying five invariants for DownloadURL: auth'd first hop with 302 to a signed URL, direct 2xx body, retry on 503, honor Retry-After on 429, and error on 302-without-Location. The suite is cross-SDK by construction so other runners can adopt it incrementally; only the Go runner is wired now. Ruby / Python / TypeScript runners get skip entries for these five cases with a follow-up note. Parity work on whether those SDKs should expose a convenience uploads.download(id) is still deferred.
|
Folded in the review scope from the plan. Two commits on top of the existing fix: 69e0a40 — Retry
148e127 — Add conformance coverage for the download 2-hop flow
Verification
Not done here
Still deferred per the plan: parity decision for Ruby/Python/TS/Swift, Smithy trait for auth-routable URL semantics, audit of other hand-rolled helpers consuming URLs from API responses, and wiring the other conformance runners for |
Review carefully before merging. Consider a major version bump. |
|
Appreciated, but I don't think any of these rise to "major version bump." Assessing each: 1.
2. Pre-PR This is additive — a failure that used to surface immediately now surfaces after
3. Pre-PR On filename: pre-PR set Version bump SDK is |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
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="conformance/runner/go/main.go">
<violation number="1" location="conformance/runner/go/main.go:493">
P2: Do not ignore `io.Copy` errors when draining download responses; propagate read failures so tests fail on partial/broken downloads.</violation>
</file>
<file name="conformance/tests/downloads.json">
<violation number="1" location="conformance/tests/downloads.json:48">
P2: These retry cases never assert hop-1 Authorization/User-Agent, so a regression that retries the download unauthenticated would still pass.
(Based on your team's feedback about scoping required headers by request type.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- TestDownloadURL_AuthHopRetriesOn503 asserted elapsed >= 2*baseDelay, but two sleeps at exponential backoff total baseDelay + 2*baseDelay, so the looser bound would still pass if the second delay regressed to baseDelay. Require 3*baseDelay total. - Propagate io.Copy errors when draining download bodies in the conformance runner so partial/broken downloads surface instead of silently passing. - Assert hop-1 Authorization on the retry conformance cases. The happy-path case already does this; retry cases were missing it, so a regression that issued the first attempt unauthenticated could have slipped through.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The Kotlin conformance runner had no name-based skip list — it relied on tag matching and runtime exception handling. With downloads.json in play it went straight to the operation dispatcher's "Unknown operation" throw and failed all 5 cases. Add a KOTLIN_SKIPS map mirroring the Ruby/Python/TypeScript runners. Parity wiring for DownloadURL in the Kotlin dispatcher is deferred; this keeps CI green in the meantime.
github-actions
Bot
added
kotlin
and removed
breaking
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Narrow the retryable-status set to 429 / 502 / 503 / 504 (plus transport errors), matching Client.singleRequest's @retryable markings. Previously retried all 5xx, so downloads retried on 500s even though the main GET path surfaces those immediately via ErrAPI(500, ...). - Drain error bodies up to MaxErrorBodyBytes before Close() on the retry branch so the TCP connection can return to the keep-alive pool ahead of the next attempt. checkResponse only consumes the first maxErrorMessageLen*2 bytes for the error message; the rest goes to io.Discard. - Rewrite the loop header comment to describe what the code actually does on MaxRetries<=0 (skip + ErrUsage fallback) and on exhaustion (return last per-attempt error directly) instead of claiming bug-for-bug parity with doRequestURL's error shape.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Update the retry doc comment to say 429/502/503/504 (and note 500+ surface without retry), matching the actual retryable set after aligning with Client.singleRequest. - Collapse the resp==nil fallback to just the ErrUsage path; exhaustion after real attempts is already handled inside the loop via `return nil, lastErr`, so the %w branch was unreachable. - Restore 500 to TestDownloadURL_APIError's table now that the retry loop no longer delays it. Assert only one request was made so a future regression that broadens the retry set would surface here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The happy-path conformance test already asserted Authorization was present on the first hop, but didn't verify the signed-URL hop stayed unauthenticated — the exact invariant the original UploadsService.Download bug violated in production. Catching that cross-SDK is directly on- mission for this suite. - conformance/schema.json: add headerAbsent to the assertion enum and an optional index field (0-based; negative indexes count from the end). - conformance/runner/go/main.go: add an Assertion.Index field, a requestHeadersAt helper, and a headerAbsent case. Thread the index through existing headerPresent/headerInjected so either can target any captured request (default index 0 preserves prior behavior). - conformance/tests/downloads.json: on the three 2-hop cases (happy path, 503 retry, 429 retry), assert Authorization is present on request 0 and absent on request -1 (the unauthenticated signed-URL hop). Validated the new assertion catches a real regression: temporarily re-authenticated fetchSignedDownload — the three 2-hop cases all FAILed with "Expected header Authorization absent on request index -1, got Bearer ...", then reverted.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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="conformance/runner/go/main.go">
<violation number="1" location="conformance/runner/go/main.go:731">
P2: `headerAbsent` uses `headers.Get(...) == ""`, which cannot distinguish a missing header from a present header with an empty value; this can produce false passes.
(Based on your team's feedback about distinguishing absent vs empty values when using `Get`-style accessors.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MaxRetries: - NewClient panics on MaxRetries<1, matching the MaxPages<=0 pattern. Zero attempts is always a misconfiguration, and both retry loops can now assume >=1 without divergent guarding. WithMaxRetries and HTTPOptions.MaxRetries docs updated to note the floor. The conformance runner's panic-string check picks up the new message. - Keep fetchAPIDownload's resp==nil fallback as defense-in-depth for direct-struct-built Clients that bypass NewClient. 500 error metadata alignment: - In fetchAPIDownload's non-retry dispatch, emit ErrAPI(500, "Server error (500)") directly for 500 instead of delegating to checkResponse. checkResponse currently marks all 5xx Retryable=true, which contradicts the fact that this loop (and Client.singleRequest) intentionally does not retry 500. Other statuses still go through checkResponse. - Update the fetchAPIDownload docstring to describe what the code actually does: retry set matches singleRequest's @retryable; 500 mirrors singleRequest's non-retryable ErrAPI; other non-retried statuses flow through checkResponse. Conformance runner headerAbsent: - Use Values() instead of Get() so a header present with an empty value fails the absence assertion. Get conflates missing and empty.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Redirect drain now reads up to MaxErrorBodyBytes (1 MB) instead of 4 KB. net/http requires reading to EOF for connection reuse; a 4 KB cap left larger redirect bodies unreusable. The 1 MB cap still guards against unbounded reads. - Retry branch now reads only the maxErrorMessageLen*2 prefix that checkResponse actually uses for the error message, then io.Discard-s the remainder up to MaxErrorBodyBytes to enable keep-alive. Previously allocated up to 1 MB per retry even though only ~1 KB was consumed. - Align the NewClient doc-header description of WithMaxRetries with the panic-on-<1 validation added in a89afd2: now says "Total attempt count for GET (default: 3, minimum 1)" to match WithMaxRetries and HTTPOptions.MaxRetries.
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Marks Upload.download_url and CampfireLineAttachment.download_url as API-host URLs that require the two-hop authenticated-then-signed fetch rather than pre-signed external URLs. The trait's /// block carries the full consumer contract (hop mechanics, test-fidelity requirements, retry semantics, pagination exemption, public-vs-internal primitive division) so hand-rolled helpers can implement the flow without reading SDK source. Guards regression with scripts/check-auth-routable-consumers.sh — a call-site deny-list that fails if fetchSignedDownload is invoked outside go/pkg/basecamp/download.go. After PR #278, the only legitimate caller is fetchAPIDownload, which runs the authenticated first hop before the signed fetch; any other caller is either re-inventing the two-hop flow or skipping hop 1. Wired into make check via auth-routable-check. Fixes the TypeScript uploads test fixture that still used example.com, so the spec-shape-only test no longer teaches the wrong URL shape. openapi.json diff is exactly two new x-basecamp-auth-routable-url: {} entries on the Upload and CampfireLineAttachment download_url property schemas; no generator in any SDK consumes field-level x-basecamp-* extensions, so no downstream code churn.
Marks Upload.download_url and CampfireLineAttachment.download_url as API-host URLs that require the two-hop authenticated-then-signed fetch rather than pre-signed external URLs. The trait's /// block carries the full consumer contract (hop mechanics, test-fidelity requirements, retry semantics, pagination exemption, public-vs-internal primitive division) so hand-rolled helpers can implement the flow without reading SDK source. Guards regression with scripts/check-auth-routable-consumers.sh — a call-site deny-list that fails if fetchSignedDownload is invoked outside go/pkg/basecamp/download.go. After PR #278, the only legitimate caller is fetchAPIDownload, which runs the authenticated first hop before the signed fetch; any other caller is either re-inventing the two-hop flow or skipping hop 1. Wired into make check via auth-routable-check. Fixes the TypeScript uploads test fixture that still used example.com, so the spec-shape-only test no longer teaches the wrong URL shape. openapi.json diff is exactly two new x-basecamp-auth-routable-url: {} entries on the Upload and CampfireLineAttachment download_url property schemas; no generator in any SDK consumes field-level x-basecamp-* extensions, so no downstream code churn.
Step 5 previously required test stub URLs to use the configured API base host. After rebasing onto main, that conflicts with two already- merged conventions: - Go's `download_test.go` (PR #278) uses `storage.3.basecamp.com` for ~16 primitive-level tests so the SDK's host-rewrite step is visible in test setup. - The TS `UploadsService.download` test added by PR #281 follows the same pattern at the service level. Both correctly assert the auth-hop sequence (Bearer on hop 1, none on hop 2), which is the actual load-bearing guard against the PR #278 bug shape — that bug passed because the test never asserted the auth hop fired, not because of the host choice. Relax step 5 to make the auth-hop assertion the explicit MUST, document both URL host conventions as acceptable (with API-host preferred for fixture-shape mirroring), and keep the schema-shape-only exemption.
Problem
Every call to
UploadsService.Downloadreturned 401. SDK users couldn'tdownload any uploaded file.
ref: https://app.basecamp.com/2914079/buckets/27/card_tables/cards/9812641548
Solution
Authenticate the download URL.
upload.download_urlis an authenticatedendpoint (requires Bearer auth, 302-redirects to a signed URL), not a
pre-signed URL as the old code assumed.
Why this fix
AccountClient.DownloadURLalready implemented the correct flow forauthenticated storage URLs and continued to work in production. Extracting
that logic into a shared
Client.fetchAPIDownloadhelper and routingUploadsService.Downloadthrough it reuses proven code and keeps the twomethods in sync.
In-PR scope additions (review cycle)
fetchAPIDownloadnow retries on networkerrors and 429/502/503/504 (matching
Client.singleRequest's@retryableset), with exponential backoff andRetry-Afteron 429.500 is surfaced non-retryable, mirroring
singleRequest.MaxErrorBodyBytesbeforeClose()so connections can return to thekeep-alive pool.
fetchAPIDownloadnow rejectsnon-absolute / non-http(s) URLs itself, so
UploadsService.Downloadis guarded against malformed
download_urlvalues from API responsesin addition to the existing
AccountClient.DownloadURLcheck.loadUploadFixtureseedshand-rolled tests from
spec/fixtures/uploads/get.jsonso the APIresponse shape can't silently drift from what the server actually
returns — the failure mode that let the original bug ship.
conformance/tests/downloads.jsoncodifies the 2-hop invariant (auth'd first hop, unauthenticated signed
hop, retry shape) cross-SDK. Go runner is wired; Ruby/Python/TS/Kotlin
runners skip these cases with a follow-up note. The suite asserts
Authorizationpresent on hop 1 and absent on hop 2 — catches theexact regression pattern this PR fixes.
Potentially breaking
NewClientnow panics onMaxRetries < 1(previously< 0). Zerowas already broken before this PR:
doRequestURLran zero iterationsand returned a malformed
%!w(<nil>)error;fetchAPIDownloadhitits
ErrUsagefallback. The panic surfaces the misconfiguration atconstruction instead of hiding it. No known caller sets 0.
Additional info
Manually verified against live Basecamp:
send_filereturns 200 in one hop).