fix(#20): heid-code-review fixups — INV-CUT-2 completeness on cancel/stream (slice-2)
Triaged the heid-code-review panel (Gróa + Hulda substantive, Regin zero=weak).
Adopted (genuine adds):
- cancel_turn + stream_turn gain a defensive `except ApiError -> SessionApiFailed`
default after their discriminated branches. INV-CUT-2 ("every ApiError is mapped;
default SessionApiFailed") now holds STRUCTURALLY on those routes, not by coupling
to the SDK's internal guarantee that it maps them to discriminated types. + tests.
- get_session_tools error-path test (symmetric with messages).
- Contract § Error map amended: added the stream ProtocolError rows
(Malformed*/TurnIdFlip -> ratatoskr same-named), clarified the cancel row (the SDK
RAISES the typed races -> ratatoskr exceptions, only a 200/cancelled=False is a
CancelResult; caller surface stays exception-based per DEC-2), and noted the
ApiError default holds on stream+cancel too.
Rejected (category-5, wrong-grounding) — 2/3 arms flagged create's bound-502 as
"should gate on error_code like list's 422+cursor_invalid". Verified against the SDK
parser (not in the arms' file set): the bound-502 body is
{"error_code":"bifrost_handshake_failed","detail":{"bifrost_error":...}}, and the
SDK's envelope parser PREFERS the nested detail (which lacks error_code), so
ApiError.error_code resolves to "unknown" — gating would REGRESS handshake detection
(the cli/web integration tests caught it). INV-002 also makes the handshake the sole
bound-502 cause. Kept the any-bound-502 mapping; documented WHY in code + contract.
Accepted-as-is: create_session -> Mapping annotation (intentional open-world
passthrough, already documented in the route-map note; category 3).
Suite 493 green; wt.py mypy + ruff clean. Patch.
This commit is contained in:
@@ -158,18 +158,23 @@ others, they get their own row here — the default is NOT a general "any 404
|
||||
| SDK `AgentNotAvailable` / `TurnLaunchUnavailable` / `SessionRetired` (stream-open) | ratatoskr `AgentNotAvailable` / `TurnLaunchUnavailable` / (retired → `SessionApiFailed`) — same names, passthrough |
|
||||
| SDK `ConnectionDropped` (mid-stream) | `SseConnectionDropped` |
|
||||
| SDK `ResumeError` subclasses (in resilient stream) | resilient `stream_turn` absorbs; terminal → `SseConnectFailed` |
|
||||
| SDK `Cancel*` (cancel_turn) | folded into `CancelResult`; late-cancel race (B-CAN-3) returns `cancelled=False`, never raises |
|
||||
| SDK `MalformedSseId` / `MalformedSseData` / `TurnIdFlip` (stream `ProtocolError`) | ratatoskr same-named types — same-name rewrap of the discriminated stream protocol errors |
|
||||
| SDK `Cancel*` (cancel_turn) — the SDK RAISES the typed races | 404 `turn_not_found` → `CancelTurnNotFound`; 409 `turn_finished` → `CancelAlreadyCompleted`; other `CancelError` → `CancelFailed`. A 200 (incl. `cancelled=False`, the B-CAN-3 late-cancel no-op) returns a `CancelResult` — never raises. The caller surface stays exception-based (DEC-2; matches the pre-cutover CLI/web handlers). |
|
||||
| `ApiError(404)` on `sessions.create` | `AgentNotFound` |
|
||||
| `ApiError(404)` on `sessions.write_history` | `AuthoredHistoryUnavailable` (hide-existence) |
|
||||
| `ApiError(422 cursor_invalid)` on `sessions.list` | `InvalidCursor` |
|
||||
| `ApiError(502 bifrost_handshake_failed)` on bound `sessions.create` | `BifrostHandshakeFailed` |
|
||||
| `ApiError(422 cursor_invalid)` on `sessions.list` | `InvalidCursor` (dual-key: status 422 AND error_code; the flat cursor body surfaces the code) |
|
||||
| `ApiError(502)` on bound `sessions.create` | `BifrostHandshakeFailed` — NOT gated on error_code (unlike list's 422): INV-002, the synchronous handshake is the SOLE bound-502 cause; and the SDK's envelope parser prefers the nested `detail` (which carries `bifrost_error`, not `error_code`), so no distinguishing top-level `error_code` surfaces. The route+status IS the discriminator. |
|
||||
| **`ApiError` (any other status/route) — the default** | `SessionApiFailed(status, error_code, body)` |
|
||||
|
||||
The default row is load-bearing: any `ApiError` not matched above surfaces as the
|
||||
generic `SessionApiFailed` carrying the raw `status`/`error_code`/`body` — the
|
||||
adapter does NOT invent per-route semantics the contract doesn't list, and does NOT
|
||||
leave an `ApiError` un-mapped. Each slice adds/confirms its route's rows here before
|
||||
the old path is deleted.
|
||||
leave an `ApiError` un-mapped. **This default holds on EVERY route, including the
|
||||
stream and cancel** (each carries a defensive `except ApiError → SessionApiFailed`
|
||||
after its discriminated branches — the SDK maps those routes to discriminated types
|
||||
today, but the default guarantees INV-CUT-2 structurally, not by SDK-internal
|
||||
coupling). Each slice adds/confirms its route's rows here before the old path is
|
||||
deleted.
|
||||
|
||||
## Slice plan (incremental, DEC-4)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user