Merge remote-tracking branch 'origin/main' into claude/issue-20312-save-door-sdui-manifest

Author: claude

.github/workflows/ci.yml

@@ -556,14 +556,19 @@ jobs:
           echo 'Items on this shard (a package name, or a package plus a k/n file-level slice):'
           cat "$RUNNER_TEMP/shard-packages.txt"
 
-      # ⛔ A FILE-LEVEL SLICE BUILDS ITS DEPENDENCY CLOSURE HERE, IN A RUN THAT
-      # CARRIES NO PASSTHROUGH, so that the sharded run in the next step can be
-      # `--only` (#16395).
-      #
-      # Turbo folds a run-level passthrough into the hash of EVERY task in the
-      # run, not only the task that receives it -- and `-- "--shard=k/n"` is the
-      # whole reason a slice gets its own invocation at all (the next step's
-      # comment says why it cannot ride the shared run). Measured on turbo
+      # ⛔ A FILE-LEVEL SLICE BUILDS ITS DEPENDENCY CLOSURE HERE, IN ITS OWN
+      # GUARDED STEP, so the slice leg in the next step REPLAYS it. Since #19278
+      # that leg carries its slice in `OS_TEST_SHARD`, which only the `test`
+      # task declares, so its build tasks hash exactly as they do here (61 of 61
+      # identical, measured) and it schedules its own closure: this step is no
+      # longer what makes the leg correct, only what keeps the closure build off
+      # the test step's stall-guard site (see the last paragraph below).
+      #
+      # Why the step first existed (#16395, when the slice was still a
+      # passthrough and the leg was `--only`): turbo folds a run-level
+      # passthrough into the hash of EVERY task in the run, not only the task
+      # that receives it -- and `-- "--shard=k/n"` was then the whole reason a
+      # slice got its own invocation at all. Measured on turbo
       # 2.10.10, `--filter=@objectstack/cli`, `turbo run test ... --dry=json`
       # (60 tasks: 59 `build` + 1 `test`):
       #
@@ -677,12 +682,15 @@ jobs:
 
           # Split the shard's ITEMS into the whole packages, which share one
           # turbo run as they always have, and the file-level slices, which
-          # cannot: `--shard=k/n` is passed through to vitest by turbo as a
-          # RUN-level argument, so it would reach every package in the run —
-          # and on any package with fewer test files than n that is a hard
-          # vitest failure (or, with --passWithNoTests, silently no tests at
-          # all). A slice therefore gets its own invocation, filtered to the one
-          # package the partitioner sliced.
+          # cannot: a slice's `k/n` travels as `OS_TEST_SHARD` in the turbo
+          # run's environment, and a run has ONE environment — set on the
+          # shared run it would reach every package's `test` task, which
+          # turbo.json declares it on. (Before #19278 it travelled as a
+          # `--shard=k/n` passthrough, which reached every package's vitest: a
+          # hard failure on any package with fewer test files than n, or with
+          # --passWithNoTests silently no tests at all.) A slice therefore gets
+          # its own invocation, filtered to the one package the partitioner
+          # sliced.
           FILTERS=""
           SLICES=""
           while read -r PKG SLICE; do
@@ -727,74 +735,73 @@ jobs:
               PKG="${LEG%%=*}"
               SLICE="${LEG#*=}"
               LOG="$RUNNER_TEMP/test-core-slice-$(printf '%s' "$PKG" | tr -c 'A-Za-z0-9' '-').log"
-              # `--only` (#16395): the step above already built this slice's
-              # dependency closure in a passthrough-free run, so this run must
-              # schedule the ONE task the passthrough is for. Without it turbo
-              # re-hashes the whole `^build` closure under `--shard=k/n` and
-              # rebuilds it -- that comment carries the measurement. ⚠ The build
-              # step is load-bearing for this flag: a sliced package whose build
-              # never ran fails LOUDLY here (its imports resolve to a missing
-              # dist), never as a silent green.
+              # THE SLICE RIDES IN `OS_TEST_SHARD` (#19278), a variable turbo.json
+              # declares on this package's `test` task and its vitest.config.ts
+              # reads into vitest's `shard` (vitest 4.1.11 reads no shard
+              # variable of its own). Not a `-- --shard=k/n` passthrough, and so
+              # neither `--only` nor `--force`. The partitioner's `--self-test`
+              # fails a package it can slice whose config or task does not.
+              #
+              # Why the two flags existed, and why they are gone:
               #
-              # ⛔ `--force` (#18671) IS NOT REDUNDANT BESIDE `--only` -- do not
-              # delete it as a no-op. Read the paragraph above backwards: if
-              # `--only` is what stops turbo re-hashing the `^build` closure,
-              # then under `--only` this task's hash NO LONGER CARRIES that
-              # closure, and the one path by which a change in a dependency
-              # reaches this task is gone with it. What is left is the package's
-              # own files plus the hand-declared `$TURBO_ROOT$` inputs in
-              # turbo.json. So a slice that IS in the affected set -- and this
-              # shard's package set is exactly the affected set, computed two
-              # steps up -- can match a main-seeded cache entry across the very
-              # change that put it in that set, and be replayed instead of run.
+              #   `--only` (#16395). Turbo folds a run-level passthrough into the
+              #   hash of EVERY task in the run (turbo 2.10.10, cli's plan, plain
+              #   vs `-- --shard=1/2`: 0 of 62 hashes identical), so the leg
+              #   re-hashed and rebuilt the `^build` closure the step above had
+              #   just built. `--only` scheduled the test alone -- and dropped the
+              #   closure out of the test's HASH with it (#18671), leaving the
+              #   package's own files plus the hand-declared `$TURBO_ROOT$`
+              #   inputs. A slice in the affected set (on a PR, queue or push run
+              #   this shard's set IS the affected set) could then match a
+              #   main-seeded entry across the very change that put it there.
+              #   Run 34746808828 did: it replayed `@objectstack/cli:test` on
+              #   this leg -- `Cached: 1 cached, 1 total`, `73ms >>> FULL
+              #   TURBO` -- out of a log a main push run had produced ~15
+              #   minutes earlier, on a commit that did not contain the PR under
+              #   test. All six shards reported success,
+              #   `check-test-completeness` graded the replay OK, the shard
+              #   attestation said "ran to completion", and the red reached
+              #   `main`. Neither of those two can tell a run from a replay.
               #
-              # That is not a hypothetical. Run 34746808828 replayed
-              # `@objectstack/cli:test` on this leg -- `Cached: 1 cached, 1
-              # total`, `73ms >>> FULL TURBO` -- out of a log a main push run
-              # had produced ~15 minutes earlier, on a commit that did not
-              # contain the PR under test. All six shards reported success,
-              # `check-test-completeness` graded the replay OK, and the shard
-              # attestation said "ran to completion". Neither of those two can
-              # tell a run from a replay; the red reached `main`.
+              #   `--force` (#18671, PR #19271). Made the leg execute; a constant
+              #   bypass cannot alarm, and the hash stayed blind to the closure.
               #
-              # Measured on this tree (turbo 2.10.10) with a package whose
-              # `test` task has the same shape (`dependsOn: ["^build"]`), after
-              # a source change in an upstream package PLUS the closure rebuild
-              # the step above performs -- i.e. exactly this job's sequence:
+              # A declared env reaches ONLY the task that declares it: same plan,
+              # plain vs `OS_TEST_SHARD=1/2` is 61 of 62 identical, the one that
+              # moves is `cli#test`, and `1/2` vs `2/2` moves that one alone. The
+              # closure keeps the hashes the step above gave it (61 of 61 equal
+              # to `turbo run build --filter=$PKG`) and replays, while the test's
+              # hash carries the closure again. `cli#test`, `--dry=json`:
               #
-              #   --only              (before)  1 task   `1 cached, 1 total`   81ms >>> FULL TURBO   <- NOT RUN
-              #   --only --force      (here)    1 task   `0 cached, 1 total`   1.15s                 <- runs
-              #   no --only           (option)  6 tasks  `0 cached, 6 total`   1m54.963s             <- runs, and rebuilds the closure
+              #   this leg, OS_TEST_SHARD=1/2          clean  1ee03ac2f26389a6
+              #     + packages/spec/src/ui/view.zod.ts        04eabd0b5a9db364  moves
+              #     + packages/types/src/env.ts               4b15652b8c493471  moves
+              #     + connector-slack/src/* (not in closure)  1ee03ac2f26389a6  still (control)
+              #   old leg, --only --force -- --shard=1/2
+              #     clean / + view.zod.ts                     d5e7ec263710fe59  still (the defect)
               #
-              # The third row is why the repair is `--force` rather than
-              # "drop `--only`": dropping it re-executes the closure the step
-              # above just built, which is #16395's bill paid twice per job.
-              # That bill re-measured TODAY at cli's real scale, same tree,
-              # `--dry=json` against a cache the passthrough-free build had
-              # just filled: the `--only` leg plans 1 task; dropping `--only`
-              # plans 60 and every one of the 60 is a cache MISS, against 57
-              # HIT / 3 MISS for the same plan without the passthrough (2 of
-              # those 3 are `#build` tasks for packages that declare no `build`
-              # script, so they never execute; the real miss is the test). Cost
-              # of re-executing that closure here, `--concurrency=2` on a
-              # SHARED container: 5m11.262s, against 60ms `>>> FULL TURBO` for
-              # the same command when its hashes are left alone.
+              # Executed (cli, slice 1/16 for time; `packages/client/src/index.ts`
+              # edited -- a cli dependency no `$TURBO_ROOT$` input names -- then
+              # the step above rebuilt the closure, 57 of 59 cached):
               #
-              # `--force` re-executes THIS ONE TASK and nothing else, which is
-              # why `--only` stays: the two are a pair. Precedent, for the same
-              # reason in one sentence -- a gate must not be satisfiable by a
-              # replayed artifact -- is the docs-build gate's `TURBO_FORCE`
-              # below. Two things fall out of it that are worth keeping:
-              # `--summarize` now records a REAL duration for the sliced
-              # package (measure-test-shard-timings.mjs refuses to read one
-              # from a replay), and the log says `cache bypass, force
-              # executing <hash>` where it used to say `cache hit, replaying
-              # logs <hash>`.
+              #   old leg, no --force  `cache hit, replaying logs`  69ms FULL TURBO  <- NOT RUN
+              #   old leg, --force     `cache bypass, force executing` (same hash)   <- runs
+              #   this leg             `cache miss, executing`, 59 of 60 cached      <- runs
               #
-              # ⚠ What `--force` does NOT do is make the hash honest. It stays
-              # blind to the closure, so nothing in this repo yet fails when a
-              # slice leg's hash stops moving for an upstream source change.
-              set -- pnpm turbo run test "--filter=$PKG" --only --force --concurrency=4 --summarize --log-order=stream -- "--shard=$SLICE"
+              # and on an unchanged tree this leg replays (`60 cached, 60 total`,
+              # 104ms): with the closure in the hash a HIT means these inputs were
+              # already tested, which is why no `--force`. Cost: 62 tasks planned,
+              # 59 HIT / 3 MISS against the cache the step above fills (the test,
+              # plus two `#build` tasks of packages with no `build` script, which
+              # never execute), where the passthrough without `--only` was 62 MISS.
+              # The step above is no longer load-bearing for correctness -- this
+              # run schedules its own closure -- but it keeps that build on its
+              # own stall-guard site, and here it replays. `--summarize` records
+              # the slice only as the sha256 of `OS_TEST_SHARD` (`cliArguments`
+              # is empty); measure-test-shard-timings.mjs resolves that digest
+              # against the k/n the partitioner can emit and refuses one it
+              # cannot, so a slice is never timed as the whole package.
+              set -- env "OS_TEST_SHARD=$SLICE" pnpm turbo run test "--filter=$PKG" --concurrency=4 --summarize --log-order=stream
             fi
             LOGS="$LOGS $LOG"
             node scripts/run-with-stall-guard.mjs --log "$LOG" --stall-minutes 10 \

packages/cli/vitest.config.ts

@@ -643,7 +643,7 @@
 // `node_modules` exclusion: an exact-path list matches nothing it does not name.
 import { defineConfig } from 'vitest/config';
 import path from 'path';
-import { parseCLI } from 'vitest/node';
+import { parseCLI, type TestUserConfig } from 'vitest/node';
 import {
   runFilterPreflight,
   runProjectCliOverridePreflight,
@@ -710,6 +710,35 @@ runProjectCliOverridePreflight({
   parse: parseCLI,
 });
 
+// #19278 — THE FILE-LEVEL SLICE ARRIVES AS AN ENV VAR, NOT AS A PASSTHROUGH.
+// `scripts/partition-test-shards.mjs` slices this package (`FILE_SHARDED_PACKAGES`),
+// and Test Core runs each slice as `OS_TEST_SHARD=k/n turbo run test`. The
+// value reaches vitest HERE because vitest 4.1.11 reads no shard variable of its
+// own (no `VITEST_SHARD`: the variables it reads are enumerable in its dist),
+// and it reaches this process at all only because `turbo.json` declares
+// `OS_TEST_SHARD` in this package's `test` task `env` — which is also what
+// puts the slice in the task hash. Unset (every local run, the whole-package
+// leg, the nightly) it is `undefined`, and the run is unsharded as before; a
+// `--shard` on the command line still wins, because vitest merges the CLI
+// options OVER this block.
+//
+// Why a passthrough (`-- --shard=k/n`) is no longer the carrier: turbo folds a
+// run-level passthrough into the hash of every task in the run, so the slice
+// leg had to be `--only`, and `--only` drops the `build` closure out of the
+// test's hash — a slice could replay across the very upstream change that put
+// it in the affected set. An env declared on the task reaches only the task.
+//
+// ⚠️ Typed against vitest's CLI-options type (`TestUserConfig`), spread rather
+// than written as a literal key: vitest declares `shard` on its CLI options and
+// NOT on `InlineConfig`, the type of this `test` block (a literal `shard:` is
+// TS2769 here: "'shard' does not exist in type 'InlineConfig'"), yet it
+// resolves the two as one object (`deepMerge(configDefaults, test, cliOptions)`)
+// — measured on 4.1.11, it honours this key, projects included. The
+// partitioner's `--self-test` fails when a sliced package's config stops
+// reading the variable — it looks for the read in code position, so the read
+// lives at its use site in the `test` block below, not in a helper binding
+// that could outlive the spread.
+
 export default defineConfig({
   resolve: {
     // Array form with an ANCHORED pattern, per the trap the gate documents:
@@ -784,6 +813,9 @@ export default defineConfig({
     ],
   },
   test: {
+    // The file-level slice, when Test Core runs one (#19278) — see the section
+    // above `export default` for why it is spread and typed this way.
+    ...({ shard: process.env.OS_TEST_SHARD } satisfies Pick<TestUserConfig, 'shard'>),
     // A late console.* must not redden a green suite (#10374): vitest's worker
     // forwards console output over RPC and discards the promise, and a write
     // landing after teardown's rpcDone() snapshot is rejected into an unhandled