fix: align all skills on @icp-sdk/core ^5 and pin the dependencies that drift off it - #368
fix: align all skills on @icp-sdk/core ^5 and pin the dependencies that drift off it#368marc0olo wants to merge 5 commits into
Conversation
… below 6.0.0 skills/wallet-integration installed @icp-sdk/core unpinned, which resolves to 4.2.3 today -- two majors below every other skill, and unusable alongside @icp-sdk/auth (peer @icp-sdk/core@^5). Cause: @dfinity/oisy-wallet-signer 6.0.0 regressed its peers (core ^5 -> ^4, utils -> ~4.0.3, canisters -> ~3.1.0), and the unpinned install now takes it. - wallet-integration: pin oisy ^4.1.3 and core ^5; Prerequisites now list the peers 4.1.3 actually declares; new pitfall 11 documents the 6.0.0 regression - binding-generation.md: replace the incorrect "core starts at 5.x, there is no 0.x or 1.x release" claim with the real version line and why 6.x must not be taken; correct "bindgen depends on core" (it has no core dependency -- the code it generates imports core); bindgen floor -> ^0.4.0 - dfx-migration.md, icp-cli/SKILL.md pitfall 12: pin ^5 with the ERESOLVE reason Verified: the wallet-integration install line now resolves with @icp-sdk/auth present, one copy of @icp-sdk/core at 5.4.0, no ERESOLVE. Closes #364
Skill Validation ReportProject Checks |
There was a problem hiding this comment.
Pull request overview
This PR fixes cross-skill dependency drift by pinning @icp-sdk/core to the 5.x line in installation guidance and by pinning @dfinity/oisy-wallet-signer below 6.0.0 to prevent npm from resolving an incompatible dependency tree. It also updates icp-cli documentation to correct prior factual claims about @icp-sdk/core version history and @icp-sdk/bindgen’s dependency relationship, and adjusts eval expectations accordingly.
Changes:
- Update
wallet-integrationprerequisites/install guidance and add a pitfall documenting theoisy-wallet-signer@6.0.0peer regression. - Update
icp-clidocs to pin core to^5, bump bindgen floor to>= 0.4.0, and correct prior factual inaccuracies. - Extend eval suites to assert the new pinning guidance and updated version floors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/wallet-integration/SKILL.md | Pins oisy signer below 6.0.0, pins core to ^5, and adds a new pitfall documenting the regression and its impact. |
| skills/icp-cli/SKILL.md | Updates pitfall guidance for TypeScript binding generation to pin core to ^5 and raise bindgen floor. |
| skills/icp-cli/references/dfx-migration.md | Pins install command (core@^5, bindgen@^0.4.0) and briefly explains why not latest. |
| skills/icp-cli/references/binding-generation.md | Corrects bindgen/core relationship, pins core to ^5, and updates bindgen minimum version guidance. |
| evaluations/wallet-integration.json | Adds an adversarial eval case to ensure agents avoid unpinned oisy/core installs and explain incompatibility. |
| evaluations/icp-cli.json | Updates expected behaviors for bindgen minimum version and core pinning guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
@icp-sdk/vetkeys 0.6.0 shipped and depends on @icp-sdk/core@^6.1.0, while @icp-sdk/auth, @icp-sdk/signer and @icp-sdk/canisters still peer on ^5. Because vetKeys declares core as a dependency rather than a peer, npm raises no ERESOLVE -- it silently installs two copies of @icp-sdk/core. The vetKeys clients take a caller-supplied HttpAgent, so an agent built from core 5 is handed to code typed against core 6 and Principal/HttpAgent identity no longer matches across that boundary. Both skills previously said "@icp-sdk/vetkeys (>=0.5)", which permits 0.6.0. - vetkeys, encrypted-maps: pin ^0.5, document why 0.6.0 must not be installed - wallet-integration: the claim that vetkeys "still requires ^5" is now false; rewritten, and notes that oisy 6.0.0 and vetkeys 0.6.0 leave the 5.x line in opposite directions - binding-generation.md, wallet-integration: "pins ^5" -> "anchors core to the 5.x line", addressing the review (vetkeys/SKILL.md uses ^5.4) - new adversarial eval: vetkeys 0.6.0 duplicates @icp-sdk/core Verified: the full skill surface (auth + vetkeys + wallet + canisters) resolves with exactly one @icp-sdk/core at 5.4.0.
|
Thanks — the wording point is correct and is fixed in 4d4e03b. Chasing it also surfaced something larger that the review did not catch, so this PR now covers more than it did. On the review commentsBoth comments make the same point: "Every IC skill pins Agreed.
The larger problemThe same sentence contained a substantive error, not just an imprecise one:
It matters more than a version bump, because the failure is silent: npm i @icp-sdk/vetkeys @icp-sdk/auth # vetkeys unpinned — what our skills said ("≥0.5")vetKeys declares And it is concretely harmful rather than merely untidy — the vetKeys clients take a caller-supplied agent: constructor(agent: HttpAgent, canisterId: string) // HttpAgent, Principal from @icp-sdk/coreso an Both The rule this PR now establishes
Verified — the whole skill surface now composes with exactly one core: New eval
|
…undling runtime deps (#434) Closes #433. Declares `@icp-sdk/core` as a **`peerDependency`** and stops bundling runtime dependencies into `dist/`. Released as **0.7.0** — breaking: consumers must now install `@icp-sdk/core` themselves. ## Why both halves are needed #433 asks for the peer change, which is correct — core is a shared singleton whose classes cross our public API (callers construct the `HttpAgent` they pass in), and as a plain dependency npm silently installed a second copy. But the peer alone would have been cosmetic. `vite.config.ts` set no `rollupOptions.external`, and Vite lib mode externalises nothing by default, so the tarball already inlined a private **289 kB copy of core's agent** — in every install, invisible to npm. Moving core to a peer and rebuilding produces a byte-identical bundle. So this PR does both. ## Changes - `@icp-sdk/core` → `peerDependencies` (`^5.0.0 || ^6.0.0`), kept in `devDependencies` for build/test. - Externalise runtime deps in the Vite build. **`dist/lib`: ~430 kB → ~56 kB.** - `@noble/curves` / `@noble/hashes` → `dependencies`. They are imported by shipped code, so declaring them as dev dependencies was wrong — it only worked because the build inlined them. - New CI job pinning core to the peer-range floor and running the full suite against it. `idb-keyval` stays a regular dependency: no idb-keyval value crosses the public API, and IndexedDB is keyed by (origin, dbName, storeName), so two copies address the same store. ## Peer range spans both majors We use only `Actor.createActor` and the candid `IDL`, unchanged across v5 and v6 — which is why the v5→v6 bump in #432 needed zero source changes. Typechecked against core 5.0.0, 5.2.1, 5.4.0, 6.0.0 and 6.1.0: all clean. This matters: a peer of `^6.1.0` next to `auth@^5` is a hard ERESOLVE, which would turn "silently broken" into "cannot install" until the rest of the SDK moves to core 6. The dual range fixes the hazard without that. It should also let [icskills](https://github.com/dfinity/icskills) drop its `@icp-sdk/vetkeys@^0.5` pin — worth confirming against dfinity/icskills#368 before closing. ## Verification All CI green, including the new `frontend_ic_vetkeys_core_floor` job — full canister suite (4 files, 45 tests) against a live replica on core 5.0.0, so both ends of the peer range are tested. <details> <summary>Note: #433's stated mechanism is overstated</summary> Recording this so we do not carry an inaccurate rationale forward. #433 claims `instanceof` checks fail and Principals get mis-encoded across copies. Neither holds: core has no `instanceof HttpAgent`/`instanceof Agent` anywhere, candid's `PrincipalClass.covariant` tests `x._isPrincipal`, and `Principal.isPrincipal` has an explicit structural fallback. The genuine cross-copy defects are narrower — `actor.js` does `e instanceof RejectError` on errors thrown by the consumer's agent, so error enrichment is silently skipped (degraded diagnostics, not corruption); v6's Actor passes `effectiveTarget:` to a v5 agent that only reads `effectiveCanisterId`, silently ignored (benign here, but fragile); and `Principal`s returned by vetKeys were built by the bundled core, so a consumer's `instanceof Principal` failed. So the case rests on packaging correctness and ~290 kB of dead weight, not an acute bug biting users today. </details> ## Release After merge: `git tag npm/0.7.0 && git push origin npm/0.7.0`, then the docs workflow. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…equence 0.7.0 moves @icp-sdk/core from a dependency to a peer spanning both majors (^5.0.0 || ^6.0.0), so vetKeys no longer forces a core version and coexists with @icp-sdk/auth on a single core. Verified: vetkeys 0.7 + auth resolves to one @icp-sdk/core at 5.4.0, fully deduped. The explicit core pin now matters more, not less: because core is a peer, `npm i @icp-sdk/vetkeys` alone resolves it to 6.1.0 (highest in range), while auth/signer/oisy still require ^5. - vetkeys, encrypted-maps: version tables and prose 0.5 -> 0.7; pin notes rewritten around the peer range instead of the duplication that 0.6.0 caused - wallet-integration: drop the claim that vetkeys requires ^5 (it accepts both) - binding-generation.md: correct the duplicate-core consequence. The previous text claimed it breaks instanceof on Principal/HttpAgent, which overstates it -- core is duck-typed (candid tests _isPrincipal, Principal.isPrincipal has a structural fallback). The real effects are narrower and quieter: the effectiveTarget instanceof Principal shim in pollForResponse stops matching, and error instanceof AgentError fails so reject errors lose enrichment. - eval case rewritten: the 0.6.0 duplication no longer exists; it now tests that core is named explicitly at ^5 against the ^5 || ^6 peer range No code-sample changes: 0.5.0 -> 0.7.0 is API-identical (public typings diff is empty, 69 declarations either side).
…behavioural
Two corrections after review feedback.
1. wallet-integration referenced @icp-sdk/vetkeys, which is both out of scope
for that skill and false there: with oisy pinned below 6.0.0, the wallet
stack resolves @icp-sdk/core to 5.4.0 even unpinned, because oisy 4.1.3 and
@icp-sdk/canisters both peer ^5. The drift-to-6 only happens when vetkeys is
in the project. The paragraph now names only the pin that matters here
(oisy) and defers the repo-wide rationale to the icp-cli skill.
2. The pin notes carried package archaeology that changes nothing an agent
types -- the core version history, the 0.5.0/0.6.0 packaging story, and the
duck-typing internals behind the duplicate-core failure. Removed; what
remains is the command, the reason, and the failure mode.
Evals: version literals removed from every expected behaviour except the ^5
pin itself, which is the guidance. A case asserting a point-in-time snapshot
of the npm ecosystem goes stale on every upstream release -- this one was
rewritten three times in a day. Behavioural assertions ("names core explicitly
rather than letting npm resolve it") survive; detecting that the pin itself has
gone stale is what the version-coherence guard in #366 is for.
marc0olo
changed the title
Review flagged that vetkeys/SKILL.md instructs `@icp-sdk/core@^5` in the
install snippet but `^5.4` in the compatibility line. The suggested fix was to
pick either value, but they are not equivalent: ^5.4 has no requirement behind
it any more.
It entered the skill because @icp-sdk/vetkeys 0.5.0 depended on
"@icp-sdk/core": "^5.4.0". 0.7.0 peers ^5.0.0 || ^6.0.0, so the floor dropped
to 5.0.0, and both core APIs the skill uses (safeGetCanisterEnv, rootKey) are
present in core 5.0.0. Raising the snippet to ^5.4 would have propagated a
constraint no package asks for.
That was also the only exception in the repo, so the earlier hedge in
binding-generation.md ("the 5.x line, written ^5 here, ^5.4 where a later
minimum is needed") is no longer needed -- every skill now pins ^5 flat.
|
Fixed in e8dc66e — the inconsistency was real, though the resolution isn't the one suggested. The comment offers two equivalent-looking options ("either make the earlier install snippet
Taking the first option and raising the install snippet to One knock-on: Re-ran the affected eval ( Minor note for maintainers: the review body reports "Copilot reviewed 9 out of 9 changed files", but the per-file summary table it renders is still the 6-file list from the first review pass — |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
skills/icp-cli/references/binding-generation.md:89
- “Every IC skill pins
@icp-sdk/core@^5” is factually too broad; only a subset of skills mention/install@icp-sdk/coreat all. Consider narrowing the claim to “any skill that recommends installing@icp-sdk/corepins it to ^5” (or similar) to avoid an incorrect universal statement.
**Important — pin `@icp-sdk/core` to `^5`; do not take `latest`.** `6.x` is published, but `@icp-sdk/auth` and `@icp-sdk/signer` peer on `@icp-sdk/core@^5`, so core `6` alongside them fails with `ERESOLVE`. Do not reach for `--legacy-peer-deps` to get past it: that installs two copies of core in one tree, which degrades silently instead of failing. Every IC skill pins `@icp-sdk/core@^5` for this reason.
skills/wallet-integration/SKILL.md:66
- This says “every IC skill names it”, but not all skills in the repo mention
@icp-sdk/core. Consider narrowing the wording (e.g., “other frontend skills here” or “other skills that install core”) to avoid an incorrect universal claim.
The pin that matters here is `@dfinity/oisy-wallet-signer`: its `latest` (`6.0.0`) peers `@icp-sdk/core@^4` and drags the whole tree down a major. Pinned below `6.0.0`, this stack resolves on the 5.x line that `@icp-sdk/auth` and `@icp-sdk/canisters` also require, so wallet integration and Internet Identity login coexist. `@icp-sdk/core` is named explicitly for the same reason every IC skill names it — see the `icp-cli` skill for the repo-wide rationale.
| # Binding Generation | ||
|
|
||
| icp-cli does not have a built-in `dfx generate` command. Use `@icp-sdk/bindgen` (>= 0.3.0) to generate TypeScript bindings from `.did` files. It depends on `@icp-sdk/core` (>= 5.0.0). | ||
| icp-cli does not have a built-in `dfx generate` command. Use `@icp-sdk/bindgen` (>= 0.4.0) to generate TypeScript bindings from `.did` files. bindgen itself has no runtime dependency on `@icp-sdk/core` — the code it *generates* imports from `@icp-sdk/core` (`^5`). |
Three skills recommended installs that silently resolved
@icp-sdk/coreto the wrong major —wallet-integrationdown to core 4,vetkeys/encrypted-mapsup to core 6 — so a dapp combining either with Internet Identity login could not produce a valid dependency tree. Every skill now names@icp-sdk/core@^5explicitly and pins the two dependencies whoselatestdrifts off it.For the reviewer: the diff is small (9 files, +55/−21). One eval behaviour fails, and it is pre-existing and filed as #367 — details at the bottom. Everything else is green.
Closes #364.
Problem
skills/wallet-integration/SKILL.mdinstalled@icp-sdk/coreunpinned. Run today, that resolved to core 4.2.3 — two majors below every other IC skill — and contradicted the same skill's own Prerequisites, which stated>= 5.0.0:Cause:
@dfinity/oisy-wallet-signer6.0.0 regressed its peers backwards —@icp-sdk/core^5→^4,@dfinity/utils→~4.0.3,@icp-sdk/canisters→~3.1.0. The skill was written correctly against 4.1.x; the unpinned command took 6.0.0.Impact:
internet-identity(→@icp-sdk/auth, peer core^5) andwallet-integration(→ core^4) could not be used in the same project — a common combination.Changes
wallet-integration/SKILL.md^4.1.3+ core^5; Prerequisites list the peers 4.1.3 actually declares; new pitfall 11 on the 6.0.0 regressionvetkeys/SKILL.md@icp-sdk/vetkeys→^0.7; name@icp-sdk/core@^5explicitly in the installencrypted-maps/SKILL.mdicp-cli/references/binding-generation.md^0.4.0icp-cli/references/dfx-migration.mdicp-cli/SKILL.md^5with theERESOLVEreasonevaluations/{icp-cli,wallet-integration,vetkeys}.jsonTwo factual corrections worth calling out:
@icp-sdk/corestarts at version 5.x — there is no 0.x or 1.x release" was false. The published line is1.0.0-beta.0…5→4.0.0…4.2.3(stable) →5.x→6.x, and it failed to warn that 6.x now exists — the sentence that would push someone onto a broken install.@icp-sdk/core" was false — bindgen's only dependency iscommander. The code it generates imports core.Verification
The whole skill surface, installed together:
One copy of
@icp-sdk/core, noERESOLVE.npm run validate— 29 skills passed, 19 warnings, identical to baseline.Why
^5and not^6Core 6.1.0 is published, but
@icp-sdk/auth(8.0.3),@icp-sdk/signer(5.6.3) and@dfinity/oisy-wallet-signer(4.1.3) all still require^5. Installing core 6 alongside them is a hardERESOLVE.@icp-sdk/vetkeys0.7.0 now peers core as^5.0.0 || ^6.0.0, so it no longer constrains the choice in either direction — but that makes naming core explicitly more important, not less:npm i @icp-sdk/vetkeyson its own resolves core to6.1.0, the highest in the range. The move to^6is tracked in #365 and deliberately not done here.Note on
@icp-sdk/vetkeys, which moved twice while this PR was open0.6.0shipped mid-review depending on@icp-sdk/core@^6.1.0as a plain dependency, not a peer. Beside@icp-sdk/auththat produced noERESOLVE— npm simply installed two copies of core. This PR briefly pinned^0.5to avoid it, and the packaging was reported upstream as dfinity/vetkeys#433.0.7.0fixed it by making core a peer spanning both majors, so the pin was lifted to^0.7. Verified: vetkeys 0.7 + auth resolves to a single@icp-sdk/core@5.4.0, fully deduped. The upgrade is a drop-in — the public typings are byte-identical between 0.5.0 and 0.7.0 (69 exported declarations either side, empty diff), so no code samples changed.Evals
Version literals are deliberately kept out of the expected behaviours except the
^5pin itself, which is the guidance. A case asserting a point-in-time snapshot of npm goes stale on every upstream release — the vetkeys case below was rewritten three times in a day before being made behavioural. Detecting that the pin itself has gone stale is the job of the guard proposed in #366, not of an eval.Cases added/changed — all run with baseline
wallet-integrationcase 5 — NEWvetkeyscase 6 — NEWicp-clicase 6 — "Frontend TypeScript bindings" (two behaviours updated)icp-clicase 15 — "Full-stack Motoko config artifacts" (two behaviours updated)The one failure is the pre-existing #367 issue described below, not a regression. Note this case's core behaviour ("if a version is referenced") is satisfied vacuously by the baseline, so its 4/6 is not a meaningful comparison — the discriminating behaviours here are the recipe and
mops generate candidones.icp-clicase 15 does not reach 6/6. The failing behaviour is pre-existing and not caused by this PR — the model writes the bindgen Vite plugin with acandid:key instead ofdidFile:.Verified rather than assumed, by stashing the whole change set and re-running the case against the original content:
Identical failure, identical reason. Root cause and suggested fix are filed as #367: the skill uses two different keys for "path to the
.didfile" —candid:(icp.yaml) appears 4× inSKILL.md,didFile:(vite.config.js) appears 0× there. Deliberately out of scope to keep this PR to version coherence.Related
@icp-sdk/signer(#39, approved but conflicting) and@icp-sdk/auth;@icp-sdk/vetkeysis no longer part of that chaindidFile/candidconfusion above{ agent }retraction; it touchesbinding-generation.mdtoo, so it should land after this one