Conversation
There was a problem hiding this comment.
🟡 Changes recommended
README setup documentation still has two unresolved issues involving PocketIC setup and pnpm instructions.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR patches dependency advisories, standardizes installs on pnpm, and retains Bun runtime testing.
Changes:
- Updates direct and transitive dependencies through pnpm overrides.
- Removes the stale
bun.lockand updates Bun CI installation. - Refreshes contributor setup instructions and workflow badges.
File summaries
| File | Summary | Findings |
|---|---|---|
README.md |
Updates setup instructions and badges. | Nit (3 votes): Add the explicit PocketIC setup step before the Bun suite. Nit (2 votes): Update the linked examples/README.md to use pnpm instead of bun i. |
pnpm-workspace.yaml |
Removes the temporary Vite release-age exemption. | None. |
pnpm-lock.yaml |
Records patched dependency resolutions. | None. |
package.json |
Upgrades dependencies and adds pnpm overrides. | None. |
.github/workflows/e2e-test-nodejs.yml |
Installs dependencies with pnpm for Bun runtime tests. | None. |
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 4/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Outstanding documentation and dependency-rationale corrections must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
examples/README.md:43
- This instruction refers to commands “that follow,” but the build and test commands to be run with Bun are all above this paragraph and nothing follows it. As written, the guide still tells a Bun user to run the preceding
pnpmcommands; point the instruction at the commands above (or move it before them).
Dependencies are always installed with pnpm so that `pnpm.overrides` and the
release-age policy apply. To run the suites with [bun](https://bun.sh/) instead,
keep the `pnpm i` above and replace `pnpm` with `bun` in the commands that follow.
package.json:59
- The resolver comparison in the PR description is stale relative to this change: it says the pnpm graph uses
brace-expansion1.1.13/2.0.3 and overrides^1.1.13/^2.0.3, but this file now uses^1.1.16/^2.1.2and the lockfile resolves 1.1.18/2.1.4. Please update that table so the security rationale accurately describes the graph being merged.
"brace-expansion@>=1.0.0 <2.0.0": "^1.1.16",
"brace-expansion@>=2.0.0 <2.1.2": "^2.1.2",
- Files reviewed: 5/7 changed files
- Comments generated: 1
- Review effort level: Lite
7209cf9 to
55da9d6
Compare
Clears all 24 open Dependabot alerts. Bumps vite and vitest, and extends pnpm.overrides to cover the remaining transitive advisories. Removes bun.lock. pnpm.overrides, minimumReleaseAge and onlyBuiltDependencies are pnpm-only, so bun.lock resolved a second dependency graph that bypassed them, pinning the brace-expansion, picomatch and yaml versions those overrides exclude. Dependabot supports bun for version updates but not security updates, so security PRs left it stale and failed `bun i --frozen-lockfile`. The bun job now installs with pnpm and still builds and tests with bun. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
55da9d6 to
933e72a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Documentation setup instructions omit required PocketIC and canister toolchain prerequisites.
Get a fresh assessment by requesting another Copilot review.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
README.md:35
- Because
.npmrcsetsignore-scripts=true,pnpm idoes not runpackages/pic/postinstall.mjs; a fresh checkout therefore has nopackages/pic/pocket-ic, and the documented test setup fails until the explicitpnpm run setupscript is run. Add that step after installation.
pnpm i
examples/README.md:18
pnpm build:examplesinvokesicp buildinscripts/build-examples.sh, but this setup installs neither the ICP CLI normops; those tools are only provisioned by the CI-onlysetup-canister-toolchainaction. A contributor following this guide on a fresh machine therefore reachescommand not foundbefore the examples build. Document or automate this toolchain prerequisite before this command.
pnpm build:examples
- Files reviewed: 13/15 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The dependency, lockfile, CI, and documentation changes span multiple install paths and warrant final human review.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 13/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
Clears all 24 open Dependabot alerts (1 critical, 13 high, 8 moderate, 2 low). Supersedes #275 and #283.
Bumps
viteto^7.3.5andvitestto^4.1.11, and extendspnpm.overridesto cover the 21 transitive advisories. Drops theminimumReleaseAgeExclude: [vite]entry, annotated for removal after 2026-04-16.Removing
bun.lockpnpm.overrides,minimumReleaseAgeandonlyBuiltDependenciesare pnpm-only, sobun.lockresolved a second dependency graph that bypassed them. Against the overrides as they stood onmain:pnpm.overridesonmainbrace-expansion@>=1 <2→^1.1.13brace-expansion@>=2 <2.0.3→^2.0.3picomatch@>=2 <3→^2.3.2picomatch@>=4 <4.0.4→4.0.4yaml@>=2 <2.8.3→2.8.3This PR then raises several of those bounds to clear the open advisories, so the versions now resolved are
brace-expansion1.1.18 / 2.1.4,picomatch2.3.2 / 4.0.4 andyaml2.8.3.Dependabot cannot keep
bun.lockcurrent either: bun is supported for version updates but not security updates, so security PRs updatedpackage.jsonandpnpm-lock.yamlonly, leaving it stale and failingbun i --frozen-lockfile.e2e_test_bunnow 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 auditclean ·pnpm i --frozen-lockfileup to date ·pnpm test:pic65 passed ·bun run buildagainst the pnpm tree🤖 Generated with Claude Code