Run DownloadURL conformance in Python, Ruby, and TypeScript#280
Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands the conformance runners (Python, Ruby, TypeScript) to execute the DownloadURL operation live, aligning those runners with the Go runner’s existing coverage and ensuring the mock infrastructure can handle the redirect “second hop” behavior.
Changes:
- Add
DownloadURLdispatch to the TypeScript runner and wire it to the SDK’sdownloadURLAPI. - Add
DownloadURLdispatch to the Ruby and Python runners, passing through the rawpathfrom each conformance test case. - Update runner skip lists/reasons so the non-retry
downloads.jsoncases run live while the retry semantics remain capability-skipped.
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 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| conformance/runner/typescript/runner.test.ts | Dispatches DownloadURL and updates TypeScript skip reasons to only skip retry-specific cases. |
| conformance/runner/ruby/runner.rb | Dispatches DownloadURL, threads path through the operation mapper, and adjusts mocking to accommodate redirect hop behavior. |
| conformance/runner/python/runner.py | Dispatches DownloadURL, threads path through the operation mapper, and adjusts respx routing to accommodate redirect hop behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: bbf4da994a
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 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.
1 issue found across 2 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/python/runner.py">
<violation number="1" location="conformance/runner/python/runner.py:194">
P2: The DownloadURL mock is pinned to `https://3.basecampapi.com`, which breaks matching for DownloadURL tests that use `configOverrides.baseUrl`.</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 3 out of 3 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ddb5f7541
ℹ️ 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".
jeremy
force-pushed
the
download-conformance
branch
from
3ddb5f7 to
c7e6436
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 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.
jeremy
force-pushed
the
download-conformance
branch
from
580ce47 to
3c3603a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 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.
Wire DownloadURL dispatch into the three non-Go runners and route the mock catch-all so the relative second hop matches. Three of the five downloads.json cases now run live (302 to signed URL, direct 2xx body, no-Location protocol error). Two retry cases stay skipped, but with capability-based reasons: each SDK's download path (Python's get_no_retry, Ruby's http.get_no_retry, TS's raw fetch bypassing the retry middleware) doesn't yet implement 5xx / Retry-After retry, so unskipping would guarantee red. Go runner is unchanged.
Tighten Python and Ruby DownloadURL mock routes from "any URL on this method" to "any URL on https://3.basecampapi.com". The catch-all has to remain path-flexible because hop 2 lands on a relative Location resolved against the rewritten first-hop URL, but it doesn't have to be host-flexible. A misroute to a different origin now fails instead of silently consuming a queued response.
Address review feedback on the catch-all routing and the TS dispatcher: - Python and Ruby DownloadURL routes now derive their host from the active test client's base URL (configOverrides.baseUrl when set, else the default https://3.basecampapi.com). A future DownloadURL case with configOverrides would otherwise route through a different origin and the mock would silently not match. - TypeScript DownloadURL dispatch now throws if tc.path is empty rather than silently concatenating "undefined" into the synthetic URL. The TestCase interface marks path as optional, so the type system doesn't catch this.
The base branch added a headerAbsent assertion type with an optional index field (0-based; negative counts from end), so the happy-path DownloadURL case can now assert Authorization is present on the auth'd hop and absent on the unauthenticated signed-URL hop. Mirror the Go runner: thread index through headerPresent/headerInjected (default 0 preserves prior behavior) and add headerAbsent in Python, Ruby, and TypeScript.
The DownloadURL mock route is origin-wide so hop 2's relative-resolved URL is served, but a regression that misroutes hop 1 to a different path on the same origin would otherwise pass silently — downloads.json doesn't have a requestPath assertion. Add a runner-level invariant in Python, Ruby, and TypeScript: for the DownloadURL operation, the first captured request's path must equal tc.path. Path correctness on hop 1 is implicit to the operation, so it belongs in the runner rather than the shared spec.
- Python and Ruby DownloadURL dispatch now raises if path is empty, matching the TypeScript runner. An empty path would silently produce the bare host URL "https://storage.3.basecamp.com" and fail later in a less obvious way. - TypeScript: explicitly void result.body.cancel() and attach a no-op .catch() to suppress any unhandled-promise-rejection from the discarded Promise. Fire-and-forget semantics are unchanged.
jeremy
force-pushed
the
download-conformance
branch
from
e5a8538 to
4e7ef30
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 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.
Summary
DownloadURLin the Python, Ruby, and TypeScript conformance runners; route the mock catch-all so the relative second hop matches.downloads.jsoncases now run live in those runners (302 → signed URL, direct 2xx body, no-Location protocol error).get_no_retry, Rubyhttp.get_no_retry, TS rawfetchbypassing the retry middleware). Unskipping today would guarantee red.Notes
fix-uploads-download-auth).downloads.jsonand the previous "runner does not yet dispatch" skip stubs were introduced there. Base is set accordingly; once Fix 401 on file downloads #278 lands, this can rebase ontomainautomatically.body.cancel()deviation: fire-and-forget, not awaited. Awaiting MSW's mockedReadableStream.cancel()hangs past vitest's 5s test timeout. Matches the TS SDK's own download tests attypescript/tests/download.test.ts. Comment in place atconformance/runner/typescript/runner.test.ts:223.Test plan
cd conformance/runner/go && go run .— 66 pass / 0 fail / 0 skip (5 download cases live)cd conformance/runner/python && uv sync && uv run python runner.py— 63 pass / 0 fail / 3 skip (3 download live, 2 retry capability skip + 1 unrelatedmaxItems)cd conformance/runner/ruby && bundle install && ruby runner.rb— 58 pass / 0 fail / 8 skip (3 download live, 2 retry capability skip + 6 unrelated)cd conformance/runner/typescript && npm ci && npm test— 62 pass / 0 fail / 4 skip (3 download live, 2 retry capability skip + 2 unrelated)conformancejob in.github/workflows/test.ymlinvokes all four runners green