[FIX] Reach the escrow route the contract serves, and retry a 401 once on the typed path - #434
Open
justin13888 wants to merge 7 commits into
Open
Conversation
… client
`capsule_sdk::recovery` built `{api_root}/backup/escrow` from a `const` and sent it
with hand-written `reqwest` calls. The committed Kynos document serves
`GET`/`PUT /v1/auth/escrow`, so every networked recovery flow — enroll, the
stale-cache refresh, and the guided re-wrap's escrow replace — failed against a real
server while S-D12 read `done`.
The route is not fixed by editing the constant. Both operations are
`application/octet-stream` in each direction, which is a media type spargen lowers, so
both are already generated and neither is narrowed out in `build.rs`; `AGENTS.md`
requires that everything which parses or serializes is generated, the byte-serving
endpoints included. `RecoveryClient` now holds one `AuthenticatedClient` and
orchestrates `fetch_escrow`/`store_escrow`, so the path is a function of the document
and cannot drift again.
What the move changes:
- `RecoveryClient::new` is fallible (`RecoveryError::InvalidBaseUrl`) — the generated
client parses its base once at construction rather than per call. The two FFI
callers each grow a `?`.
- `RecoveryError` drops `Body(reqwest::Error)` and `Auth(AuthError)`, which nothing can
construct once the reqwest path is gone, and gains `Transport`, `Unauthorized`,
`Malformed` and `InvalidBaseUrl`, plus an `error_code()` returning the stable
`error.escrow.*`/`error.auth.*` code a client localizes.
- A refused credential keeps its auth identity across the FFI boundary:
`Unauthorized` maps to `FfiError::Auth`, where a failed refresh used to arrive as
`RecoveryError::Auth`. A request-construction failure maps there too — these
operations take no parameters and their base URL is already parsed, so the only way
either fails before a byte leaves is the bearer provider, and a dead session must
reach a caller as one rather than as a transport blip.
Both in-repo mocks were answering whichever path they were handed, which is why the
wrong route survived. They now route on `/v1/auth/escrow` and answer `501` elsewhere,
so a route regression fails loudly instead of reading as "no escrow stored", and their
refusals carry real RFC 9457 bodies because a generated operation decodes them.
The proof the old tests could not give is a new case in
`capsule-server/tests/sdk_client.rs`: the SDK stores and fetches a real wrap over a
socket against the assembled router, and asserts the bytes come back byte-identical
and still open under the recovery secret.
Refs #408
Deploying capsule with
|
| Latest commit: |
732967f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d25a7761.capsule-22k.pages.dev |
| Branch Preview URL: | https://fix-sdk-escrow-route-and-401.capsule-22k.pages.dev |
…oute Three defects an adversarial read of the previous commit turned up. **An unreachable server was reported as an expired session.** reqwest builds every failure of the request it executes with `error::request(..)`, so `is_request()` is true for connection-refused, DNS and TLS failures, and spargen's taxonomy files all of them under `RequestConstruction` next to the genuine pre-flight ones. Mapping that class to `Unauthorized` therefore told an offline device to sign in again — the one remedy that cannot work without a network. The source discriminates them instead: a bearer provider that could not mint a token is boxed as the generated runtime's own `AuthError`, and nothing else on this path is. A closed port is now `Transport`, with a test that binds a socket, drops it, and points the client at the address. **`error_code()` guessed where it should have read.** A `400` reported `error.escrow.malformed` even when the server said otherwise; a `413` reported it too, so a client localizing the code would tell a user their recovery blob was corrupt when it was merely too large; and the `500`'s `error.escrow.unavailable` — the one code that route bothers to set — was thrown away into a transport string. `Malformed` now carries the server's own code, `413` carries `error.request.too_large`, and `500` is its own `Unavailable` variant. The mocks stop inventing `error.*` strings that exist in no catalog and use `capsule_i18n::error_codes` throughout. `FfiError::Escrow` gains the `code` the enum's own doc already promised every variant carries, so `error.escrow.not_stored` reaches a native client — the distinction between "set up a recovery key" and "we could not read the one you have". **The route was pinned by accident.** The socket test relied on a wrong path producing something other than `NotEnrolled`, which held only because Kynos's unmatched-route `404` carries no code and therefore fails to parse. It now asserts the route directly through the fixture's in-process client: what the SDK stored is read back at `/v1/auth/escrow`, and a rotation seeded at that path is what the SDK fetches next. A client on any other path satisfies neither. Relatedly, an uncoded `404` is deliberately *not* read as `NotEnrolled` any more. Reading every `404` as "this account has escrowed nothing" is precisely what let a wrong route look like an empty escrow for a whole slice; an intermediary answering `404 text/html` is a broken path, not an enrollment state. Refs #408
The generated client had only the proactive half of the refresh contract: the token provider refreshes when the stored token is within its skew of expiry, before the request leaves. That cannot cover a token the server stops honouring early — a revocation mid-flight, or a clock the two ends disagree about — and the hand-written clients closed that race years ago while the typed path did not. `RefreshOn401` is an `rest::HttpBackend` wrapping `ReqwestBackend`, installed by `AuthenticatedClient::build_client` through `Client::with_backend`. On a `401` it refreshes once through a new `pub(crate) Session::refresh_rejected` and replays the request once. It touches no generated code and covers every generated operation at once, so there is no per-call retry loop to keep in step and nothing to redo when the document is re-sourced. Why the transport seam and not spargen's `Middleware`: `Next::run` takes `self` by value, and `Next` is neither `Clone` nor constructible outside the generated runtime, so a middleware physically cannot send twice. `RetryBackend` is the precedent this follows, including its rule that a request whose `try_clone()` is `None` — a one-shot streaming body — is executed once and never replayed. `Session::refresh_rejected` wraps `ensure_refreshed(RefreshTrigger::Rejected(stale))` rather than reusing `Session::refresh`, because `refresh` re-reads the *current* token and would refresh again on top of a concurrent rotation, spending a single-use refresh token the server had already closed. Passing the exact token the server refused is what lets the existing single-flight gate coalesce. Exactly once, and by construction: the replay is straight-line code, not a loop with a counter. Four properties are pinned as unit tests — one refresh and one replay carrying the rotated token; a persistent `401` surfaced after exactly two upstream requests; a request with no bearer never retried; and a refresh that itself fails surfacing the **server's** `401` rather than a synthesized transport error, so the typed `Status401` mapping still fires and the caller reads the `error.*` code that separates an expired token from an unreadable revocation ledger. Over a socket, `a_token_the_server_stopped_honouring_is_refreshed_and_the_call_replayed` reproduces the race against the real router: the server validates `exp` against its injected clock, so advancing the fixture past `ACCESS_TOKEN_TTL` revokes the access token for real while the refresh token lives, and the client is handed the same pair with a far-future deadline. The pre-flight half cannot fire, so the call only succeeds through the reactive layer. Both new tests were confirmed to fail with the backend uninstalled. `reqwest_client()` becomes one shared client for the process. It owns a connection pool, and the FFI's escrow verbs build a fresh `AuthenticatedClient` per call because the API root is a per-call argument — so a per-client transport meant a fresh TLS handshake for every escrow read. Nothing here is configured per instance, so there is nothing to vary. Refs #408
`POST /v1/albums/{album_id}/upgrade` had no client. It is one of the four
`application/cbor` operations `build.rs` narrows out of the generated client — spargen
0.4's `classify_media` does not know that media type — and it was the only one of the
four with nothing hand-written behind it, so the SDK could not start the ceremony at
all.
`capsule_sdk::upgrade::UpgradeClient::begin` posts the signed intent **verbatim**. The
bytes are the canonical CBOR `capsule_core::crypto::upgrade` signed, and the server
verifies that signature against the proposing device's DSK in the account's published
directory; re-encoding them here would detach them from the signature and the failure
would look like a forged proposal.
Every refusal keeps its own identity and the code the *server* stamped, because these
are the refusals an admin reads: `409 error.album.upgrade_in_flight` carries the live
`intent_id`, `403 error.album.upgrade_proposer` means the signing device is not
published, and a client that flattened either into "malformed" would have someone
re-signing intents forever. The `413` body backstop carries no problem body at all, so
its code is the client's — `error.request.too_large`, not the intent-malformed code.
The phase decodes into typed ids and a `jiff::Timestamp`, so a caller compares instants:
the deadline is the one field in this ceremony where a string comparison would be a
correctness bug rather than an inconvenience. An unparseable deadline is a malformed
response, never a silent `None`, which would tell a client the ceremony never expires.
`GET` and `DELETE` on the same path are plain JSON and *are* generated; the module doc
says so and deliberately does not duplicate them.
Proven over a socket in `the_sdk_proposes_an_album_upgrade_over_a_socket`, which is the
only shape that can prove anything here: the directory is anchored, the album
provisioned, and the intent signed with the same `capsule-core` types the server
verifies with, so what the test asserts is that the bytes the SDK put on the wire are
the bytes that verify. A mock answering `200` would have proven only that the client can
post.
Refs #408
Five standing falsehoods in `capsule-sdk`'s own documentation, and the two `SLICES.md` rows this issue moves. - The document is **OpenAPI 3.2** and has been since Kynos was pinned with `openapi_as(SpecVersion::V3_2)`. `lib.rs` said 3.1 twice and `build.rs` once. - `mise run openapi` does not exist. The tasks are `openapi-kynos` and `openapi-check-kynos`. - `build.rs` said `capsule_sdk::directory` hand-writes two of the four `application/cbor` operations and "the other two have no client yet". One of those two had a client all along (`verify::StorageVerifyClient::fetch_receipt`) and the other now does (`capsule_sdk::upgrade`), so all four are named, with the one upstream change that retires all four. - The sync feed is not gRPC. `lib.rs` said `sync` stays hand-written because its protocol is too stateful for codegen, which is true of `upload` and false of `sync`: `S-D28` made the feed `GET /v1/sync`, a generated operation, and what is hand-written is the cursor and anti-rewind state machine over it. `ffi/tests.rs` still called `sync_pull` gRPC, and `FfiError`'s doc still offered foreign apps a "bare HTTP/gRPC status" to avoid. `SLICES.md`: `S-D12` records the route defect and its closure, and carries the escrow store response as an owed item pointing at #442. `S-D17` flips to `MIXED | done` — the Area corrects because the layer is live code in this workspace that does not re-scope, even though the client under it is regenerated — with the backend, the rejected `Middleware` alternative, and the socket case named, plus the reason `capsule_sdk::sync` keeps its own loop. Refs #408
Three review findings on this branch.
**The client no longer invents an `error.*` code.** A body-less `413` carries no problem
body, so there is no code to carry — and both hand-written clients were filling that gap
with `error.request.too_large`. Every other code either module reports is the one the
*server* stamped; a code minted on this side asserts that the server said something it
did not, and a client localizing it reads the SDK's guess as the server's judgement. Both
sites now report `code: None` with the English detail, and the variant already carries
the actionable half ("these bytes will not do, do not resend them"). The upgrade test
that asserted the minted code now asserts its absence.
**The auth/transport split has a test on both sides.** `RequestConstruction` carries two
completely different events — a bearer the session could not mint, and a connection that
never opened — and only the boxed source separates them. The transport side was pinned;
the auth side was not, so a spargen change to how a provider failure is boxed would have
silently demoted every expired refresh token to `Transport`, and the FFI would tell a
user to retry where it must tell them to sign in again.
`a_session_that_cannot_mint_a_bearer_is_an_auth_failure` drives a session whose stored
token is past expiry against a mock that serves no `/refresh`, on both the read and the
write path. Confirmed to fail with the downcast arm disabled.
**`reqwest_client()`'s doc stops overclaiming.** Sharing one client for the process does
not remove every per-construction client: `Client::with_backend` still builds its own
default `reqwest::Client` internally, one per `AuthenticatedClient`. That one only
assembles requests — every byte is executed through the backend, and so through the
shared client — so it opens no connection and costs one throwaway allocation. The comment
now says so, and names the `with_client_and_backend` constructor spargen would need to
remove even that.
Refs #408
…route-and-401-retry-408
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
capsule-sdk's recovery client called a route the contract does not serve, and threesmaller gaps rode along with it. This lands the route fix on the transport
AGENTS.mdmandates, the reactive
401layerS-D17asks for, a client for the fourthapplication/cboroperation, and truthful crate docs.Summary
The defect.
capsule-sdk/src/recovery/mod.rsbuilt{api_root}/backup/escrowfrom aconstand sent it with hand-writtenreqwestcalls. The committed Kynos document servesGET/PUT /v1/auth/escrow. Every networked recovery flow — enroll, the stale-cacherefresh, the guided re-wrap's escrow replace — failed against a real server while
S-D12read
done. It survived because the path was a string constant no gate checks and themodule's own mock answered whichever path it was handed.
The fix is not the constant. Both escrow operations are
application/octet-streamineach direction, a media type spargen lowers, so both are already generated and neither is
narrowed out in
build.rs.RecoveryClientnow holds oneAuthenticatedClientandorchestrates
fetch_escrow/store_escrow; the path is a function of the document andcannot drift again.
Alongside it:
S-D17—RefreshOn401, anrest::HttpBackendwrappingReqwestBackend, installedby
AuthenticatedClient. Every generated operation now recovers from a401with onesingle-flight refresh and exactly one replay. No generated code is touched.
capsule_sdk::upgrade— a client forPOST /v1/albums/{album_id}/upgrade, the fourthapplication/cboroperation, sending the signed intent verbatim.OpenAPI 3.1→3.2,mise run openapi→mise run openapi-kynos, thehand-written CBOR client count, and the gRPC-sync-feed claims in
client.rs,lib.rs,auth.rsandffi.rs(sync.rsstill carries four such lines and is outside this lane'smanifest — see Unresolved review notes).
Both in-repo escrow mocks now route on
/v1/auth/escrowand answer501elsewhere, so aroute regression fails loudly instead of reading as "no escrow stored" — and the socket test
asserts the route from both ends through the fixture's in-process client rather than relying
on how the router happens to render a
404.Six commits plus a base merge, each coherent on its own and each revertable alone:
1a2656549d208416fb5f5S-D17—RefreshOn4019143e74capsule_sdk::upgrade17f17f67c71817732967fchore/freeze-capsule-core-api-399atf508bf1a(clean; no conflicts, no overlapping files)Validation
Every command below was run inside the worktree on the final head
732967f9— after themerge of
chore/freeze-capsule-core-api-399atf508bf1a— and its result observed.mise run check-rustis reported as its own fifteen steps because the aggregate task waskilled twice by the machine (exit 137/143) under parallel-lane load, never by a check failing;
each step was then run and observed individually.
cargo nextest run -p capsule-sdkcargo nextest run -p capsule-sdk --features fficargo nextest run -p capsule-server --test sdk_clientmise run format-check-rustmise run lint-check-rustmise run doc-check-rust--document-private-itemsgate)mise run i18n-checkmise run i18n-guardmise run openapi-check-kynosmise run architecture-checkmise run license-checkmise run translate-readme-checkmise run build-rustmise run build-check-wasmmise run build-ffimise run lint-check-ffimise run gen-bindingstarget/bindingsand are not committed, soFfiError::Escrow's newcodemember changes no tracked filemise run verify-examplesmise run test-rustcapsule-core --features ffi729/729,capsule-sdk --features ffi177/177mise run check-docs-truthmise run check-mdDeliberate negative controls. Three of the new tests were confirmed to fail with the
change backed out, so none passes vacuously:
Client::with_backendreverted towith_client,client::tests::a_401_is_refreshed_once_and_the_call_replayedanda_token_the_server_stopped_honouring_is_refreshed_and_the_call_replayedboth fail with theserver's
401;rest::AuthErrordowncast arm disabled,a_session_that_cannot_mint_a_bearer_is_an_auth_failurefails — the case exists preciselybecause that arm had no test;
store_escrowagainst/backup/escrowanswers an undocumented404.CI on this branch (last observed at
17f17f64, pre-merge): every required check green(
requiredSUCCESS), including Rust fmt+clippy+build, Rust tests, all four cross builds,docs-truth, markdown and commit lint. One job fails — Build Capsule.apk + :core JVM smoke —
and it is pre-existing: the same job fails on PR #426, this branch's base, and nothing
here touches Kotlin, Android or Gradle.
Risks and rollout
Client-only.
openapi.json,capsule-server/src/**andcapsule-core/**are untouched, sothe wire contract is unchanged and
openapi-check-kynosstays byte-identical.RecoveryClient::newbecoming fallible is a source break inside this crate only(
capsule-sdkispublish = false); both call sites are inffi.rs.RecoveryErrorlosesBodyandAuthand gains five variants. Nothing outsidecapsule-sdkmatches on it.FfiError::Escrowgains acode: Option<String>member, which changes the uniffibinding shape for Swift and Kotlin (decision 6). The generated bindings are emitted to
target/bindingsand are not committed, so no tracked file changes andmise run gen-bindingspasses; the only in-repo consumer matchesEscrow { .. }and is unaffected.A native app that constructed the variant positionally would need the extra field — none
does today, and the change is what lets a client tell "set up a recovery key" from "we could
not read the one you have".
AuthenticatedClient's observable behaviour changes: every generated operation now replaysonce on a
401. The operations it serves carry in-memory bodies; the one streamed shaperides the hand-written
upload.rs, andtry_clone() == Noneshort-circuits it regardless.git revertper commit; no persisted data and no deployed behaviour.Related Issues
Closes #408
Filed by this lane for work it deliberately did not do:
spargen: classify_media has no application/cbor, forcing four hand-written clients in capsule-sdk. The upstream change that retirescapsule_sdk::directory,capsule_sdk::upgrade,verify::fetch_receiptand the fourOmitRules inbuild.rs.sdk: fetch_receipt decodes JSON where the contract serves application/cbor. Thesame defect class and the same cause as this issue, excluded by decision 1 below.
sdk: store_escrow discards stored_at/replaced, so the stale-cache rule has nothing to read. Carried on theS-D12row as its owed remainder.Contributor Checklist
Decisions taken
Issue 408 - sdk: recovery calls a route the contract does not serve, and three smaller gaps
Plan: v1 (planned against f433d91; executed on the head of lane #399's branch)
Branch: fix/sdk-escrow-route-and-401-retry-408
Base: the head of lane #399's branch (chore/freeze-capsule-core-api-399), stacked; the PR targets that branch until it merges
Worktree: /var/mnt/scratch/golem/dev/Capsulsaurus/Capsule.worktrees/Capsule-fix-sdk-escrow-route-and-401-retry-408
Cause: capsule-sdk/src/recovery/mod.rs:41 hand-writes a reqwest path to
{api_root}/backup/escrow; the contract serves GET/PUT /v1/auth/escrow, and those operations are already generated (octet-stream is a media type spargen lowers). The path survived because S-D28 re-sourced the document from Kynos and nobody re-checked the hand-written client.Touches: capsule-sdk/src/{recovery/mod.rs,client.rs,auth.rs,upgrade.rs (new),lib.rs,ffi.rs}, capsule-sdk/build.rs, capsule-server/tests/sdk_client.rs (append-only), SLICES.md (rows/blocks S-D12 and S-D17 ONLY)
Will not: touch Cargo.toml default-members (#399 owns it), capsule-server/openapi.json, capsule-server/src/, capsule-sdk/src/sync.rs, capsule-sdk/src/verify.rs, capsule-core/
Lane: serialised behind #399
Settled: capsule-sdk joins default-members — settled by #399. Base branch = head of PR #418 → this lane stacks on #399's head.
Decisions taken.
Deliverable boundary - all four issue bullets in one lane, minus the receipt-decode defect.
Taken: Ship the escrow fix, S-D17, the upgrade client, the doc corrections and the filed spargen issue as five slices on one branch; the in-process server harness at capsule-server/tests/sdk_client.rs:45-129 already exists.
Rejected: Also fix capsule-sdk/src/verify.rs:305-313, which decodes GET /v1/upload/{id}/receipt as JSON while the handler returns Binary (capsule-server/src/routes/receipts.rs:91) - not in sdk: recovery calls a route the contract does not serve, and three smaller gaps #408, changes the release-gate path that verify/tests.rs:263-304 mocks as JSON, and needs an attestation-key fixture over a socket: a second issue's worth of test work.
Reverses: git revert the branch; the receipt defect is untouched either way.
Filed: the lane files "sdk: fetch_receipt decodes JSON where the contract serves application/cbor".
Escrow transport - move to the generated client rather than fix the path string.
Taken: Replace recovery/mod.rs's hand-written reqwest calls with AuthenticatedClient::{fetch_escrow, store_escrow}; AGENTS.md requires generated parsing, the generated methods exist, and the route cannot drift again.
Rejected: Change ESCROW_PATH to "v1/auth/escrow" and keep the reqwest path - keeps a second parser the rule forbids and a route string no gate checks.
Reverses: restore recovery/mod.rs from the parent commit and set ESCROW_PATH to "v1/auth/escrow".
S-D17 seam - an HttpBackend wrapper, not the Middleware trait.
Taken: RefreshOn401 as rest::HttpBackend wrapping ReqwestBackend, installed via Client::with_backend in client.rs::build_client, replaying with Request::try_clone() and refreshing through a new pub(crate) Session::refresh_rejected. No generated code touched; every generated operation is covered.
Rejected: spargen's Middleware trait - Next::run consumes self and Next is not Clone/constructible (rest_client.rs:1250-1275), so a middleware cannot send twice. Also rejected: copying sync.rs:472-499's per-call loop into callers - the duplication S-D17 exists to remove.
Reverses: revert client.rs to Client::with_client and delete refresh_rejected.
sync.rs's bespoke 401 loop - leave it.
Taken: SyncConsumer keeps its own refresh-and-retry; it has a static-token mode (SyncAuth::Static, sync.rs:460) with no session to refresh and interleaves 401 with the shared retry engine's transient class.
Rejected: Route SyncConsumer through AuthenticatedClient - AuthenticatedClient::new takes a Session unconditionally (client.rs:60); adopting it means dropping static-token mode or widening AuthenticatedClient, both larger than this issue.
Reverses: construct SyncConsumer's client with with_backend and delete the 401 arm of the pull loop once AuthenticatedClient grows a static-token constructor.
Decisions taken inside the manifest, during delivery (same shape; numbering continues the record).
An uncoded 404 on the escrow fetch is a wire failure, never "no escrow".
Taken: A
404whose body is not a parseableCodedProblemmaps toRecoveryError::Transport, never toNotEnrolled. Only the server's own codedbody proves an unenrolled account; an intermediary's
404must not read as"enroll first". Pinned by
an_uncoded_404_is_not_read_as_an_empty_escrow.Rejected: Fall back to
NotEnrolledon any404, which is what the hand-written client did.Evidence: that reading is precisely the defect class this issue exists to close -
it let
backup/escrowlook like an account that had escrowed nothing for a wholeslice. An intermediary answering
404 text/htmlis a broken path, not anenrollment state.
Reverses: map a bare
404toNotEnrolledinwire_error'sDecodearm - and reintroducethe blindness.
FfiError::Escrow gains a
codefield.Taken: Add
code: Option<String>, populated fromRecoveryError::error_code(), so Swiftand Kotlin can switch on
error.escrow.*-not_stored("set up a recovery key")against
unavailable("we could not read the one you have"). The enum's own docalready claimed every variant carries the catalog code, and
Escrowwas the onethat did not. This is a uniffi binding shape change; see Risks and rollout.
Rejected: Fold the code into the
messagestring. Evidence: foreign clients cannot switchon a substring, which is the entire reason the
{ error, code }contract exists.Also rejected: leave it and file an issue - this lane is the one that introduced
error_code(), and shipping an accessor no boundary can read is a half-done fix.Reverses: drop the field and the three
code:initializers.A body-less 413 carries no code - the client never mints one.
Taken: Both hand-written clients report
code: Nonewith an English detail on413(
recovery/mod.rs,upgrade.rs). The transport backstop sends no problem body, sothere is no code; every other code either module reports is the one the server
stamped, and a code minted on this side asserts the server said something it did
not. The typed variant already carries the actionable half ("these bytes will not
do, do not resend them").
Rejected: Mint
error.request.too_largeclient-side, which is what the first version ofthis branch did. Evidence: a client localizing it would read the SDK's guess as
the server's judgement, and it is the only code in either module with no server
behind it. Also rejected: stamp the code server-side in the
413backstop -capsule-serveris outside this lane's manifest; recorded here as a note for thelane that owns
problem.rs/limits.rs.Reverses: restore the two
REQUEST_TOO_LARGEarms and their assertions.Manifest widened by one file - capsule-sdk/src/ffi/tests.rs.
Taken: Repoint the ffi module's own test mock from
/api/backup/escrowto/api/v1/auth/escrow, and give itsPUTa JSONStoreEscrowResponseand its emptyGETan RFC 9457 body. The record's Touches line namescapsule-sdk/src/ffi.rs;the
ffimodule's tests live in the siblingffi/tests.rs, and the plan did notforesee that that mock hardcodes the route. Without it
ffi::tests::ffi_escrow_and_device_directory_round_tripfails against its ownmock, so slice 1 cannot be delivered coherently at all. Recorded here rather than
taken silently, per the manifest rule.
Rejected: Stop and return "needs re-plan". Evidence: the file is the test submodule of a
file already in the manifest, the change is a route string and two response bodies
in a test double, and no other lane touches
capsule-sdk/src/ffi/**- freezing alane over a mock's path constant costs the whole deliverable and buys nothing.
Reverses: revert the
ffi/tests.rshunk; the test then fails, which is the honest signal.RecoveryError::Auth deleted, and RequestConstruction split by its source.
Taken: Drop the
Auth(#[from] AuthError)variant - once the reqwest path is gone nothingcan construct it - and instead discriminate spargen's
RequestConstructionclassby downcasting its source to the generated runtime's
AuthError, which only thebearer provider produces. A dead session therefore still reaches a caller as
Unauthorized->FfiError::Auth, preserving the re-auth signal the hand-writtenpath gave, while a refused connection stays
Transport. Both sides are pinned:a_session_that_cannot_mint_a_bearer_is_an_auth_failureandan_unreachable_endpoint_is_a_transport_failure_not_an_auth_one.Rejected: (a) Keep
Authas an unconstructible variant - a promise no code path can keep, ina public error enum. (b) Map the whole
RequestConstructionclass toUnauthorized- the first attempt, refuted by the adversarial read: reqwest builds every failure
of the request it executes with
error::request(..), sois_request()is true forconnection-refused, DNS and TLS, and the mapping told an offline device to sign in
again.
Reverses: restore the variant and map
RequestConstructiontoTransportunconditionally;the cost is that a dead session reads as a network blip at the FFI boundary.
One shared reqwest client for the process.
Taken: Make
client::reqwest_client()aOnceLockreturning clones. The FFI's escrowverbs build a fresh
AuthenticatedClientper call because the API root is aper-call argument, and a
reqwest::Clientowns a connection pool - so before this,every escrow read paid a fresh TLS handshake where the old code rode the session's
shared client. Nothing here is configured per instance, so there is nothing to
vary. The residual cost is named in the code:
Client::with_backendstill buildsits own request-assembly client per construction, which opens no connection.
Rejected: Cache an
AuthenticatedClientonFfiSession. Evidence: the base URL is aper-call argument, so the cache needs a key and an eviction rule - larger than the
regression it fixes, and outside this issue.
Reverses: inline the builder again; correctness is unaffected either way.
Unresolved review notes
Raised against this diff before committing (a focused sub-agent read the first commit
adversarially) and in the lane review of
17f17f64. Everything actionable was fixed in49d2084and7c71817; what remains is outside the lane manifest and is listed here ratherthan widened into silently.
capsule-core/src/lifecycle/backup.rs:29still documentsPUT /backup/escrow. Thesame stale route, in a doc comment.
capsule-core/**is explicitly out of this lane'smanifest (core: freeze the capsule-core public API and remove the dead surface #399 owns that tree), so it is untouched. One-line fix for whoever holds it next.
SLICES.md:3384lists the surface asGET/PUT /v1/auth/backup/escrow. Also wrong —the route is
/v1/auth/escrow. That row is neitherS-D12norS-D17, and this lane mayedit only those two, so it is untouched.
capsule-sdk/src/sync.rs:213,:258,:430still describe the feed in gRPC terms(
:11names it historically, which is fine). Pre-existing, andsync.rsis named in therecord's Will not list — decision 4 keeps its bespoke
401loop — so its prose was leftalone. The issue's doc-truth bullet named
client.rs, which is fixed, along withlib.rs,auth.rsandffi.rs.413backstop sends noerror.*code at all. Decision 7 stops the client frominventing one, which leaves the gap where it belongs: on the server. Stamping
error.request.too_largeincapsule-server's body-limit backstop(
problem.rs/limits.rs) is the real fix, andcapsule-server/src/**is outside thislane's manifest. Noted for the lane that owns it; no issue filed, per this lane's brief.
Client::with_backendbuilds one throwawayreqwest::ClientperAuthenticatedClient.It only assembles requests — execution goes through the shared client below it — so it opens
no connection and costs one allocation. Removing even that needs a
with_client_and_backendconstructor spargen does not expose. That is generator work of the same kind as the
application/cborgap and would land alongside spargen: classify_media has no application/cbor, forcing four hand-written clients in capsule-sdk #440; spargen: classify_media has no application/cbor, forcing four hand-written clients in capsule-sdk #440 was not edited to say so.RecoveryError,UpgradeErrorandFfiErrorare not#[non_exhaustive]. This changeadds variants and a field to all three. Nothing outside
capsule-sdkmatches on any of them(
capsule-sdkispublish = false, and the only in-repo consumer ofFfiError::Escrowmatches
{ .. }), so nothing breaks — but a future variant would be a source break again.Marking them
non_exhaustiveis itself a break and was not taken unilaterally.