SDKs conform to SPEC §9: credential-bearing values are never rendered - #837
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
typescript
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
8222509 to
1305034
Compare
There was a problem hiding this comment.
Pull request overview
Updates all six SDKs toward SPEC §9 credential-rendering protections and closes related §6 error-mapping gaps.
Changes:
- Projects credential-bearing download URLs and sanitizes transport errors.
- Standardizes HTTP fallback messages and extracts
error_descriptionhints. - Adds cross-SDK conformance tests and specification updates.
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 41 out of 41 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
SPEC.md |
Defines §6 and §9 behavior. |
typescript/src/download.ts |
Sanitizes download URLs and errors. |
typescript/src/errors.ts |
Standardizes fallback messages. |
typescript/tests/download.test.ts |
Tests download credential handling. |
typescript/tests/errors.test.ts |
Tests HTTP fallbacks. |
swift/Sources/Basecamp/BasecampError.swift |
Standardizes API messages. |
swift/Sources/Basecamp/HTTP/HTTPClient.swift |
Projects download errors and URLs. |
swift/Tests/BasecampTests/DownloadTests.swift |
Tests credential handling. |
swift/Tests/BasecampTests/ErrorTests.swift |
Tests fallback messages. |
ruby/lib/basecamp.rb |
Extracts error hints. |
ruby/lib/basecamp/api_error.rb |
Propagates hints. |
ruby/lib/basecamp/client.rb |
Sanitizes signed-download failures. |
ruby/lib/basecamp/http.rb |
Projects hook URLs and hints. |
ruby/lib/basecamp/rate_limit_error.rb |
Adds body-derived hints. |
ruby/test/basecamp/download_test.rb |
Tests download projections. |
ruby/test/basecamp/errors_test.rb |
Tests hint extraction. |
ruby/test/basecamp/http_test.rb |
Tests runtime error mapping. |
python/src/basecamp/_async_http.py |
Projects async hook URLs. |
python/src/basecamp/_http.py |
Projects synchronous hook URLs. |
python/src/basecamp/_security.py |
Adds URL display projection. |
python/src/basecamp/async_auth.py |
Severs parser context. |
python/src/basecamp/auth.py |
Severs parser context. |
python/src/basecamp/download.py |
Sanitizes signed-download failures. |
python/src/basecamp/errors.py |
Extracts and truncates hints. |
python/src/basecamp/oauth/device.py |
Removes retained parser contexts. |
python/src/basecamp/oauth/exchange.py |
Removes retained parser contexts. |
python/tests/oauth/test_device.py |
Tests device-response secrecy. |
python/tests/oauth/test_exchange.py |
Tests exchange-response secrecy. |
python/tests/test_auth.py |
Tests refresh-response secrecy. |
python/tests/test_download.py |
Tests URL and error projections. |
python/tests/test_errors.py |
Tests hints and fallbacks. |
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt |
Sanitizes download behavior. |
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/ErrorParsing.kt |
Standardizes fallback messages. |
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/services/BaseService.kt |
Updates error parser invocation. |
kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt |
Tests credential handling. |
kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/ErrorTest.kt |
Tests fallback messages. |
go/pkg/basecamp/download.go |
Projects download errors and URLs. |
go/pkg/basecamp/download_test.go |
Tests download secrecy and classification. |
go/pkg/basecamp/helpers.go |
Standardizes API fallback messages. |
go/pkg/basecamp/helpers_test.go |
Tests fallback behavior. |
go/pkg/basecamp/http.go |
Projects download observability URLs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
1305034 to
b07cc6e
Compare
![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: 822250908b
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…#788) Four surfaces across the six SDKs: Download transport errors. Hop-1 and hop-2 network failures now carry fixed messages ("Network error" / "Download failed"), and the transport error — whose rendering is the signed or rewritten URL — is never chained. Where a classifier legitimately reads the chain, a peer-free sentinel is projected in its place: Go chains the bare context.Canceled/DeadlineExceeded so shouldTripCircuit and callers' errors.Is still classify, Swift a fresh CancellationError()/URLError(.cancelled) so isCancellation still matches. Kotlin and TypeScript rethrow cancellation raw before the wrap, so their chains are simply severed. Pre-dial signed-URL validation. Ruby's undialable-Location refusal and Swift's download Invalid URL guards render the parsed origin, or the fixed token "unparsable" — never the input, which is the same credential the dial would have carried. Python raising boundary. "from None" suppresses the rendering but leaves the JSONDecodeError — whose .doc retains the whole credential-bearing body — in __context__. The credential sites (token refresh sync+async, OAuth exchange, device authorization, device token poll, hop 2) now construct the error inside the except and raise after it; tests assert __context__ is None on the RAISED exception. Hop-1 hook URL. downloadURL accepts any absolute URL and §14 step 2 preserves its query through the origin rewrite, so a signed storage URL smuggles its signature into hop 1. All six SDKs now render hop-1 request hooks (start/end/retry — and Go's debug log line) as origin+path only, with no query or fragment; the wire request keeps the query. API-path hooks are untouched: those URLs carry no credential (§9). Plus the two §6 gaps found while checking the caps: Step 5: the fallback message is the fixed code-bearing phrase "Request failed (HTTP {status})" in TypeScript, Kotlin, Go's checkResponse and Swift — never the wire reason phrase (absent under HTTP/2, blank for an unregistered 599) or Apple's localized status table. Ruby, Python and Go's raw client path already rendered it. Step 3: Ruby and Python now extract error_description as the hint on the API path (only their OAuth paths did), truncated per §9, with class-constant hints (AuthError, ForbiddenError) filling in only when the body carries none and Retry-After still winning on 429.
…sport chains, tighten the origin projections Review of #837 found the hop-1 projection incoherent as landed: the thrown error was fixed while the same raw transport error still reached on_request_end/on_retry (all six SDKs) and stayed chained on Ruby's and Python's shared request path. A download-marked hop 1 now projects every argument a hook or caller sees — Go's loggingTransport RequestResult.Error, TypeScript's emit seams (status retries keep their status error), Kotlin's request-end result, Swift's retry directive and the transport-.network passthrough at both hops, Ruby's and Python's single-request boundary (raised outside the rescue/except so the implicit cause is severed too). Ordinary API requests are untouched; the projection is gated on the download flow. SPEC §9 now says so, and its "does not restrict what hooks receive" sentence names this one exception. Python's OAuth transport failures (token refresh sync+async, exchange, device authorization and poll) chained the httpx error, which retains the request whose form body is the table's last row (client_secret, refresh_token, device_code) — the same raising boundary now applies there. Origin projections keep a non-default port (Ruby, Swift) and bracket an IPv6 host (Swift); hook-URL projections drop userinfo (Ruby, Python). TypeScript's Location resolution is now a pre-dial boundary: a Location that fails URL construction renders `unparsable`, since Node's ERR_INVALID_URL retains its input. §6 adjacents in Python: 401/403 messages are truncated like every other arm, and an unmapped 5xx (599) is retryable per step 12; the body is parsed once for message and hint. Tests: hook error projection asserted in every SDK; Go's hook test uses an atomic and asserts hook counts; Ruby's fragment case actually carries a fragment. SPEC's "current state" and step-5 status sentences are rewritten as history, and §14 step 4a says what a hop-2 failure renders precisely.
b07cc6e to
d1a1e7d
Compare
|
Review round 1 addressed in The load-bearing finding, raised independently by all three reviewers, was that the hop-1 projection was incoherent: the thrown error was fixed while the raw transport error still reached Also fixed: Python's OAuth transport failures no longer chain the Left open for a human call:
|
There was a problem hiding this comment.
💡 Codex Review
basecamp-sdk/swift/Sources/Basecamp/HTTP/HTTPClient.swift
Lines 510 to 516 in d1a1e7d
Project cancelled URL errors before returning
When URLSession reports cancellation as URLError(.cancelled) with NSURLErrorFailingURLStringErrorKey, this branch returns the raw error, retaining the hop-1 URL and any signed query for the caller and onOperationEnd. Fresh evidence after the earlier review is the explicit cancellation bypass added here; keep cancellation terminal, but return a fresh URL-free cancellation sentinel instead of the transport error.
ℹ️ 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".
… drop base-URL userinfo from hook URLs, sever Ruby's status-error cause on hop 1 Swift's download projection kept a custom Transport's own .network message (the #567 passthrough); on the download hops that message can render the URL, so both hops now project to the fixed message. Kotlin's and Swift's hook-URL projection was a textual strip of query and fragment, which kept userinfo a configured base URL carries through the origin rewrite; both now rebuild origin+path from a parse (bracketed IPv6, non-default port) with a textual fallback that strips userinfo too. Ruby's download-mode status errors were still raised inside Faraday's rescue, making the request-retaining Faraday exception MRI's implicit cause; they now leave the rescue and raise with cause: nil like the network arm.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 252bf25371
ℹ️ 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.
2 issues found across 42 files
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="python/src/basecamp/errors.py">
<violation number="1" location="python/src/basecamp/errors.py:196">
P3: When the response contains `"error_description": ""`, `_hint_from` discards the field even though §6 treats every string value as the hint. Preserve empty strings by checking only `isinstance(hint, str)`.</violation>
<violation number="2" location="python/src/basecamp/errors.py:317">
P2: When a 500, 502, 503, or 504 response has no usable body message, this mapping still emits `Server error (500)` or `Gateway error (...)`, contrary to the fixed `Request failed (HTTP {status})` fallback. Use the parsed message or the status-specific fixed phrase in these mapped 5xx branches.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if data is None: | ||
| return None | ||
| hint = data.get("error_description") | ||
| return hint if isinstance(hint, str) and hint else None |
There was a problem hiding this comment.
P3: When the response contains "error_description": "", _hint_from discards the field even though §6 treats every string value as the hint. Preserve empty strings by checking only isinstance(hint, str).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/src/basecamp/errors.py, line 196:
<comment>When the response contains `"error_description": ""`, `_hint_from` discards the field even though §6 treats every string value as the hint. Preserve empty strings by checking only `isinstance(hint, str)`.</comment>
<file context>
@@ -169,27 +169,52 @@ def __init__(
+ if data is None:
+ return None
+ hint = data.get("error_description")
+ return hint if isinstance(hint, str) and hint else None
+
+
</file context>
There was a problem hiding this comment.
Not doing this — an empty error_description is indistinguishable from an absent one in Go (an empty Hint renders nothing) and would suppress the class-default hints in Ruby and Python, so treating it as absent is the cross-SDK-consistent reading. Leaving open in case the author reads step 3 more literally.
…unparsable fallbacks, one hop-1 message Swift's raw cancellation arm returned URLSession's URLError(.cancelled) — whose userInfo carries the failing URL — unwrapped; it now returns the fresh sentinel projectedDownloadCause already builds, so the #567 shape contract holds (a URLError(.cancelled) still arrives as a URLError) without the URL. TypeScript's terminal projection likewise keeps an abort's identity as a fresh DOMException cause with fixed text. Python's hop-2 boundary catches httpx.InvalidURL (outside HTTPError) as the pre-dial refusal — though a malformed Location actually fails hop 1 first, because httpx builds the redirect request even with follow_redirects=False, which the download boundary already projects (tested both ways). TypeScript refuses a non-HTTP(S) resolved Location by origin, as Ruby does. Every hook-URL projection (Kotlin, Swift, Ruby, Python) renders the fixed token `unparsable` when the URL yields no complete origin, instead of any textual strip of it. Hop-1 transport failures on a download now read "Network error" in Ruby and Python too, matching the other four SDKs. Tests: Go's dead-port test starts the API server first so it cannot reclaim the freed port; Kotlin's fragment case carries a fragment; Python's README retryable row matches step 12.
|
Rounds 2 and 3 addressed in Fixed: Swift's cancellation arm and transport- One observation for the author, per the review discipline in AGENTS.md: this was the third round on the same instrument — the download error path — each round finding one more egress (cause chain, then message passthrough, then the raw cancellation). The reason the rounds converged rather than accreted is that the end state is a single invariant, now true in all six SDKs: every error or hook argument produced on a download hop is built from a projection, never from the transport's value. If a future round finds a fourth egress on this path, that is evidence the invariant has a hole worth stating in SPEC §9, not a new selector to add. Still open for a human call: Codex on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed4e8642a1
ℹ️ 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.
All reported issues were addressed across 16 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Follow-up to #802, which merged the credential-scoped §9 rule. This PR brings all six SDKs into conformance and closes the two adjacent §6 gaps found while verifying the caps.
Closes #788.
§9 conformance
Download transport errors — fixed message, no raw cause in the generic chain. The transport's rendering of a signed/rewritten URL was the leak (
Download failed: #{e.message},Network error: ${cause.message},failed to download file: %w,ErrNetwork'sHint: cause.Error()). Hop-1 and hop-2 network failures now carry fixed messages, and the transport error is never chained. Where a classifier legitimately reads through the chain, a peer-free sentinel is projected in its place, exactly as §9 prescribes:context.Canceled/context.DeadlineExceeded—shouldTripCircuitand callers'errors.Isstill classify (tested).CancellationError()/URLError(.cancelled)—isCancellationstill matches, and the fresh sentinel carries no failing-URL rendering (tested).Pre-dial signed-URL validation — origin only /
unparsable. Ruby'sredirect to undialable download URL: #{Security.truncate(url)}(the cap kept a short query intact) and Swift's two downloadInvalid URL: \(url)guards now render the parsed origin, or the fixed tokenunparsable.Python raising boundary.
from Nonesuppresses the rendering but leaves theJSONDecodeError— whose.docretains the whole credential-bearing body — in__context__. The credential sites (auth.py/async_auth.pytoken refresh,oauth/exchange.py,oauth/device.py×2,download.pyhop 2) now construct inside theexceptand raise after it. Tests assert__context__ is Noneon the raised exception, the trap §9 documents.Hop-1 hook URL — origin+path only.
downloadURLaccepts any absolute URL and §14 step 2 preserves its query through the origin rewrite, so a signed storage URL smuggles its signature into hop 1. All six SDKs now project the hop-1 request-hook URL (start/end/retry — and Go's debug log line) to origin+path; the wire request keeps the query, and tests pin both halves. API-path hooks are untouched — those URLs carry no credential (§9), and Go gates the projection on a download-marked context so the sharedloggingTransportcontract is unchanged for API requests.§6 gaps
Step 5 — the fixed code-bearing phrase.
Request failed (HTTP {status})replacesresponse.statusText(TypeScript),status.description(Kotlin, parameter removed),resp.Status(GocheckResponsedefault arm) andlocalizedString(forStatusCode:)(Swift). The wire reason phrase does not exist under HTTP/2 and is blank for an unregistered code — the 599 case, which used to yield a blank message, is now tested in every SDK that changed. Ruby, Python and Go's raw client path already rendered it.Step 3 —
error_description→hint. Ruby and Python never extracted it on the API path (only their OAuth paths did). Both now do, truncated per §9, threaded througherror_from_response/handle_error; Ruby's class-constant hints (AuthError,ForbiddenError) fill in only when the body carries none, and a concreteRetry-Afterstill wins on 429.Verification
make checkrun on Linux.rgfinds none of the old interpolations (Download failed: #{,Network error: ${,failed to download file: %w,Invalid URL: \(at the download sites,statusText ||,status.description,API error: %s,localizedString(forStatusCode:)).Out of scope
error_description(spec renders it; reasons stated in §9).dialFailurealready covers it.ErrNetworkat ordinary API sites (no credential in those URLs, per §9).Summary by cubic
Brings all six SDKs into conformance with SPEC §9 — credential-bearing values are never rendered — and closes two adjacent §6 gaps found while verifying the caps. Also aligns §12's
on_paginateprose with its interface comment (Ruby and Python). Closes #788.§9 conformance
context.Canceled/DeadlineExceeded, TypeScript a freshDOMExceptionwith fixed text) so classifiers still read identity; Ruby severs both stored and MRI-implicit causes; hop-1 hooks see the same projection.unparsablewhen no complete origin exists — Ruby and Swift for the validation guards, Ruby and TypeScript for an unparsable or non-HTTP(S) hop-2 Location, Python forhttpx.InvalidURL, and every hook-URL projection that lacks a complete origin.exceptand raise after it, so the raised exception retains neither__cause__nor__context__(which would carry the body viaJSONDecodeError.doc).§6 gaps
Request failed (HTTP {status})in Go, TypeScript, Kotlin, and Swift — never the wire reason phrase, which is absent under HTTP/2 and blank for an unregistered code like 599.error_descriptionas the hint on the API path, truncated per §9; class-constant hints fill in only when absent, andRetry-Afterstill wins on 429.Written for commit ed4e864. Summary will update on new commits.