Skip to content

fix(dsh): declare @deepseek-ai/dsh-tools as a host peer dependency - #393

Open
BrokenShine (ShineBreaker) wants to merge 1 commit into
microsoft:mainfrom
ShineBreaker:fix/dsh-host-peer-deps
Open

BrokenShine (ShineBreaker) wants to merge 1 commit into
microsoft:mainfrom
ShineBreaker:fix/dsh-host-peer-deps

Conversation

@ShineBreaker

Copy link
Copy Markdown

What

plugins/dsh declared @deepseek-ai/dsh-tools as a regular dependency. The dsh runtime already resolves @deepseek-ai/dsh-* packages from its own installation, so this installed a second copy under the plugin's node_modules: on a dsh 0.2.2-rc.1 profile, dsh-skillopt loaded dsh-tools 0.2.0-rc.2 while the host loaded 0.2.2-rc.1. dsh-market's package diagnostics flags exactly this as a shared-host-package-dependency warning.

Change

Moved @deepseek-ai/dsh-tools from dependencies to peerDependencies, and added it to devDependencies for local tests, following the dsh packaging rule that packages whose instances must be shared with the host belong under both peerDependencies and devDependencies:

The version range is unchanged (>=0.1.1-rc.2 <0.2.0 || >=0.2.0-rc.1 <0.3.0), preserving the existing dsh 0.1.x / 0.2.x support; as a peer range it now also participates in dsh's compatibility check (the package declares no engines.dsh).

Verification

  • node scripts/canary.mjs in plugins/dsh: all checks pass.
  • Installed this branch into a dsh 0.2.2-rc.1 profile: require.resolve('@deepseek-ai/dsh-tools') from the plugin resolves to the host's copy (0.2.2-rc.1), no private node_modules/@deepseek-ai copy remains, skillopt_status still works, and dsh-market's diagnostics return no findings.

The dsh runtime resolves @deepseek-ai/dsh-* packages from its own
installation, so declaring dsh-tools as a regular dependency installs a
second copy beside the host's. On a 0.2.2-rc.1 install this pulled in
dsh-tools 0.2.0-rc.2 under the plugin's node_modules, putting two
different tool-registry implementations in one process; dsh-market also
flags it as a shared-host-package-dependency warning.

Follow the upstream packaging rule: packages whose instances must be
shared with the host belong under peerDependencies (compatibility
checking and runtime resolution) and devDependencies (local tests).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The duplicate copy has a second cause besides dependencies, and moving the range to peerDependencies doesn't fix that one: the range doesn't admit the host version you tested with. Under npm's semver rules, a prerelease only satisfies a comparator that has a prerelease on the same major.minor.patch. >=0.2.0-rc.1 <0.3.0 therefore accepts 0.2.0-rc.2 and 0.2.2, but not 0.2.2-rc.1. I checked with the semver package bundled with npm (semver.satisfies(v, range)):

0.2.0-rc.2  true
0.2.2-rc.1  false   (true only with includePrerelease)
0.2.2       true
0.1.5-rc.1  false
maxSatisfying(['0.2.0-rc.2','0.2.2-rc.1']) -> 0.2.0-rc.2

That is the 0.2.0-rc.2 copy you saw. As a peer dependency, the same range makes a 0.2.2-rc.1 host an invalid peer for npm (npm ls reports it as invalid, and npm install may install its own copy or fail with ERESOLVE). Any compatibility check that uses semver.satisfies without includePrerelease will also refuse it. Your manual install worked because the dsh runtime resolves the host's copy no matter what.

If the plugin is meant to support the 0.2.x release candidates, each one needs its own comparator: for example >=0.1.1-rc.2 <0.2.0 || >=0.2.0-rc.1 <0.3.0 || >=0.2.2-rc.1 <0.3.0. Otherwise the README could say that only the listed rc versions and final releases are supported. The devDependencies entry has the same range, so local tests can't install 0.2.2-rc.1 either, which is the version you checked against.

Reviewed with help from Claude Code.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants