Skip to content

Commit 02cc23a

Browse files
claudeclaude
committed
test: list the client half of the resumable 308 contract (#1432)
Addresses the third review P1 on #1717. Listing the daemon's handleResumableUpload proved it still PRODUCES 308; nothing proved the client still CONSUMES the released one. src/remote/upload-stream.ts owns that half and was entirely outside the manifest, so a newer client could stop accepting `upload-offset`, change how it reads `Range: bytes=0-N`, or emit a different resumed `Content-Range` without moving one of the 141 listed digests. Now listed (141 -> 151): UploadStreamResponse, streamFileToHttpRequest, streamFileToHttpRequestAttempt, buildUploadRequestHeaders, isUploadResumeStatus, isUploadRedirectStatus, parseUploadResumeOffset, parseNonNegativeIntegerHeader, firstHeaderValue, MAX_UPLOAD_REDIRECTS. streamFileToHttpRequestAttempt is listed despite its size, unlike createDaemonHttpServer which stays in `uncovered`. The distinction is stated at the declaration: the HTTP server only dispatches to handlers that are each digested, while the attempt loop IS the resume state machine — it decides whether a 308 continues the upload and what the next request carries, so its sequencing alone can break a released daemon while every helper keeps its digest. 6 new planted-red mutations prove the client half moves the ledger: a dropped `upload-offset` fallback, narrowed Range parsing, a changed resumed Content-Range, 308 no longer treated as continue, a narrowed UploadStreamResponse, and dropped header-value coercion. 33 -> 39 tests. Closure fail-closed surfaced two more stops: UploadStreamProgressOptions waived (local byte-progress rendering) and URL/URLSearchParams added to the global set. README now carries a `/upload` resume row in the producer/consumer table, and names the pattern behind three rounds of review: the coverage sentence kept getting written ahead of the coverage, so the table and the `uncovered` notes are the claims to trust — they are checkable against surface.ts, prose is not.
1 parent b2da20c commit 02cc23a

6 files changed

Lines changed: 101 additions & 8 deletions

File tree

‎test/wire-compat/README.md‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,12 @@ place version skew is designed in was the one place with no gate.
3333

3434
The mutation lane exists because the first two answer "the manifest describes
3535
today's source", never "the manifest describes the *right* source". A gate can
36-
list 141 declarations, pass every check, and still miss the seam that breaks a
36+
list 151 declarations, pass every check, and still miss the seam that breaks a
3737
skewed peer — which is exactly what review found in the first version of this
3838
directory. Each case there mutates one real listed declaration the way a real
3939
break would (method renamed, envelope unframed, auth header dropped, upload
4040
ticket field renamed, 308 downgraded, artifact route narrowed, health-version
41-
check defeated) and asserts the
41+
check defeated, 308 resume contract broken on the client side) and asserts the
4242
digest moves. Two guards keep them honest: the mutation is applied **inside the
4343
declaration's own span** so it cannot silently hit a sibling that shares the
4444
substring, and the unmutated digest must equal the ledger's so the case is
@@ -96,13 +96,19 @@ admits the gap (AGENTS.md, "a registry claim is not a semantic check").
9696
| `/rpc` | method sets, request projections, `createRpcError`, envelope serializers | `buildHttpRpcPayload`, lease-method mapping, `parseDaemonHttpResponseBody`, `toDaemonHttpRpcError` |
9797
| `/health` | `DaemonHealthPayload`, `buildDaemonHealthPayload` | `RemoteDaemonHealth`, `readHealthPayload`, `readRemoteDaemonHealth` |
9898
| `/upload` | route resolver, preflight/finalize/308 handlers, body parsers | `UploadPreflightResponse`, `parseUploadPreflightResult`, direct/legacy/finalize senders, `PreparedUploadArtifact` |
99+
| `/upload` resume (308) | `handleResumableUpload` emits 308 + offset headers | `isUploadResumeStatus`, `parseUploadResumeOffset`, `buildUploadRequestHeaders`, `streamFileToHttpRequestAttempt` |
99100
| `/artifacts/*` | route resolver, inventory and download framing | `buildDaemonArtifactUrl`, `downloadRemoteArtifact`, materialization |
100101

101-
A client-only change breaks an older daemon just as surely as the reverse — and the `/health`
102-
consumer is the sharpest case of all, since narrowing `readHealthPayload` or `readRemoteDaemonHealth`
103-
disables the very refusal ADR 0006 was written to guarantee. Both took two review rounds to get
104-
right: the first version digested only payload *types*, the second still omitted the auxiliary
105-
consumers while claiming both sides.
102+
A client-only change breaks an older daemon just as surely as the reverse. Two consumers are the
103+
sharpest cases: narrowing `readHealthPayload` or `readRemoteDaemonHealth` disables the very refusal
104+
ADR 0006 was written to guarantee, and narrowing `parseUploadResumeOffset` or
105+
`buildUploadRequestHeaders` breaks resume against a daemon still emitting the released 308 contract.
106+
107+
That took three review rounds, and the pattern is worth naming: each round the coverage sentence was
108+
written ahead of the coverage. Round one digested only payload *types*; round two added producers but
109+
left the auxiliary consumers out; round three still had the daemon *producing* 308 with nothing
110+
proving the client still *consumes* it. Prefer the table above and the `uncovered` notes over any
111+
prose claim — those are checkable against `surface.ts`, and a sentence is not.
106112

107113
The one remaining gap: `createDaemonHttpServer`, the 200-line dispatcher, and the `/health` and
108114
`/rpc` path literals inside it. Everything it dispatches *with* is digested individually, so what is

‎test/wire-compat/closure-policy.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@ export const WIRE_CLOSURE_WAIVERS: Readonly<Record<string, string>> = {
4949
'Re-export alias of the listed kernel DaemonResponse; the shape is fixed by packages/kernel/src/contracts.ts#DaemonResponse.',
5050
'src/daemon/types.ts#DaemonArtifact':
5151
'Re-export alias of the listed kernel DaemonArtifact; the shape is fixed by packages/kernel/src/contracts.ts#DaemonArtifact.',
52+
'src/remote/upload-stream.ts#UploadStreamProgressOptions':
53+
'Byte-progress callback options for local upload rendering. The bytes counted are digested through the request/response declarations; these options never cross the wire.',
5254
'src/remote/upload-progress.ts#UploadProgressSink':
5355
'A callback for client-local upload progress rendering. Its events never leave the client, unlike the daemon progress stream, whose envelope and events are listed.',
5456
'packages/contracts/src/request-progress.ts#RequestProgressSink':

‎test/wire-compat/ledger.json‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,17 @@
141141
"src/remote/upload-client.ts#shouldRetryDirectUpload": "sha256:29059f076ff7f2b9540ad15408be48792b2d63a601ef2b9fb2d54f5576d49bbd",
142142
"src/remote/upload-client.ts#tryDirectUploadWithResume": "sha256:0f57cf9bb1eefa3b988e5d723b2eebf2c4a622c3f7c808f35eae070771acafa6",
143143
"src/remote/upload-client.ts#uploadDirectArtifact": "sha256:5f26f8f7b6ada2fe2089a214fb8350e3a9f65725212952c17f6787b3ca8fe232",
144-
"src/remote/upload-client.ts#uploadLegacyArtifact": "sha256:f3ccab9c5ba18fc2f2000ea8e2a4bbb3cc97f7b50f6111192e513013f635cdd7"
144+
"src/remote/upload-client.ts#uploadLegacyArtifact": "sha256:f3ccab9c5ba18fc2f2000ea8e2a4bbb3cc97f7b50f6111192e513013f635cdd7",
145+
"src/remote/upload-stream.ts#MAX_UPLOAD_REDIRECTS": "sha256:12ff46dfe33a1dce95bdb79c90c17a3ba4f3919e7dc4b3a1b04590fff430f79f",
146+
"src/remote/upload-stream.ts#UploadStreamResponse": "sha256:724e284c2a6bd7b4b3a49b1d8c61c8923d6cc24777c3d76224a69d190e56adce",
147+
"src/remote/upload-stream.ts#buildUploadRequestHeaders": "sha256:827576b13a9f3e6cea445119a6a29b2ac5eaa23b66436cf810b2d09765c82a4c",
148+
"src/remote/upload-stream.ts#firstHeaderValue": "sha256:f6dba645814124f2ea2267e64a5d27c3bcff6dfc00ed692946180fc20e3d41de",
149+
"src/remote/upload-stream.ts#isUploadRedirectStatus": "sha256:4b18a4f97a02a7a609eba85d7d657b3cd2142d5ba6662a9b08cf4c0c9ed02006",
150+
"src/remote/upload-stream.ts#isUploadResumeStatus": "sha256:9f730ccd1c3401c326f8df67d1fd8243fef757357a38a1f53736df2b90bd8fdc",
151+
"src/remote/upload-stream.ts#parseNonNegativeIntegerHeader": "sha256:5cbe9c6994459d673d988a51407e65556e28175b187ce0a9d27cbb6331962834",
152+
"src/remote/upload-stream.ts#parseUploadResumeOffset": "sha256:297184ea22a6dc1cda9aee17034122ae0ea2f43d0355647f6fcf02f3f4b5ca0e",
153+
"src/remote/upload-stream.ts#streamFileToHttpRequest": "sha256:eaf2ea49957034f6a7e17d34092dc6cb1998812bddfd68381a0ac34d8a2de58c",
154+
"src/remote/upload-stream.ts#streamFileToHttpRequestAttempt": "sha256:da39a79fa7c1f81e55caf613eedc347c0f3a9a9711b265a4db185677532d9552"
145155
},
146156
"compatibleChanges": []
147157
}

‎test/wire-compat/module-resolution.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ const TS_GLOBALS = new Set([
5656
'Required',
5757
'ReturnType',
5858
'Set',
59+
'URL',
60+
'URLSearchParams',
5961
'Uint8Array',
6062
'WeakMap',
6163
]);

‎test/wire-compat/surface.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ const CLIENT_PROGRESS = 'src/daemon/client/daemon-client-progress.ts';
5454
const CLIENT_TRANSPORT = 'src/daemon/client/daemon-client-transport.ts';
5555
const UPLOAD_CLIENT = 'src/remote/upload-client.ts';
5656
const REMOTE_ARTIFACTS = 'src/remote/daemon-artifacts.ts';
57+
const UPLOAD_STREAM = 'src/remote/upload-stream.ts';
5758

5859
function from(file: string, ...names: string[]): WireDeclarationRef[] {
5960
return names.map((name) => ({ file, name }));
@@ -303,6 +304,31 @@ export const WIRE_SURFACE: readonly WireSurfaceGroup[] = [
303304
// The prepared artifact whose fields (sha256, sizeBytes, fileName,
304305
// artifactType, contentType) ARE the preflight body the daemon parses.
305306
...from('src/remote/upload-client-artifact.ts', 'PreparedUploadArtifact'),
307+
// Consumer side of the resumable 308 contract. Listing the daemon's
308+
// `handleResumableUpload` proves it still PRODUCES 308; it says nothing
309+
// about the client still CONSUMING the released one. These own which
310+
// offset headers are accepted (`x-upload-offset`, `upload-offset`,
311+
// `Range: bytes=0-N`) and what `Content-Range` a resumed PUT emits.
312+
//
313+
// `streamFileToHttpRequestAttempt` is listed despite its size, unlike
314+
// `createDaemonHttpServer` above: that one only dispatches to handlers
315+
// that are each digested, while this IS the resume state machine — it
316+
// decides whether a 308 continues the upload and what the next request
317+
// carries, so a change to its sequencing alone can break a released
318+
// daemon while every helper below keeps its digest.
319+
...from(
320+
UPLOAD_STREAM,
321+
'MAX_UPLOAD_REDIRECTS',
322+
'UploadStreamResponse',
323+
'streamFileToHttpRequest',
324+
'streamFileToHttpRequestAttempt',
325+
'buildUploadRequestHeaders',
326+
'isUploadRedirectStatus',
327+
'isUploadResumeStatus',
328+
'parseUploadResumeOffset',
329+
'parseNonNegativeIntegerHeader',
330+
'firstHeaderValue',
331+
),
306332
// Consumer side of /artifacts/*: URL construction, the inventory/header/
307333
// body materialization the client performs, and the artifact fields it
308334
// rewrites on the way through.

‎test/wire-compat/wire-mutations.test.ts‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,53 @@ const MUTATIONS: readonly WireMutation[] = [
213213
from: 'artifacts/',
214214
to: 'artifact/',
215215
},
216+
// The client half of the resumable 308 contract. Listing the daemon's
217+
// handleResumableUpload proves it still PRODUCES 308; these prove the client
218+
// still consumes the released one — which offset headers it accepts, how it
219+
// reads `Range`, what `Content-Range` a resumed PUT emits, and that 308 is
220+
// still the status it treats as "continue".
221+
{
222+
breakClass: '308 consumer: the client stops accepting a released offset header',
223+
file: 'src/remote/upload-stream.ts',
224+
name: 'parseUploadResumeOffset',
225+
from: "headers['x-upload-offset'] ?? headers['upload-offset']",
226+
to: "headers['x-upload-offset']",
227+
},
228+
{
229+
breakClass: '308 consumer: Range-header offset parsing narrows',
230+
file: 'src/remote/upload-stream.ts',
231+
name: 'parseUploadResumeOffset',
232+
from: '/^bytes=0-(\\d+)$/',
233+
to: '/^bytes 0-(\\d+)$/',
234+
},
235+
{
236+
breakClass: '308 consumer: the resumed request emits a different Content-Range',
237+
file: 'src/remote/upload-stream.ts',
238+
name: 'buildUploadRequestHeaders',
239+
from: "'content-range': `bytes ${startOffset}-${payloadSize - 1}/${payloadSize}`,",
240+
to: "'content-range': `bytes=${startOffset}-${payloadSize - 1}/${payloadSize}`,",
241+
},
242+
{
243+
breakClass: '308 consumer: the client stops treating 308 as resume-and-continue',
244+
file: 'src/remote/upload-stream.ts',
245+
name: 'isUploadResumeStatus',
246+
from: 'statusCode === 308',
247+
to: 'statusCode === 208',
248+
},
249+
{
250+
breakClass: '308 consumer: the streamed response shape a caller parses narrows',
251+
file: 'src/remote/upload-stream.ts',
252+
name: 'UploadStreamResponse',
253+
from: 'statusMessage?: string;',
254+
to: '',
255+
},
256+
{
257+
breakClass: '308 consumer: header value coercion drops a released form',
258+
file: 'src/remote/upload-stream.ts',
259+
name: 'parseNonNegativeIntegerHeader',
260+
from: 'firstHeaderValue(value)',
261+
to: 'undefined',
262+
},
216263
];
217264

218265
for (const mutation of MUTATIONS) {

0 commit comments

Comments
 (0)