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.4 KiB

Raw Permalink Blame History

Pull Requests

Readiness

Static gates first: required checks pass, pnpm check:fallow --base origin/main is clean when code-quality/dead-code risk is relevant, CI guards are green, and no conflict markers or unmerged paths remain.
A local unit-only run is not CI-green. Use pnpm test:unit for the repo unit bundle, or vitest run --project unit-core --project subprocess-stub when invoking Vitest directly. The Integration Tests and Coverage jobs run the provider-integration project — verify those green on the actual PR head.
Device-facing behavior is not merge-ready without real simulator/emulator/device evidence for the changed path. Fixture-backed tests prove contracts; they do not replace a live run that creates or observes the artifact/state the feature claims to handle. If live verification is blocked, state the blocker and the exact command/device needed, and downgrade the PR to residual risk rather than calling it ready.
Command-surface changes preserve CLI, Node.js, daemon, MCP, help, and docs coverage where that surface is affected, without duplicating command contracts across layers.
Runtime output stays agent-friendly: compact defaults, top offenders first for diagnostics/perf, bounded arrays in JSON, artifact paths for large raw data, progressive lookup for deeper detail.
Close every manual agent-device session opened during verification (docs/agents/device-verification.md) and report any cleanup that could not be completed.
Two readiness claims, never blurred: published and reported means the branch is pushed, the PR body carries the evidence gathered at a named commit, and CI on the head is the authority still to come; merge-ready means the required checks are green on the actual head and, where the change touches a device-facing path, the live evidence for that path exists (the device-facing bullet above; docs-only and pure-tooling changes owe none). "Don't wait for CI" licenses the first, not the second — say which one you are claiming.

Rebasing onto a moving `main`

main has no "require branches up to date" rule; a rebase is not owed to GitHub. Rebase when there is a conflict, or when the commits main gained since your base touch a surface your change depends on or that decides your gates:

pnpm check:affected --base <your-merge-base> --head origin/main   # what main gained, by gate

If that plan names only files and gates disjoint from yours, the rebase buys nothing but another full validation cycle. Evidence in the PR body is stamped with the commit it was gathered at, so a rebase dates it rather than invalidating it, and CI on the new head re-establishes it. A merge queue is the answer once independent migration units regularly land against each other; until then this rule is.

PR body

Conventional commit prefixes (feat:, fix:, chore:, perf:, refactor:, docs:, test:, build:, ci:). No bracketed bot tags like [codex]. Ready-for-review by default; draft only when asked or when the work is intentionally incomplete.

## Summary: user/API behavior, not an implementation file tour. Lead with what changed for operators, clients, command authors, or platform behavior. A compact before/after helps when it clarifies the workflow or bug fix. For new or changed public APIs, include 1-3 concrete CLI/Node/MCP examples a reviewer can scan. Closes #123 when applicable.
## Validation: meaningful evidence in concise prose — scenario names, manual device/browser evidence, changed screenshots, CI status, notable failures/retries and their outcome. Avoid command accounting for routine local gates; name an exact command only when it is unusual, manually reproducible evidence, or needed to explain a residual risk. For docs-only changes, say why runtime validation does not apply instead of writing a command checklist.
Call out real tradeoffs, known gaps, and follow-ups; omit boilerplate when there are none.
Note touched-file count and whether scope expanded beyond the initial command family.

Reviewing

Review against the linked issue, not only the diff. State the issue's motivating behavior and verify the PR fixes that.
Check relevant ADRs before reviewing architecture, routing, command-surface, platform-boundary, diagnostics, or testing-strategy changes. An ADR conflict is a review finding unless the PR updates or supersedes the ADR explicitly.
Read dependency notes (Blocked by: ..., linked PRs, sibling branches) before judging correctness. A base/sequence problem outranks detail review.
Trace the real production route from command surface through daemon/request routing to the platform backend. Tests that mock away the router, or exercise only a helper, do not prove the shipped path.
For each key regression test, identify what deletion or revert would make it fail. If reverting the implementation still passes, the test is vacuous.
For recurring failures, prefer a design that makes the class impossible at the owning interface; keep one small regression as evidence rather than enumerating examples. If a custom guard needs repeated exceptions or reconstructs compiler/schema behavior, move the invariant to its source of truth instead of extending the guard.
Check for hidden behavior changes separately from intended refactors: output shape, warning/error propagation, artifact paths, fallback/retry tiers.
Verify tests cover the issue's motivating failure, not just the new abstraction. Prefer before/after evidence when an issue reports a concrete divergence.
Green CI is necessary but insufficient for device-facing or routing-sensitive work.
Check whether the tightening pass removed code/tests the change made obsolete.
Treat the CI Size workflow as review evidence; local size comparisons are not required by default. Escalate scrutiny when a PR adds roughly 700 or more net production lines (excluding tests, generated data, fixtures, and documentation) or increases npm unpacked size by more than 3 kB. Consider gross additions and deletions too, so a move-dominated change is not mistaken for pure growth. These thresholds trigger investigation, not automatic rejection: ask an independent reviewer whether a deeper owning interface, stronger types, less ceremony, reuse of an existing construction path, or deletion of superseded code can make the change materially smaller. The PR should itemize justified growth and record why a smaller design was rejected.

6.4 KiB Raw Permalink Blame History