SPEC §9: peer-derived text in observer-facing errors (boundary unresolved) - #802
SPEC §9: peer-derived text in observer-facing errors (boundary unresolved)#802jeremy wants to merge 1 commit into
Conversation
…lved) Split out of #793 so §6's settled Retry-After work can ship on its own. This half is not ready: its central boundary has moved three times in one review cycle and the fourth candidate's premise has already failed a spot check. What is settled here and stands on its own: - The scope clause, and the three contracts that sit outside it - §6's error body message and the field-keyed composition over it, §6's statusless api_error for a malformed 2xx body whose entire diagnostic is the wire value it embeds, and §23's origin-only rendering. Each requires specific text to reach the caller, so the truncation cap governs and this section does not. - Rule 3's cancellation-classification requirement. Go's shouldTripCircuit reaches context.Canceled and context.DeadlineExceeded only through ErrNetwork's Cause, and Swift's isCancellation walks .network causes, so a naive "drop the cause" would trip the breaker on a cancelled request and retry a cancelled one respectively. Projection onto a freshly constructed peer-free sentinel preserves both. - The parse-failure fallback token. §8's same-origin check rejects a URL that does not parse, and §23 still wants an origin in the error, so the origin component is the fixed token `unparsable`. - Python's __context__ obligation, which survives whichever boundary wins: CPython attaches the active exception at RAISE time, after any constructor has returned, so the remedy is a raising boundary rather than a constructing one. Clearing __context__ before the raise is silently overwritten. What is not settled is marked CONFLICT in the section itself, with all three failed formulations, why each failed, and the unverified premise of the ingress candidate - so nobody reads the closure claim as load-bearing.
|
Two review findings arrived on #793 against the pre-split head, both against SPEC §9 text that now lives here. Reproducing them so they stay next to the section they are about. Both look correct to me on reading; neither is fixed yet, since this PR is not ready and its central boundary is still open. 1.
My read: correct, and it is the same shape as Ruby's 2. The "binds at construction" sentence is stale (Codex,
My read: correct. "The rule binds at construction" survived from the second formulation and now contradicts the third. It is a live example of why this section is not ready — two formulations are simultaneously present in the text. It is covered by the Both are listed here rather than patched, because patching either now would be a fifth move on a boundary that has already moved three times. |
There was a problem hiding this comment.
Pull request overview
Defines SPEC §9’s handling of peer-derived text in observer-facing errors, while documenting an unresolved sanitization boundary.
Changes:
- Introduces closed-vocabulary rendering and cause projection rules.
- Adds Python exception-context requirements.
- Defines
unparsablefallback behavior for event-feed URLs.
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.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - **A generated method returns the transport error directly.** In `go/templates/client.tmpl`, only the `$isIdempotent` branch calls `doWithRetry`; the else-branch ends `return c.Client.Do(req)`, so every **non-idempotent** generated operation hands the caller a raw `*url.Error` that no constructor ever sees. | ||
| - **A log line takes it without an error object at all** — generated Go's `c.Logger.Debug("request failed", …, "error", err)`. | ||
|
|
||
| So the contract is stated over what the SDK *does with the value*, which is a closed set, rather than over where the value was made, which is not: **a peer-bearing transport error must be projected before it egresses, and there are exactly five egresses** — returned or thrown to the caller, passed to a hook or observer callback, written to a log, left in the generic unwrap chain (rule 3), or attached by the runtime as exception context (Python; below). A path that does none of those five may hold the raw value freely. |
|
|
||
| **Where no contract requires the text to reach the caller, peer-derived text in an observer-facing error is rendered from a closed vocabulary keyed on the error's type. It is never composed from peer input and then bounded by length.** | ||
|
|
||
| The leading clause is the scope, not a hedge: it is the boundary the previous section just drew, carried into the sentence that binds so the sentence cannot override it. Three contracts already sit on the other side and are untouched — §6's Error Body Parsing Algorithm `message` and the field-keyed composition over it; §6's statusless `api_error` for a malformed 2xx body, whose entire diagnostic *is* the malformed wire value it embeds; and §23's origin-only rendering of a refused redirect `Location` or a rejected continuation URL. Each requires specific text to reach the caller, so each is governed by the truncation cap above and this section makes no demand of it. What is left is the class four rounds on #788 were actually about — a decoder's rendering of bad bytes, a transport library's rendering of a URL it failed on, a close reason nobody asked to see — and there no contract stands behind the text, so this section governs. |
![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: a91dd69d01
ℹ️ 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".
| - **A generated method returns the transport error directly.** In `go/templates/client.tmpl`, only the `$isIdempotent` branch calls `doWithRetry`; the else-branch ends `return c.Client.Do(req)`, so every **non-idempotent** generated operation hands the caller a raw `*url.Error` that no constructor ever sees. | ||
| - **A log line takes it without an error object at all** — generated Go's `c.Logger.Debug("request failed", …, "error", err)`. | ||
|
|
||
| So the contract is stated over what the SDK *does with the value*, which is a closed set, rather than over where the value was made, which is not: **a peer-bearing transport error must be projected before it egresses, and there are exactly five egresses** — returned or thrown to the caller, passed to a hook or observer callback, written to a log, left in the generic unwrap chain (rule 3), or attached by the runtime as exception context (Python; below). A path that does none of those five may hold the raw value freely. |
There was a problem hiding this comment.
When implementers use this normative “exactly five egresses” list to audit the SDKs, peer-derived text can still escape through derived renderings that do not carry the original transport error across one of these boundaries—for example, BasecampError.toJSON() serializes message and hint (typescript/src/errors.ts:165-175), while the OpenTelemetry hooks export error.message (typescript/src/hooks/otel.ts:261-264). The preceding conflict block explicitly acknowledges that these examples defeat the list, so retaining the known-false closure claim leaves this security requirement incomplete and can cause an implementation to be declared converged while it still leaks peer text.
Useful? React with 👍 / 👎.
|
|
||
| **Routing every construction through one sanctioned constructor is the recommended mechanism, not the contract.** It covers the common path cheaply and remains the right first move in each SDK; it simply does not discharge the obligation on its own, and an SDK is not converged when its constructors are clean. The test is whether a peer-bearing value can reach any of the five, not whether the type has one construction site. | ||
|
|
||
| **Python owes a second boundary, because in Python the constructor cannot discharge this alone.** What follows is an obligation on **Python only** — it falls out of a CPython runtime behaviour with no equivalent in the other five, which inherit nothing from it and owe nothing extra on its account. |
There was a problem hiding this comment.
When Ruby raises a sanitized SDK exception from inside a rescue, the runtime also attaches the active exception as its implicit cause at raise time, so constructor-only projection does not make this a Python-only obligation. The repository already accounts for this in ruby/lib/basecamp/oauth/exchange.rb:206-217, using raise ..., cause: nil specifically because the implicit chain would expose peer input through full_message; without the same raising-boundary treatment, Ruby network paths can sanitize their custom cause field while traceback or cause-aware telemetry still emits the raw Faraday error and URL. Declaring that the other five SDKs owe nothing extra also conflicts with the later recommendation to fix Ruby solely in network_error.rb.
Useful? React with 👍 / 👎.
Not ready for review. Opened to hold verified work and to record why its central boundary is still open. Split out of #793 so that PR's settled §6 material could ship without waiting on this.
Why this is separate
§6 was decided: a status rule, a composition table walked back across 11 branches, a definition of "retry", the representability tiers, zero open findings. §9 moved its central boundary three times in one review cycle, and the fourth candidate's premise failed a spot check before it was written. That is a section still being designed, not one being reviewed.
The boundary, and why three formulations failed
on_request_endbefore wrapping (typescript/src/services/base.ts,kotlin/.../BasecampHttpClient.kt), and generated Go's non-idempotent branch returnsc.Client.Do(req)directly, bypassingdoWithRetryentirely.BasecampError.toJSON()serialisingmessageandhint;typescript/src/hooks/otel.tsandgo/pkg/basecamp/otel/otel.gowritingerr.Error()into span status,recordExceptionand attributes; Kotlin'sBasecampHooksprintln.The common defect is that all three enumerate destinations, and destinations are an open set — they grow with every integration anyone writes (a new tracing backend, a
to_dict()for structured logging, a Sentry hook). Renaming "call sites" to "egresses" changed the granularity, not the openness.Current best candidate, with its premise unverified
Sanitize at ingress — the point where the transport hands the SDK a failure — so no raw peer-bearing value ever circulates and no sink needs enumerating. Every failed member above dissolves without being named, including
otel.ts,toJSON, and integrations not yet written.The premise is not verified and is already known to be weaker than first stated. "One transport module per SDK" does not hold: a spot check found ~5–7 ingress points in TypeScript, because the five
oauth/*modules anddownload.tsare hand-written outside the generated transport by AGENTS.md's own design. Ingress is still a better shape than egress — it is bounded by the SDK's own architecture rather than by what anyone does with a value — but the closure argument has to be made honestly this time, and it has not been made yet.This is marked
[CONFLICT]in the section itself, so nobody reads the closure claim as load-bearing.What is carried here and stands on its own
messageand the field-keyed composition over it, §6's statuslessapi_errorfor a malformed 2xx body (whose entire diagnostic is the wire value it embeds), and §23's origin-only rendering. Each requires specific text to reach the caller, so the truncation cap governs and this section makes no demand.shouldTripCircuitreachescontext.Canceledandcontext.DeadlineExceededonly throughErrNetwork'sCause, and Swift'sisCancellationwalks.networkcauses. A naive "drop the cause" would trip the circuit breaker on a cancelled or timed-out request and retry a cancelled one. Projection onto a freshly constructed peer-free sentinel preserveserrors.IsandisCancellationwhile carrying no peer bytes.unparsable, selected by the parse outcome.__context__obligation, which survives whichever boundary wins, since CPython attaches the active exception at raise time, after any constructor has returned. Verified in a REPL, including the trap that clearing__context__before the raise is silently overwritten — a fix that passes a unit test on the constructed object and still leaks in production traceback output.Leak sites found and recorded
typescript/src/hooks/otel.ts(4 renderings),go/pkg/basecamp/otel/otel.go(5),typescript/src/errors.tstoJSON(),kotlin/.../BasecampHooks.kt(2) — all on #788 with negative results from the same sweep (metric labels are clean, usingerror.name; no serialization member outside TypeScript).Refs #788.
Summary by cubic
Codifies §9 rules for rendering peer-derived text: observer-facing errors must use a closed vocabulary keyed by error type instead of composing peer input, and recognized causes are projected to peer-free sentinels to preserve cancellation classification. Adds the fixed origin token
unparsablewhen a URL cannot parse, clarifies Python’s__context__raising boundary, and updates §23 and Security notes accordingly. The sanitize boundary (ingress vs egress) remains unresolved and is marked in-spec as a conflict; this PR lands the settled parts.Review focus
api_error, and §23 origin-only rendering (these remain governed by the truncation cap).unparsablein §23 references; Security section clarifies projection vs redaction.__context__to prevent runtime-attached peer exceptions from leaking; constructor-only fixes are insufficient.Required SDK changes (follow-up)
context.Canceled/DeadlineExceeded, SwiftCancellationError/URLError(.cancelled)) to new peer-free instances and chain them; do not chain unrecognized causes.unparsableas the origin component when URL parsing fails; continue origin-only projection for parsed URLs.__context__after the raise;from Nonealone is not sufficient.err.Error()directly).Written for commit a91dd69. Summary will update on new commits.