Repository navigation
docs: fix contributor setup instructions - #285
Conversation
73a75d4 to
c680097
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Example toolchain installation is incomplete, and removing workspaces may break Bun installs.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates contributor setup documentation, fixes a broken badge, and removes duplicate workspace metadata.
Changes:
- Adds PocketIC and example build guidance.
- Corrects the test workflow badge.
- Removes the redundant
workspacesdeclaration.
File summaries
| File | Summary |
|---|---|
README.md |
Adds PocketIC setup instructions and fixes the badge. |
package.json |
Removes duplicate workspace metadata; Bun compatibility needs preservation. |
examples/README.md |
Adds build guidance, but toolchain setup remains incomplete. |
Review details
Suppressed comments (1)
package.json:11
- Removing this field breaks the Bun installation path: the example test packages all declare
@dfinity/picasworkspace:*(for example,examples/counter/tests/package.json:5), but Bun discovers the root workspace frompackage.json, notpnpm-workspace.yaml. A freshbun installwill therefore not link/resolve these packages. Keep this field for Bun or replace it with a Bun-supported workspace configuration; pnpm's continued resolution does not cover Bun installs.
"scripts": {
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
c680097 to
3c329bf
Compare
|
On the suppressed comment about removing the root Keeping it as-is, deliberately: after this stack nothing supported runs The cost is that |
There was a problem hiding this comment.
🔵 Needs a closer look
examples/README.md still omits building packages/pic before example tests.
Review details
Suppressed comments (1)
examples/README.md:39
- Following this setup on a clean checkout still makes
pnpm test:examplesfail: the workspace dependency resolves@dfinity/pictopackages/pic, whose entry point isdist/index.js, butpnpm idoes not build that directory and this sequence only runsbuild:examples. Please run the package build before the example build (for example, addpnpm buildhere).
- Build all examples. This compiles the canisters and generates the
`declarations/` that the tests import, so it must run before the tests:
```bash
pnpm build:examples
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
3c329bf to
6396284
Compare
|
Valid — fixed. Following the guide on a clean checkout did fail:
Added a Verified by deleting |
- Both setup guides omitted the `pocket-ic` download. `.npmrc` sets `ignore-scripts=true`, so the `postinstall` that fetches the binary never runs on a plain install; `pnpm run setup` is now an explicit step. - `build:examples` generates the gitignored `declarations/` the tests import and needs the ICP CLI toolchain, neither of which was documented. - The two CI badges pointed at workflow files that do not exist. - Drops the `workspaces` field, which duplicated pnpm-workspace.yaml for bun installs and is ignored by pnpm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6396284 to
4ceff2e
Compare
> **Stack of 4 — merge bottom to top.** Review each layer against the one below it, not `main`. > > **→** 1. dfinity#284 — security fix: dependency advisories, `bun.lock` removal > 2. dfinity#285 — contributor setup docs > 3. dfinity#286 — `consumer_install` job, retires `e2e_test_bun` > 4. dfinity#288 — consumer install guidance > > Repo installs are pnpm-only: `pnpm.overrides`, `minimumReleaseAge` and `onlyBuiltDependencies` are pnpm-only fields, so a second installer resolves a graph that bypasses them. Bun remains a supported **consumer** runtime, covered by `consumer_install (bun)` from layer 3. > > Scoped to the security fix and the install-path change it forces. Pre-existing documentation gaps in `README.md` and `examples/README.md`, including the `pnpm run setup` step and the canister toolchain, are fixed in dfinity#285. Clears all 24 open Dependabot alerts (1 critical, 13 high, 8 moderate, 2 low). Supersedes dfinity#275 and dfinity#283. Bumps `vite` to `^7.3.5` and `vitest` to `^4.1.11`, and extends `pnpm.overrides` to cover the 21 transitive advisories. Drops the `minimumReleaseAgeExclude: [vite]` entry, annotated for removal after 2026-04-16. ## Removing `bun.lock` `pnpm.overrides`, `minimumReleaseAge` and `onlyBuiltDependencies` are pnpm-only, so `bun.lock` resolved a second dependency graph that bypassed them. Against the overrides as they stood on `main`: | `pnpm.overrides` on `main` | pnpm-lock.yaml | bun.lock | | --- | --- | --- | | `brace-expansion@>=1 <2` → `^1.1.13` | 1.1.13 | 1.1.12 | | `brace-expansion@>=2 <2.0.3` → `^2.0.3` | 2.0.3 | 2.0.2 | | `picomatch@>=2 <3` → `^2.3.2` | 2.3.2 | 2.3.1 | | `picomatch@>=4 <4.0.4` → `4.0.4` | 4.0.4 | 4.0.3 | | `yaml@>=2 <2.8.3` → `2.8.3` | 2.8.3 | 2.8.2 | This PR then raises several of those bounds to clear the open advisories, so the versions now resolved are `brace-expansion` 1.1.18 / 2.1.4, `picomatch` 2.3.2 / 4.0.4 and `yaml` 2.8.3. Dependabot cannot keep `bun.lock` current either: bun is supported for [version updates but not security updates](https://docs.github.com/en/code-security/dependabot/ecosystems-supported-by-dependabot/supported-ecosystems-and-repositories), so security PRs updated `package.json` and `pnpm-lock.yaml` only, leaving it stale and failing `bun i --frozen-lockfile`. `e2e_test_bun` now installs with pnpm and still builds and tests with bun. Contributor-facing commands move to pnpm to match, including the eight per-example READMEs. ## Verified `pnpm audit` clean · `pnpm i --frozen-lockfile` up to date · `pnpm test:pic` 65 passed · `bun run build` against the pnpm tree 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
> **Stack of 4 — merge bottom to top.** Review each layer against the one below it, not `main`. > > 1. dfinity#284 — security fix: dependency advisories, `bun.lock` removal > 2. dfinity#285 — contributor setup docs > **→** 3. dfinity#286 — `consumer_install` job, retires `e2e_test_bun` > 4. dfinity#288 — consumer install guidance > > Repo installs are pnpm-only: `pnpm.overrides`, `minimumReleaseAge` and `onlyBuiltDependencies` are pnpm-only fields, so a second installer resolves a graph that bypasses them. Bun remains a supported **consumer** runtime, covered by `consumer_install (bun)` from layer 3. > > The job name is required by convention; add the four `consumer_install:required (…)` contexts to the ruleset only after this merges, or the layers below block on a job they do not have. Adds a `consumer_install` job that verifies the published package actually installs and runs, and retires the `e2e_test_bun` matrix it replaces. ## The gap `@dfinity/pic` delivers the `pocket-ic` binary from a `postinstall` script. Three of the four supported package managers now block lifecycle scripts by default, each with its own opt-in: | | postinstall by default | opt-in | | --- | --- | --- | | npm 12 | blocked | `allowScripts` | | pnpm 10 | blocked | `onlyBuiltDependencies` | | bun 1.3 | blocked | `trustedDependencies` | | yarn 1 | runs | — | Nothing covered this: every example depends on `"@dfinity/pic": "workspace:*"`, so the tarball was never installed in CI. `npm pack` appeared only in `release.yml`, to publish. ## The job Packs the package, installs it with each manager, asserts the binary is present and executable, then starts and stops a `PocketIcServer` against it. All four legs pass on ubuntu. The opt-in lives in `scripts/smoke-test-install.sh` alongside the guides, so a change to the pnpm, bun or yarn mechanism fails the build. **npm is only partly covered.** npm keys `allowScripts` by resolved spec, so installing from a tarball needs the `file:` path where a consumer installing from the registry writes the package name. This leg proves npm still honours `allowScripts`, but not the key form the guide documents. Closing that needs a post-release leg installing `@dfinity/pic` from the registry; noted in the script. Runs on ubuntu only: the opt-in mechanisms are not platform specific, and the darwin binary download stays covered by `e2e_test_nodejs` on `macos-latest`. ## Replacing `e2e_test_bun` That job ran the jest and vitest examples under bun. With no bun-specific code in `packages/pic` and no example using `bun:test`, it largely duplicated `e2e_test_nodejs` against runtime-agnostic code, while covering neither bun as a package manager nor bun as a test runner. Bun as a *test runner* is still uncovered — `using-bun.mdx` documents a `bun:test` and `bunfig.toml` workflow that nothing exercises, before or after this change. A `bun:test` example would be the fix; out of scope here. Renames a required check. The `e2e_test_bun:required` contexts have already been dropped from ruleset `4081516`, so nothing blocks this; see the note above for when to add the `consumer_install` ones. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ity#288) > **Stack of 4 — merge bottom to top.** Review each layer against the one below it, not `main`. > > 1. dfinity#284 — security fix: dependency advisories, `bun.lock` removal > 2. dfinity#285 — contributor setup docs > 3. dfinity#286 — `consumer_install` job, retires `e2e_test_bun` > **→** 4. dfinity#288 — consumer install guidance > > Repo installs are pnpm-only: `pnpm.overrides`, `minimumReleaseAge` and `onlyBuiltDependencies` are pnpm-only fields, so a second installer resolves a graph that bypasses them. Bun remains a supported **consumer** runtime, covered by `consumer_install (bun)` from layer 3. Fixes the consumer install instructions. PicJS downloads the `pocket-ic` binary from a `postinstall` script, and npm, pnpm and bun all block install scripts by default — so the binary is missing and `PocketIcServer.start()` fails. Three problems: - `getting-started.mdx` never showed how to install `@dfinity/pic`, or that the install script needs permitting. No guide did. - `running-tests.mdx` stated the binary is "downloaded when installing `@dfinity/pic`", which is untrue by default for three of the four supported package managers. - `using-bun.mdx` held the only opt-in documented anywhere. Adds an **Installing PicJS** section to the getting started guide with the install command and required entry per package manager, corrects the running tests guide, and points the bun guide at the shared section. ## Opt-ins, each verified against a registry install | Package manager | Entry in `package.json` | | --- | --- | | npm 12 | `"allowScripts": { "@dfinity/pic": true }` | | pnpm 10 | `"pnpm": { "onlyBuiltDependencies": ["@dfinity/pic"] }` | | bun 1.3 | `"trustedDependencies": ["@dfinity/pic"]` | | yarn 1 | not needed | Confirmed by installing `@dfinity/pic` from the registry with each and asserting the binary lands. The `consumer_install` job from dfinity#286 covers the pnpm, bun and yarn mechanisms on every run; npm's is only partly covered there, because a tarball install keys `allowScripts` by `file:` path rather than by name. Tabs match the idiom in `using-jest.mdx`, `using-vitest.mdx` and `using-bun.mdx`. Nothing in this repo renders MDX — the docs build is typedoc plus a copy — so the rendered page is worth an eye. Longer term this friction is better removed than documented: shipping the binary as platform-specific `optionalDependencies` gated on `os`/`cpu` needs no install script and no opt-in, which is the approach esbuild, swc and sharp all moved to. Out of scope here. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes contributor setup instructions. All four issues predate this stack.
pocket-icdownload..npmrcsetsignore-scripts=true, so thepostinstallthat fetches the binary does not run on a plain install. Verified: after deleting the binary,pnpm i --frozen-lockfiledoes not restore it andpnpm run setupdoes. Both guides now list it as a step.build:examplesgenerates the gitignoreddeclarations/that the specs import (counter.spec.tsimports../../declarations/counter.did.js), and rootbuildonly buildspackages/pic. It also needs the ICP CLI toolchain, which no guide mentioned.test-nodejs.ymlandtest-bun.yml; neither file exists.workspaces. It duplicatedpnpm-workspace.yamlexactly for bun installs, and pnpm reads only the YAML. Verified all 12 workspace projects still resolve.🤖 Generated with Claude Code