diff --git a/docs/acceptance/README.md b/docs/acceptance/README.md index d918c46ef..f08206aa4 100644 --- a/docs/acceptance/README.md +++ b/docs/acceptance/README.md @@ -343,6 +343,10 @@ PR after verification. There is no cleanup or status-only PR. descriptor-guided Timestamp and Duration decoding for daemon stdlib-JSON responses across unary HTTP and SSE, preserving ProtoJSON strings, typed failures, and the registered raw response. Status: proposed. +- [TypeScript SDK malformed-success decoding](sdk-malformed-success-decoding.md) - + cause-free protocol errors for malformed successful unary HTTP and ordinary SSE payloads, + retaining safe status and request-ID metadata while preserving server, transport, and + cancellation causes. Status: proposed. - [Canonical Shell command tool](canonical-shell-command-tool.md) — canonical `Shell` and `ShellStatus` model-facing names, safe legacy `Bash` input normalization, and diff --git a/docs/acceptance/sdk-malformed-success-decoding.md b/docs/acceptance/sdk-malformed-success-decoding.md new file mode 100644 index 000000000..5f4f69a12 --- /dev/null +++ b/docs/acceptance/sdk-malformed-success-decoding.md @@ -0,0 +1,147 @@ +# TypeScript SDK malformed-success decoding - acceptance plan + +**Contract:** human-reviewed/v2 +**Work classification:** Architectural - this changes the durable public diagnostic and security policy for `ProtocolError` at the TypeScript SDK's HTTP successful-response boundary. +**Decision record:** [ADR 0349](../adr/0349-typescript-sdk-malformed-success-decoding.md) +**Phase:** TypeScript SDK HTTP transport hardening +**Status:** proposed, 2026-09-18. Drafted from issue #1694 with no unresolved human decisions. +**Delivery:** Split, with an explicit checkpoint waiver. The Plan / Interface PR records the security and compatibility boundary; in this session the directing human explicitly instructed the implementation to proceed as the next `gh stack` layer without waiting for the plan to merge. `/plan-orchestrate` is not used because its merged-baseline precondition is intentionally waived. +**Expected tasks:** 1 +**Issue:** [stacklok/mecatl#1694](https://github.com/stacklok/mecatl/issues/1694). +**Plan PR:** [#1698](https://github.com/stacklok/mecatl/pull/1698) +**Approved baseline:** absent by explicit human exception; the directing human requested two sequential `gh stack` PRs and explicitly said there is no need to wait for the Plan / Interface PR to merge. + +The TypeScript SDK rejects malformed successful unary HTTP responses and +ordinary SSE data frames without retaining runtime-dependent decoder exceptions. +The cause-free error keeps safe transport metadata while established server, +network, authentication, cancellation, and streaming behavior remains intact. + +This policy applies at the shared HTTP boundary described by +[ADR 0349](../adr/0349-typescript-sdk-malformed-success-decoding.md). It does not +change public method signatures or the server protocol. + +## Human decisions + +None — issue #1694 defines the exact cause, metadata, exclusion, documentation, and compatibility policy, and the directing human authorized the two-PR stack. + +## Interface contract + +- **gRPC / protobuf:** None - no service, method, message, field, descriptor, or + generated protobuf output changes. The policy applies only after successful + HTTP or SSE responses reach the hand-written TypeScript transport. +- **Exported Go APIs / interfaces:** None - no Go package, symbol, signature, or + API snapshot changes. +- **Tool schemas:** None - response decoding is an SDK transport concern and does + not change model-facing tools or dispatch. +- **CLI / config:** None - no flag, environment variable, settings key, default, + or precedence changes. +- **Events / persistence:** None - no event, snapshot, cursor, cache, log, or + migration changes. Ordinary SSE iteration and cancellation keep their current + lifecycle. +- **Security / authority:** After a unary response body has been acquired, + successful unary HTTP and ordinary SSE JSON, + well-known-type, and protobuf decode failures produce the existing generic + `ProtocolError` with `code: "protocol"`, `transport: "http"`, HTTP status, + and `X-Request-ID` when available, but no `cause`. The SDK copies no decoder + message or rejected response value into the error and performs no arbitrary + string redaction. Failure to read a unary response body after headers remains + a caused `ProtocolError`; a valid SSE `event: error` frame and a non-2xx HTTP + response continue through the existing typed server-error boundary. +- **Compatibility / migration:** Public TypeScript types and method signatures + are unchanged. This intentionally removes runtime-specific diagnostic detail + from `ProtocolError.cause` for malformed successful HTTP and SSE payloads. + The four existing generic messages remain exact and `toJSON()` remains + response-content-free. Non-2xx typed server errors retain code, status, + request ID, and cause; credential-provider and HTTP authentication failures, + fetch/network failures, post-header body-read failures, abort, cancellation, + and stream/control semantics remain unchanged. The `ProtocolError` TSDoc and + generated SDK reference own the public error contract. The TypeScript SDK + connection guide links that reference and briefly calls out safe logging; + architecture and implementation notes record the living design. + +## In scope - 2 scenarios, in implementation order + +### Scenario 1 - Malformed successful payloads expose only safe error metadata + +The malformed-success decode paths share the cause-free boundary from +[ADR 0349](../adr/0349-typescript-sdk-malformed-success-decoding.md), covering +both unary HTTP responses and ordinary SSE data frames after body acquisition. + +**Acceptance:** + +- AC1.1: A successful unary response with malformed JSON rejects with + `ProtocolError`, `code: "protocol"`, `transport: "http"`, the HTTP status, + the response request ID when present, and no own `cause` property; its message + is exactly `The mecatl server returned invalid JSON`, and neither the message + nor `toJSON()` contains a planted response canary. A second case without an + `X-Request-ID` proves that no request ID is synthesized. + - verify: vitest:sdk/typescript/test/http-decoding-errors.test.ts#bWFsZm9ybWVkIHN1Y2Nlc3NmdWwgSFRUUCBhbmQgU1NFIHBheWxvYWRzIG9taXQgZGVjb2RlciBjYXVzZXM - `sdk/typescript/test/http-decoding-errors.test.ts :: "malformed successful HTTP and SSE payloads omit decoder causes"` +- AC1.2: A successful unary response that parses as JSON but fails + well-known-type normalization or protobuf-es decoding has the same cause-free, + canary-free metadata contract and the exact message + `The mecatl server returned an invalid response`. + - verify: vitest:sdk/typescript/test/http-decoding-errors.test.ts#bWFsZm9ybWVkIHN1Y2Nlc3NmdWwgSFRUUCBhbmQgU1NFIHBheWxvYWRzIG9taXQgZGVjb2RlciBjYXVzZXM - `sdk/typescript/test/http-decoding-errors.test.ts :: "malformed successful HTTP and SSE payloads omit decoder causes"` +- AC1.3: After one valid ordinary SSE data frame is yielded, malformed JSON and + descriptor/protobuf failures in the next frame follow the same cause-free, + canary-free policy. The exact messages remain `The mecatl SSE stream contained + invalid JSON` and `The mecatl SSE stream contained an invalid event`. Cases + with and without `X-Request-ID` prove its faithful propagation, the failing + pull rejects, and no later frame is yielded. + - verify: vitest:sdk/typescript/test/http-decoding-errors.test.ts#bWFsZm9ybWVkIHN1Y2Nlc3NmdWwgSFRUUCBhbmQgU1NFIHBheWxvYWRzIG9taXQgZGVjb2RlciBjYXVzZXM - `sdk/typescript/test/http-decoding-errors.test.ts :: "malformed successful HTTP and SSE payloads omit decoder causes"` + +### Scenario 2 - Neighboring error and lifecycle semantics stay unchanged + +The cause policy leaves the typed server-error contract from +[ADR 0248](../adr/0248-sdk-compatibility-and-error-contract.md) and the shared +[TypeScript SDK architecture](../architecture.md#typescript-sdk) intact. + +**Acceptance:** + +- AC2.1: Independently exercised non-2xx RFC 9457 responses and valid SSE + `event: error` frames retain their typed code, HTTP status, request ID, + message, and established parsed-problem cause. + - verify: vitest:sdk/typescript/test/http-decoding-errors.test.ts#c2VydmVyIGFuZCBhdXRoZW50aWNhdGlvbiBlcnJvcnMgcmV0YWluIHRoZWlyIHR5cGVzIGFuZCBjYXVzZXM - `sdk/typescript/test/http-decoding-errors.test.ts :: "server and authentication errors retain their types and causes"` +- AC2.2: Credential-provider rejection and an HTTP 401 response remain + `AuthenticationError` values with their established causes and available + request ID/status metadata. + - verify: vitest:sdk/typescript/test/http-decoding-errors.test.ts#c2VydmVyIGFuZCBhdXRoZW50aWNhdGlvbiBlcnJvcnMgcmV0YWluIHRoZWlyIHR5cGVzIGFuZCBjYXVzZXM - `sdk/typescript/test/http-decoding-errors.test.ts :: "server and authentication errors retain their types and causes"` +- AC2.3: Fetch/network failures remain `TransportError` values with their native + cause. A unary post-header response-body read failure remains a + `ProtocolError` with its native cause. Abort/cancellation preserves the + current signal reason and transport classification, while SSE reader failures + continue to escape the decoder boundary unchanged. + - verify: vitest:sdk/typescript/test/http-decoding-errors.test.ts#SFRUUCB0cmFuc3BvcnQgYm9keS1yZWFkIGFuZCBjYW5jZWxsYXRpb24gZmFpbHVyZXMgcmV0YWluIHRoZWlyIGNhdXNlcw - `sdk/typescript/test/http-decoding-errors.test.ts :: "HTTP transport body-read and cancellation failures retain their causes"` +- AC2.4: The TypeScript SDK error documentation, connection guide, architecture, + and implementation notes state the malformed-success cause policy and its + retained metadata without changing generated public signatures. + - verify: inspection - `task sdk:api:check`, `task sdk:docs:check`, `task docs`, + and `task site:build` prove generated-reference freshness and documentation + integrity; review proves the behavioral wording. + +## Out of scope + +| Item | Defer-to | Decision | +|---|---|---| +| Non-2xx server-error normalization | Existing SDK error contract | Preserve `errorFromProblem` behavior and causes. | +| Fetch, network, authentication, and abort classification | Existing transport contract | Preserve existing error types, causes, and signal behavior. | +| Best-effort decoder-message redaction | Not planned | Omit the arbitrary decoder cause instead of maintaining an unsafe string filter. | +| Server, wire, stream, and control lifecycle changes | Not part of #1694 | Keep current protocols and iterator/control behavior. | + +## Definition of done + +1. Focused SDK lint, typecheck, unit, build, API, and documentation checks pass. +2. `task lint`, `task test`, `task docs`, `task site:build`, and the offline demo + pass. +3. `task ac-trace-strict` resolves every named proof when this plan becomes + `landed`. +4. Tests are observed failing against the planted pre-fix decoder-cause behavior + before the shared helper is implemented. +5. `/panel-review` reports no ship blockers or unwaived reviewer failures. +6. The stacked Implementation PR reports interface conformance and links this + Plan / Interface PR; humans retain merge authority for both PRs. + +## Deferred decisions and known risks + +- JavaScript runtimes can change native decoder messages. The tests therefore + plant response canaries and assert the SDK-owned error surface rather than a + runtime-specific exception string. diff --git a/docs/adr/0349-typescript-sdk-malformed-success-decoding.md b/docs/adr/0349-typescript-sdk-malformed-success-decoding.md new file mode 100644 index 000000000..71421f0aa --- /dev/null +++ b/docs/adr/0349-typescript-sdk-malformed-success-decoding.md @@ -0,0 +1,80 @@ +# ADR 0349 - Cause-free TypeScript SDK malformed-success decoding + +- Status: Proposed +- Date: 2026-09-18 +- Scope: the TypeScript SDK's shared HTTP unary and SSE successful-response + decoding boundary +- Supersedes: none +- Superseded by: none + +## Context + +The TypeScript SDK's HTTP transport decodes successful unary response bodies and +ordinary SSE data frames in two stages. It first parses JSON, then normalizes +descriptor-declared well-known types and decodes the value with protobuf-es. +Failures at either stage become the existing `ProtocolError` with HTTP status +and transport metadata. + +Those errors currently retain the native JSON or protobuf decoder exception as +`Error.cause`. Decoder messages differ among JavaScript runtimes and can include +an excerpt of the rejected response. An application logger that recursively +captures causes can therefore copy untrusted response content, including an +ephemeral presentation URL, into its logs. The SDK does not log these errors, +and `MecatlError.toJSON()` already omits causes, but neither fact controls how an +application logs an `Error` instance. + +The neighboring causes have different semantics. A parsed non-2xx RFC 9457 +problem is the server's typed rejection and remains useful to the application. +A fetch failure or abort is the transport failure itself. Removing those causes +would discard established diagnostics without addressing malformed successful +payloads. + +## Decision + +1. Once a successful unary response body has been acquired, the shared HTTP + transport creates a cause-free `ProtocolError` when its JSON parsing, + well-known-type normalization, or protobuf-es decoding fails. Ordinary SSE + data-frame JSON and protobuf decoding follows the same boundary. +2. These errors retain the existing generic SDK-authored message, `protocol` + code, `http` transport, HTTP status, and the response's `X-Request-ID` when + present. They never copy the decoder message or rejected value into the + error. +3. The SDK does not attempt best-effort redaction of arbitrary decoder messages. + Cause-free construction is single-sourced inside the shared HTTP transport, + but private helper and call-site topology are not part of the durable public + contract. +4. A failure while acquiring the unary response body after headers remains a + caused `ProtocolError`; it is a transport/read failure rather than a decoder + rejection. SSE reader failures likewise remain outside the decoder boundary. +5. A successfully parsed SSE frame whose event type is `error` continues through + `errorFromProblem`. Non-2xx problem normalization, credential-provider and + HTTP authentication errors, fetch and network `TransportError` causes, abort + behavior, and stream/control lifecycle semantics remain unchanged. +6. The public `ProtocolError`, `MecatlErrorOptions`, and transport method + signatures do not change. This is an intentional compatibility narrowing of + diagnostic detail for malformed successful HTTP and SSE payloads. + +## Consequences + +Application loggers cannot recover rejected successful-response content by +walking `ProtocolError.cause` at this boundary. HTTP status and response request +IDs remain available for correlation when supplied, and the generic message +still distinguishes JSON parsing from response or event decoding. + +Runtime-specific decoder detail is no longer available on these four errors. +Applications that need to inspect a malformed upstream payload must capture it +at an explicitly secured transport or server boundary rather than through the +SDK error object. + +Typed server rejections, authentication failures, and transport/body-read +failures keep their established causes. This makes the policy depend on whether +a value was rejected by the successful-payload decoder or by an adjacent error +or transport boundary, rather than merely on which API threw it. + +## See also + +- [Issue #1694](https://github.com/stacklok/mecatl/issues/1694) +- [Malformed-success decoding acceptance plan](../acceptance/sdk-malformed-success-decoding.md) +- [ADR 0248 - SDK compatibility discovery and the typed error contract](./0248-sdk-compatibility-and-error-contract.md) +- [ADR 0279 - TypeScript SDK architecture](./0279-typescript-sdk-architecture.md) +- [TypeScript SDK architecture](../architecture.md#typescript-sdk) diff --git a/docs/adr/README.md b/docs/adr/README.md index d778e6204..c603579cb 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -81,6 +81,7 @@ Documentation/citation conventions are in [`docs/design/README.md`](../design/RE - [0341 - Deno reuses the ConnectRPC gRPC transport](./0341-typescript-sdk-deno-grpc.md) *(supersedes ADRs 0339 and 0340 for the HTTP-only transport and Node-compatibility exclusions)* - [0347 — Run-ID-addressed prompt-free controls](./0347-run-id-addressed-prompt-free-controls.md) *(proposed; supersedes ADR 0288 Decision 6 and ADR 0304 Decision 3 in part)* - [0348 — TypeScript SDK MCP authorization lifecycle](./0348-typescript-sdk-mcp-authorization-lifecycle.md) *(proposed; supersedes ADR 0304 Decision 3 only for the authorization-lifecycle resource)* +- [0349 - Cause-free TypeScript SDK malformed-success decoding](./0349-typescript-sdk-malformed-success-decoding.md) *(proposed; narrows decoder diagnostics at the HTTP successful-response boundary)* - [0342 - Gate runs on unresolved live context windows](./0342-context-window-admission.md) *(supersedes ADR 0016 only for pre-swap run admission)* - [0346 - Prompt-cache breakpoints are protocol-native, never vendor-keyed](./0346-unified-prompt-cache-dialect.md) *(supersedes ADR 0100's prompt_cache_breakpoint deferral, its root cache_control dialect arm, and its OpenRouter TTL deferral; extends ADR 0334 to OpenRouter)* - [0036 — `engine/` is its own Go module (monorepo via `go.work`)](./0036-engine-module.md)