docs: keep size review in CI and local feedback fast - #1842
Conversation
|
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
9a90699 to
5653f9c
Compare
|
Not ready at exact
Pure model tests do not cover these paths. |
5653f9c to
159ca67
Compare
|
All three addressed in 159ca67 (rebased onto current main).
Also fixed the two 🤖 Addressed by Claude Code |
|
Not ready at exact
Also refresh the stale evidence SHAs and scope disclosure (13 files, not 10). The pristine-head/untracked correction is sound. |
|
Addressed in 48761c7 (rebased onto current main; PR body evidence block regenerated on this head, scope disclosure now 15 files).
Lint/typecheck/fallow clean; 🤖 Addressed by Claude Code |
48761c7 to
388e027
Compare
|
Housekeeping: my earlier fix push ( 🤖 Addressed by Claude Code |
|
Reviewed exact head
Exact-head deterministic, integration, and smoke lanes are green. Coverage is the inherited #1860/main snapshot-handler ratchet, not introduced by this diff. |
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.
…ce 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.
…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).
…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).
…er 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.
388e027 to
52f025d
Compare
|
Rebased onto current main (past #1860, so the inherited ratchet red is gone) and fixed one thing I caught myself while validating the rebase.
Evidence: two consecutive full-suite runs green afterwards (919 files / 6,990 tests each); Everything from the previous round is unchanged and now actually attached to the PR head ( 🤖 Addressed by Claude Code |
…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.
|
All three addressed at exact P1 — non-atomic stale removal. Correct, and The regression is now planted rather than raced, which is why it's deterministic: Architecture — extract past 500. Done: Body evidence. Regenerated at exact Full unit suite 920 files / 6,997 tests; gate manifest ok; lint/typecheck/fallow clean; a real 🤖 Addressed by Claude Code |
…t 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.
|
Re-reviewed exact head
The latest workflow reporter-closure fix and its regression look sound. Exact-head deterministic, Coverage, Integration, Android, and macOS lanes are green; iOS and Linux smoke were still pending at review time. |
…ublishing 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.
|
All three addressed at exact P1 — split takeover ownership. You're right, and the fault was mine for adding age reclamation to close a leak: it recreated exactly the hazard the mutex removes, since "old" cannot distinguish an abandoned holder from a slow one (paused, SIGSTOPed, a loaded runner). 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 — so it has one holder for its whole life, and it can neither force-remove nor release a replacement. The two-owner section, and with it the "A's release deletes B's mutex" path, are gone by construction rather than narrowed. The cost is the leak you'd expect me to name: a process killed inside a three-syscall critical section with no I/O between them wedges one cache entry, loudly, with the path to clear in the error message. I chose that over the alternative deliberately — a wedged entry is a self-describing error, a wrong reclamation is a silently deleted live claim. Planted interleaving, as asked: a holder whose mutex is a day old is still never displaced — Architecture. Second split done: publishing the report to a PR is a different question from measuring and formatting it, so it moved to Body/evidence. Regenerated at exact Full suite 921 files / 7,000 tests; size lanes 14/14; lint, typecheck, fallow clean; a real 🤖 Addressed by Claude Code |
|
Updated review at exact head The delayed-holder regression now plants the actual pre-fix takeover shape—an aged directory mutex. It passes 9/9 on current code and fails against pre-fix Local required gate passed before push: Minor body cleanup after CI: the claim-protocol paragraph says 8 tests but now has 9, and the evidence block is stamped to the prior head, so refresh both on the final exact head. |
|
Re-reviewed exact |
|
Both body items done at exact On the fixture correction ( I'm not touching the 🤖 Addressed by Claude Code |
thymikee
added
the
ready-for-human
|
Re-reviewed readiness at exact head |
thymikee
changed the title
1 participant
Summary
Keep package-size measurement in CI and keep the local developer loop focused on the smallest
relevant correctness gates.
and size comparisons are not required by default.
npm unpacked size by more than 3 kB. The thresholds trigger a design review, not an automatic
rejection.
existing construction path, or deletion of superseded code can make the change materially smaller.
they are needed to diagnose a reported result.
This replaces the earlier
pnpm pr:evidenceandpnpm size --baseimplementation. The PR now addsno commands, scripts, tests, cache protocol, worktree orchestration, or workflow behavior.
Validation
pnpm check:affected --runpassed on exact headc21ccbc14. Becausedocs/agents/testing.mdowns the affected selector's guidance, the selector deliberately failed opento the full local gate set; all runnable checks passed. GitHub remains authoritative for the queued
CI lanes.
Scope: 3 documentation files. No runtime, package, workflow, or device-facing behavior changed.