[Fix] Say why the downloads badge counts the v0.1.1 draft - #449
Merged
Merged
Conversation
#447 selects stable tags by the absence of a `-` in the tag rather than by GitHub's `prerelease` flag, and justified it by saying the flag is set on v0.1.1 by mistake. It is not. That tag is titled "v0.1.1 draft", its body calls it experimental, and it was never published as a release: this project has shipped four, v0.1.0, v0.1.2, v1.0.0 and v1.0.1. The real reason is the stronger one. All three assets under that tag are the v0.1.0 binaries, so their 42 downloads are downloads of the app as it then stood and belong to v0.1.0. Filtering on GitHub's flag would silently drop them, which is precisely why the flag is not used. No behaviour changes and no number moves: the badge still reads 240, and the tag rule gives the right answer for all eight tags. What changes is that a reader who checks the claim now finds it true. STATS.md also gains the per-release breakdown, because "four releases" beside a CSV listing five tags is the confusion this correction exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126Pv8GFR6dszndiaG58D4G
|
Warning Review limit reachedNext included review available in 19 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
juanmaguitar
added a commit
that referenced
this pull request
Sep 11, 2026
…st, Fixes #399, Fixes #387 (#450) ## Why `build.files` in package.json was an exclusion list, so every top-level entry of the checkout shipped inside `app.asar` unless someone had named it. That shape is what packaged the App Store Connect key in #390, and it is why a developer's checkout ships different contents from a CI checkout (#387): a baseline `npm run pack:dir` on trunk `4110349` with seeded leftovers packed `test-results/`, the VitePress build under `docs/.vitepress/dist/`, and even gitignored personal files (`CLAUDE.local.md`, `AGENTS.override.md`) next to `tests/`, `docs/`, `scripts/`, `.buildkite/`, `fastlane/` and every root Markdown file. This is not a size change and the PR does not claim one: #399 is about which way the default points. ## What changes - **`build.files` is now a positive list**: `package.json`, `src/**/*` (minus `src/renderer/index.jsx`, which the esbuild bundle replaces), and `local-playground-web/**/*` (optional, served by `src/main.js` when present). The dugite trims stay as `!` rules; electron-builder applies only the negative patterns of `files` to `node_modules`, so production dependencies ship exactly as before (same 28538 entries under `node_modules`). `!vendor` and `!.codesigning` go, because unlisted is excluded. - **Every file under `src/` is checked, not just the archive root.** `src/**/*` is recursive, so the expected set is derived from Git — tracked under `src/`, minus the esbuild entry point, plus the two build outputs that replace it — and compared against a recursive walk inside the archive. An untracked file anywhere under `src/` fails by name. Deriving beats enumerating here: a hand-written list of eighty entries is the exclusion list again. - **The one native module ships its binary and nothing else, on Windows too.** The signed Windows artifact carried nineteen MSVC intermediates beside `fs_ext.node` (`.iobj`, `.ipdb`, `.lib`, `.exp`, the `.vcxproj` pair, eight `.tlog` logs under `obj/`), about 680 KB that electron-builder's default excludes miss by an extension. Three `!` rules trim them. Two assertions hold the line: the build directory in `app.asar.unpacked` must contain exactly the binary, and the module is now *loaded* inside the packaged main process rather than only resolved, so a rule that overreached into the binary, or an ABI mismatch, fails in CI instead of on a contributor's first site. - **The bundled npm CLI is now asserted too.** `src/main.js`, `src/install-runner.js` and `src/script-runner.js` all resolve `npm/package.json` to run installs and build scripts, and nothing checked that it survived packaging. It is in `REQUIRED_MODULES` beside the Playground CLI, the file-lock native module and dugite. - **The packaged smoke test reads the payload against the same list**, inside and outside the archive: the root of `app.asar` through Electron's asar-aware fs from the packaged main process, and the root of `app.asar.unpacked` on disk. Anything outside `node_modules`, `package.json`, `src` (and the optional `local-playground-web`) fails, and the three required entries must be present. The in-asar `.codesigning` check is subsumed; the plain-fs walk of the whole payload for `.codesigning` stays, since it covers places neither root listing reaches. - **CI seeds the leftovers before packaging**: the packaged job stages a signing key, `test-results/`, a top-level `dist/intermediate/` and `docs/.vitepress/dist/` and then proves none ships, on macOS and Windows. - **Root cause of the "nested dist" in #387, confirmed before fixing**: electron-builder always excludes its own output directory (`directories.output`, so a top-level `dist/` never shipped), but nothing covered a `dist/` anywhere else. The one in the asar is `docs/.vitepress/dist/`, the VitePress build. `test-results/` had no rule at all. Not in this PR: `build/icon.png`, which `src/main.js` references on Linux only. electron-builder excludes `buildResources` by default, so that path has never resolved in a packaged app; the allow-list does not change it, and Linux is outside the v1.1.0 validation scope (#388). ## How to test this Platforms: macOS and Windows, because the assertion is about the packaged payload and the payload is built per platform. **Automated, any platform with a checkout:** In the repository root: ``` npm run build:once && CSC_IDENTITY_AUTO_DISCOVERY=false npm run pack:dir npm run test:e2e:packaged ``` Fifteen tests pass. To watch the new assertion fail on the old rule: check out trunk's package.json (`git show trunk:package.json > package.json`), rebuild with the same two commands, and run `npx --no-install playwright test --project=packaged -g allow-listed`. It fails listing every root entry that used to ship (verified on macOS: 27 offenders, `test-results` and `docs` among them). **Automated, run in CI at `010ec0e`:** the packaged job seeds all four leftovers and then packages, on both platforms. Thirteen tests passed on **macOS** (job `103260902735`) and thirteen on **Windows** (job `103260902938`), all seven checks green. **At `7a40ce8`:** fifteen pass on **macOS** (job `103300828500`) and fifteen on **Windows** (job `103300828243`). The Windows run is the one that proves the native-module trim: it is the platform that leaves the MSVC working set behind, and its build directory now holds exactly `fs_ext.node`. The module also loads for real on both. Those two runs are the CI half of "a local build and a CI build contain the same tree". **Payload verified on the signed Buildkite artifacts, both platforms.** Buildkite build **#1020** is the build for `7a40ce8`, all three platform steps green. Its Windows `.exe` was inspected directly, with its SHA-256 (`3c693d13…496c9eb`) matched against the artifact Buildkite lists, so the file is tied to that exact build and commit. The macOS `.dmg` inspected is from the branch's previous build (the three earlier commits are payload-identical to each other; `7a40ce8` changes only the Windows tree, as the row below shows): | Check | macOS `.dmg` | Windows `.exe` | |---|---|---| | `app.asar` root | `node_modules`, `package.json`, `src` | same | | `app.asar.unpacked` root | `node_modules` | same | | `src/` contents vs `git ls-files` | exact match | exact match | | `tests`, `docs`, `test-results`, `dist`, `scripts`, `fastlane`, `.buildkite` | absent | absent | | `.codesigning` anywhere in the payload | absent | absent | | credential manager and git-lfs in the bundled Git | absent | absent | | `fs-ext-extra-prebuilt/build/Release` | `fs_ext.node` only | `fs_ext.node` only (at `7a40ce8`; 19 MSVC intermediates before) | | Signature | Developer ID Automattic, notarized | Authenticode not checkable from macOS | **Same tree, local versus CI (#387's third acceptance): discharged.** A local `pack:dir` on this branch and the signed macOS artifact list **28620 identical entries**, the same paths with no diff at all. The Windows artifact of build #1020 differs from the previous Windows artifact in exactly the 19 entries `7a40ce8` set out to remove, and in nothing else. The two platform artifacts now differ only in the bundled Git (262 entries on macOS, 417 on Windows). **Manual pass done — JuanMa, Windows 11, Buildkite build #1020 (commit `7a40ce8`), with Git for Windows installed on the machine, which is the case that matters.** The installed app booted and painted the site list. On a site created with the app: two tickets linked (`ticket/60001`, `ticket/60002`), a switch between them, PR #13456 applied, and an Update trunk that offered to save the five applied files, then reset the checkout; afterwards, plain `git status` in the site directory read "On branch trunk, up to date with origin/trunk, working tree clean" and `git branch` listed both ticket branches. During a fresh "Create WordPress Core site", a PowerShell loop over `Get-Process` caught the Git processes at ``` ...\resources\app.asar.unpacked\node_modules\dugite\git\cmd\git.exe ...\resources\app.asar.unpacked\node_modules\dugite\git\mingw64\bin\git.exe ``` and never at the system Git. That is the bundled binary spawning from the signed installation, with a competing Git on `PATH` and ignored, as the invariant says. **Starting state:** 1. Download the macOS `.dmg` (or `.zip`) and the Windows `.exe` from that build. 2. Install and launch each. The first window paints and the site list shows "WordPress Core". 3. macOS: in Terminal, from any directory, list the asar root: `npx @electron/asar list "/Applications/WordPress Contributor Toolkit.app/Contents/Resources/app.asar" | awk -F/ 'NF>=2{print $2}' | sort -u` Expected: exactly `node_modules`, `package.json`, `src`. 4. Windows: in PowerShell, from any directory (the install folder is named after the package, not the product): `npx @electron/asar list "$env:LOCALAPPDATA\Programs\electron-setup-wordpress-core\resources\app.asar" | ForEach-Object { ($_ -split '/')[1] } | Sort-Object -Unique` Expected: the same three entries. 5. Either platform: create a site or open an existing one and run **Update trunk** or link a ticket, so the bundled Git spawns from `app.asar.unpacked`. On **Windows**, also run an `npm install` on a site, which exercises the bundled npm CLI and the two shims copied out of `src/`. 6. Same tree, local versus CI (#387's third acceptance). On **macOS**, in the repository root, build locally and compare the installed Buildkite artifact against it: ``` npm run build:once && CSC_IDENTITY_AUTO_DISCOVERY=false npm run pack:dir npx @electron/asar list dist/mac-*/*.app/Contents/Resources/app.asar | sort > /tmp/local.txt npx @electron/asar list "/Applications/WordPress Contributor Toolkit.app/Contents/Resources/app.asar" | sort > /tmp/ci.txt diff /tmp/local.txt /tmp/ci.txt && echo "same tree" ``` Run it from a checkout that has already run the test suites, so the leftovers are present locally and their absence means something. **What must not have happened:** - No `tests`, `docs`, `test-results`, `.codesigning` or any root Markdown file in the asar listing. - The bundled Git still works from the installed app (a ticket link or trunk update completes), which proves the `asarUnpack` rule and the dugite trims survived the new list. - The Windows install still runs npm scripts (the `win-spawn-patch.js` and `electron-node-compat.js` shims are copied out of `src/` at run time; a dropped `src/` file would surface as a failed `npm install`). ## Risks and limitations - The two lists (package.json and the spec) are still edited by hand, deliberately: widening what ships should be a visible edit in two places. They cannot silently diverge, because a third test derives the allowed set from `build.files` and asserts it equals the literal list. - `local-playground-web` is allowed but not asserted present, because it is not in the repository. A checkout that carries one ships it, as before. - Linux artifacts (AppImage, deb, snap) are built with the same list but not validated here (#388). The Linux window icon path in `src/main.js` was already unreachable in packaged builds and stays so; recorded here rather than filed, per the no-new-issues rule for v1.1.0. - The local-versus-CI tree comparison is a step of the manual pass, not something CI does on its own: no job has both artifacts. What CI does prove, on both platforms, is that the four machine-dependent leftovers are gone and the roots match the list. With a positive list there is nothing machine-dependent left that could differ, but the diff above is the evidence rather than the argument. - **`node_modules` is not governed by the allow-list.** electron-builder collects production dependencies itself and applies only the `!` patterns of `files` to them, so a leftover inside a dependency has to be named. The one such case found, the MSVC working set of `fs-ext-extra-prebuilt` on Windows, is trimmed in `7a40ce8` and asserted on both platforms; the packaged smoke test would not have noticed it before, because the module was resolved but never loaded. Only Windows can fail the new assertion, so its packaged CI job is the proof, and the signed Windows artifact of build #1020 was re-inspected: the build directory holds exactly `fs_ext.node`. - Code-signing and notarization are unaffected by `files`. The macOS artifact is signed with Automattic's Developer ID and notarized, confirmed by `codesign` and `spctl`. The Windows Authenticode signature could not be checked from macOS. - electron-builder strips the `build` key from the packaged `package.json`, so an artifact cannot be tied to a commit from the inside. The Windows artifact is tied to `7a40ce8` from the outside, by its SHA-256 against Buildkite build #1020. The macOS `.dmg` inspected predates `7a40ce8`; that commit changes only the Windows tree, and the macOS packaged CI job at `7a40ce8` (15/15) covers the rest. ## Related Fixes #399 Fixes #387 Follow-up to #390 and #396 (the key incident), #377 (the `test/` to `tests/` move an exclusion list would have had to track). --- <details> <summary>Design decisions and alternatives considered</summary> - **Exact set, not "no offenders only".** The issue asked that the asar contain no top-level entry outside the list. Requiring the three runtime entries to be present as well means a broken glob (say `src/**` stops matching after a rename) fails here rather than at first launch. - **Adding `!test-results` and `!**/dist` instead** (what #387 first proposed) was rejected: it keeps the exclusion shape #399 identifies as the problem, and a `dist/` glob would need care not to strip `dist/` directories inside published npm packages, which many have. - **Reading the asar from outside with `@electron/asar`** was rejected in favour of `readdirSync(app.getAppPath())` inside the packaged process: it uses the same asar-aware fs the app runs on, needs no extra dependency in the test, and is the pattern the other in-asar assertions already use. - **Seeding in the workflow, not in the test.** The test runs after packaging, so it cannot seed. The seeds live in the packaged job's existing canary step. Locally a checkout that has run the suites or the docs build carries two of the four already, which is what TESTING.md now says; `.codesigning` exists only on a signing machine, so the complete proof is the CI run. - **`build/icon.png` left out.** Adding it would put a new `build` entry in every asar for a Linux-only path that has never resolved in a packaged app. Out of scope for v1.1.0. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **0 [fix here] · 3 [follow-up] — 2 of the 3 follow-ups fixed in the branch, 1 recorded as a limitation.** - **Review:** completed — `/self-review`, judgement pass in a fresh subagent context against `.github/instructions/code-review.instructions.md`; reviewed head `b1579be` (reviewed as the working tree at that content) / base `trunk` `4110349`; evidence: `npm run lint` clean, `npm test` 1284 pass 2 skip 0 fail, `npm run test:e2e:packaged` 11/11 on macOS against a build made with the new list; outcome below. - **Since review:** `4110349` → `b1579be` → `c7f430d` → `010ec0e` → `23b41f9` (trunk merged in from GitHub, #449 and #452, no overlap with this diff) → `7a40ce8`. `7a40ce8` trims the native module's Windows build leftovers found while inspecting the signed artifact, and turns the native-module check from resolve into load; lint, the unit suite (1285 pass) and the packaged suite (15/15 on macOS) re-run on it. The self-review ran against the working tree that became `b1579be`, plus the two fixes listed below. `c7f430d` adds the npm CLI assertion, written while checking the runtime paths the allow-list could drop. `010ec0e` answers CodeRabbit's one finding. Each step was re-run through lint, the unit suite and the packaged suite (13/13 on macOS at `010ec0e`). - **Review:** completed — delta pass on `c7f430d`, `010ec0e` and `7a40ce8` (the commits no bot reviewed), judgement in a fresh subagent context against `.github/instructions/code-review.instructions.md`, at head `64d9bd0` (trunk merged in, no overlap). 0 [fix here] · 1 [follow-up] · 1 style. Follow-up: `tests/e2e/packaged/smoke.spec.js:87` runs the host `git` for `ls-files` instead of the bundled binary through `tests/unit/helpers/git.cjs`; a machine without Git on PATH fails the packaged suite for a reason unrelated to packaging. Style: the bundled-Git test's doc block sits above the native-module block, apart from its test. Both left for a follow-up. CI on `64d9bd0`: all seven checks green on both platforms. - **Review:** completed — CodeRabbit, on `c7f430d` only. Its incremental review of `010ec0e` did not run: "Review rate limited", and it replies that it does not re-review already-reviewed commits. So the last commit carries the self-review and the CI evidence, not a bot pass. Full detail on `c7f430d`: Its first two runs did not happen: the check read `pass` with the message "Review rate limited". The third ran. One Major finding, fixed in `010ec0e` and answered inline: `src/**/*` is recursive and nothing asserted the nested contents. It proposed enumerating the runtime files by hand; the fix derives the expected set from Git instead, which closes the same gap without a list that has to be maintained. Verified by dropping a file into `src/`, packaging, and watching the test name it. Fixed here: 1. *The drift guard was one-directional* (`tests/e2e/packaged/smoke.spec.js`). Removing a pattern from `build.files` failed nothing. Added a test that derives the allowed set from `package.json` and asserts it equals the literal list in the spec. Verified by deleting `local-playground-web/**/*` from package.json: the test fails naming that entry. 2. *The one negative pattern was unverified* (same file). Both payload assertions read only tree roots, so `!src/renderer/index.jsx` could stop matching silently. The asar test now also lists `src/renderer` and asserts `index.jsx` is absent while `index.html`, `index.js` and `index.css` are present — which doubles as a check that the renderer bundle was built before packaging. Deferred, with reason: 3. *The Linux window icon* (`src/main.js:490`) resolves to `<app.asar>/build/icon.png`, which has never shipped: electron-builder excludes `directories.buildResources` unconditionally, so the old exclusion list did not carry it either. Pre-existing and Linux-only, and Linux formats are explicitly outside the v1.1.0 validation scope (#388). Recorded in Risks rather than filed as an issue. Style notes from the same pass, all applied: the stale `vendor` sentence in `.gitignore:33` now points at the allow-list; the two failure messages named the whole listing instead of the offenders; the local-equivalence claim in `TESTING.md` was unconditional and is now hedged (`.codesigning` exists only on a signing machine, so the full proof is the CI run). The `dist/intermediate` seed in the workflow was noted as redundant — electron-builder already excludes its own output directory — and kept deliberately: it is the seed that documents *why* a top-level `dist/` is safe, and it guards a future re-introduction of a permissive pattern. </details> <details> <summary>Implementation notes</summary> - electron-builder 26.15.3, `node_modules/app-builder-lib/out/fileMatcher.js`: `getMainFileMatchers` adds `package.json` when the list is positive, prepends `!**/node_modules/**`, `!<buildResources>{,/**/*}` and `!<output>{,/**/*}`, and appends the default name and extension excludes. `getNodeModuleFileMatcher` takes only the `!` patterns from `files`, which is why the dugite trims keep working. - Baseline asar root on trunk `4110349` (macOS, local, seeded): `node_modules`, `tests`, `src`, `docs`, `scripts`, `.buildkite`, `.claude`, `.agents`, `test-results`, `fastlane`, `.bundle`, `TESTING.md`, `STATS.md`, `README.md`, `playwright.config.js`, `package.json`, `LICENSE`, `Gemfile.lock`, `Gemfile`, `eslint.config.mjs`, `CONTRIBUTING.md`, `CLAUDE.md`, `CLAUDE.local.md`, `AGENTS.override.md`, `AGENTS.md`, `.ruby-version`, `.nvmrc`, `.coderabbit.yaml`. Absent: top-level `dist`, `.codesigning`, `build`, `vendor`. - With the allow-list: `node_modules` (28538 entries, unchanged), `src` (81), `package.json`. `app.asar.unpacked` holds only `node_modules` (`@php-wasm`, `dugite`, `fs-ext-extra-prebuilt`). - Local-versus-CI tree comparison: recorded in a comment once the CI artifact for this head exists. </details> <details> <summary>Screenshots or recording</summary> Nothing on screen changed: this PR touches packaging and a test. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01XJUaihwanFHxEps959WLhs --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#447 selects stable tags by the absence of a
-in the tag rather than by GitHub'sprereleaseflag, and justified it by saying the flag is set on v0.1.1 by mistake. It is not.That tag is titled "v0.1.1 draft", its body calls it experimental, and it was never published as a release: this project has shipped four, v0.1.0, v0.1.2, v1.0.0 and v1.0.1.
The real reason is the stronger one. All three assets under that tag are the v0.1.0 binaries, so their 42 downloads are downloads of the app as it then stood and belong to v0.1.0. Filtering on GitHub's flag would silently drop them, which is precisely why the flag is not used.
No behaviour changes and no number moves: the badge still reads 240, and the tag rule gives the right answer for all eight tags. What changes is that a reader who checks the claim now finds it true.
STATS.md also gains the per-release breakdown, because "four releases" beside a CSV listing five tags is the confusion this correction exists to prevent.
Claude-Session: https://claude.ai/code/session_0126Pv8GFR6dszndiaG58D4G
Why
What changes
How to test this
Starting state:
What must not have happened:
Risks and limitations
Related
Design decisions and alternatives considered
Review outcome (required — see AGENTS.md)
<previous SHA> → <current SHA>checked; re-review outcomeImplementation notes
Screenshots or recording