mirror of https://github.com/callstack/agent-device.git synced 2026-09-14 20:06:34 +08:00

Files

T

Michał Pierzchała ef2094c9d7 docs: keep size review in CI and local feedback fast (#1842 )

* feat(size): measure a base ref in one command (pnpm size --base <ref>)

The Size workflow already compares base and PR builds; locally that needed a
manual checkout, install, build, --json, and --compare dance, so budgets were
negotiated late. --base <ref> does the workflow's recipe in a detached worktree
under .tmp/size-base/<sha> (kept for reuse, other bases pruned) and compares
against it: first run ~1-2 min, later runs against the same base ~3s.

Documents the local caveat: npm tarball/unpacked rows compare a fresh base
against a working tree that may carry locally built helper artifacts.

* feat(tooling): pnpm pr:evidence — one paste-ready, SHA-stamped evidence block for PR bodies

Composes what the repo already measures instead of hand-transcribing it after
every rebase: exact merge-base and head, changed-file areas, the affected
selector's plan (local vs GitHub-authoritative, fail-open summarized), the
layering guard verdict, depgraph counts with a real delta against the base (a
throwaway git worktree, no install — the script analyzes its cwd while its
imports resolve from this checkout), and, behind flags, the changed-line
coverage table and pnpm size --base. It claims nothing about CI: the last line
links the head's checks. ~20s default tier.

The pure model (grouping, report parsing, rendering) has node:test coverage
registered as the pr-evidence-model gate, run in the Affected-check Selector
job next to the selector it reads.

* fix(tooling): pr:evidence measures pristine head/base worktrees from an os.tmpdir scratch; size --base gets a per-SHA lock, completion stamp, and non-destructive eviction

Review (three P1s):
- pr:evidence created its scratch under an untracked .tmp/ that a fresh
  checkout lacks (ENOENT). Scratch now lives under os.tmpdir(), which exists
  by construction; a real entrypoint regression runs the whole pipeline with
  --base HEAD (no origin/main needed) and asserts JSON shape plus cleanup of
  both worktrees and the scratch.
- Untracked or uncommitted production files could move the layering/depgraph
  numbers the block labels as HEAD's. Head is now measured from a pristine
  worktree of the head commit exactly like base, and the affected plan takes
  the head SHA (the literal HEAD folds the working tree in). The dirty flag
  now counts untracked files and says they are not in the block.
- size --base force-pruned other cached bases without locking and trusted a
  dist/src that could be half-built. Per-SHA .lock (pid, O_EXCL) held from
  before the worktree exists until the base report is read; a live lock on
  the same base fails fast, a stale one is replaced; eviction skips worktrees
  whose lock owner is alive; dist/.size-base-complete marks a finished build.
  Orchestration tests run the real script against a throwaway git repo with
  pnpm/npm shimmed on PATH (build once, reuse, live lock, stale lock,
  interrupted build, guarded vs idle eviction). Also fixes the /tmp →
  /private/tmp realpath mismatch those tests surfaced (git lists worktrees by
  real path, so the registration check removed a live worktree).

* fix(tooling): symlink-identity locks with compare-then-unlink; evict under the victim's lock; pr:evidence registers worktrees on add and cleans up exhaustively

Review (three P1s):
- Lock creation/takeover races: the lock is now a symlink whose target is
  the owner identity (pid:nonce), created with its identity in one syscall
  (no empty-file window), taken over only by compare-then-unlink on the exact
  identity judged stale, and verified after creation; release unlinks only a
  link that still names this run. Real overlapping-process tests: two runs on
  one base (exactly one builds, the other fails fast), and a takeover race
  against a simulated other taker across delays straddling the acquire window
  (a live lock is never unlinked, both never proceed).
- Cross-base eviction: a victim is removed only while holding its own lock,
  acquired through the same path, so a run wanting it after the check finds
  it locked rather than half-removed; a live-locked victim is skipped.
- pr:evidence worktrees: withWorktrees registers each worktree the moment its
  add succeeds and sweeps every resource on the way out, collecting failures
  instead of stopping at the first; planted reds for both (second add fails →
  first removed; removal of the middle one throws → the others still go).

* test(size): serialize the size --base orchestration file with the other real spawners

Caught running the full unit suite on the rebased branch: the file passed in
isolation but intermittently failed under broad file parallelism, where it took
14s versus ~5.5s alone. It spawns node scripts/size-report.mjs per case, which
spawns git and the shimmed package managers under it — the SUBPROCESS_STUB_TESTS
class exactly (starved spawns surface as a vitest test timeout instead of the
orchestration assertion the case is about), so it joins that serialized project
with its spawn named at the entry, per docs/agents/testing.md. No rerun layer is
involved: the flake is removed, not retried. Two full-suite runs green after.

* refactor(size): extract the base-cache claim protocol and make stale takeover atomic

Review (P1 + architecture):

Stale-claim removal was compare-then-unlink (readlink then unlink; lstat then
rm for a stray file), so another taker could replace the observed entry with
its live claim between the two syscalls and this run would delete the
replacement. Removal now happens only while holding the entry's takeover mutex
— an atomically created directory — and re-verifies the claim inside it. A
replacement can appear only by creating one on a free path (the abandoned
claim occupies it until the unlink) or by another takeover (needs the mutex),
so removal cannot delete a replacement. A mutex leaked by a process killed
inside its sub-millisecond critical section is reclaimed by age, and even a
wrong reclamation is contained: both takers re-verify inside, and the winner
is still decided by the atomic symlink() that follows.

The protocol moves out of size-report.mjs into scripts/size-base-cache.mjs
(AGENTS.md: extract past 500 LOC) — 719 → 536, with the entry lifecycle
(claim → evict others → ensure worktree → build if unstamped → measure →
release) owned by the module behind withPreparedBaseWorktree. Mirrored tests
in scripts/__tests__/size-base-cache.test.ts plant every dangerous
interleaving directly on the filesystem: replacement-after-observation, a
takeover held by another run, age reclamation, release-after-retarget, and a
stray non-symlink. They need no subprocess and run in 9ms, so the raced
single-process case was dropped from the orchestration file, which keeps only
what real processes can show. Planted red: removing the mutex makes the
contended case delete the claim it must not touch.

* ci(size): preserve the reporter's whole module graph, and gate that it stays whole

The Size workflow measures the base commit with the PR's reporter, so it
copies the reporter out of the tree before checking the base out. Extracting
size-base-cache.mjs made the reporter a two-file graph while the step still
copied one file, and the base measurement died with ERR_MODULE_NOT_FOUND —
after every deterministic gate had passed, because nothing local reproduces
that copy.

The step now copies the scripts directory, so a further split cannot leave an
import behind, and size-report-preserved-closure.test.ts holds it to the
reporter's real relative-import closure and to running the preserved copy
rather than the checked-out tree. Planted red: restoring the single-file copy
fails both cases, naming scripts/size-base-cache.mjs. Verified by running the
reporter from a copied directory exactly as the workflow does.

* fix(size): the takeover mutex has one holder for life; split report publishing out of the reporter

Review (P1 + architecture):

Age-based reclamation of the takeover mutex reintroduced the split ownership
the mutex exists to prevent: a holder that is merely slow — paused or
SIGSTOPed past any threshold — could have its mutex force-removed and
replaced, putting two takers inside the supposedly exclusive section, where
either could unlink the claim the other had just created; the unconditional
pathname-based release could also delete the replacement mutex. The mutex is
now a symlink naming its holder, created in one syscall, never reclaimed at
any age, and released only by the run that owns it. A mutex leaked by a
process killed inside a three-syscall critical section wedges one cache entry
with the path to clear in the message, rather than silently deleting another
run's live claim. Planted red: restoring age reclamation displaces a
day-old delayed holder, which the new case pins.

Publishing the report to a PR is a separate question from measuring and
formatting it, so it moves to scripts/size-report-comment.mjs with the marker
and retry policy it owns; its existing regression drives it through the real
script unchanged. scripts/size-report.mjs is 386 LOC — under the 500 tripwire
and below the 512 it had on base.

* test: prove delayed size cache holder is preserved

* docs: keep size review in CI

2026-08-19 14:22:52 +02:00

6.2 KiB

Raw Permalink Blame History

Migrating one command onto the request-bound platform runtime (ADR 0019 unit)

The checklist for one command unit. ADR 0019 §6–10 holds the rules and the why; issue #1739 holds the wave plan and per-unit budgets; this file is the order of operations, with the seam, fixture, and gate you touch at each step. Everything here was rediscovered during the snapshot unit (#1779, 134 files) — do not rediscover it again.

Before writing code

Post the unit on #1739 first: exact denominator (every inventory source or supported device-runtime cell), parity source, facet owner, expected deletions, evidence tier (request-scoped vs durable-resource), and the size budget. A unit without a posted budget has no acceptance criterion; the late budget negotiation on #1779 is the incident.
One integration owner. Parallel agents audit (platform parity, tests, size, architecture) and implement only clearly disjoint lanes — a platform package's operations, a fixture builder, the test migration. The shared spine — plan → facts → bind in the handler, the descriptor's platformExecution, the cutover row — moves serially under one owner. Two agents editing the spine produced the rebase churn on #1779.
Ask the owning-interface question before adding any gate: "can this invariant be made impossible at the seam (a type only one function can produce, a required parameter, one construction path)?" If yes, do that; a cutover-row extension that recognizes a call shape is the fallback, not the default, and it is what AGENTS.md's "reconstructing a compiler" principle forbids once it needs a second omission patch.

The unit, in order

Each step names its declaration site; read that, not prose.

Step	Where
Declare the use(s) with the one neutral `defineUse`; required-only by default, a preferred operation needs a recorded measurement in the unit review (§9)	`packages/contracts/src/platform-runtime-operations.ts`
Plan resolution is pure and lives in contracts (`resolve<Command>RuntimePlan`), never in the handler	`packages/contracts/src/*-runtime-plan.ts`
Handler: resolve plan → `admitRuntimePlan({ device, plan, inspectFacts })` → hand the returned `AdmittedRuntimePlan` to the command's facts-first binder, which unwraps it (`unwrapAdmittedRuntimePlan`) and binds once, on the admitted device, with the admitted plan's use → operate. The token is minted only by admission and read back by exact identity, so admit-before-bind — and admit-this-device-before-binding-it — is a type and runtime fact, not a gate. No admission-only binds, no `requireCommandSupported`, no capability bucket	`src/daemon/handlers/session-runtime-admission.ts`; the shared snapshot/diff owning interface `resolveBoundSnapshotCaptureRuntime` in `src/daemon/snapshot-runtime-binding.ts` as the model
Descriptor flips to `platformExecution: { kind: 'device-runtime', use(s) }` in the same PR; the discriminator is exhaustive, so a forgotten descriptor fails typecheck	`src/core/command-descriptor/registry.ts`
Each platform package reports exact-owner facts and implements the operations; provider ownership fails closed (missing behavior never falls through to a local owner)	`packages/platform-/src/`, `packages/provider-/src/**`
Add one row to the parametrized cutover table: `legacyRetirement` (what must be gone), `runtimeTypeNames`, `operations`, `singularExecution` with lexical `operationOwners`. The mechanism already carries the planted-red proof; a row that leaves a claim unstated is rejected by `cutoverRowDefects`	`scripts/layering/runtime-command-cutover-table.ts` (rule ids allocate upward; snapshot is R32)
Delete: the legacy adapter, the descriptor's capability bucket + `requireCommandSupported` wiring, the retired route names — the row's `legacyRetirement` is the machine-checked list	wherever `legacyRetirement` points
Tests: bind a fake runtime at the seam the handler consumes (`inspectFacts` / `bindDevice`), never a `dispatchCommand` mock. Move the command's existing tests off the dispatch mock in this PR	`src/daemon/__tests__/snapshot-runtime-fixture.ts` (fixture shape), `src/daemon/handlers/__tests__/session-command-harness.ts` (`mockInspectDeviceRuntimeFacts`, `mockBindDeviceRuntime`), `src/__tests__/test-utils/runtime-operation-facts.ts` (facts builders)
Cross-cutting facets (freshness, system-chrome guard, presentation) land inside the first consuming unit and get one contract owner; later units consume, never fork	ADR 0019 §10

Evidence the unit review must contain

Request-scoped tier: the typed use declaration, fact coverage for every denominator cell, the enumerated legacy-parity cell table, and the cutover-gate row. Durable-resource tier adds ADR §4–5 lifecycle evidence. Importing durable machinery promotes the tier — say so.
Planted-red for anything new that is not a table row (a facet, a package boundary): revert, run, quote the failing line. A row needs no planted-red of its own.
Size: use the CI Size workflow's root-bytes-removed vs package-bytes-added and four checkpoint metrics against the posted budget. Do not build a base checkout or run a local size comparison by default. Move-dominated is the rule; net growth is itemized, not explained away.
Live evidence for the changed path on at least one real target per family the denominator claims (docs/agents/device-verification.md); fixture-backed parity does not replace it.
Layering: pnpm check:layering green — R3 seam list narrows in the unit that removes an area's last platform import and never grows; R9/R10 must not grow (docs/agents/testing.md).

What "done" is not

A migrated command with a legacy fallback, a provider/local fallback, or a dispatchCommand branch left "just in case" — the row's retirement claim rejects it, and so does review.
Tests green because they mock the old seam. Grep the command's name across vi.mock('.../core/dispatch.ts') users before calling the tail closed.
A per-command policy file in scripts/layering/. If the row's generalized columns cannot express the invariant, first return to step 3; extensions are the exception and each one names what the seam could not make impossible.

6.2 KiB Raw Permalink Blame History Unescape Escape