Michał Pierzchała
05a1d76f2e
* test: gate daemon RPC wire compatibility against the last released tag (#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. * test: close wire-surface overclaim and make the closure fail closed (#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. * test: list the consumer half of the auxiliary HTTP boundaries (#1432) 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. * 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. --------- Co-authored-by: Claude <noreply@anthropic.com>
7.8 KiB
Contributing
Thanks for helping improve agent-device. This guide is the shortest path from a fresh checkout to a reviewable change. Detailed testing and device procedures live in the linked focused guides.
Set up the repository
Requirements:
- Node.js 22 or newer
- pnpm at the version pinned in
package.json - Android SDK tools (
adb) for Android work - Xcode (
simctl/devicectl) for Apple-platform work
pnpm install
pnpm build
package.json's packageManager field is the source of truth for pnpm, and CI rejects a different
version. Node distributions that include Corepack can activate it with corepack enable pnpm.
Newer Node distributions may not bundle Corepack; in that case, install the pinned pnpm version
using your Node version manager or the pnpm installation guide, then
confirm it with pnpm --version before installing dependencies.
The root install does not install the much larger Expo test-app dependency graph. If your change
touches examples/test-app, install it separately:
pnpm test-app:install
pnpm test-app:typecheck
Build the surface you changed
pnpm build compiles the TypeScript CLI and library. If a running development daemon must pick up
that build, use pnpm rebuild:cli; it builds and then stops the worktree-scoped daemon.
Build only the Apple runner target you changed:
pnpm build:xcuitest:ios
pnpm build:xcuitest:macos
pnpm build:xcuitest:tvos
pnpm build:xcuitest:visionos
Append :clean to any platform build when DerivedData may be stale, for example
pnpm build:xcuitest:macos:clean. pnpm build:xcuitest remains the shared iOS-and-macOS gate for
changes that affect both runners; it is not an all-platform build.
Android and macOS helper builds remain separate because they require their native toolchains:
pnpm build:android
pnpm build:macos-helper
There is intentionally no catch-all development build. Native toolchains are expensive and
independent, so agents and contributors should run the command for the surface they changed.
Use pnpm build:macos-helper:clean if a Swift cache was created in another worktree.
Prepare the npm package
pnpm publish and package-manager pack commands run prepack, which first checks synchronized MCP
metadata and then runs pnpm package:npm. This is the one completeness-oriented aggregate: it
builds the TypeScript distribution and all four Apple runner targets, clean-builds the macOS helper,
packages the Apple runner source, and rebuilds both Android helper APKs. Any failed build stops
packaging. It deliberately does not stop the worktree's development daemon; use pnpm rebuild:cli
when a running daemon needs to pick up a new TypeScript build.
pnpm package:npm is a release guard, not a routine development command. Use the specific commands
above while iterating.
Released-surface baselines roll forward on publish
Compatibility gates baseline against the last released tag, not against main, so publishing is
what advances them — there is no separate baseline-refresh step and no regenerate command. Tagging a
release makes that commit's test/wire-compat/ledger.json the new baseline for
pnpm check:daemon-wire-compat, and its .ad corpus tags the new ceiling for
pnpm check:replay-compat. The practical consequence for a normal PR: wire churn within an
unreleased branch is free, and only the net change since the last publish has to carry a
DAEMON_RPC_PROTOCOL_VERSION bump or a compatibleChanges acknowledgment. After a release that
bumped the protocol version, the acknowledgments accumulated against the previous one no longer
match any current digest and are dropped — git history keeps the audit trail.
Validate a change
Use the smallest trustworthy loop while editing:
pnpm check:quick # lint + TypeScript
pnpm test:maestro-compat # example of a focused family suite
pnpm exec vitest run path/to/file.test.ts
Before pushing a normal code change, let the repository derive the required gates:
pnpm check:affected --run
The selector combines the committed diff with staged, unstaged, and untracked files. Unknown, workflow, lockfile, and selector-owning changes fail open to the full local set. It reports device/toolchain checks that remain GitHub-authoritative instead of trying to run them implicitly.
For broad refactors or when explicitly requested, run the deterministic core aggregate:
pnpm check
pnpm check covers formatting, lint, typechecking, layering, dependency-graph parity, production
exports, MCP metadata, the distributable build, bundle ownership, Fallow, unit tests, and local smoke
tests. It is intentionally not a simulation of every CI job: coverage, provider integration,
history-backed compatibility, specialized toolchains, and live device/browser lanes remain separate.
GitHub CI is authoritative.
Useful direct entry points:
pnpm testorpnpm test:unit— root unit projectspnpm test:coverage— coverage plus coverage-only projectspnpm test:integration— Node and provider-backed integration suitespnpm perf --platform iosorpnpm perf --platform android— device performance harnesspnpm check:fallow --base origin/main— changed-code quality gatepnpm fallow:all— full-tree audit, including grandfathered baseline findingspnpm fallow:baseline— intentionally regenerate both reviewed Fallow baselines
See docs/agents/testing.md for gate ownership, shared test utilities,
mutation/fuzz lanes, contention policy, and test-speed rules. For real devices, follow
docs/agents/device-verification.md; a fixture-backed test does
not prove that a native path was active.
Test app and Maestro compatibility
The Expo fixture app owns its setup, simulator/device, Metro, replay, and Maestro instructions in
examples/test-app/README.md.
The stable compatibility entry points are:
pnpm test:maestro-compat
pnpm maestro:conformance
pnpm test-app:maestro:ios
pnpm test-app:maestro:android
The first two are deterministic and device-free. The test-app suites need the app, Metro when applicable, and a real simulator or emulator.
Contribution guidelines
- Keep dependencies minimal and prefer built-in Node APIs.
- Preserve the CLI's compact, agent-friendly JSON output.
- Open and close sessions explicitly in tests and manual verification.
- Add or adjust integration coverage when introducing a command or changing a wire response.
- Run the focused gate that owns the behavior; do not replace missing coverage with a broad, assertion-free test.
Conservative code comments
When code deliberately chooses a slower or more conservative path, leave a short comment at the
decision site naming the prevented failure and the condition for revisiting the choice. Use the
grep-able CONSERVATIVE: prefix when the decision is expected to outlive the current change.
// CONSERVATIVE: Preserve external runner artifacts because the checkout does not own their cache
// root. Revisit only if external artifacts get an ownership marker that makes cleanup safe.
Dependency updates
Renovate proposes weekly lockfile maintenance, grouped development-dependency updates, individual runtime-dependency updates, and GitHub Action digest bumps. Automerge is disabled: dependency PRs need green CI and human review.
Read the release notes, inspect the affected-check plan, and treat a green update as a merge candidate rather than an automatic merge:
pnpm check:affected --run
Issues
Issue labels describe workflow state, not ownership. See
docs/agents/triage-labels.md.
When reporting a problem, include the OS and Node version, relevant Xcode or Android SDK versions, and the exact command and output.