From 2c1f8a13f6f661543d4156b0e6b7a753260bd91d Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Thu, 22 Oct 2026 21:31:20 +0800 Subject: [PATCH 1/4] Measure problem entries, not bare cards Since #401 each card sits in an entry that filtering hides along with it, but #410's layout test toggled only the card and counted cards by their attribute, so cards inside hidden entries measured as one row of 57. The test now hides and restores whole entries and counts what is rendered, and the outline measures the entries the grid lays out. --- tests/browser/lobby.test.js | 17 +++++++++++------ web/problem-grid.js | 8 ++++---- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/tests/browser/lobby.test.js b/tests/browser/lobby.test.js index 6531024bd..43cc35db3 100644 --- a/tests/browser/lobby.test.js +++ b/tests/browser/lobby.test.js @@ -6368,10 +6368,10 @@ lobbyTest( "cards and outline must agree with the native picker before paint", ); await grid.evaluate((node) => { - for (const [index, card] of [ - ...node.querySelectorAll(".problem-card"), + for (const [index, entry] of [ + ...node.querySelectorAll(".problem-entry"), ].entries()) - card.hidden = index > 0; + entry.hidden = index > 0; }); await page.waitForFunction(() => { const path = document.querySelector(".problem-picker-outline path"); @@ -6390,8 +6390,9 @@ lobbyTest( return Math.abs(bounds.x + bounds.width - expected) < 1; }); await grid.evaluate((node) => { - for (const card of node.querySelectorAll(".problem-card")) - card.hidden = false; + // Filtering hides both the entry and its card, so both come back. + for (const item of node.querySelectorAll(".problem-entry, .problem-card")) + item.hidden = false; }); const bounds = await page.evaluate(() => { const setup = document @@ -6456,7 +6457,11 @@ lobbyTest( await first.click(); assert.equal( await grid.evaluate((node) => { - const cards = [...node.querySelectorAll(".problem-card:not([hidden])")]; + // Rendered cards, not unhidden ones: filtering hides the enclosing + // entry, and a card inside it measures as a zero box at the top. + const cards = [...node.querySelectorAll(".problem-card")].filter( + (card) => card.checkVisibility(), + ); const top = cards[0].getBoundingClientRect().top; return cards.filter( (card) => Math.abs(card.getBoundingClientRect().top - top) < 1, diff --git a/web/problem-grid.js b/web/problem-grid.js index 5ccc60f5e..0f1ad01f9 100644 --- a/web/problem-grid.js +++ b/web/problem-grid.js @@ -32,11 +32,11 @@ export function mountProblemGrid() { const x = top.left - origin.left; const y = top.top - origin.top; const padding = parseFloat(getComputedStyle(grid).paddingRight); - const cards = [...grid.querySelectorAll(".problem-card")] - .filter((card) => !card.hidden) - .map((card) => card.getBoundingClientRect()); + const entries = [ + ...grid.querySelectorAll(":scope > .problem-entry:not([hidden])"), + ].map((entry) => entry.getBoundingClientRect()); const right = - Math.max(top.right, ...cards.map((card) => card.right + padding)) - + Math.max(top.right, ...entries.map((entry) => entry.right + padding)) - origin.left; const edge = top.right - origin.left; const join = bottom.top - origin.top; From 87b930ffb31496c0a72d53465b7f9bb73ceac7df Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Thu, 22 Oct 2026 21:31:21 +0800 Subject: [PATCH 2/4] Undo text fields with the host's modifier The whiteboard test told the page it runs on Linux and then expected Control+Z to undo typing in a field, but a field's own undo follows the host: Chromium on macOS edits text with Cmd, so the test could only pass on Linux and Windows. It now undoes with the host's modifier and still presses the board's own chords in the field, so the guard that leaves text targets alone is checked on every host. --- tests/browser/whiteboard-shortcuts.test.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/browser/whiteboard-shortcuts.test.js b/tests/browser/whiteboard-shortcuts.test.js index c261c3004..0c08c5313 100644 --- a/tests/browser/whiteboard-shortcuts.test.js +++ b/tests/browser/whiteboard-shortcuts.test.js @@ -744,7 +744,9 @@ test("text targets and focus outside the drawing toolbar keep their own undo and field.focus(); }, kind); await page.keyboard.type("abc"); - await page.keyboard.press("Control+z"); + // The field's own undo follows the host: Chromium on macOS edits text with + // Cmd, whatever platform the page is told it runs on. + await page.keyboard.press("ControlOrMeta+z"); assert.equal(await page.evaluate(() => globalThis.keyTaken), false, kind); assert.equal( await page @@ -753,7 +755,7 @@ test("text targets and focus outside the drawing toolbar keep their own undo and "", kind, ); - await page.keyboard.press("Control+Shift+z"); + await page.keyboard.press("ControlOrMeta+Shift+z"); assert.equal(await page.evaluate(() => globalThis.keyTaken), false, kind); assert.equal( await page @@ -762,6 +764,13 @@ test("text targets and focus outside the drawing toolbar keep their own undo and "abc", kind, ); + // On a macOS host the presses above are Cmd, which a page told it runs on + // Linux never treats as the board's undo, so they prove nothing there. + // The board's own chords must still leave a text target alone. + for (const chord of ["Control+z", "Control+Shift+z"]) { + await page.keyboard.press(chord); + assert.equal(await page.evaluate(() => globalThis.keyTaken), false, kind); + } assert.equal((await inspect(page)).strokes.length, 1); } await page.locator("#end").focus(); From 9c68b5c3d4077a773fd57715bfecaa93985c802d Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Thu, 22 Oct 2026 21:31:22 +0800 Subject: [PATCH 3/4] Move tokio past its Interval race RUSTSEC-2026-0384 reports a race in tokio::time::Interval on 1.54.0, which is now yanked, and cargo-audit fails the portable lane on it. 1.54.1 carries the fix with no other dependency change, and the manifest now requires it, so a fresh resolve cannot return to 1.54.0. --- Cargo.lock | 4 ++-- Cargo.toml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d4b5ff7d9..64e2d4bf2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2707,9 +2707,9 @@ checksum = "fd3ca314f692efd6c868f8408f53fe444634a845f96c028b97d35f6a1f79f0ee" [[package]] name = "tokio" -version = "1.54.0" +version = "1.54.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a0c168a474b95bfecfe908a65d4bdffc523144b649a74c09c286afb35d53e0bc" +checksum = "b52fd3255258d26fac7264ab5a76b53c13367136dc011d3d6ad4f8ed070396a3" dependencies = [ "bytes", "libc", diff --git a/Cargo.toml b/Cargo.toml index 2c68304a3..97d339edf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -47,7 +47,7 @@ rusqlite = { version = "0.40.2", default-features = false, features = ["bundled" rust-embed = { version = "8.13.0", features = ["include-exclude"] } serde_json = "1.0.154" serde = { version = "1.0.229", features = ["derive"] } -tokio = { version = "1.54.0", features = ["fs", "macros", "net", "rt-multi-thread", "sync", "time"] } +tokio = { version = "1.54.1", features = ["fs", "macros", "net", "rt-multi-thread", "sync", "time"] } tokio-tungstenite = { version = "0.30.0", default-features = false, features = ["connect", "rustls-tls-webpki-roots"] } tokio-util = "0.7.20" tower-http = { version = "0.7.1", default-features = false, features = ["compression-gzip"] } @@ -77,7 +77,7 @@ livekit-protocol = "0.8.2" livekit-net = "0.1.4" # The paused clock, so the Live socket's keepalive and idle limits are tested # at their real values instead of waiting them out. -tokio = { version = "1.54.0", features = ["test-util"] } +tokio = { version = "1.54.1", features = ["test-util"] } # Release archives distribute the executable, not a debugging target. Cargo # already omits debug information in this profile; omit linker symbols too. From 9baed37296cbaf26698f9c97be6f759e8ce8bcbd Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Thu, 22 Oct 2026 21:31:22 +0800 Subject: [PATCH 4/4] Report one CI status and test the merge queue Two pull requests green against their own bases broke main together, because nothing ran them side by side. The workflow now runs on merge_group, check also collects the mutation plan and shards so it is the one status to require, the weekly avatar job moves in beside the gates it shares caches with, and the vendor and Chromium restores become composite actions every job uses. Publishing waits on the lanes rather than check, whose skipped mutation jobs would otherwise skip it. --- .../codetrial-contribute/collaboration.md | 10 +- .claude/skills/codetrial-triage/pulls.md | 6 +- .claude/skills/codetrial-verify/SKILL.md | 20 +- .claude/skills/codetrial-verify/ci-failure.sh | 10 +- .claude/skills/codetrial-verify/review.md | 4 +- .github/actions/chromium/action.yml | 27 +++ .github/actions/restore-vendor/action.yml | 31 ++++ .github/workflows/avatar.yml | 55 ------ .github/workflows/check.yml | 172 ++++++++++-------- docs/development.md | 28 +-- tests/test_test_gate.py | 96 +++++----- 11 files changed, 241 insertions(+), 218 deletions(-) create mode 100644 .github/actions/chromium/action.yml create mode 100644 .github/actions/restore-vendor/action.yml delete mode 100644 .github/workflows/avatar.yml diff --git a/.claude/skills/codetrial-contribute/collaboration.md b/.claude/skills/codetrial-contribute/collaboration.md index a4f3f5b6d..94f134173 100644 --- a/.claude/skills/codetrial-contribute/collaboration.md +++ b/.claude/skills/codetrial-contribute/collaboration.md @@ -138,11 +138,11 @@ triage's outdated ones. ## Make CI green before asking for review "Before asking someone to review, you MUST resolve any regressions flagged by -GitHub Actions." (#256, #260). Open the failing `gates ()` job, not -`check` (`.claude/skills/codetrial-verify/ci-failure.sh ` names the failed -gates and whether `main` fails them too), and rerun that lane with -`./scripts/test.sh ` -([codetrial-verify](../codetrial-verify/SKILL.md) describes the lanes). A bad +GitHub Actions." (#256, #260). Open the failing job, not `check` +([codetrial-verify](../codetrial-verify/SKILL.md) says which one, and +`.claude/skills/codetrial-verify/ci-failure.sh ` names the failed gates and +whether `main` fails them too), and rerun a failing lane with +`./scripts/test.sh `. A bad commit message skips the rest of the portable lane, so fix the messages first. diff --git a/.claude/skills/codetrial-triage/pulls.md b/.claude/skills/codetrial-triage/pulls.md index d31caca61..53fd3b2a8 100644 --- a/.claude/skills/codetrial-triage/pulls.md +++ b/.claude/skills/codetrial-triage/pulls.md @@ -225,9 +225,9 @@ last; a stale base is a rebase request; one `main` shares, or an environment limit, is the maintainer's. The sweep's `status` lists them (`failing:` in `why`), and `ci-failure.sh` in [codetrial-verify](../codetrial-verify/SKILL.md#failed-github-actions-checks) -tells those cases apart for one PR in one call. Read the failing -`gates ()` job, not `check` or `mutation`, which only collect results ([codetrial-verify](../codetrial-verify/SKILL.md) -describes the lanes). A failing commit-message check skips the rest of the +tells those cases apart for one PR in one call. Read the failing job that +[codetrial-verify](../codetrial-verify/SKILL.md) names, not `check`, which only +collects results. A failing commit-message check skips the rest of the portable lane, so name it first. A second PR from the same author for the same change supersedes the first or is superseded by it; report which in the sweep output and leave the diff --git a/.claude/skills/codetrial-verify/SKILL.md b/.claude/skills/codetrial-verify/SKILL.md index 196dd29eb..fa918886c 100644 --- a/.claude/skills/codetrial-verify/SKILL.md +++ b/.claude/skills/codetrial-verify/SKILL.md @@ -15,12 +15,12 @@ make check # every gate, plus a live Gemini credential check CI runs one `gates ()` job per lane in parallel: `gates (rust)`, `gates (portable)` and `gates (browser)`. The `check` job only collects their -results and `mutation` does the same for the mutant shards, so neither log -says what failed: open the failing lane, or the failing `mutants` shard when -`mutation` is the red one. On a pull request the portable lane -checks the commit messages first, and a bad message skips the rest of that -lane only. A gate names its lane where it is declared, one naming none is -portable, and a test proves the three lanes together cover the full run. +results and the mutation jobs', so its log does not say what failed: open the +failing lane, or, when every lane is green, the failing `plan-mutants` job or +`mutants` shard. On a pull request the portable lane checks the commit +messages first, and a bad message skips the rest of that lane only. A gate +names its lane where it is declared, one naming none is portable, and a test +proves the three lanes together cover the full run. Reproduce a failure with the same lane locally. `scripts/test.sh` is a list of named gates that all run before it reports, so @@ -130,7 +130,7 @@ regression check in the existing lane. ## Failed GitHub Actions checks -The lanes and the aggregate `check` and `mutation` jobs are described above; +The lanes and the aggregate `check` job are described above; telling a stale base, a regression, a race and an environment limit apart is in [collaboration.md](../codetrial-contribute/collaboration.md#make-ci-green-before-asking-for-review); reading the job log, and why it is `--log`, is the recipe in @@ -152,9 +152,9 @@ locally with the same head. A `cargo-audit` failure on a PR whose base predates the advisory fix is a stale base, answered by a rebase (#334), not a code change; `stale_base` (defined in the script's header) says exactly that, and the PR sweep lists such a PR in `rebase` on its own. The aggregate `check` job -only collects the lanes, so its entry points at the failing `gates ()` -one. A check outside Actions has only its link, and `held` checks have no log -until a maintainer approves the run. +only collects results, so its entry points at the failing job it collected, +as in the lanes section above. A check outside Actions has only its link, and +`held` checks have no log until a maintainer approves the run. The raw calls behind it, when one answer needs checking: diff --git a/.claude/skills/codetrial-verify/ci-failure.sh b/.claude/skills/codetrial-verify/ci-failure.sh index f7572916e..c3017a271 100755 --- a/.claude/skills/codetrial-verify/ci-failure.sh +++ b/.claude/skills/codetrial-verify/ci-failure.sh @@ -151,11 +151,11 @@ failing=$(printf '%s\n' "$checks" | jq -r "$defs"'.[] | select(failed) | if $read then . else .gates = null | .errors = null | .excerpt = null end' done | jq -s .) -# The check job only collects the lanes, so it says nothing of its own. A -# failing check outside Actions (cubic) has no gates to read, so a rebase is not -# known to clear it, and it rules a stale base out. Each gate that reads data -# from outside the tree maps to the files whose change would make the failure -# the PR's own; a gate added here needs nothing else. +# The check job only collects the lanes and the mutation jobs, so it says +# nothing of its own. A failing check outside Actions (cubic) has no gates to +# read, so a rebase is not known to clear it, and it rules a stale base out. +# Each gate that reads data from outside the tree maps to the files whose change +# would make the failure the PR's own; a gate added here needs nothing else. printf '%s\n' "$checks" | jq --argjson failing "$failing" --arg head "$head" \ --argjson pr "$pr" --argjson meta "$meta" --argjson jobs "$main_jobs" \ "$defs"' diff --git a/.claude/skills/codetrial-verify/review.md b/.claude/skills/codetrial-verify/review.md index 06ba4a289..d8c4c53c9 100644 --- a/.claude/skills/codetrial-verify/review.md +++ b/.claude/skills/codetrial-verify/review.md @@ -101,7 +101,7 @@ report what it reports. For each area, look for: - Ordering. A resume racing the message it replays, a check then an act across an await, a message handled before the state it assumes. - Tests. A new branch no test reaches; a surviving mutant in code the diff adds - (CI's mutation job names them, as on #262; it is skipped when the diff + (CI's `mutants` shards name them, as on #262; it is skipped when the diff changes no Rust, so `web/` code has no mutation evidence and each new branch needs a test that reaches it); a browser test of the shapes [SKILL.md](SKILL.md) says reviewers reject (Writing a test). @@ -200,7 +200,7 @@ Past about 500 changed lines, or when several reviewers are run, split them by lens rather than by file, each reading the whole diff for one thing: the specification checklist; the owners in step 2; trust boundaries, workflows and what reaches a log or a public thread; failure paths and boundaries; tests, -including what CI's mutation job says survived; and comments and messages +including what CI's `mutants` shards say survived; and comments and messages against the register in [codetrial-conventions](../codetrial-conventions/SKILL.md). A reviewer handed a lens reports only that lens, so silence on the others is not a pass. diff --git a/.github/actions/chromium/action.yml b/.github/actions/chromium/action.yml new file mode 100644 index 000000000..9f6393ab4 --- /dev/null +++ b/.github/actions/chromium/action.yml @@ -0,0 +1,27 @@ +name: Install Chromium +description: Restore Playwright's Chromium and install its system libraries. + +# The browser binary, not the npm package. package-lock.json pins the +# playwright version, and the version is what decides which build of Chromium +# lands in this directory. Needs `npm ci` first, since it runs that playwright. +# +# The install is one command on a cache hit too: it then only installs the +# system libraries, which are not in the cached directory. Without it the +# browser suites skip themselves and the gate passes while testing nothing. +outputs: + key: + description: The exact key, for the step that saves the browser. + value: ${{ steps.restore.outputs.cache-primary-key }} + cache-hit: + description: Whether the exact key was restored. + value: ${{ steps.restore.outputs.cache-hit }} +runs: + using: composite + steps: + - id: restore + uses: actions/cache/restore@v6 + with: + path: ~/.cache/ms-playwright + key: playwright-${{ runner.os }}-${{ hashFiles('package-lock.json') }} + - shell: sh + run: npx playwright install --with-deps chromium diff --git a/.github/actions/restore-vendor/action.yml b/.github/actions/restore-vendor/action.yml new file mode 100644 index 000000000..a3c4f321b --- /dev/null +++ b/.github/actions/restore-vendor/action.yml @@ -0,0 +1,31 @@ +name: Restore vendored bytes +description: Restore the web/vendor tree the portable lane saves. + +# One definition of the key for every job that reads this cache: the gate +# lanes, the mutation shards, the release legs and the avatar check. +# +# Keyed on the whole tree, not on SHA256SUMS alone. The cached path holds the +# committed vendor files as well as the fetched ones, so a key that only reads +# the pins is unchanged when a committed file is edited: the restore then puts +# the old bytes back over the edit and verify-vendor passes on content the pull +# request does not contain. Hashing the tree costs a re-fetch when a README +# under web/vendor changes, which is the cheaper side of that trade. +# +# At restore time the directory holds only checked-out files, so the key is a +# function of the commit. No restore-keys: a stale entry here is 24 MB of bytes +# that fail their hash. +outputs: + key: + description: The exact key, for the step that saves the tree. + value: ${{ steps.restore.outputs.cache-primary-key }} + cache-hit: + description: Whether the exact key was restored. + value: ${{ steps.restore.outputs.cache-hit }} +runs: + using: composite + steps: + - id: restore + uses: actions/cache/restore@v6 + with: + path: web/vendor + key: vendor-${{ hashFiles('web/vendor/**') }} diff --git a/.github/workflows/avatar.yml b/.github/workflows/avatar.yml deleted file mode 100644 index 7ee25a2bd..000000000 --- a/.github/workflows/avatar.yml +++ /dev/null @@ -1,55 +0,0 @@ -name: avatar - -on: - schedule: - - cron: '17 5 * * 1' - workflow_dispatch: - -permissions: - contents: read - -env: - # Match the check workflow so its frequently refreshed build cache is reusable. - CARGO_PROFILE_DEV_DEBUG: line-tables-only - CARGO_PROFILE_TEST_DEBUG: line-tables-only - RUSTFLAGS: -Clink-arg=-fuse-ld=mold - # The toolchain action otherwise disables the main checks' incremental mode. - CARGO_INCREMENTAL: 1 - -jobs: - avatar: - runs-on: ubuntu-24.04 - timeout-minutes: 30 - steps: - - uses: actions/checkout@v7 - - run: sudo apt-get update && sudo apt-get install -y libglib2.0-dev mold - # browser-check.sh builds the server, and webrtc-sys needs Clang 21+. - - name: Install WebRTC C++ compiler - uses: ./.github/actions/webrtc-clang - - uses: dtolnay/rust-toolchain@stable - # Consume the check workflow's caches without creating a second build tree - # in cache storage for a job that only runs weekly. - - name: Restore cargo cache - uses: ./.github/actions/restore-cargo-cache - - name: Report disk after restoring the build cache - run: df -h / - - name: Restore vendored bytes - uses: actions/cache/restore@v6 - with: - path: web/vendor - key: vendor-${{ hashFiles('web/vendor/**') }} - - uses: actions/setup-node@v7 - with: - node-version: 24 - cache: npm - - run: npm ci - - name: Restore Chromium - uses: actions/cache/restore@v6 - with: - path: ~/.cache/ms-playwright - key: playwright-${{ runner.os }}-${{ hashFiles('package-lock.json') }} - - run: npx playwright install --with-deps chromium - - run: BROWSER_CHECK_FLOW=avatar BROWSER_CHECK_REQUIRE_MODEL=1 ./scripts/browser-check.sh - - name: Report disk after the browser check - if: always() - run: df -h / diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 39080dc9b..127d938ef 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -6,14 +6,26 @@ on: push: branches: [main] pull_request: + # A merge queue tests each pull request on top of the main it will land on. + # Two pull requests green against their own bases can still break main + # together, as #401 and #410 did; nothing else here runs them side by side. + merge_group: + # The avatar check needs a real model download, so it runs weekly rather + # than on every change. It is the only job a schedule or a dispatch starts. + schedule: + - cron: '17 5 * * 1' + workflow_dispatch: permissions: contents: read # A second push to a branch makes the first run's answer worthless, so stop # paying for it. Never on main: those runs are the record of what passed. +# +# The event is in the group so a weekly avatar run on main never queues behind, +# or displaces, the push run that publishes a release. concurrency: - group: check-${{ github.ref }} + group: check-${{ github.event_name }}-${{ github.ref }} cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} env: @@ -61,6 +73,7 @@ env: jobs: gates: name: gates (${{ matrix.lane }}) + if: github.event_name != 'schedule' && github.event_name != 'workflow_dispatch' strategy: fail-fast: false matrix: @@ -187,33 +200,14 @@ jobs: id: cargo-cache uses: ./.github/actions/restore-cargo-cache - # Keyed on the whole tree, not on SHA256SUMS alone. The cached path holds - # the committed vendor files as well as the fetched ones, so a key that - # only reads the pins is unchanged when a committed file is edited: the - # restore then puts the old bytes back over the edit and verify-vendor - # passes on content the pull request does not contain. Hashing the tree - # costs a re-fetch when a README under web/vendor changes, which is the - # cheaper side of that trade. - # - # At restore time the directory holds only checked-out files, so the key - # is a function of the commit. No restore-keys: a stale entry here is - # 24 MB of bytes that fail their hash. - # # Every lane fetches and verifies these bytes; only the portable lane # saves them, below, so three lanes do not race to write one key. - name: Restore vendored bytes id: vendor - uses: actions/cache/restore@v6 - with: - path: web/vendor - key: vendor-${{ hashFiles('web/vendor/**') }} + uses: ./.github/actions/restore-vendor - # The browser binary, not the npm package. package-lock.json pins the - # playwright version, and the version is what decides which build of - # Chromium lands in this directory. - # - # The install itself is not cached. It is 90 packages now that eslint is - # here, but they are small and pure JavaScript, and the step runs in + # The npm install itself is not cached. It is 90 packages now that eslint + # is here, but they are small and pure JavaScript, and the step runs in # seconds against a cargo build that runs in minutes. A cache round trip # would not show up next to that. # @@ -221,26 +215,15 @@ jobs: # ranges, so an install resolves them fresh on every run and the gate that # passed yesterday is not the gate running today. The exact pins in # package.json only cover the four direct dependencies. - # - # Both browser-driving lanes restore it; the browser lane saves it, below. - - name: Restore Chromium - if: matrix.lane != 'portable' - id: chromium - uses: actions/cache/restore@v6 - with: - path: ~/.cache/ms-playwright - key: playwright-${{ runner.os }}-${{ hashFiles('package-lock.json') }} - - name: Install node dependencies run: npm ci - # tests/browser/face-detector.test.js skips itself when no browser is - # downloaded, so without this the gate would pass while testing nothing. - # Left as one command on a cache hit too: it then only installs the system - # libraries, which are not in the cached directory. + # Both browser-driving lanes restore Chromium; the browser lane saves it, + # below. - name: Install Chromium if: matrix.lane != 'portable' - run: npx playwright install --with-deps chromium + id: chromium + uses: ./.github/actions/chromium # The gate skips this audit when the binary is missing, which is right for # a contributor's laptop and wrong for CI: a silent skip here means the @@ -305,21 +288,25 @@ jobs: - name: Run gates run: ./scripts/test.sh "${{ matrix.lane }}" + # A merge group runs on a queue ref that is deleted once it lands, and a + # cache saved there is visible to no other ref, so none of the saves below + # run for one. + # # After the gates pass, so the bytes saved are bytes verify-vendor # accepted. Under the restore step's key: hashing the tree now would also # hash what the fetch added, and no later restore would ask for that key. - name: Save vendored bytes - if: matrix.lane == 'portable' && steps.vendor.outputs.cache-hit != 'true' + if: github.event_name != 'merge_group' && matrix.lane == 'portable' && steps.vendor.outputs.cache-hit != 'true' uses: actions/cache/save@v6 with: path: web/vendor - key: ${{ steps.vendor.outputs.cache-primary-key }} + key: ${{ steps.vendor.outputs.key }} - name: Save Chromium - if: matrix.lane == 'browser' && steps.chromium.outputs.cache-hit != 'true' + if: github.event_name != 'merge_group' && matrix.lane == 'browser' && steps.chromium.outputs.cache-hit != 'true' uses: actions/cache/save@v6 with: path: ~/.cache/ms-playwright - key: ${{ steps.chromium.outputs.cache-primary-key }} + key: ${{ steps.chromium.outputs.key }} # The release job makes the same comparison, but only after a merge: a # `build.rs` that stops recording the commit would pass every pull @@ -354,7 +341,7 @@ jobs: # the gates decided about the code. `cancelled()` is excluded because a # superseded run is killed mid-step and has nothing coherent to save. - name: Save cargo cache - if: ${{ !cancelled() && matrix.lane == 'rust' && steps.cargo-cache.outputs.cache-hit != 'true' }} + if: ${{ !cancelled() && github.event_name != 'merge_group' && matrix.lane == 'rust' && steps.cargo-cache.outputs.cache-hit != 'true' }} uses: actions/cache/save@v6 with: path: | @@ -364,40 +351,32 @@ jobs: target key: ${{ steps.cargo-cache.outputs.key }} - # The fast answer: every gate lane passed. Mutation is answered separately - # below, so a pull request is not held behind shards that can run for most of - # an hour. `!cancelled()` rather than `always()`: a run superseded by a newer - # push has nothing to report, while a lane that failed or timed out still - # reaches this job and fails it. + # The one status: every gate lane passed, and on a pull request the mutation + # plan and every shard did too. Planning only runs on pull requests, and the + # shards only after a plan that found mutants, so a skipped shard job after a + # successful plan means the plan counted none. A push or a merge group plans + # nothing, so it is answered as soon as the lanes finish. + # + # `!cancelled()` rather than `always()`: a run superseded by a newer push has + # nothing to report, while a lane or shard that failed or timed out still + # reaches this job and fails it. Not on a schedule or a dispatch, which run + # no gates. check: - needs: gates - if: ${{ !cancelled() }} - runs-on: ubuntu-24.04 - timeout-minutes: 5 - steps: - - name: Require every gate lane - env: - RESULT: ${{ needs.gates.result }} - run: | - if [ "$RESULT" != success ]; then - echo "::error::gate lanes: $RESULT. This job only collects them; the failed gates lane has the cause." - exit 1 - fi - - # One status for mutation testing, failing closed. Planning only runs on pull - # requests, and the shards only after a plan that found mutants, so a skipped - # shard job after a successful plan means the plan counted none. - mutation: - needs: [plan-mutants, mutants] - if: ${{ !cancelled() }} + needs: [gates, plan-mutants, mutants] + if: ${{ !cancelled() && github.event_name != 'schedule' && github.event_name != 'workflow_dispatch' }} runs-on: ubuntu-24.04 timeout-minutes: 5 steps: - - name: Require mutation planning and every shard + - name: Require every gate lane and mutation shard env: + GATES: ${{ needs.gates.result }} PLAN: ${{ needs.plan-mutants.result }} MUTANTS: ${{ needs.mutants.result }} run: | + if [ "$GATES" != success ]; then + echo "::error::gate lanes: $GATES. This job only collects them; the failed gates lane has the cause." + exit 1 + fi case "$MUTANTS/$PLAN" in success/success | skipped/success | skipped/skipped) ;; *) echo "::error::mutants: $MUTANTS (plan: $PLAN)"; exit 1 ;; @@ -580,10 +559,7 @@ jobs: # fetched MediaPipe binaries and asserts their content types, and # cargo-mutants runs the real suite once per mutant. - name: Restore vendored bytes - uses: actions/cache/restore@v6 - with: - path: web/vendor - key: vendor-${{ hashFiles('web/vendor/**') }} + uses: ./.github/actions/restore-vendor - name: Fetch any vendored bytes the cache did not have run: ./scripts/fetch-vendor.sh @@ -661,8 +637,12 @@ jobs: # The release is a binary, not a directory beside a binary: `rust-embed` # compiles `web/` (including its WASM) into it. Native runners avoid a # cross-platform linker or packaging toolchain. + # + # After `gates` rather than `check`: on a push `check` adds nothing, and the + # mutation jobs it waits on are skipped, which the default `success()` reads + # as a reason to skip everything downstream. build: - needs: check + needs: gates if: github.event_name == 'push' && github.ref == 'refs/heads/main' name: ${{ matrix.name }} timeout-minutes: 45 @@ -724,10 +704,7 @@ jobs: # three platform legs re-download 24 MB that one job already fetched and # stored on this commit, and the fetch script no-ops on a hit. - name: Restore vendored bytes - uses: actions/cache/restore@v6 - with: - path: web/vendor - key: vendor-${{ hashFiles('web/vendor/**') }} + uses: ./.github/actions/restore-vendor - name: Fetch browser assets run: ./scripts/fetch-vendor.sh shell: bash @@ -978,7 +955,7 @@ jobs: # One rolling release for the current main build. GitHub releases require a # tag; `latest` avoids exposing an implementation SHA as a release name. release: - needs: [check, build] + needs: [gates, build] if: github.event_name == 'push' && github.ref == 'refs/heads/main' runs-on: ubuntu-24.04 timeout-minutes: 15 @@ -1087,3 +1064,40 @@ jobs: # deletes above are for. gh release edit "$staged" --tag "$tag" --target "$GITHUB_SHA" --draft=false published=true + + # The avatar flow end to end, with the real model rather than a stub. It + # reads the gates' caches and saves none: a second build tree in cache + # storage for a weekly job would evict the one every pull request uses. + avatar: + if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' + runs-on: ubuntu-24.04 + timeout-minutes: 30 + steps: + - uses: actions/checkout@v7 + # browser-check.sh builds the server, and webrtc-sys needs Clang 21+. + - name: Install system packages + run: sudo apt-get update && sudo apt-get install -y libglib2.0-dev mold + - name: Install WebRTC C++ compiler + uses: ./.github/actions/webrtc-clang + - name: Install Rust + run: | + rustup update stable + rustup default stable + - name: Restore cargo cache + uses: ./.github/actions/restore-cargo-cache + - name: Report disk after restoring the build cache + run: df -h / + - name: Restore vendored bytes + uses: ./.github/actions/restore-vendor + - name: Install node dependencies + run: npm ci + - name: Install Chromium + uses: ./.github/actions/chromium + - name: Run the avatar browser check + env: + BROWSER_CHECK_FLOW: avatar + BROWSER_CHECK_REQUIRE_MODEL: 1 + run: ./scripts/browser-check.sh + - name: Report disk after the browser check + if: always() + run: df -h / diff --git a/docs/development.md b/docs/development.md index e0f06cf1a..f7bf21bb0 100644 --- a/docs/development.md +++ b/docs/development.md @@ -32,13 +32,18 @@ clone only `HEAD`; the portable lane keeps full history for the commit-message check. The Rust lane deletes the runner's unused toolchains only when less than 40 GB is free. Running `./scripts/test.sh` locally still runs every gate. -The `check` status requires every lane to succeed and reports as soon as the -lanes finish. Mutation testing has a status of its own, `mutation`, which on a -pull request requires the plan and every shard to pass; shards may be skipped -only when the plan found no mutants. Mutation planning checks the Rust diff -before installing its tools, so a PR with no Rust changes starts no mutation -shards and pays no mutation-tool setup cost. Rust diffs still use the complete -diff-selected mutant set and the existing exclusions. +`check` is the only status a branch rule for main should require. It needs +every lane to succeed, and on a pull request the mutation plan and every shard +too; shards may be skipped only when the plan found no mutants. A push to main +or a merge queue entry plans no mutants, so `check` reports as soon as the +lanes finish. The workflow also runs on `merge_group`, so once main has a merge +queue, it tests each pull request on top of the main it will land on; two pull +requests that pass against their own bases can still break main together. A +merge group saves no caches, since its queue ref is gone once it lands. +Mutation planning checks the Rust diff before installing its tools, so a PR +with no Rust changes starts no mutation shards and pays no mutation-tool setup +cost. Rust diffs still use the complete diff-selected mutant set and the +existing exclusions. Every skip the gate sees joins its `not checked by this run:` summary: a gate's own skip, a Node browser test's skip or todo, a missing formatter in @@ -52,10 +57,11 @@ Browser tests that wait out a timer jump the fake clock with `page.clock.fastForward` rather than `runFor`, which also steps every frame and interval in between and once made a single test take eighteen minutes. -The scheduled avatar check restores the same Cargo, vendored-asset and Chromium -caches as the main checks. Its Rust profile and linker flags match so dependency -artifacts can be reused; it only restores these caches rather than saving a -second copy for the weekly job. Chromium's system dependencies are still +The weekly avatar check is the `avatar` job in the same workflow, started only +by the schedule or a manual dispatch, and neither starts anything else. It +restores the Cargo, vendored-asset and Chromium caches through the same +composite actions under `.github/actions/` as the lanes, and saves none of them, +so the weekly job keeps no second copy. Chromium's system dependencies are still installed on every fresh runner. The gate picks its own compiler and Python, so running it directly builds with diff --git a/tests/test_test_gate.py b/tests/test_test_gate.py index 575887831..2985ca33b 100644 --- a/tests/test_test_gate.py +++ b/tests/test_test_gate.py @@ -19,6 +19,10 @@ ROOT = support.ROOT SCRIPT = ROOT / "scripts/test.sh" +# The events that start only the avatar job. +NOT_AVATAR = ( + "github.event_name != 'schedule' && github.event_name != 'workflow_dispatch'" +) def gates_job(): @@ -224,38 +228,42 @@ def run_status(self, command, **env): check=False, ).returncode - def test_check_waits_only_for_the_gate_lanes(self): + def test_check_requires_the_lanes_and_explains_every_mutation_skip(self): check = support.job("check") - self.assertIn(" needs: gates\n", check) - self.assertIn(" if: ${{ !cancelled() }}\n", check) + self.assertIn(" needs: [gates, plan-mutants, mutants]\n", check) + self.assertIn(f" if: ${{{{ !cancelled() && {NOT_AVATAR} }}}}\n", check) command = support.run_block(check) - for state in ("success", "failure", "cancelled", "skipped", ""): - with self.subTest(state=state): - self.assertEqual( - self.run_status(command, RESULT=state) == 0, - state == "success", + states = ("success", "failure", "cancelled", "skipped", "") + # The lanes are checked first, so one passing mutation answer stands in + # for the rest. + for gates in states: + with self.subTest(gates=gates): + status = self.run_status( + command, GATES=gates, PLAN="success", MUTANTS="success" ) - - def test_mutation_status_rejects_failed_or_unexplained_skips(self): - mutation = support.job("mutation") - self.assertIn(" needs: [plan-mutants, mutants]\n", mutation) - self.assertIn(" if: ${{ !cancelled() }}\n", mutation) - command = support.run_block(mutation) + self.assertEqual(status == 0, gates == "success") accepted = { # Shards ran and passed. ("success", "success"), # The plan found no mutants, so no shard started. ("success", "skipped"), - # A push to main plans nothing. + # A push or a merge group plans nothing. ("skipped", "skipped"), } - states = ("success", "failure", "cancelled", "skipped", "") for plan in states: for mutants in states: with self.subTest(plan=plan, mutants=mutants): - status = self.run_status(command, PLAN=plan, MUTANTS=mutants) + status = self.run_status( + command, GATES="success", PLAN=plan, MUTANTS=mutants + ) self.assertEqual(status == 0, (plan, mutants) in accepted) + def test_publishing_waits_on_the_lanes_not_on_check(self): + # A push skips the mutation jobs `check` waits on, and the default + # `success()` would carry that skip through `check` to both of these. + self.assertIn(" needs: gates\n", support.job("build")) + self.assertIn(" needs: [gates, build]\n", support.job("release")) + def test_each_lane_has_a_bounded_job(self): timeouts = {lane: int(entry["timeout"]) for lane, entry in lanes().items()} self.assertIn(" timeout-minutes: ${{ matrix.timeout }}\n", gates_job()) @@ -315,34 +323,25 @@ def test_real_node_skips_and_todos_are_reported(self): ], ) - def test_avatar_reuses_compatible_check_caches_without_saving_them(self): - check = support.WORKFLOW.read_text() - avatar = (ROOT / ".github/workflows/avatar.yml").read_text() - - def environment(workflow): - block = support.section(workflow, "\nenv:\n", "\njobs:\n", "env") - return dict(re.findall(r"^ ([A-Z_]+): (.+)$", block, re.M)) - - check_env, avatar_env = environment(check), environment(avatar) - for name in ( - "CARGO_PROFILE_DEV_DEBUG", - "CARGO_PROFILE_TEST_DEBUG", - "RUSTFLAGS", - ): - self.assertEqual(avatar_env[name], check_env[name], name) - self.assertEqual( - avatar_env["CARGO_INCREMENTAL"], check_env.get("CARGO_INCREMENTAL", "1") + def test_avatar_reuses_the_gate_caches_without_saving_them(self): + avatar = support.job("avatar") + self.assertIn( + " if: github.event_name == 'schedule'" + " || github.event_name == 'workflow_dispatch'\n", + avatar, ) - - def cache_inputs(workflow, name): - block = support.step(workflow, name) - return support.section(block, " with:\n", "\n #", name).strip() - - cargo = "uses: ./.github/actions/restore-cargo-cache" - for workflow in (check, avatar): - self.assertIn(cargo, support.step(workflow, "Restore cargo cache")) - for name in ("Restore vendored bytes", "Restore Chromium"): - self.assertEqual(cache_inputs(check, name), cache_inputs(avatar, name)) + # The schedule and the dispatch exist for this job alone. + gates = gates_job() + self.assertIn(f" if: {NOT_AVATAR}\n", gates) + for name, action in ( + ("Restore cargo cache", "restore-cargo-cache"), + ("Restore vendored bytes", "restore-vendor"), + ("Install Chromium", "chromium"), + ): + with self.subTest(step=name): + uses = f" uses: ./.github/actions/{action}" + self.assertIn(uses, support.step(avatar, name).splitlines()) + self.assertIn(uses, support.step(gates, name).splitlines()) self.assertNotIn("actions/cache/save", avatar) self.assertNotIn("uses: actions/cache@", avatar) @@ -375,8 +374,9 @@ def test_each_lane_installs_what_its_gates_need(self): self.assertEqual( condition(gates, f"Install {name}"), "matrix.lane == 'portable'" ) - for name in ("Restore Chromium", "Install Chromium"): - self.assertEqual(condition(gates, name), "matrix.lane != 'portable'") + self.assertEqual( + condition(gates, "Install Chromium"), "matrix.lane != 'portable'" + ) # After the gates pass, so the binary is this checkout's. for name in ( "Prove the build names its commit", @@ -393,11 +393,11 @@ def test_each_lane_installs_what_its_gates_need(self): with self.subTest(step=name): self.assertEqual( condition(gates, name), - f"matrix.lane == '{lane}' && " + f"github.event_name != 'merge_group' && matrix.lane == '{lane}' && " f"steps.{step_id}.outputs.cache-hit != 'true'", ) self.assertIn( - f"key: ${{{{ steps.{step_id}.outputs.cache-primary-key }}}}", + f"key: ${{{{ steps.{step_id}.outputs.key }}}}", support.step(gates, name), ) self.assertLess(