test: add daemon RPC wire-surface compatibility gate - #1717
Conversation
#1432) ADR 0006 fixes exactly when DAEMON_RPC_PROTOCOL_VERSION must be bumped, and nothing checked that it was. The runtime guard (readRemoteDaemonHealth) refuses a mismatched peer, but only fires when someone remembered the bump — a wire change that skipped it left both sides advertising protocol 2 while parsing different payloads, which is the failure ADR 0006 exists to prevent. Local daemons cannot skew (isReusableDaemonInfo takes over on any package version mismatch). Cross-machine is skewed by design — proxy, cloud/limrun, a remote macOS host — and ADR 0006 explicitly rules package version out as the compatibility gate there, so the one boundary where skew is intended was the one boundary with no gate. test/wire-compat/surface.ts declares the wire surface grouped by the ADR bullet each group serves, quoting it, with an `uncovered` note where a bullet is only partly digestible (the /health and /rpc literals inside http-server.ts stay reviewer-owned: a moved route 404s at connect time rather than misparsing). ledger.json records what each declaration hashes to, at which protocol version. Two gates, split for the same reason the replay-compat corpus splits: - unit-core holds the ledger to its source and prints the digest to paste; - Released-Surface Compatibility reads the ledger at the last RELEASED tag and requires the drift since then to carry a bump or a compatibleChanges ack. From one commit a bumped ledger and an unbumped one are both just an edited file, so only a released baseline can tell them apart. Acks are keyed by the digest they cover, so one "added an optional field" cannot launder later changes. Digests ignore comments and formatting; the manifest's closure is derived from the AST, so a field typed by an unlisted sibling fails rather than sitting outside the gate. CI cost: one added job (checkout + toolchain + two node scripts, ~1 min), mirroring the existing full-history replay-compat job.
|
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
|
Exact-head review at P1 — the manifest materially overclaims ADR 0006 coverage. P1 — imported/re-exported payload shapes escape closure. The bump/ack/removal model tests are meaningful for declarations already listed, but the released-tag lane is intentionally bootstrap-only until a release carries the ledger and neither test class catches these completeness holes. No device evidence applies to this tooling-only change; green CI does not clear the gate-integrity defects. No ready label applied. |
|
One additional readiness item: the PR title is not in the repository’s required conventional format. Please rename it to something like |
thymikee
changed the title
…1432) Addresses both review P1s on #1717. P1 — the manifest materially overclaimed ADR 0006 coverage. It quoted all four bullets while digesting only the payload TYPES, so the producer and consumer seams could break a skewed peer without moving a listed digest. Now listed on both sides of every boundary: JSON-RPC method sets and the projections that turn each method's params into a DaemonRequest, createRpcError/sendJson/ writeRpcResponseEnvelope, resolveToken and the auth-hook types, upload preflight/finalize/308 handlers and the resumable ticket shape, artifact route and download/inventory framing, REST error mapping, and the client's own payload builder, lease-method mapping, response parser and error projection. 57 -> 117 declarations. What stays out is now named rather than implied: createDaemonHttpServer's dispatch wiring and the /health and /rpc literals inside it. Everything it dispatches WITH is digested individually, and a moved route 404s at connect time rather than misparsing — the loud failure, not the silent one. P1 — imported and re-exported payload shapes escaped the closure. declarationHomes() scanned only the manifest's own files and the walk continued silently when a name could not be placed, so a listed type could gain foo?: ImportedShape from a new module and stay green. Resolution is now explicit and fails closed: relative imports, workspace specifiers (through the owning package's own exports map, so a re-pointed export cannot drop a type), and facade re-export chains. Every referenced name must land on a listed declaration, a waiver with a written reason, a declared external module, or the TS/Node global set. Fixed two extractor blind spots the walk exposed: a declaration's own generic parameters and `as const` were being reported as references. Planted-red proofs (wire-mutations.test.ts): 13 cases independently mutate method naming, response serialization, response parsing, auth projection, upload ticket shape, 308 framing, artifact framing, REST error mapping, and progress framing, each asserting the digest moves; 3 probes prove the closure really reaches across a package boundary, a facade re-export, and a plain relative import. Mutations apply inside the declaration's own span — a whole-file replace silently hit a sibling sharing the substring, which is how the first draft of one case passed vacuously. The largest waiver pair (InternalRequestOptions, CommandFlags) rests on ADR 0006's own additive rule: they reach the peer inside DaemonRequest's untyped flags/input bags, and the decision says a new flag needs no bump. Digesting them would fire the gate on every new CLI flag and train reviewers to rubber-stamp acks.
|
Both P1s were right. Fixed in P1 — manifest overclaimed ADR 0006 coverage. Correct, and it was the exact failure the manifest's own
What remains outside is now named, not implied: P1 — imported/re-exported shapes escaped closure. Also correct, and the Running it fail-closed immediately surfaced 10 real gaps, of which 4 were genuine wire types I'd missed ( Planted-red proofs ( One place I narrowed instead of expanding, flagged for your call.
Generated by Claude Code |
|
Exact-head re-review at One P1 remains: the expanded manifest still omits the consumer half of the auxiliary HTTP boundaries while claiming both sides of response/upload/artifact framing. There are no red checks: every completed job is green, including the compatibility and coverage lanes; only iOS Smoke Tests remains pending, which explains the current UNSTABLE status. No device evidence applies. No ready label applied. |
Addresses the remaining review P1 on #1717. The manifest claimed both sides of response/upload/artifact framing while listing nothing from upload-client.ts, daemon-artifacts.ts, or the health consumer in daemon-client-transport.ts, so those parsers could narrow without moving a listed digest or protocol 2. Now listed (117 -> 141 declarations): - /health consumer: RemoteDaemonHealth, readHealthPayload, readDaemonHttpHealth, readRemoteDaemonHealth. This is the sharpest of the three — narrowing the reader or the comparison disables the very refusal ADR 0006 exists to guarantee, and nothing else in the repo would notice. - /upload consumer: UploadResponse, UploadPreflightResponse, UploadPreflightResult, parseUploadPreflightResult, requestUploadPreflight, uploadDirectArtifact, tryDirectUploadWithResume, shouldRetryDirectUpload, finalizeDirectUpload, uploadLegacyArtifact, ARTIFACT_HASH_ALGORITHM, isStringRecord, and PreparedUploadArtifact — whose sha256/sizeBytes/fileName/artifactType/ contentType fields ARE the preflight body the daemon parses. - /artifacts/* consumer: DaemonArtifactEndpoint, buildDaemonArtifactUrl, isRemoteDaemon, DownloadRemoteArtifactParams, downloadRemoteArtifact, materializeRemoteArtifacts, resolveMaterializedArtifactPath. Running the closure fail-closed over the new files surfaced three more stops, each decided rather than skipped: PreparedUploadArtifact listed (it is payload), UploadProgressSink waived (client-local rendering, never leaves the process), and src/daemon/types.ts#DaemonArtifact waived as a re-export alias of the listed kernel type, matching its DaemonRequest/DaemonResponse siblings. 10 more planted-red mutations cover the new seams: health version-read and mismatch-refusal defeated, RemoteDaemonHealth field dropped, preflight parser narrowed, preflight/legacy response shapes narrowed, finalize body key renamed, ticket field renamed, artifact tenant header dropped, artifact URL moved. A fourth closure probe proves the upload-consumer files are genuinely reached by the walk rather than merely listed. 22 -> 33 tests. The README now states the coverage as a producer/consumer table per boundary, so the claim is checkable at a glance instead of asserted in prose.
|
Right again — I listed the consumer half for Coverage is now stated as a table in the README so the claim is checkable at a glance rather than asserted in prose:
The
Running the closure fail-closed over the new files surfaced three more stops, each decided rather than skipped: 10 new planted-red mutations (33 tests total): health version-read defeated, mismatch refusal short-circuited,
Two rounds, two real overclaims — the pattern is that I kept writing the coverage sentence ahead of the coverage. The README table and the Generated by Claude Code |
|
Exact-head re-review at The new health, artifact, preflight, legacy, direct/finalize, and prepared-artifact consumer coverage is meaningful, and the scoped mutations/closure probe are non-vacuous. One material That module owns the client side of resumable framing: Add the declarations that own this parser/header surface (or a focused declaration boundary around it), plus planted scoped mutations proving that changing accepted offset headers/ |
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.
|
Correct, and the framing is the useful part: "the existing mutation proves the daemon still produces 308, not that the client still consumes the released 308 contract." Listed: On 6 new planted-red mutations, each scoped to its declaration:
39 tests total. Closure fail-closed surfaced two more stops:
Three rounds, three real gaps, and the pattern is consistent enough to name: each time I wrote the coverage sentence ahead of the coverage. I've put that in the README explicitly, with the instruction to trust the producer/consumer table and the Generated by Claude Code |
|
Exact-head re-review at The prior P1 is fixed. The manifest now includes the actual The six new regressions are meaningful: each pins the exact unmutated declaration to the ledger, applies a scoped mutation inside that declaration, and proves the digest moves across offset headers, Range grammar, Content-Range, 308 status, response shape, and header coercion. The closure additions and documented remaining server-dispatch gap are appropriately narrow. No new actionable findings. GitHub is CLEAN/MERGEABLE with all 30 checks green, including Released-Surface Compatibility, Coverage, Integration, and all platform smokes. Device evidence is N/A for this tooling-only gate. |
thymikee
added
the
ready-for-human
Merging main brought in the daemon RPC wire-compat gate (#1717), which adds the `own:daemon-wire-compat` selector rule. The manifest already reported the category as represented, but only incidentally: `packages/kernel/src/errors.ts` — the sample standing in for "workspace package source" — happens to be listed in WIRE_SURFACE_FILES. That satisfies the universe check while leaving the path-filter reachability assertion pointed at `packages/**` rather than at the ledger directory, so a `paths-ignore` excluding `test/wire-compat/**` would not have been caught. Naming the directory directly closes that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SkS4S8XXrfkJ8TD1VBKkvJ
2 participants
Implements a comprehensive gate to enforce ADR 0006 daemon RPC protocol versioning rules, ensuring wire-surface changes are accompanied by appropriate protocol version bumps or compatibility acknowledgments.
Summary
This PR adds a two-part compatibility checking system for the daemon RPC wire surface:
test/wire-compat/wire-compat.test.ts): Runs on every PR to verify the ledger matches the current sourcescripts/wire-compat/run.ts): Runs in CI with full git history to compare against the last released tagThe system uses AST-based declaration digesting to detect shape changes without false positives from unrelated edits, and enforces ADR 0006's rules: breaking changes require a protocol version bump, additive changes can be acknowledged without a bump.
Key Changes
test/wire-compat/declaration-digest.ts): AST-based hashing of individual type declarations usingoxc-parser, with comment/whitespace normalization to avoid false positives from formatting changestest/wire-compat/surface.ts): Declares which declarations cross the RPC boundary, grouped by the ADR 0006 rule each serves, with explicit coverage gaps documentedtest/wire-compat/ledger.json,test/wire-compat/ledger.ts): JSON-based ledger recording declaration digests at each protocol version, plus compatible-change acknowledgments keyed by digesttest/wire-compat/wire-compat.test.ts): Verifies ledger consistency with source, closure over referenced types, and proper documentationscripts/wire-compat/model.ts,scripts/wire-compat/model.test.ts): Pure logic for determining whether wire changes are justified (bump, ack, or additive), with comprehensive test fixturesscripts/wire-compat/run.ts): Reads the ledger from the last released tag and compares against current state.github/workflows/ci.yml): Newreleased-surface-compatjob with full git historyscripts/check-affected/): Wire-compat gate is selected when manifest files or ledger changeImplementation Details
oxc-parser(the same AST parser used elsewhere in the repo) rather than regex to avoid enumerating every declaration formhttps://claude.ai/code/session_01TZkMfMyWpV3pv3hWUAn9vN