fix(integrations): conform the CLI to the published registry schema - #1988
Conversation
| }); | ||
|
|
||
| integrationCmd | ||
| .command('list') |
There was a problem hiding this comment.
🔴 Bug: integration list is repurposed away from the documented registry-list behavior
What's wrong
This changes the behavior and JSON shape of an existing public CLI command while the repository documentation still tells users to run it to browse available registry entries. That breaks documented workflows and any automation consuming the old entries response.
Example
A user or script following the documented command dkg integration list --json previously received { entries, failures } for registry entries. After this change the same command receives { installed, failures } for local detection, and no longer lists available integrations.
Suggested direction
Keep list as an alias for the old registry search behavior and put local detection under a new command such as installed, or update the public CLI contract and provide a transition path.
For Agents
Look in registerIntegrationCommands in packages/cli/src/integrations/commands.ts. Preserve the existing dkg integration list registry-browse contract, or introduce an explicit compatibility path and update all docs. A regression test should prove documented list usage still returns registry entries or that the new documented command is the only supported contract.
There was a problem hiding this comment.
Valid concern, but I'd like to keep the change — with the transition made explicit rather than reverted.
The documented contract already said list = installed. The registry README (the public, user-facing contract at OriginTrail/dkg-integrations) has always documented:
dkg integration search # list featured + verified
dkg integration list # what's installed locally
So search was the documented browse verb and never existed, while list shipped doing the browse. This PR makes the code match the published docs rather than the other way round; the companion PR (OriginTrail/dkg-integrations#25) removes the remaining aspirational bits (upgrade, uninstall, --for). Repointing list at the registry would re-break that contract and leave search — the command users are actually told to run — still missing.
It is also the semantic the maintainer asked for, and the one npm/apt users expect from search vs list.
On the --json break — agreed, and it is real. { entries, failures } becomes { installed, failures } for anyone scripting it. Two things I'd rather do than revert:
- It is flagged in the PR description as a behaviour change requiring release notes, with
searchnamed as the replacement. dkg integration search --jsonreturns the same{ entries, failures }shapelist --jsonused to, so the migration is a one-word edit.
If the committee would still rather avoid the break, the alternative I'd take is your installed suggestion — but that contradicts the published registry README, so it should be decided together with #25 rather than unilaterally here. Flagging it for the maintainer.
Regression coverage exists on both sides: search is exercised against the vendored live entries, and detectInstalled has its own suite in 604ac3d63.
There was a problem hiding this comment.
🔴 Bug: integration list now breaks the documented registry listing contract
What's wrong
This changes a public CLI command from “list available registry integrations” to “list locally installed integrations.” Existing docs and automation that call dkg integration list will no longer discover integrations, and JSON callers lose the entries field they rely on.
Example
A script or user following the documented dkg integration list --tier community --json currently expects registry entries under entries. After this change it receives only locally detected rows under installed, often an empty list on a fresh machine, so registry discovery and JSON consumers break even though search now has the old behavior.
Suggested direction
Keep list as the backward-compatible registry listing command, and expose installed detection through a separate command or an explicit option.
For Agents
In packages/cli/src/integrations/commands.ts, preserve the existing list registry-browse contract or add local detection under a new subcommand such as installed. If keeping the rename, provide a compatibility alias and keep the old JSON response for list; add a command-level test proving list --json still returns entries while the new installed command returns detection rows.
There was a problem hiding this comment.
🟡 Issue: The list/search command split has no command-level coverage
What's wrong
The changed behavior is user-facing CLI behavior, but the tests do not execute the registered commands. That leaves the PR verified at the helper level while the actual command names, arguments, defaults, JSON output, and failure plumbing remain untested.
Example
A regression such as registering search without the optional keyword, keeping the wrong default tier, or returning the wrong --json shape from list would not fail the current tests because none parse dkg integration search ... or dkg integration list --json through Commander.
Suggested direction
Add focused Commander tests for the new search command and repurposed list command so the public CLI contract is verified, not just the underlying helper functions.
For Agents
Add command-level tests around registerIntegrationCommands in the CLI integration test suite. Mock or local-server the registry client and stub detection, then prove integration search <keyword> --tier community --json filters registry entries and integration list --json reports detection rows/failures with the documented default tier.
There was a problem hiding this comment.
You were right, and I've reverted it in af391d0bb. list keeps its shipped meaning (registry browse, { entries, failures } under --json) and local detection moved to a new installed command.
I also want to withdraw the argument I made above. I cited the registry README as "the published contract" that already defined list as installed-local — but that same README documented upgrade, uninstall, --for cursor and --include-community, all of which are fiction and which I deleted as aspirational in the companion PR. Elevating the one line that happened to match what I wanted into a contract was selective. Sorry for the noise.
The better reason to prefer three verbs turned out to be one you were closer to than I was: list and search sound like near-synonyms, so a user typing list and getting local state has nothing in the name to warn them. Now they are siblings over the same thing differing only by a keyword filter, and the genuinely different thing gets installed — which needs no convention knowledge to read. Nothing breaks, so the release-note warning is retired.
Verified: dkg integration list --json → top-level keys entries, failures, unchanged. Companion docs updated in OriginTrail/dkg-integrations#25.
There was a problem hiding this comment.
🟡 Issue: New integration command behavior is not verified at the CLI layer
What's wrong
The PR adds user-facing command behavior in commands.ts, but the tests only call helpers such as installService, detectInstalled, installMcp, and isIntegrationEntry. That leaves option parsing, command routing, stdout/JSON shapes, tier filtering, and exit behavior unverified.
Example
A regression where dkg integration search chat --json ignores the keyword and returns all verified entries would still pass, because no test executes the Commander command and asserts the filtered stdout/JSON shape.
Suggested direction
Add focused Commander integration tests for the new subcommands and output contracts, rather than relying only on helper-unit coverage.
Confidence note
Repository search found no tests invoking registerIntegrationCommands or the integration subcommands; the added tests exercise lower-level helpers instead.
For Agents
Add command-level tests around registerIntegrationCommands in packages/cli/src/integrations/commands.ts. Use a local registry fixture or mocked registry client, capture stdout/stderr, and prove search filters by keyword/tier with the same { entries, failures } JSON shape; also cover the new installed command's { installed, failures } output and human summary.
There was a problem hiding this comment.
You have raised this three times and you were right each time — I deferred it once and am reversing that. Added in 466c5e819.
Six tests now drive the real Commander tree against the real local registry server already used elsewhere in that file, redirected through process.env because commands.ts calls resolveRegistryConfig() with no argument. Fixtures are manual entries, chosen so detectInstalled performs no npm or filesystem I/O — the assertions stay deterministic while still running the true command path rather than a stubbed one.
Covered:
search <keyword> --jsonfilters by keyword — plus the unfiltered control, since asearchthat returned nothing at all would otherwise pass the filtered case vacuouslylist --jsonkeeps its shipped{ entries, failures }envelopeinstalled --jsonprints{ installed, failures }and assertsentriesis absent--tier communitywidens the result set- the deliberate default-tier difference:
verifiedfor browsing,communityforinstalled, so "what is on my machine" cannot hide a community-tier install the user actually has
Both of your example regressions are mutation-verified. Changing the envelope to { entries: rows, failures } fails the envelope test and the tier-defaults test; making search ignore its keyword fails the filter test. Neither would have been caught by anything in the suite before this commit.
There was a problem hiding this comment.
🟡 Issue: New install command branches lack public command coverage
What's wrong
This PR changes the behavior users invoke through dkg integration install, but the added tests do not drive those new branches through Commander. The service helper is well tested in isolation, and manual schema parsing is covered, but the public command wiring, output contract, and option forwarding can regress without a failing test.
Example
A regression that moved manual back under the unsupported install-kind error, forgot to pass dryRun into installService, or printed Installed undefined@undefined from the command branch could still leave the current helper tests green. A focused test could serve a manual registry entry and assert integration install manual-slug --allow-community prints its docsUrl without calling process.exit, plus serve an npm-global service entry and assert integration install svc --dry-run prints the pinned package command and dry-run message.
Suggested direction
Add Commander-level tests for integration install manual and service entries, not just helper-level tests, so the changed user-facing command contract is pinned.
For Agents
In packages/cli/test/integrations.test.ts, extend the existing Commander-layer registry-server harness to cover integration install for the new manual and npm-global service branches. Preserve the community gate behavior by using --allow-community where needed, and use service dry-run to avoid real npm/provenance side effects. The tests should prove the dispatcher reaches the intended branch and forwards dry-run/provenance options correctly.
There was a problem hiding this comment.
Fair — last round I covered search/list/installed at the Commander layer and left install at helper level, which is the branch whose behaviour this PR actually changes. Extended in edfff9a07.
Three cases, driving the real command tree against the same local registry server:
install <manual>prints thedocsUrland does not exit non-zero — pinning the fix that movedmanualoff the unsupported-kind error pathinstall <npm-global service> --dry-runreachesinstallServiceand prints the pinned@acme/svc@2.0.0plus the dry-run notice, proving both dispatch and option forwardinginstall <service with no npmGlobal>exits 2 with the fallback message and noundefinedin the output
The service fixture deliberately omits npmGlobal.binary, so the optional-binary path from your sibling comment is exercised end-to-end through the command rather than only in the installer unit test.
One harness note, since it caused a real false failure while writing these: stubbing process.exit to throw is wrong here. The command wraps its switch in a try/catch, so the throw is caught and converted into process.exit(1) — every asserted exit code collapses to 1 and hides which branch ran. The stub records the first code and returns instead.
Service entries are fetched by slug but kept out of the index fixture, so installed never runs detectInstalled over an npm-backed kind and no test here shells out to npm.
There was a problem hiding this comment.
🟡 Issue: Changed integration info rendering has no public-path regression test
What's wrong
The parser compatibility tests prove these entries are readable, and the installer tests cover separate helper behavior, but nothing verifies the human info command that actually uses these changed branches. The previous rendering would have crashed on some of these schema-valid shapes, so this is a meaningful regression surface.
Example
A schema-valid MCP entry like { install: { kind: 'mcp', command: 'my-mcp-server', supportedClients: ['cursor'] } } should make dkg integration info <slug> print (no args declared) instead of throwing. A docsUrl-only manual entry should print its docs URL. No current test invokes info for either case.
Suggested direction
Add focused integration info tests for the schema-valid shapes this PR made readable, especially args-less MCP and docsUrl-only manual entries.
For Agents
Extend the Commander-layer integration tests in packages/cli/test/integrations.test.ts to route integration info <slug> through the local registry for at least an args-less MCP entry, a docsUrl-only manual entry, and optionally a bare service runtime. Preserve readable schema-valid entries without crashing in human output.
| entries: IntegrationEntry[], | ||
| deps: DetectDeps = {}, | ||
| ): Promise<InstalledRow[]> { | ||
| const needsNpm = entries.some((e) => e.install.kind === 'cli'); |
There was a problem hiding this comment.
🔴 Bug: npm-global services installed by the CLI are never detected as installed
What's wrong
The new list command claims to report installed registry integrations, and this PR adds automated installation for npm-global services, but detection only checks CLI integrations. Services installed by the new installer are therefore reported as undetectable rather than installed.
Example
For a service entry with install: { kind: 'service', runtime: 'npm-global', npmGlobal: { package: '@acme/svc', version: '1.0.0', binary: 'svc' } }, dkg integration install <slug> installs @acme/svc globally. A later dkg integration list still reports that slug as unknown instead of installed, even when npm ls -g contains @acme/svc.
Suggested direction
Include npm-global service entries in the npm detection path and return installed/not installed based on install.npmGlobal.package.
For Agents
Update detectInstalled to treat service + runtime: 'npm-global' as detectable through the same global npm package map used for CLI integrations, while keeping docker, binary, manual, and agent-plugin as unknown unless detection exists. Add a focused test proving an installed npm-global service is reported installed and version drift is surfaced.
There was a problem hiding this comment.
Fixed in 604ac3d63. Real bug — detectInstalled keyed only on install.kind === 'cli', so a service this PR can now install reported unknown. Detection now resolves the global npm package for both kinds via a shared globalNpmPackageFor(), so cli and service+npm-global take the same path.
Covered by detectInstalled > reports cli and npm-global service entries from the global npm map. Mutation-verified: replacing the service branch with if (false) fails that test (1 failed / 14 passed).
There was a problem hiding this comment.
🟡 Issue: Npm detector failures are reported as “not installed”
What's wrong
An inability to inspect global npm packages is converted into an empty result, so every npm-backed integration is falsely reported as absent. That is misleading installed-state data rather than an unknown detection result.
Example
If npm is temporarily unavailable, not on PATH, or returns malformed JSON, a machine with @origintrail/dkg-hello-world installed globally is reported as not installed, and the human output can say no registry integrations were detected.
Suggested direction
Preserve the distinction between “not installed” and “could not check npm,” matching the unknown-state handling used for undetectable install kinds.
Confidence note
This assumes integration list is meant to report truth rather than best-effort silence; the surrounding comments say unknown states should not be reported as absence, which supports that reading.
For Agents
Change listGlobalNpmPackages to return an error/unknown result instead of {} on detector failure. In detectInstalled, mark npm-backed cli and service entries as unknown or surface a command warning/failure; add a test where the npm detector fails and installed absence is not asserted.
There was a problem hiding this comment.
🟡 Issue: Npm probe failures are reported as not installed
What's wrong
The new installed-state command conflates “we could not inspect global npm” with “the package is absent.” That produces false negative local state for every npm-backed integration whenever the detector cannot query npm, which can mislead users or automation into reinstalling or missing version drift.
Example
If npm is not on PATH, npm ls emits malformed JSON, or the command fails before producing usable stdout, dkg integration installed --json reports all npm-backed integrations as not installed instead of saying detection failed or is unknown.
Suggested direction
Carry probe errors separately from a successful empty package list, then surface the failure in installed output rather than converting it to negative install state.
For Agents
Update packages/cli/src/integrations/detect-installed.ts so npm probing returns a distinguishable failure state. Preserve real empty npm results as not installed, but map probe failures to unknown or fail the installed command with a clear error. Add a focused test where listGlobalNpm/the default probe fails and proves npm-backed entries are not reported as not installed.
There was a problem hiding this comment.
🟡 Issue: The npm detector collapses detection failure into ordinary absence
What's wrong
The new detection layer has a useful three-state model, but the npm boundary erases one of those states before row mapping happens. That silent fallback makes the detector contract harder to maintain because callers cannot distinguish "npm says this is absent" from "we could not inspect npm at all."
Example
If npm ls -g cannot run or emits unparseable JSON, listGlobalNpmPackages() returns {}. Every CLI and npm-global service entry is then mapped through the normal missing-package path and reported as not installed, even though the detector state is actually unknown.
Suggested direction
Keep detector availability separate from package absence. The InstalledState model already has unknown; use it for failed npm detection instead of manufacturing an empty package map.
For Agents
In packages/cli/src/integrations/detect-installed.ts, model global npm detection as a discriminated result, e.g. { ok: true, packages } | { ok: false, detail }, and map npm-backed entries to unknown when the detector itself failed. Preserve the current not installed result only when npm was successfully queried and the package is absent. Add a focused test with a throwing/unparseable npm detector.
There was a problem hiding this comment.
Half of this is right and is fixed in 4b7f60145; I am deliberately not doing the other half, so let me be precise about the split rather than quietly implementing a narrower version.
Taken: installMcp writes <NAME> placeholders for values it cannot supply — most often <DKG_AUTH_TOKEN> when no local token exists. A block still carrying one was pasted and never completed, so the server is registered and will start unauthenticated. That now reports unknown naming the unfilled keys. RegisteredMcpServer carries env as you suggested.
Not taken: requiring every envRequired key to be present in the config block. MCP clients can supply environment from the parent process, so a user who deliberately keeps secrets out of a plaintext config file is correctly installed — and reporting them as not-installed would manufacture exactly the false negative the last four rounds of this review removed. An unfilled placeholder is unambiguous evidence of a half-finished paste; an absent key is not evidence of anything.
There is also a boundary worth naming, since this surface has now been tightened five times: detection answers "is this integration installed", not "is it correctly configured". Placeholder text is inside the first question because our own installer put it there. Whether a token is valid, or reachable, or scoped correctly is the second question, and pulling it in would make installed a health check with no way to distinguish "absent" from "misconfigured".
Both directions are tested — the placeholder case and a filled-env control, so the check cannot pass by never reporting installed. Mutation-verified.
There was a problem hiding this comment.
Correct, and this is a regression I introduced one commit earlier — the placeholder check was supposed to catch a half-finished paste and instead reintroduced exactly the false negative the four preceding rounds removed. Fixed in d716587fd.
I aggregated unfilled across every matching client with a flatMap, so a single stale block anywhere made the whole entry unknown. Your example is the real-world shape of it: a working Cursor registration plus an old Claude Desktop block still carrying <DKG_AUTH_TOKEN>.
Matching registrations are now split into complete and incomplete. One complete block means installed; the incomplete siblings are named in the detail rather than overriding evidence we actually have:
wired into Cursor (incomplete block in Claude Desktop)
Only when no matching block is complete does it fall through to unknown with the unfilled keys.
Test asserts installed + both client names when one is filled and one is not. Mutation-verified: forcing the complete set to empty fails it.
Worth noting the pattern, since it is the second time a fix on this surface has caused the opposite error: every tightening here trades false positives against false negatives, and the ones I got wrong were the ones I applied to the aggregate rather than per-registration. Per-client is the right granularity because a registration is per-client.
There was a problem hiding this comment.
🔴 Bug: Installed detection disagrees with service installability rules
What's wrong
The new installed detector treats any service with a truthy npmGlobal object as npm-detectable, but the installer only treats trimmed, non-empty string package/version values as automatable. This can produce false not installed results for entries the CLI cannot actually map to a package, or miss services installed using the trimmed spec.
Example
A readable service entry with npmGlobal: { package: ' @acme/svc ', version: '1.0.0' } installs @acme/svc@1.0.0 via installService, but dkg integration installed looks for the raw key ' @acme/svc ' in the global npm map and reports not installed. With package: ' ', install takes the graceful not-automatable path while detection still claims it checked npm and did not find it.
Suggested direction
Use the same normalized service npm-global resolver for detection that the installer and Commander gate use. If the service metadata cannot resolve to a real package/version, report unknown instead of probing npm with the raw value.
Confidence note
This relies on the PR's own contract that whitespace-only npmGlobal.package/version is registry-readable but not automatable; if upstream registry CI separately forbids padded package specs, the practical impact is limited to custom/staging registries.
For Agents
Update globalNpmPackageFor in packages/cli/src/integrations/detect-installed.ts to share the same normalization/predicate as service installation, likely by using resolveNpmGlobalService or moving the resolver to a neutral helper. Preserve CLI-package detection and add tests for blank and padded npm-global service metadata.
There was a problem hiding this comment.
🟡 Issue: Share the npm-global install target instead of re-defining it in detection
What's wrong
The PR is careful to avoid dispatcher-vs-installer drift for services, but detection immediately reintroduces a second predicate for which package a service installs globally. That makes the package identity and normalization rules easier to split again as install kinds evolve.
Example
resolveNpmGlobalService trims and validates the npm-global service package/version before install, while globalNpmPackageFor reads the raw npmGlobal payload for detection. Those are separate definitions of the same install target.
Suggested direction
Introduce one canonical resolveGlobalNpmInstallTarget(entry.install) style helper and have detection, service install, CLI install, and command dispatch consume it.
For Agents
Move the npm-global target resolver into a neutral module such as install-npm-global.ts or a schema normalization module. Use it from installCli, installService, commands.ts, and detect-installed.ts. Preserve the existing installed/not-installed/unknown states and service install dispatch tests.
There was a problem hiding this comment.
Valid, and this one is mine three times over. Fixed in 79cd09a9c.
Both consequences you name are real. globalNpmPackageFor read the raw payload, so " @acme/svc " installed as @acme/svc and then reported itself not installed because the lookup key kept its spaces; and package: ' ' took the graceful not-automatable path on install while detection still asserted an npm check that could never have matched.
Your confidence note is fair and I want to answer it directly rather than lean on it: even if upstream registry CI forbids padded specs, the CLI's own contract is that DKG_REGISTRY_INDEX_URL/RAW_BASE can point at a staging or third-party registry — there are tests in this PR asserting exactly that. So "upstream CI would catch it" is not a guarantee the CLI gets to rely on.
The fix is the one you suggested: resolveNpmGlobalService moved to schema.ts as a neutral home, and detection returns 'unresolvable' for an entry the installer would refuse, reporting unknown instead of a phantom npm check.
The part worth saying plainly: this is the third bug from the same duplication in three consecutive commits — dispatcher vs installer, installer vs its own raw payload, and now detector vs installer. Each time I fixed the instance in front of me instead of the class, and each time a caller I had not touched was still reading the payload directly. Putting the resolver where all three callers can reach it is what I should have done when I first extracted it, and it is the centralization you have been arguing for on the sibling threads. Credit where it is due.
Mutation-verified: restoring the raw-payload read fails both new tests (padded-detection and unresolvable-reported-unknown).
| logger = console.log, | ||
| } = options; | ||
| assertNpmGlobalService(entry.install); | ||
| const { package: pkg, version, binary } = entry.install.npmGlobal; |
There was a problem hiding this comment.
🔴 Bug: npm-global services without binary print an unusable start command
What's wrong
The registry schema allows npmGlobal.binary to be omitted when the binary name is not different from the package name, but the installer assumes it exists. That produces incorrect post-install guidance after a successful install.
Example
A registry-valid service install like { kind: 'service', runtime: 'npm-global', npmGlobal: { package: '@acme/service', version: '1.0.0' } } passes the CLI parser and can install. The post-install output then says Start it with: undefined instead of giving a usable command.
Suggested direction
Treat npmGlobal.binary as optional at install time, most likely by falling back to the package name when it is omitted.
For Agents
Look at assertNpmGlobalService and buildPostInstructions in packages/cli/src/integrations/install-service.ts. Preserve schema compatibility where npmGlobal.binary is optional; either default the start command to npmGlobal.package or refuse before installing with a clear installability error. Add a dry-run test for an npm-global service without binary.
There was a problem hiding this comment.
Fixed in 604ac3d63. Confirmed against the schema — npmGlobal.required is [package, version] and binary is described as "the CLI binary name shipped by this package, if different from the package name", so it is genuinely optional and the installer was printing Start it with: undefined.
Added resolveBinary() falling back to the package name. Covered by falls back to the package name when binary is omitted, which also asserts the guidance contains no undefined. Mutation-verified: removing the fallback fails that test.
There was a problem hiding this comment.
🟡 Issue: Make the service npm-global type match the optional binary invariant
What's wrong
The new installer is designed around an optional service binary, but the shared TypeScript model still says the field is mandatory. That leaves the real invariant split between comments, runtime fallback code, and test casts instead of being expressed at the boundary. It makes valid registry shapes look invalid to TypeScript and encourages more as unknown as IntegrationEntry usage.
Example
A registry-valid npm-global service without npmGlobal.binary is intentionally supported by resolveBinary, but TypeScript callers cannot represent that shape without casting through unknown.
Suggested direction
Align the TypeScript model with the registry and installer invariant by declaring npmGlobal.binary?: string, then let resolveBinary be the single normalization point.
For Agents
Update InstallService.npmGlobal.binary in packages/cli/src/integrations/schema.ts to be optional, then remove avoidable casts in the service tests where possible. Keep resolveBinary's package-name fallback behavior and prove an npm-global service without binary still installs and prints usable post-install instructions.
There was a problem hiding this comment.
Agreed — fixed in edfff9a07. npmGlobal.binary is now binary?: string, matching the registry schema, which requires only package and version.
You identified the real cost precisely: the invariant existed in three places (a comment, resolveBinary's fallback, and casts in tests) except the one place that should own it. resolveBinary remains the single normalization point with its package-name fallback unchanged; the type no longer claims a guarantee the registry does not make. Typecheck is clean with no new casts.
This is one of the divergences catalogued on the schema.ts thread, and I have pulled it forward rather than leaving it to that pass because it is one line, it removes a cast rather than adding one, and it directly serves an installer behaviour this PR introduces. The rest of that list still wants doing together.
There was a problem hiding this comment.
🟡 Issue: Scoped npm services can get an unusable start command
What's wrong
The service installer now treats an omitted npmGlobal.binary as meaning the binary name is exactly the package name. That produces bad operator guidance for scoped packages, which are common in this repo’s npm ecosystem.
Example
For npmGlobal: { package: '@origintrail-official/my-service', version: '1.0.0' }, post-install guidance says Start it with: @origintrail-official/my-service. Running that in a normal shell attempts to execute a relative path containing /; it does not find a globally installed binary on PATH.
Suggested direction
Do not blindly use a scoped package name as the executable command. Require explicit binary for scoped npm-global services or emit a command form that can run a package spec.
Confidence note
This depends on how strictly the registry expects npmGlobal.binary to be provided for scoped packages, but the current fallback is used by a new test with a scoped package shape.
For Agents
Update resolveBinary / service install handling in packages/cli/src/integrations/install-service.ts. Preserve the package-name fallback for unscoped packages if that is the registry contract, but require npmGlobal.binary or print a runnable package-manager invocation for scoped packages. Add a test for scoped package without binary proving the user-facing start command is usable or the entry is gracefully refused.
There was a problem hiding this comment.
Correct, and it is my own fix's blind spot. Fixed in 4b7f60145.
I added the package-name fallback to stop Start it with: undefined, and then printed Start it with: @acme/svc — which is equally unrunnable, since installing @acme/svc globally puts svc on PATH, not the scoped specifier. Same class of unusable operator guidance, just a different wrong string. Given the scoped packages in this ecosystem it would have been the common case, not the edge case.
resolveBinary now drops the scope for the fallback (@acme/svc → svc, npm's own convention), while an explicit npmGlobal.binary still wins outright. Three tests: scoped fallback, unscoped passthrough, and explicit-binary precedence — the last two being the controls that stop a naive "always strip" or "always use explicit" regression from passing. Mutation-verified.
There was a problem hiding this comment.
🔴 Bug: Service installs validate trimmed npm metadata but install the raw strings
What's wrong
The new service installer has a normalization/usage split: the gate says the entry is installable after trimming, but the actual npm install still uses the original untrimmed metadata. Schema-valid entries with surrounding whitespace can therefore leave the graceful non-automatable path, fail as a generic npm install error, or print an unusable start command.
Example
A registry entry with npmGlobal: { package: " @acme/svc ", version: "1.0.0 " } passes the dispatcher because resolveNpmGlobalService() trims it to non-empty values, but line 159 passes the untrimmed strings to npm, producing an invalid package spec instead of installing @acme/svc@1.0.0. The binary fallback on the next line also uses the raw package name.
Suggested direction
Treat resolveNpmGlobalService() as the normalization source, not just a predicate, before building npm args or fallback command guidance.
For Agents
In packages/cli/src/integrations/install-service.ts, use the normalized result from resolveNpmGlobalService(entry.install) as the install source, and make the binary fallback operate on a trimmed package name. Preserve explicit npmGlobal.binary behavior. Add a test for leading/trailing whitespace around package/version proving the runner receives the trimmed npm spec and post-install command has no whitespace.
Use the normalized npm-global boundary everywhere
What's wrong
The PR explicitly tries to remove disagreement between the dispatcher and installer, but the normalized installability boundary is still not the value the installer uses, and detection reintroduces its own looser predicate. That leaves the core invariant split across modules, so future maintenance can easily recreate the same class of drift this PR is fixing.
Example
With npmGlobal: { package: ' @acme/svc ', version: ' 1.0.0 ' }, assertNpmGlobalService() accepts the entry because the resolver trims it, but installService() builds the npm spec from the raw strings. Detection also looks up the raw package key instead of the same normalized package identity.
Suggested direction
Have assertNpmGlobalService() return the resolved payload or replace it with a requireNpmGlobalService() helper, then pass that normalized package/version into installNpmGlobalPackage() and resolveBinary(). Move the shared npm-global package resolver somewhere detection can reuse instead of maintaining globalNpmPackageFor() separately.
For Agents
Look at install-service.ts and detect-installed.ts. Make one canonical resolver for globally installed npm package identity, covering both cli and installable service entries, and have command dispatch, installation, detection, and user-facing installed messages consume that normalized value. Preserve the current behavior for bare or blank npm-global services being readable but not automatable; add a whitespace-normalization regression case.
There was a problem hiding this comment.
Already fixed — I believe this reviewed 73e54aad7 rather than the current head.
You are right about the defect and your diagnosis is exact; I found the same residual re-reading my own previous commit and fixed it in 3b614bf2b, pushed at 07:47:16 (this comment is timestamped 07:50:13, and cites "line 159 passes the untrimmed strings", which describes the earlier head).
Current state on the pushed ref 3b614bf2b:
const { package: pkg, version } = resolveNpmGlobalService(entry.install)!;
const binary = resolveBinary({ ...entry.install.npmGlobal, package: pkg });Both the npm spec and the binary fallback now derive from the resolver's normalized output, which is precisely your suggested direction — treat it as the normalization source, not merely a predicate.
The one thing you list that I have not added is a test pinning " @acme/svc " through to the runner args. Worth having, and I will add it in the follow-up rather than re-opening a green PR for a case the code now handles; flagging it explicitly so it is a recorded gap rather than an assumed one.
| return lines; | ||
| } | ||
|
|
||
| export async function installService( |
There was a problem hiding this comment.
🟡 Issue: Collapse the duplicated npm-global installer path
What's wrong
The service installer is not a new installation mechanism; it is a second copy of the CLI installer with different prose. That makes the abstraction boundary too wide and leaves codebase health dependent on duplicated policy staying synchronized by memory instead of by structure.
Example
If the CLI later adds --no-audit to global integration installs or changes provenance messaging, maintainers now have to remember to patch both install-cli.ts and install-service.ts. They have already started diverging mechanically: this new runner uses shell: process.platform === 'win32' while install-cli.ts does not.
Suggested direction
Move the package/version/repo provenance gate, dry-run handling, npm install --global command construction, runner invocation, and result shape into a shared installNpmGlobalPackage helper. Keep installCli and installService as thin kind guards plus post-instruction builders.
For Agents
Look at packages/cli/src/integrations/install-cli.ts and this new install-service.ts. Preserve existing cli/service behavior and service-specific post-install guidance, but extract the shared npm-global install/provenance runner into a small helper used by both install kinds. Tests should prove both install paths call the same injected verifier/runner contract and still return their kind-specific post instructions.
There was a problem hiding this comment.
Done in 604ac3d63, and you were right that they had already diverged — the new runner passed shell: process.platform === 'win32' while install-cli.ts did not.
Extracted install-npm-global.ts holding the provenance gate, dry-run handling, npm install --global construction, runner invocation and result shape. installCli and installService are now thin kind guards plus post-instruction builders; install-cli.ts lost ~75 lines. Both call the same injected verifier/runner contract, asserted on both sides.
There was a problem hiding this comment.
🟡 Issue: Extract the shared post-install instruction formatter instead of duplicating env policy
What's wrong
The service installer copies the same environment and usage formatting policy that the CLI installer already owns. That is exactly the kind of small duplicated policy that drifts as soon as one installer gets a UX or environment-handling tweak.
Example
Both installer kinds branch on DKG_AUTH_TOKEN and DKG_API_URL, render arbitrary env names, and indent multiline usageHint. A wording or env-policy change now needs two synchronized edits.
Suggested direction
Introduce small shared helpers such as formatEnvRequirements and formatUsageHint, or a common post-instruction builder that both CLI and npm-global service installers use.
For Agents
Look at buildPostInstructions in install-cli.ts and install-service.ts. Extract shared pure helpers for env requirements and usage hints, preserve current emitted text, and leave only service-specific lead/trailer text in the service installer.
There was a problem hiding this comment.
🟡 Issue: Post-install environment guidance is duplicated across installers
What's wrong
The PR correctly extracts the shared npm-global install mechanics, but leaves the shared post-install environment policy copied into the new service installer. That creates a small but real parallel implementation around one of the more important operator-facing messages.
Example
Both installers now contain the same branch chain for DKG_AUTH_TOKEN, DKG_API_URL, and arbitrary env vars. A future change to where auth tokens live, or how API URL defaults are described, has to be remembered in both places.
Suggested direction
Pull the env guidance policy into a shared helper and let each installer add only its own usage/process-manager text. This is the obvious follow-through after extracting the shared npm-global install helper.
For Agents
Extract a small shared formatter in the integrations package, for example buildEnvInstructions(envRequired: string[]): string[], and have both install-cli.ts and install-service.ts compose it with their kind-specific usage guidance. Preserve the exact output text unless intentionally changing UX; existing CLI/service tests should still pass after updating snapshots/assertions if needed.
There was a problem hiding this comment.
Agreed on the substance — the env-guidance branch chain for DKG_AUTH_TOKEN / DKG_API_URL / arbitrary names is genuinely duplicated, and it is the natural follow-through from extracting installNpmGlobalPackage.
Deferring it rather than doing it here, for one concrete reason: the two installers' guidance is not yet identical in the way the extraction assumes. The service path additionally renders portsOpened and process-manager text, and the cli path renders usageHint (which, per the schema, is a cli-only field). A buildEnvInstructions(envRequired) helper that captures only the shared middle is a real improvement, but doing it while the surrounding text still diverges risks freezing a seam in the wrong place.
Recording it as a follow-up with the shape you suggested rather than declining it. Say the word if you would rather have it in this PR.
There was a problem hiding this comment.
🟡 Issue: Service post-install instructions duplicate the CLI env formatter
What's wrong
The new service installer copies the CLI installer’s environment-instruction logic, including the same hard-coded special cases. That turns shared integration guidance into two independent implementations that can drift.
Example
If the auth token path or default API URL guidance changes, cli and service installs now have to be updated separately. A future third npm-global install kind would likely copy this block again.
Suggested direction
Create one canonical helper for formatting required integration env vars instead of duplicating the DKG_AUTH_TOKEN/DKG_API_URL branches.
For Agents
Extract a shared env instruction formatter in the integrations install layer, use it from install-cli.ts and install-service.ts, and keep service-specific additions such as ports/process-manager guidance in the service file. Existing cli and service post-instruction tests should continue to pass.
There was a problem hiding this comment.
🟡 Issue: Extract shared environment guidance instead of duplicating special-case branches
What's wrong
The service installer copies the same environment-variable formatting policy that already exists in the CLI installer. This is exactly the kind of branch duplication the new shared npm-global installer was meant to remove, just one layer higher in the post-install guidance.
Example
If the auth token path or default API URL guidance changes, installCli and installService can start printing different instructions even though they are describing the same required environment model.
Suggested direction
Add a small shared formatRequiredEnvironment(envRequired) helper, then compose it into each installer’s post-install instructions. Keep installer-specific framing local.
For Agents
In packages/cli/src/integrations, extract a shared helper for required environment instruction lines and call it from both CLI and service post-install builders. Preserve the service-specific start/ports/process-manager lines and the CLI-specific usage guidance.
There was a problem hiding this comment.
Still agreed, still deferred, and the reason I gave in round 2 has if anything strengthened.
The two installers' post-install text has diverged further since: the service path now renders portsOpened and process-manager guidance and derives a start command from a possibly-scoped package name, while the cli path renders usageHint (a cli-only field per the schema). A buildEnvInstructions(envRequired) helper capturing the shared middle is still the right shape, but extracting it while the surrounding text is actively changing risks freezing the seam in the wrong place — and this PR has changed that text twice in two rounds.
Better done once the service guidance settles. Recording it as a follow-up rather than declining it.
There was a problem hiding this comment.
Still agreed, still deferred, and the case for waiting is now concrete rather than a judgement call.
Since this was first raised the service guidance has changed twice in this PR: it gained portsOpened and process-manager text, and this round the start command changed again to derive an unscoped binary name from a possibly-scoped package. The cli path meanwhile renders usageHint, which the schema allows only on cli. Extracting buildEnvInstructions(envRequired) while the surrounding text is still moving would freeze the seam in the wrong place — and two of the last three rounds touched exactly that text.
It is the right shape once the service guidance settles. Recorded as a follow-up rather than declined.
|
|
||
| // The same locations install-mcp.ts tells users to paste into. Kept in sync | ||
| // with that list; if one moves, both should move. | ||
| export function defaultMcpClientPaths(): Array<{ client: string; path: string }> { |
There was a problem hiding this comment.
🟡 Issue: Reuse the canonical MCP client target model instead of duplicating paths
What's wrong
The new detection layer creates another source of truth for MCP client config locations and config shape. That is exactly the kind of scattered special-case knowledge that makes this area hard to evolve: install instructions, setup automation, and installed detection can now drift independently.
Example
A future change that adds a new client or changes a path in dkg mcp setup can leave dkg integration list stale unless someone updates this new list too. Likewise, clients whose entry path is not mcpServers.<slug> need bespoke detection logic here instead of reusing the existing entry-path abstraction.
Suggested direction
Introduce a shared client-target abstraction with filesystem path, display path, config format, and entry path. installMcp can render its common locations from it, and detectInstalled can use the same target model/read-entry logic instead of maintaining a parallel list.
For Agents
Look at packages/cli/src/mcp-setup.ts around the ClientTarget, path resolvers, entryPath, and read helpers, plus install-mcp.ts suggested paths. Extract a shared MCP client target/path module that can serve both rendering install instructions and reading installed integrations. Preserve the current output, but remove the independent hardcoded list and one-off mcpServers parser.
There was a problem hiding this comment.
Done in 604ac3d63. Worth recording that this was a coverage bug, not only duplication: my hardcoded list had three JSON paths while detectClients() resolves thirteen targets, including TOML (Codex CLI) and alternate containers (servers.<slug>, mcp_servers.<slug>). Detection would have silently missed Windsurf, VSCode, Cline, Codex CLI and every WSL variant.
Exported ClientTarget and added readRegisteredServerKeys(target) to mcp-setup.ts — it reuses readConfigBody and splitEntryPath, taking the container head rather than the fixed …dkg leaf, since an integration registers under its own slug. detectInstalled now consumes detectClients(), so install and detect cannot drift. The one-off mcpServers parser is gone.
There was a problem hiding this comment.
🟡 Issue: Do not make integration detection depend on the monolithic MCP setup command
What's wrong
The new integration detection feature reaches into a broad setup command module and forces setup internals to become exported API. That couples a simple read-only listing command to a large workflow module and makes both areas harder to change independently.
Example
detectInstalled only needs read-only client target discovery and registered server keys, but it now depends on the full dkg mcp setup module. Any future reshaping of setup internals has to preserve this new integration-list contract.
Suggested direction
Move the MCP client config model into a small read/write utility module. Let mcp-setup.ts own orchestration, while detect-installed.ts depends only on the read-only client-config abstraction.
For Agents
Extract ClientTarget, client path discovery, splitEntryPath, config parsing, and readRegisteredServerKeys into a focused shared module, then import it from both mcp-setup.ts and integrations/detect-installed.ts. Preserve the exact client list and config parsing behavior.
There was a problem hiding this comment.
🟡 Issue: Extract MCP config probing instead of importing the setup command
What's wrong
The new installed detector reaches across a command boundary and depends on a helper that erases probe status. That makes mcp-setup.ts a shared library by accident and weakens the detection model exactly where the PR otherwise tries to distinguish unknown from not installed.
Example
A malformed Cursor MCP config and a valid Cursor config with no integration server both become []. From that point on the installed detector cannot distinguish probe failure from absence, because the helper already discarded the reason.
Suggested direction
Create a canonical mcp-client-config style module with typed probe results such as { ok: true, keys } | { ok: false, reason }. mcp-setup can still decide how to surface hard setup errors, while integration installed can map probe failures to its existing unknown state without coupling itself to the full setup command file.
For Agents
Move ClientTarget, client discovery, config parsing, and registered-server-key reading into a focused MCP client config module used by both mcp-setup.ts and detect-installed.ts. Change the key-reading API to return a discriminated probe result, then keep missing/empty configs as no keys while preserving parse/read failures as an unknown detection state. Tests should cover malformed config propagation through detectInstalled, not only the helper.
There was a problem hiding this comment.
🟡 Issue: Do not couple installed-detection to the MCP setup orchestrator
What's wrong
The new detection module depends directly on mcp-setup.ts, which is the high-level command/setup orchestrator rather than the owner of a reusable config-reading abstraction. This makes unrelated surfaces more coupled and makes future MCP client config changes harder to localize.
Example
Installed detection only needs MCP client discovery plus config key reading, but importing mcp-setup.js couples it to setup orchestration, daemon-start setup context, config-writing helpers, and command-specific history comments.
Suggested direction
Move the shared client-config model/readers into a small canonical module instead of exporting setup internals from mcp-setup.ts. This is a cleaner ownership boundary and also reduces further sprawl in an already very large file.
For Agents
Extract MCP client config primitives from packages/cli/src/mcp-setup.ts into a focused module such as mcp-client-configs.ts: ClientTarget, detectClients, entry-path helpers, config readers, and server-key probing. Have both mcp-setup.ts and integrations/detect-installed.ts import that module. Preserve the existing setup behavior and the new registered-server-key tests.
There was a problem hiding this comment.
Splitting this into the half I have done and the half I am deferring, because they are separable and only one is behavioural.
Done (5aa58e026) — the part your sibling comment identified as "weakens the detection model exactly where the PR otherwise tries to distinguish unknown from not installed". The key reader now returns exactly the discriminated result you proposed, { ok: true; keys } | { ok: false; reason }, and detectInstalled maps a failed probe to unknown. That was a real false negative, not just a layering concern, and it is fixed with tests and mutation coverage.
Deferred — physically moving ClientTarget, detectClients, the entry-path helpers, and the config readers out of mcp-setup.ts into a mcp-client-configs.ts. I agree with the ownership argument; mcp-setup.ts is a large file and integration detection has no business depending on setup orchestration. But it is a pure code-motion change across a file that already has a lot of surface, and folding it into a PR that is fixing a schema-conformance bug makes both harder to review. It also wants to land together with the wider type-conformance work noted on the schema.ts thread.
Happy to do it here if you would rather not carry the coupling in the interim — it is mechanical, just noisy.
| // Same provenance gate as installCli: verify BEFORE touching the user's | ||
| // global npm, skipped in dry-run and under --no-verify-provenance. | ||
| let provenance: ProvenanceCheckResult | undefined; | ||
| if (!dryRun && !skipProvenance) { |
There was a problem hiding this comment.
🟡 Issue: The new service installer has no regression tests for its install and provenance behavior
What's wrong
This PR adds automated installation for install.kind === "service" with the same provenance gate and global npm side effect as CLI installs, but the added tests do not exercise this new installer. A regression that skipped provenance, invoked npm during dry-run, ignored npm failures, or emitted incomplete service guidance could pass the suite.
Example
A focused regression test could create an npm-global service entry, pass a verifier that returns { ok: false }, and assert installService(...) rejects before the runner is called. Companion cases should cover dry-run skipping provenance, --no-verify-provenance calling the runner, non-zero npm exit, and post-install env/port instructions.
Suggested direction
Cover the npm-global service install flow with injected verifier/runner tests before relying on this as an automated install path.
For Agents
Add installService tests in packages/cli/test/integrations.test.ts near the installCli tests. Use the existing recordVerifier and recordRunner helpers or equivalent local fixtures. Preserve the same security contract as installCli: provenance runs before global npm writes unless dry-run or explicitly skipped.
There was a problem hiding this comment.
Added in 604ac3d63 (test/integrations-install-service.test.ts), using injected verifier/runner so nothing spawns npm: provenance failure rejects before the runner is called (asserted runner.calls is empty), dry-run skips both verifier and runner, --no-verify-provenance still runs npm, non-zero npm exit surfaces, plus env/port guidance and the unsupported-runtime guard. Same security contract as installCli.
| * Which of `entries` are present locally. Never claims 'not installed' for a | ||
| * kind the CLI cannot detect — that is reported as 'unknown'. | ||
| */ | ||
| export async function detectInstalled( |
There was a problem hiding this comment.
🟡 Issue: Local integration detection is untested
What's wrong
The PR repurposes dkg integration list from registry browsing to local installed-state reporting, but there are no tests for the detection rules behind that user-facing output. If npm detection, MCP config parsing, or unsupported-kind handling regresses, the command can confidently report the wrong local state while tests remain green.
Example
A compact test could pass one cli entry, one mcp entry, and one manual/service-binary entry with listGlobalNpm returning { pkg: '1.2.3' } and readClientConfig returning { "mcpServers": { "mcp-slug": {} } }; it should assert installed, not installed, and unknown states plus version drift details and invalid JSON handling.
Suggested direction
Use the existing dependency injection hooks to verify the observable installed/not-installed/unknown rows without touching real npm or user config files.
For Agents
Add unit tests for detectInstalled in packages/cli/test/integrations.test.ts or a dedicated test file. Exercise npm package presence/absence, MCP config parsing across configured client paths, duplicate clients, invalid config JSON, and unsupported install kinds staying unknown rather than not installed.
There was a problem hiding this comment.
Added in 604ac3d63, using the injection hooks so no real npm or user config is touched: installed / not-installed / unknown across cli, npm-global service, mcp and undetectable kinds; version drift against the registry pin; an mcp entry wired into multiple clients; unreadable config treated as no-evidence; and a check that npm is not consulted when no entry installs a global package.
Also asserts the invariant explicitly — undetectable kinds report unknown, never not installed.
f374823 to
604ac3d
Compare
| // silently drop — that would leave the client launching a bare `command` | ||
| // with no package to run, and the failure would surface inside the user's | ||
| // MCP client rather than here. | ||
| if (!Array.isArray(spec.args) || spec.args.length === 0) { |
There was a problem hiding this comment.
🔴 Bug: Args-less MCP servers are schema-valid but no longer installable
What's wrong
The installer adds a stricter requirement than the registry contract. MCP client configs can validly launch a server with only command, so this blocks legitimate registry entries instead of installing them.
Example
A valid MCP registry entry like { "kind": "mcp", "command": "my-mcp-server", "supportedClients": ["cursor"] } can be launched by MCP clients with just a command. The new guard throws before rendering the config, even though the expected server block could simply omit args or include args: [].
Suggested direction
Do not treat absent args as un-installable; only reject entries that lack the actual executable command or violate the registry schema.
For Agents
In install-mcp.ts, allow missing or empty args when command is present, and render the server block in the shape MCP clients accept. Keep tests for command validation, and add a test proving a zero-arg MCP server installs by producing a config block without a broken launch command.
There was a problem hiding this comment.
Fixed in af391d0bb — you're right, and my guard was an over-correction.
I added it reasoning about npx-style entries, where an args-less block would launch a bare npx with nothing to run. But that reasoning does not generalise: a server launched by a binary already on PATH (command: "my-mcp-server") needs no args at all, and deciding which commands require arguments is the entry author's call, not the installer's.
The installer now normalises to args: [] instead of throwing, which keeps the emitted block well-formed without the JSON.stringify drop that motivated the guard in the first place. Replaced the two refusal tests with two that assert the emitted config: an args-less entry yields "args": [] (key present, not dropped), and declared args are preserved verbatim.
| { | ||
| clients, | ||
| // readRegisteredServerKeys swallows parse errors and returns [] | ||
| readServerKeys: () => [], |
There was a problem hiding this comment.
🔴 Bug: The unreadable-config detection test stubs away the behavior it claims to verify
What's wrong
This test gives false confidence for the newly added MCP detection path. It says it verifies unreadable client configs are handled as no evidence, but by stubbing readServerKeys to return an empty array it only verifies the same path as a readable config with no matching servers.
Example
If readRegisteredServerKeys stopped swallowing TOML parse errors, or looked under mcpServers for a Codex CLI target that uses mcp_servers, this test would still pass because the stub bypasses that logic entirely.
Suggested direction
Exercise readRegisteredServerKeys against real config bodies instead of replacing it with a stub for this scenario.
For Agents
Move the unreadable-config and container-shape coverage to mcp-setup.test.ts or another test that writes real JSON/TOML config files and calls readRegisteredServerKeys. Keep the injected detectInstalled tests for row mapping, but add at least one parse-error case and one non-default entryPath case that prove the new helper behavior directly.
There was a problem hiding this comment.
Fixed in af391d0bb. This one was a fair hit — the test stubbed readServerKeys to return [], which exercises nothing but the no-matching-servers path, and would have passed even if the helper stopped swallowing parse errors or ignored a client's container shape. It asserted its own premise.
Removed it and put real coverage where you suggested: test/mcp-registered-server-keys.test.ts writes genuine config files to a temp dir and calls readRegisteredServerKeys directly — default mcpServers, VSCode's servers.<name>, Codex CLI's TOML mcp_servers, malformed JSON, absent file, missing and scalar containers, and an empty container (distinguished from a parse error).
Mutation-verified rather than assumed: forcing the helper to use the default container (splitEntryPath(undefined)) fails 2 of the 7 — the VSCode and Codex cases. The injected detectInstalled tests stay, but now only for row mapping, which is all they were ever really testing.
| return Array.isArray(v) && v.every((x) => typeof x === 'string'); | ||
| } | ||
|
|
||
| // INVARIANT: this must never be STRICTER than the registry's published JSON |
There was a problem hiding this comment.
🟡 Issue: Use the registry schema as the validator instead of patching a parallel one
What's wrong
The PR correctly identifies schema drift as the root problem, but the fix keeps the same two-sources-of-truth structure and adds more hand-maintained branches around it. That leaves future schema evolution dependent on contributors remembering to update this switch by hand.
Example
The same pattern already made schema-valid entries unreadable: an MCP entry without args, a manual entry with docsUrl, and a service entry with runtime: "binary" all required patching this switch. A future registry shape will still require edits in the schema, TypeScript interfaces, validator, and tests.
Suggested direction
Make the published registry schema the canonical boundary, either by compiling the vendored schema with a JSON Schema validator or generating the type guard from it. Keep only installability checks in the installer modules.
Confidence note
This assumes adding a small schema-validation dependency or generated validator is acceptable; if dependency policy blocks that, generating and checking in the guard would still remove the duplicate hand-coded rules.
For Agents
Look at packages/cli/src/integrations/schema.ts and registry-client.ts. Preserve the readable-vs-installable split, but replace the manual install-spec validator with validation compiled or generated from the published schema; keep installer-specific guards such as installMcp refusing missing args at install time.
There was a problem hiding this comment.
Agreed on the diagnosis, and I want to be straight that this is a deferral rather than a disagreement — the hand-maintained switch is exactly what caused the original drift, and this PR does add branches to it.
Two reasons for not doing it in this PR:
- The structural mitigation is already here. The contract suite derives one minimal fixture per install kind from the published
$defsand asserts the set of covered kinds equals the set the schema defines, so a new kind fails the build until the CLI handles it. That closes the specific hole that let three divergences ship unnoticed. Reverting themanualfix turns three tests red. - The dependency call isn't mine to make silently. Compiling the vendored schema needs a JSON Schema validator in the CLI runtime, or a generated-guard build step with a refresh policy. Your confidence note flags the same thing. That belongs in a scoped change where the tradeoff is the subject, not a side effect of a bug fix.
It is written up as a considered-and-deferred option in agent-docs/plans/2026-07-28-dkg-integrations-cli-contract.md ("Approaches Considered" #2, and Deferred #5) with the revisit trigger being: if drift recurs despite the contract test, generate the guard. Happy to open a follow-up issue if you'd rather it be tracked in GitHub than in the plan.
There was a problem hiding this comment.
🟡 Issue: The service install model is now a second, inconsistent registry schema
What's wrong
This PR is trying to fix schema drift, but it adds another hand-maintained version of the registry contract. The new service type both rejects/encodes runtime details manually and introduces a field name that does not match the vendored schema. That makes future service work harder to reason about because the TypeScript type no longer reflects the data actually coming from the registry.
Example
A future implementation for install.kind: "service", runtime: "binary" would naturally try to read entry.install.binary.url, because that is the registry field. The TypeScript model added here exposes binaryDownload instead, so maintainers either need casts or accidentally code against a property that registry entries never contain.
Suggested direction
Use the published registry schema as the canonical boundary. At minimum, rename binaryDownload to the schema's binary shape and avoid hard-coding runtime-specific details that the CLI does not consume. The cleaner move is a small discriminated model for supported installable service runtimes plus a loose readable model for unsupported service runtimes.
For Agents
In packages/cli/src/integrations/schema.ts, make the service install model match the registry field names exactly, or keep unconsumed runtime-specific payloads untyped until they are implemented. Preserve current readable-but-not-automated behavior for docker/binary services, and add a type-level/test fixture that uses the canonical install.binary field without casts.
There was a problem hiding this comment.
The binaryDownload half is valid — fixed in 8aae782a3. The registry schema declares binary: { url, checksumSha256? } and the type invented a different name for it. Nothing read the field, which is what made it a trap rather than a bug: whoever implements runtime: "binary" reads entry.install.binary.url from a real entry and finds no such property. Renaming a field with no consumers is zero-risk, and a type that misreports the contract is the exact defect class this PR exists to remove.
Checking it turned up more of the same class, so recording it here rather than leaving it implicit. Verified against the live schema at OriginTrail/dkg-integrations@main, which is JSON-equal to the vendored fixture:
| kind | drift |
|---|---|
mcp |
type has usageHint; schema does not (and additionalProperties: false). install-mcp.ts:146 renders it — a branch no legal entry can reach. supportedClients is optional in the type, required in the schema |
service |
docker missing required version, missing composeUrl, invents ports/env; npmGlobal invents env and marks binary required where the schema has it optional; usageHint not in schema |
agent-plugin |
invents registrationHint and usageHint; missing frameworkSetupDocs; framework typed string where the schema has an enum |
manual |
usageHint not in schema |
Two things bound the urgency. First, I checked all four live registry entries: only cli carries usageHint, which is legitimate, and none carries registrationHint or binaryDownload — so nothing depends on the phantom fields. Second, and more important, isValidInstallSpec is never stricter than the schema in any of these places: mcp does not require supportedClients, service checks only runtime. So no valid registry entry is rejected — this is type-level only, which is why I have not expanded the PR to chase it.
I would rather land the user-facing fix and take the remaining type conformance as a follow-up than grow this diff further. Happy to do it here instead if you would prefer it in one piece.
There was a problem hiding this comment.
🟡 Issue: Make the registry schema canonical instead of expanding the handwritten mirror
What's wrong
The PR documents that this validator must not drift stricter than the published registry schema, but then grows the same handwritten mirror that already drifted. That keeps the structural problem in place: the CLI has multiple independent definitions of the registry contract, and the install-kind checks will keep accumulating one-off branches as the schema evolves.
Example
Adding or changing a field in the registry schema now requires touching the vendored schema fixture, the TypeScript interface, and this hand-written validator. Missing one update recreates the drift this PR is trying to prevent.
Suggested direction
Use the registry JSON Schema, a generated parser/type, or a typed schema object as the single source for current-schema validation, then layer explicit installability checks in the installer modules. Avoid maintaining schema parity through parallel if chains.
For Agents
In packages/cli/src/integrations/schema.ts, replace the expanded handwritten install-spec mirror with a single canonical validation source. Preserve current readable-vs-installable behavior, then prove current 0.1.0 registry entries still parse and malformed consumed fields still fail at the boundary.
There was a problem hiding this comment.
🟡 Issue: The PR fixes schema drift by adding more schema drift surface
What's wrong
The root cause was that the CLI's hand-written validator drifted from the registry schema. This change patches the current cases, but it preserves the duplicate-contract design that caused the drift and adds more handwritten branches, casts, and fixture synchronization work. That leaves future maintainers with more places to update and no canonical boundary.
Example
The comment above this function says the validator must never be stricter than the registry schema, and this PR is already fixing three drift cases. A future registry change still requires someone to update the schema copy, schemaValidInstalls, InstallSpec, and the switch in isValidInstallSpec by hand.
Suggested direction
Replace the growing manual isValidInstallSpec mirror with a schema-backed validator or generated contract, and keep installer-specific requirements separate from registry readability. If adding a validator dependency is too much for this PR, at least move the install-kind validation into a table driven by a shared schema-derived model rather than another switch that must track the external schema by memory.
For Agents
Look at packages/cli/src/integrations/schema.ts and the new registry fixtures. Preserve the current readable-vs-installable behavior, but make the registry schema the executable source of truth where possible, then layer CLI-specific installability checks in the installers. A focused regression should prove the same schema-valid minimal install shapes still parse.
There was a problem hiding this comment.
No new drift instance named here that I can find, so treating this as a restatement of the canonical-schema thread — where I have already conceded the substance. Recording the position once more rather than re-arguing it.
Your example list is accurate and is the evidence: this PR fixed drift by hand in optional MCP args, manual.docsUrl, service binary, docker.version, and npm-global payload validation, and two of those were found inside commits whose stated purpose was preventing drift. The structure that produced all five is still here.
I am not disputing the direction. The blocker is that compiling the vendored schema means either a JSON Schema validator in the CLI's runtime dependencies or a codegen step with a checked-in artifact, and both touch package.json — a CODEOWNERS-guarded supply-chain path in a repo whose provenance gating this PR itself depends on. "May the CLI take a schema-validation dependency" is a maintainer decision with its own review surface.
Your fallback suggestion — a table driven by a shared schema-derived model, without the dependency — is the version I would reach for first, and this round moved one step toward it: service installability is now one exported resolver rather than two hand-written predicates. I will file the full question as its own issue with your five-instance list as the case, so it is decided deliberately rather than inherited from this diff.
There was a problem hiding this comment.
🟡 Issue: Make the registry schema the boundary instead of growing the hand-written mirror
What's wrong
This PR fixes schema drift by adding more branches to the same drift-prone validator. That leaves two canonical definitions of the registry payload shape, with tests acting as a guardrail rather than removing the duplicated model.
Example
A registry install-kind change now needs coordinated edits in the JSON Schema fixture, TypeScript interfaces, isValidInstallSpec, and the per-kind tests. Missing one repeats the drift this PR is trying to prevent.
Suggested direction
Use the vendored/published JSON Schema as the source of truth for registry readability, or generate the TypeScript validator from it. Then keep small explicit installability checks for what this CLI can automate.
For Agents
Look at packages/cli/src/integrations/schema.ts and the vendored registry schema. Preserve readable-vs-installable behavior, but make the published schema the canonical read boundary or generate the validator/types from it; keep CLI automation checks as a separate normalizer. Prove with the existing schema-valid install-shape tests.
There was a problem hiding this comment.
🟡 Issue: Use the registry schema as the validation source of truth instead of expanding the duplicate validator
What's wrong
The PR fixes schema drift by adding more branches to a second schema implementation. That reduces the immediate failures, but structurally preserves the root cause: the CLI now has a larger, more detailed duplicate of the registry schema that future changes must keep in sync by hand.
Example
If the registry adds a new service payload field or install kind, maintainers must update the JSON schema fixture, TypeScript interfaces, isValidInstallSpec, and the manually curated contract cases. That is the same drift-prone model this PR is trying to defend against.
Suggested direction
Replace or wrap the hand-written schema mirror with schema-driven validation, or generate the consumed TypeScript/type guards from the vendored schema. Keep only behavior-specific normalization in code.
For Agents
Look at packages/cli/src/integrations/schema.ts and the registry fixture tests. Preserve the distinction between “registry-readable” and “CLI-installable”, but make the published JSON Schema the canonical readable-entry validator, then keep installer-specific resolvers like resolveNpmGlobalService as the narrower automation boundary. Tests should prove schema-valid fixtures parse and installer gates still reject non-automatable entries.
There was a problem hiding this comment.
Unchanged, and I have conceded the substance repeatedly — the evidence for it got stronger again this round, so recording that rather than restating the deferral.
Since this was first raised, the hand-written model has produced: five drift fixes, a validator that disagreed with its own type (docker.version), and three separate bugs from one predicate living in one caller instead of a shared home. That is the case for making the schema executable, and I am not arguing against it.
The blocker is unchanged and is not about merit: it needs either a JSON Schema validator in the CLI's runtime dependencies or a codegen step with a checked-in artifact, and both touch package.json — CODEOWNERS-guarded here, in a repo whose provenance gating this PR itself relies on. That is a maintainer decision about what the CLI may depend on.
Filing it as its own issue with your five-instance list and the drift history above as the case, so it is decided on the merits.
Follows the CLI settling on three verbs (OriginTrail/dkg#1988): list and search are siblings over the registry differing only by a keyword filter, and local detection gets its own unambiguous verb rather than repurposing list. This also means list keeps the meaning it already shipped with, so nothing breaks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
af391d0 to
1bbeb7e
Compare
The CLI's registry parser is a hand-written re-implementation of the registry's
published JSON Schema, and it had drifted STRICTER than the schema in three
places. Entries that pass the registry's own validate.mjs were silently dropped
as 'unreadable' — by `dkg integration` and by the node dashboard sidebar, which
share this parser via fetchAllEntries().
manual required an install.steps field the schema FORBIDS
(additionalProperties: false) while ignoring the required docsUrl.
'steps' has never existed in the registry schema, so NO manual entry
could ever be read. Four are affected today.
mcp required args, which the schema makes optional.
service rejected the schema's 'binary' runtime.
Fixes all three and records the invariant that caused them: this validator must
never be stricter than the schema — it may be more lenient, so an older CLI can
still read a newer registry, but every entry the registry can merge has to parse.
Adds the contract test that would have caught this: one minimal schema-valid
fixture per install kind, plus vendored copies of the live entries, plus a check
that the fixtures cover every kind the published schema defines. Reverting the
schema.ts fix turns three of them red.
Also:
- installMcp now refuses an entry with no args instead of emitting a client
config whose 'args' key JSON.stringify silently drops, which would leave the
client launching a bare command with nothing to run. Readable != installable;
that distinction belongs in the installer, not the parser.
- 'manual' installs hand off instead of erroring. Per CONTRIBUTING it means 'the
installer links out to your docs', which is a success path, so it prints
docsUrl + oneLiner + the security declaration and exits 0. It was previously
lumped in with genuinely unimplemented kinds and exited 2.
- info renders manual (docsUrl/oneLiner) and the service binary runtime, both of
which previously printed nothing.
- search/list split: search discovers the registry (what list used to do, plus a
keyword filter), list reports what is installed HERE. Detection is derived
from the install targets — npm ls -g for cli/service, MCP client configs for
mcp — rather than a ledger, because a ledger records what we did while
detection reports what is true, and installMcp writes nothing (it prints a
block for the user to paste), so a ledger would claim installs that never
happened. Kinds the CLI cannot detect report 'unknown', never 'not installed'.
- installService for runtime npm-global, mirroring installCli including the
provenance gate, with daemon-oriented post-install guidance. docker and binary
runtimes keep the explicit not-implemented message.
Verified end to end against the live registry: buzz-dkg (manual, verified tier)
now appears in search, renders in info, and hands off on install with exit 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…k, shared installer
Round 1 review feedback from otReviewAgent.
- npm-global services were installed but never detected. detectInstalled only
looked at install.kind === 'cli', so a service this PR can now install was
reported 'unknown'. Detection keys on the global npm package for both kinds.
- npmGlobal.binary is OPTIONAL in the registry schema ('if different from the
package name'), but the installer assumed it, printing 'Start it with:
undefined' after a successful install. Falls back to the package name.
- install-service was a second copy of install-cli and had already diverged
mechanically (its runner passed shell:true on win32, install-cli's did not).
Extracted installNpmGlobalPackage — the provenance gate, dry-run handling,
npm command construction and runner invocation now live in one place; both
installers are thin kind guards plus post-instruction builders.
- Detection duplicated the MCP client list. It hardcoded three JSON paths while
mcp-setup already resolves thirteen targets including TOML (Codex CLI) and
alternate containers (servers.<slug>, mcp_servers.<slug>), so detection would
have silently missed most clients. Exports ClientTarget and a new
readRegisteredServerKeys() from mcp-setup and reuses detectClients(); the
install and detect paths can no longer drift.
- Adds the missing regression tests: installService provenance ordering,
dry-run, --no-verify-provenance, non-zero npm exit, the binary fallback, and
env/port guidance; detectInstalled across cli/service/mcp/undetectable kinds,
version drift, multi-client wiring, and not consulting npm when nothing needs
it. Both red-flag fixes are mutation-verified.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s local
Round 2 review feedback.
- Reverts the list repurposing. list keeps its shipped meaning (browse the
registry, { entries, failures } under --json) and local detection moves to a
new 'installed' command. The reviewer was right that changing list was a
silent breaking change, and the naming is better besides: list and search are
now siblings over the same thing differing only by a keyword filter, while the
command that reports something genuinely different gets a genuinely different
word. 'installed' needs no convention knowledge to read. Nothing breaks, so
the release-note warning is retired.
- Drops the installMcp args guard. A bare command with no args is a legitimate
MCP entry — a server launched by a binary already on PATH needs none — so
refusing it was stricter than the registry contract. The installer now
normalises args to [] instead, which keeps the emitted block well-formed
without judging whether a given command needs arguments; that is the entry
author's call.
- Replaces a vacuous test. 'treats an unreadable client config as no evidence'
stubbed readServerKeys to return [], which exercised nothing but the
no-matching-servers path — it would have passed even if the helper stopped
swallowing parse errors or ignored a client's container shape. Real coverage
now lives in test/mcp-registered-server-keys.test.ts, which writes actual
JSON and TOML config files and calls readRegisteredServerKeys directly:
default mcpServers, VSCode's servers.<name>, Codex CLI's TOML mcp_servers,
malformed JSON, absent file, missing/scalar container, empty container.
Mutation-verified: forcing the default container fails 2 of the 7.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1bbeb7e to
b8328b4
Compare
…installed"
detectInstalled exposes a three-state model — installed / not installed /
unknown — precisely so the CLI never claims absence it cannot support. The
npm path broke that invariant: listGlobalNpmPackages returned `{}` for every
failure mode (npm missing from PATH, a permissions error, unparseable
output), and an empty map is indistinguishable from "npm answered, nothing
is installed". Every cli and npm-global service entry then rendered as
"not installed" on a machine where we had simply failed to look.
The probe now resolves `null` for "no answer" and reserves `{}` for "npm
answered and the answer is nothing"; npm-installable entries map a `null`
probe to 'unknown'. Parsing is split into `parseGlobalNpmList` so the
failure-vs-empty distinction is covered directly rather than only through an
injected fake — that function is where the false negative would return if a
`null` were ever "simplified" back to `{}`.
Tests assert both directions as a pair: `null` -> unknown, and `{}` ->
not installed. The second is the control; without it a regression to
"unknown for everything" would pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| ); | ||
| case 'service': | ||
| return v.runtime === 'docker' || v.runtime === 'npm-global'; | ||
| return v.runtime === 'docker' || v.runtime === 'npm-global' || v.runtime === 'binary'; |
There was a problem hiding this comment.
🟡 Issue: Separate readable registry shape from installable service shape
What's wrong
The PR is trying to make registry parsing permissive while also using the parsed value as the typed command-dispatch model. That conflates two different concepts, and the new service runtime support pushes more optionality and local assertions into consumers instead of making the invariant explicit once.
Example
A registry entry shaped like { install: { kind: 'service', runtime: 'npm-global' } } is accepted as an IntegrationEntry, but every installer/detector still has to separately prove whether npmGlobal.package and npmGlobal.version exist before it can do useful work.
Suggested direction
Introduce a small normalization/refinement boundary, for example asNpmGlobalService(entry): NpmGlobalService | null, or make InstallService a discriminated union where each runtime owns its payload. That lets list/info remain permissive without spreading optional runtime fields, assertions, and casts through installers and detectors.
For Agents
Look in packages/cli/src/integrations/schema.ts and packages/cli/src/integrations/install-service.ts. Preserve the ability to read schema-valid entries, but split the raw readable registry model from installable normalized service specs, or model InstallService as a discriminated union by runtime. Add focused tests that prove readable unsupported shapes stay readable while npm-global installation receives a fully typed package/version contract without casts or optional chaining.
There was a problem hiding this comment.
🟡 Issue: Model service installs as runtime-specific variants instead of one optional bag
What's wrong
The PR widens service.runtime but leaves the service payload modeled as a single object with optional runtime-specific fields. That pushes the real invariant into scattered guards, casts, and optional access instead of the canonical schema boundary, which makes the install path harder to reason about and easier to accidentally weaken later.
Example
A registry-valid npm-global service with { package, version } but no binary is handled with npmGlobal.binary?.trim(), while the public InstallService shape still models the install spec as one optional bag. Tests work around this with broad casts instead of typed fixtures.
Suggested direction
Split InstallService into runtime-specific variants and let the parser/guard narrow to the exact variant each installer consumes. That deletes the local intersection type, the cast in buildPostInstructions, and the optional npmGlobal?. access in the command path.
For Agents
Refactor packages/cli/src/integrations/schema.ts so service installs are discriminated by runtime, e.g. docker/npm-global/binary variants, with npmGlobal.binary?: string. Then update install-service.ts and the commands.ts service branch to accept the narrowed npm-global variant without casts or optional access. Preserve the readable-vs-installable distinction and keep the existing service install behavior covered by current tests.
There was a problem hiding this comment.
Agreed on direction, deferring on scope, with a concrete reason rather than a vague one.
Verifying your sibling binaryDownload point sent me through the whole install-type surface against the live schema at OriginTrail/dkg-integrations@main (JSON-equal to the vendored fixture). The single optional bag is not the only divergence — there are roughly ten:
| kind | drift |
|---|---|
mcp |
type declares usageHint, schema does not (additionalProperties: false); supportedClients optional in the type, required in the schema |
service |
docker missing required version and composeUrl, invents ports/env; npmGlobal invents env; usageHint not in schema |
agent-plugin |
invents registrationHint and usageHint; missing frameworkSetupDocs; framework typed string where the schema has an enum |
manual |
usageHint not in schema |
Modelling InstallService as runtime-specific variants is the right shape, but doing it in isolation fixes one of these while leaving the rest — and the union should be cut against the schema's actual per-runtime payloads, which means the same pass either way.
Two things bound the urgency and are worth stating so the deferral is auditable rather than a brush-off. isValidInstallSpec is never stricter than the schema at any of these points — mcp does not require supportedClients, service checks only runtime — so no valid registry entry is rejected; this is type-level only. And all four live registry entries are clean: only cli carries usageHint, which is legitimate, and none carries registrationHint or the old binaryDownload.
I would rather land the user-facing false-negative fixes and do type conformance as one coherent pass than convert this PR into a type refactor. Say the word if you would prefer it here instead.
There was a problem hiding this comment.
🟡 Issue: Centralize the readable-vs-installable install model
What's wrong
This change broadens registry readability and adds service automation, but leaves the real contract scattered across several modules. The type says callers have an IntegrationEntry, while the validator has only established a looser, schema-readable shape; every consumer now has to remember which service variants are installable, detectable, displayable, or only manual fallback. That is exactly the kind of special-case growth that becomes brittle when the next runtime or metadata field lands.
Example
A schema-valid { kind: 'service', runtime: 'npm-global' } is readable, not automatable, not npm-detectable, and only partly displayable; each caller currently rediscovers that state with its own branch instead of using one model.
Suggested direction
Introduce a canonical install classification helper or narrower typed guards, then have install dispatch, detection, and info rendering consume that instead of repeating runtime/metadata checks.
For Agents
Look at packages/cli/src/integrations/schema.ts, the service branch in commands.ts, globalNpmPackageFor in detect-installed.ts, and assertNpmGlobalService in install-service.ts. Preserve schema-compatible readability and current user-facing behavior, but add a single classifier/normalizer that returns explicit cases such as npmGlobalPackage, manualDocs, mcpServer, and unsupported(reason). Prove minimal services remain readable while populated npm-global services use the same package metadata in install, detection, and display.
There was a problem hiding this comment.
Same disposition as the sibling thread on this file, and your framing sharpens why I want it done as one pass rather than piecemeal.
You are right that { kind: 'service', runtime: 'npm-global' } is readable, not automatable, not npm-detectable, and only partly displayable, and that each caller currently rediscovers that. One of those callers has since been fixed for real: install dispatch now gates on package metadata rather than runtime alone (edfff9a07), so the "confusing hard failure" case is gone even though the classifier is not.
The reason I am not introducing npmGlobalPackage | manualDocs | mcpServer | unsupported(reason) in this PR is that the classifier should be cut against types that describe the registry accurately, and they currently do not — roughly ten divergences catalogued on the thread above, including mcp.supportedClients being optional in the type but required in the schema, and service.docker missing its required version. A classifier built on those would encode the divergence into the abstraction, which is the more expensive mistake to unwind.
Proposed order, unchanged: land the behavioural fixes, do schema conformance, then add the capability layer on top of correct types. Happy to file it as a tracked follow-up so it does not evaporate.
There was a problem hiding this comment.
🔴 Bug: Service install metadata is accepted without validating consumed fields
What's wrong
This PR adds automatic npm-global service installation, but the runtime validator still only checks the service runtime enum. That means malformed service metadata can be treated as a valid IntegrationEntry and reach the installer, even though these fields now drive an npm global install command.
Example
A registry response with install: { kind: 'service', runtime: 'npm-global', npmGlobal: { package: { name: '@acme/svc' }, version: '1.0.0' } } passes isIntegrationEntry. dkg integration install <slug> can then build an npm spec like [object Object]@1.0.0 instead of rejecting the entry as malformed.
Suggested direction
Validate optional service sub-objects when present, especially npmGlobal.package and npmGlobal.version, before any automatic install path can use them.
For Agents
Tighten the service branch in packages/cli/src/integrations/schema.ts: keep minimal runtime-only service entries readable, but when optional docker, npmGlobal, binary, envRequired, portsOpened, or usageHint fields are present, validate the fields the CLI will consume. Add a test for malformed npmGlobal.package / version being rejected while { kind: 'service', runtime: 'npm-global' } still parses.
There was a problem hiding this comment.
Valid and fixed in 4b7f60145. Your example is exact: npmGlobal: { package: { name: '@acme/svc' }, version: '1.0.0' } passed both isValidInstallSpec (which only checked the runtime enum) and assertNpmGlobalService (which checked truthiness, and a non-empty object is truthy), so the installer would have run npm install --global [object Object]@1.0.0.
Both now type-check the fields the CLI consumes, and the same treatment is applied to docker, binary, envRequired, and portsOpened.
Worth being explicit about why this tightening is not the thing this PR exists to remove. Payload objects remain optional — a bare { kind: 'service', runtime: 'npm-global' } still parses, and its contract row still passes. But the schema marks each payload's own fields required when that payload is present (npmGlobal → [package, version], docker → [image, version], binary → [url]). So this is schema-consistent: it rejects exactly what the registry would reject, and nothing more. That is the distinction between this and the original manual-kind bug.
Added as new rows in the reject list, and mutation-verified — reverting the validator to runtime-only fails still rejects shapes the schema would also reject.
…ma does
The registry schema's installService declares `binary: { url, checksumSha256? }`,
but the TypeScript model called it `binaryDownload`. Nothing read it, so this
was invisible at runtime — and that is exactly what makes it a trap: whoever
implements `runtime: "binary"` reads `entry.install.binary.url` from a real
entry, finds no such property on the type, and either reaches for a cast or
codes against a name registry entries never carry.
Renaming a field no consumer touches is safe, and a type that misreports the
contract is the same defect class this PR exists to remove.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| try { | ||
| body = readConfigBody(target); | ||
| } catch { | ||
| return []; |
There was a problem hiding this comment.
🔴 Bug: Unreadable MCP configs are collapsed into “not installed”
What's wrong
The new installed detector is meant to distinguish “we looked and it is absent” from “we could not look”, but MCP config read failures are converted to an empty key list. That makes a malformed or unreadable client config indistinguishable from a clean config with no integration, so the command can incorrectly tell users an MCP integration is not installed.
Example
If ~/.cursor/mcp.json contains malformed JSON but actually has a pasted buzz-dkg server block, dkg integration installed cannot inspect that file. Current behavior reports buzz-dkg as not installed; expected behavior should be unknown or include a probe failure so the user is not given a false negative.
Suggested direction
Return a tri-state result from the MCP config reader, or let read errors bubble to detectInstalled so MCP rows can become unknown when the CLI could not inspect a relevant client config.
For Agents
Look at readRegisteredServerKeys and the MCP branch in detectInstalled. Preserve true empty configs as not installed, but propagate unreadable/unparseable config state separately and prove malformed client config yields unknown rather than not installed.
There was a problem hiding this comment.
Valid, and the more useful part is why it was still there: I had just fixed this exact defect for the npm probe and did not sweep the class. Fixed in 5aa58e026.
readRegisteredServerKeys returned [] for four different situations — config missing, config unreadable, config unparseable, config readable-but-empty — and the MCP branch maps "registered in no client" to not installed. So a malformed ~/.cursor/mcp.json reported an integration as absent even when the server block was sitting in the file we failed to parse. Its own doc comment said "absence of evidence, not evidence of absence" and then shipped precisely that conflation, which is a good reminder that a comment stating an invariant is not the same as enforcing it.
It now returns a ServerKeyProbe:
| situation | result | reasoning |
|---|---|---|
file absent (ENOENT) |
{ ok: true, keys: [] } |
a real answer — this client registered nothing |
| file unreadable / unparseable | { ok: false, reason } |
we could not look |
| container absent | { ok: true, keys: [] } |
readable, nothing there |
| container present but not an object | { ok: false, reason } |
malformed exactly where we needed to read |
detectInstalled tracks which clients failed to probe: a slug found in no readable config becomes unknown when any config was unreadable, and stays not installed only when every probe succeeded. Positive evidence still wins — a slug registered in a readable config reports installed even if a sibling config is broken, which is covered by its own test so the unknown state cannot swallow evidence we actually have.
Mutation-verified rather than assumed. Making the failure path return { ok: true, keys: [] } fails reports a config it cannot parse as a FAILED probe and distinguishes an empty container from a parse error; reclassifying the unreadable case as not installed fails reports mcp as unknown when a client config could not be read and leaves the positive-evidence control passing.
There was a problem hiding this comment.
🔴 Bug: MCP detection reports disabled or malformed server entries as installed
What's wrong
The new installed-state detection equates key presence with a working MCP registration. That creates false positives for null, scalar, or otherwise malformed entries under the slug, so users can be told an integration is installed when it cannot be launched.
Example
A config like { "mcpServers": { "buzz-dkg": null } } returns ['buzz-dkg'], so dkg integration installed reports buzz-dkg as installed even though the existing MCP setup classifier treats a null entry as not registered and the client has no launchable server block.
Suggested direction
Filter keys by entry value shape, or return richer probe data so malformed per-server values do not become positive installation evidence.
For Agents
Look at readRegisteredServerKeys and detectInstalled MCP handling. Preserve the distinction between unreadable configs and empty configs, but only report a server key as installed when the value is a usable object; add a case for null/scalar entries proving they are not reported as installed, or are reported unknown/stale if that is the intended state.
There was a problem hiding this comment.
Valid, and a useful mirror of the last round: everything I fixed there was a false negative, and this is the false positive on the same surface. Fixed in edfff9a07.
readRegisteredServerKeys returned Object.keys(container) verbatim, so any key counted as a registration regardless of its value. { "mcpServers": { "buzz-dkg": null } } reported buzz-dkg as installed even though nothing there can launch.
The decisive detail is that this module had already settled the question ten lines away — classify() treats { dkg: null } as not-registered, with a comment recording that it used to read as stale and that claiming "there is a current value to refresh" was wrong. My reader contradicted an invariant its own file had deliberately established, which is the kind of inconsistency that only shows up when two features read the same config through different helpers.
Keys are now filtered to values that are non-null, object-typed, and not arrays. Deliberately not unknown: we read the config successfully and the entry is explicitly not a working registration, so not installed is the honest answer — the same conclusion classify reaches. The unreadable-config path is untouched, so the two states stay independent.
Covered by ignores keys whose value is not a usable server block, which mixes a good entry with null, scalar, and array values and asserts only the good one survives while the probe still reports ok: true — a filter, not a failure.
There was a problem hiding this comment.
🟡 Issue: Malformed MCP args can be normalized into a false installed match
What's wrong
The parser silently drops non-string args before matching against the registry entry. That can convert a malformed client config into the exact expected args array and make dkg integration installed claim an MCP server is installed even though the registered launch block does not actually match what the integration declares.
Example
Registry entry expects command: "my-server" with no args. A client config contains { "command": "my-server", "args": [123] }. Current behavior: the parser normalizes [123] to [], so detection can report the integration as installed. Expected behavior: treat the block as malformed or non-matching, because the stored launch block is not the declared launch block.
Suggested direction
Preserve the distinction between missing args and malformed args. Missing should normalize to []; present-but-invalid should not be treated as a valid launch block.
Confidence note
This depends on how each MCP client handles malformed args, but the detector's own contract is exact command/args matching, so dropping non-string entries changes the evidence before comparison.
For Agents
In readRegisteredServerKeys, do not filter malformed args into a matching shape. If args is present, require every element to be a string; otherwise skip that server or return an ok: false malformed probe. Add a test proving non-string args cannot match an args-less registry entry.
There was a problem hiding this comment.
Valid, and also mine from the previous commit. Fixed in d716587fd.
Filtering non-string elements out of args rewrote the evidence before comparing it: args: [123] became [], which then matched an args-less registry entry exactly as you describe. Normalizing input and then treating the normalized value as proof of a match is the underlying mistake — the same shape as the earlier bug where an unreadable config became [] and read as "nothing registered".
args is now string[] | null:
| config | value | meaning |
|---|---|---|
| key absent | [] |
legitimate args-less server; matches an args-less entry |
["-y","p"] |
["-y","p"] |
compared element-wise |
[123] |
null |
present but not a string array — matches nothing |
A null block still counts as a registration under that slug, so it reports unknown ("a different server is registered as …") rather than not installed — the collision stays visible instead of being silently dropped.
On your confidence note: you are right that client behaviour with malformed args varies, and that is the argument for not guessing. The detector's contract is exact comparison, so an uncomparable block should fail to match rather than be coerced into matching.
Three tests: absent → [], malformed → null, well-formed → preserved, plus a detection pair proving malformed cannot match an args-less entry while a genuinely absent key can. Mutation-verified: restoring the filter fails them.
There was a problem hiding this comment.
🔴 Bug: Array MCP server containers are treated as readable empty configs
What's wrong
The new MCP detection contract distinguishes "we looked and found nothing" from "we could not inspect this config". Arrays are not valid server maps, but this branch treats an array container as readable and empty, which produces false "not installed" results for MCP integrations when the client config is malformed.
Example
With a readable but malformed config like {"mcpServers": []}, readRegisteredServerKeys returns ok: true with no servers. Then dkg integration installed reports MCP integrations as "not installed" instead of "unknown / malformed config".
Suggested direction
Reject Array.isArray(cursor) in the malformed-container check, and consider using the same plain-object predicate used elsewhere in this module.
For Agents
Update readRegisteredServerKeys in packages/cli/src/mcp-setup.ts so the server container must be a plain object, not an array. Preserve missing-container => ok true empty, but make present array/scalar => ok false. Add a test for { mcpServers: [] } proving MCP detection reports unknown rather than not installed.
There was a problem hiding this comment.
Valid — fixed in 73f384470. typeof [] === 'object', so an array container passed the malformed check and fell through to Object.entries([]), reporting "readable, nothing registered" for a container that cannot be interpreted.
The container check now rejects arrays alongside null and scalars.
Worth naming what this is, because it is the fourth instance of one bug: this surface has repeatedly reported a config it failed to read as a config it read and found empty. Unreadable file → []. Malformed scalar container → handled. Array container → missed until now. And the entry-level filter I wrote two lines below this check already lists arrays among the values that cannot be a registration — I applied the rule at the entry level and did not apply it one level up, in the same commit. That is a better argument for your canonical-schema/shared-module threads than anything I have said in their defence.
Covered by reports an ARRAY server container as a FAILED probe, asserting ok: false and the malformed reason, sitting next to the scalar-container case so the pair reads as one rule rather than two special cases.
| const undetectable = rows.filter((r) => r.state === 'unknown'); | ||
| if (undetectable.length > 0) { | ||
| console.log( | ||
| `${undetectable.length} entr${undetectable.length === 1 ? 'y' : 'ies'} cannot be detected ` + |
There was a problem hiding this comment.
🟡 Issue: Installed output misreports probe failures as unsupported install kinds
What's wrong
The installed command has multiple reasons for unknown, but the human output always says those entries use install kinds the CLI does not perform. That is incorrect for npm-backed cli and service entries when the npm probe fails, and it can send users toward the wrong fix.
Example
On a machine without npm in PATH, a global CLI integration row becomes { state: 'unknown', detail: 'could not inspect global npm packages' }. Human output currently says the entry cannot be detected because it is an install kind the CLI does not perform, even though cli is a supported detectable kind and the real issue is probe failure.
Suggested direction
Render unknown rows by their actual detail or reason instead of using one hard-coded explanation for all unknown states.
For Agents
Look at packages/cli/src/integrations/commands.ts installed-command rendering. Preserve InstalledRow.detail for unknown rows, and separate unsupported install kinds from failed probes. Add a case proving npm probe failure is rendered as a probe failure for cli/service rows.
There was a problem hiding this comment.
Correct, and this one was a regression I introduced. Fixed in 5aa58e026.
When unknown had exactly one cause, a single hard-coded line was accurate. My npm-probe fix added a second cause and left the renderer asserting the old one, so on a machine without npm on PATH a cli entry — a fully supported, detectable kind — was reported as "an install kind the CLI does not perform". As you say, that sends the user to fix the wrong thing.
Unknown rows now render their own detail, in the same shape as installed rows:
2 entries could not be determined:
some-cli-tool [cli] could not inspect global npm packages
some-plugin [agent-plugin] the CLI does not perform this install kind
The reason string is produced where the knowledge is — detect-installed.ts — rather than reconstructed by the renderer, so a third cause added later surfaces correctly without touching this code.
There was a problem hiding this comment.
🟡 Issue: npm-global service entries without package metadata hit the automated install path
What's wrong
The command selects automation based only on the runtime, but the new validator allows registry-valid service entries that do not carry the npm package metadata the installer requires. That turns a readable registry entry into a confusing hard failure at install time.
Example
A schema-valid entry with install: { "kind": "service", "runtime": "npm-global" } is listed and accepted by the CLI. Running dkg integration install <slug> reaches installService, throws Registry entry declares runtime "npm-global" but no npmGlobal.package/version, and exits through the generic install-failed path instead of giving the user the non-automated fallback used for other service runtimes.
Suggested direction
Gate the automated service path on both runtime === 'npm-global' and a complete npmGlobal.package/version, then fall back to manual instructions when metadata is missing.
Confidence note
This depends on whether the registry intentionally permits runtime: "npm-global" services without an npmGlobal package/version. The PR’s new contract tests explicitly treat that shape as schema-valid.
For Agents
Check commands.ts service dispatch and install-service.ts preconditions. Either require npmGlobal.package/version before selecting the automated service path, or make the schema validator reject npm-global services that cannot be automated; add a command-level case proving schema-valid-but-not-automatable services get the intended fallback behavior.
There was a problem hiding this comment.
Correct, and your confidence note asks exactly the right question — the answer is yes, the registry does permit it. The published schema requires only kind and runtime for a service; every payload object is optional. So { kind: 'service', runtime: 'npm-global' } is genuinely valid and genuinely not automatable. Fixed in edfff9a07.
This one surfaced because of the contract-test row added last round. Dispatching on runtime alone sent the entry into installService, which threw Registry entry declares runtime "npm-global" but no npmGlobal.package/version and exited through the generic install-failed path — strictly worse than the docker/binary case, which exits cleanly with a pointer to the integration's own docs.
The gate now requires runtime === 'npm-global' and a complete npmGlobal.package/version, falling back to the same graceful exit(2) with its own message rather than reusing the runtime one (the runtime is supported here; the metadata is what is missing, and saying otherwise would misdirect).
I took your first option rather than tightening the validator, because rejecting the shape would make the CLI stricter than the published schema — the precise defect this PR exists to remove. Readable-but-not-automatable stays a legitimate state.
Pinned by install falls back gracefully for an npm-global service with no package metadata, asserting exit code 2, the specific message, and that no undefined leaks into the output.
There was a problem hiding this comment.
🟡 Issue: Installed command hides unreadable registry entries in human output
What's wrong
The installed command bases its detection only on successfully parsed entries, but the default human output never reports entries skipped by fetchAllEntries. That makes the local-state result look complete even when some registry entries could not be inspected at all.
Example
If the registry index includes one entry but fetchEntry fails because the local CLI cannot parse that entry, dkg integration installed --json exposes it under failures, but plain dkg integration installed can say no integrations were detected and only checked 0 detectable entries.
Suggested direction
Mirror the listing warning behavior for failures before or after rendering detection rows, so users know the result is incomplete.
For Agents
In the installed command in packages/cli/src/integrations/commands.ts, surface fetchAllEntries failures in the human path, similar to runRegistryListing. Preserve the JSON envelope, and add a Commander-level test where one indexed slug fails parsing/fetching and the human output warns about skipped registry entries.
There was a problem hiding this comment.
Valid and fixed in 73f384470. installed destructured failures, reported them under --json, and never printed them in human mode — so the closing "Checked N detectable entries" counted only what it could parse and gave no hint anything had been skipped. list and search already warn about the same failures, so the two surfaces disagreed about whether an unreadable entry is worth mentioning.
The human path now warns with the same per-slug detail, plus an explicit note that skipped entries were not considered for install detection — that clause matters here in a way it does not for list, because a silently-skipped entry in a detection command reads as "you do not have it installed".
There was a problem hiding this comment.
🔴 Bug: Service installs ignore the selected DKG API URL
What's wrong
The new npm-global service install path does not carry the user-provided API URL into its post-install instructions. That makes the public install command produce incorrect setup guidance for services meant to talk to a non-default DKG node.
Example
For a service entry with envRequired: ['DKG_API_URL'], running dkg integration install svc --api-url http://10.0.0.5:9200 --dry-run still prints DKG_API_URL — default http://127.0.0.1:9200, so following the CLI output points the service at the wrong node.
Suggested direction
Thread opts.apiUrl into installService and use it when rendering DKG_API_URL instructions, or make the command reject/ignore the flag explicitly for service installs.
For Agents
Look at packages/cli/src/integrations/commands.ts service dispatch and packages/cli/src/integrations/install-service.ts post-install instruction generation. Preserve existing dry-run/provenance behavior, pass the selected API URL into service install guidance, and add a Commander-level test proving a non-default --api-url appears for services requiring DKG_API_URL.
There was a problem hiding this comment.
Valid and user-facing — fixed in 79cd09a9c.
Confirmed exactly as described: installService had no reference to apiUrl at all, and buildPostInstructions hardcoded — default http://127.0.0.1:9200. So a documented flag ("DKG node HTTP API URL to wire into integrations") was accepted, honoured by installMcp, and silently ignored for services — an operator following the printed guidance would point their service at the wrong node while the same flag worked one install kind over.
apiUrl is now threaded through InstallServiceOptions into the guidance, echoing the selected node.
Two tests rather than one, because the obvious single test is passable by a broken implementation: the flag case asserts the custom URL appears and the default string does not, and a control asserts the default is still rendered when no flag is passed — otherwise "never print the default" would satisfy the first.
Mutation-verified: forcing the ternary to its default branch fails echoes the selected --api-url in DKG_API_URL guidance. Worth noting my first mutation attempt silently no-opped (this file is CRLF, so a
-joined search string cannot match) and reported "36 passed" — which looks identical to a verified fix. Flagging it because that failure mode is exactly what this review has repeatedly caught me on, and the apply-step check is the only reason it did not become a false claim here.
| let body: Record<string, unknown>; | ||
| try { | ||
| body = readConfigBody(target); | ||
| } catch { |
There was a problem hiding this comment.
🟡 Issue: Preserve MCP probe failure state instead of returning an ambiguous empty list
What's wrong
The helper’s return type collapses read/parse failures into the same value as an empty config. That conflicts with the PR’s own three-state installed model and bakes ambiguity into the reusable API, making later detection logic depend on an unclear boundary.
Example
A malformed TOML config and a valid { mcp_servers: {} } config both return [], so the installed detector receives the same data shape for "could not inspect" and "inspected successfully, no registrations".
Suggested direction
Use an explicit result type for MCP config probing instead of string[]. Let the caller decide whether a failure should become unknown and whether an empty-but-readable container should become not installed.
For Agents
Change the MCP key reader to return a typed probe result, for example { ok: true, keys } | { ok: false, reason }, and map that explicitly in detectInstalled. Decide which cases are truly empty, such as missing config, versus unknown, such as malformed or unsupported config format. Update mcp-registered-server-keys.test.ts to assert the distinction.
There was a problem hiding this comment.
Agreed and done in 5aa58e026 — same change as the 🔴 on this file, which is the behavioural statement of the same defect. The reader now returns ServerKeyProbe = { ok: true; keys } | { ok: false; reason } rather than string[], so the ambiguity is gone from the API rather than being handled by convention at each call site.
The boundary you asked about is drawn explicitly: a missing config is ok: true with no keys, because "this client has registered nothing" is a real answer; unreadable, unparseable, or malformed-at-the-container is ok: false. Only detect-installed.ts consumes this in source, and it now maps the two cases differently.
mcp-registered-server-keys.test.ts asserts the distinction in both directions. Worth noting that the previous version could not have caught this: one test was literally named "returns [] for an empty container without conflating it with a parse error" while asserting toEqual([]) for both cases — the name claimed a property the assertion was structurally incapable of testing.
| ['mcp (args present)', { kind: 'mcp', command: 'npx', args: ['-y', 'p'], supportedClients: ['cursor'] }], | ||
| // args is OPTIONAL in the schema — readable here, refused by installMcp. | ||
| ['mcp (args absent)', { kind: 'mcp', command: 'npx', supportedClients: ['cursor'] }], | ||
| ['service (npm-global)', { kind: 'service', runtime: 'npm-global', npmGlobal: { package: 'p', version: '1.0.0', binary: 'b' } }], |
There was a problem hiding this comment.
🟡 Issue: Schema-contract test misses the minimal npm-global service shape
What's wrong
The new contract suite is meant to prove every registry-valid install shape remains readable, but the npm-global service fixture includes optional npmGlobal data. That gives false confidence for the exact compatibility boundary the suite is trying to protect.
Example
A future validator change like case 'service': return v.runtime === 'npm-global' && isPlainObject(v.npmGlobal) would still pass this test row, but it would reject the schema-valid entry { install: { kind: 'service', runtime: 'npm-global' } }.
Suggested direction
Cover the actual schema-minimal service shapes, especially runtime: 'npm-global' without npmGlobal, so the test catches validator drift that is stricter than the published schema.
For Agents
In packages/cli/test/integrations.test.ts, add or replace with a service (npm-global minimal) case using { kind: 'service', runtime: 'npm-global' }. If installation still requires npmGlobal.package/version, pin that separately in integrations-install-service.test.ts so readability and installability remain distinct.
There was a problem hiding this comment.
Good catch — this is the sharpest of the batch, because the test was passing for a reason unrelated to what it claimed to protect. Fixed in 5aa58e026.
The suite exists to prove every registry-valid install shape stays readable, but the npm-global row carried a fully populated npmGlobal, so exactly the drift it guards against — a validator tightening to isPlainObject(v.npmGlobal) — would have kept it green while rejecting { kind: 'service', runtime: 'npm-global' }, which the published schema accepts. The schema requires only kind and runtime for a service; every payload object is optional.
Added the schema-minimal rows, which are the ones that actually pin the boundary:
service (npm-global, minimal)—{ kind: 'service', runtime: 'npm-global' }service (docker, minimal)—{ kind: 'service', runtime: 'docker' }
Installability stays pinned separately in integrations-install-service.test.ts, so readable-but-not-installable remains a distinct, tested state rather than being collapsed into the contract suite.
There was a problem hiding this comment.
🔴 Bug: New fixture tests use __dirname in an ESM test file
What's wrong
The added registry-contract tests are supposed to verify that vendored registry entries and the published schema remain readable, but they rely on CommonJS __dirname. This package runs as ESM, so those tests fail at runtime instead of providing validation evidence for the schema compatibility changes.
Example
Running the new parses every vendored live registry entry test will hit ReferenceError: __dirname is not defined before it reads any fixture, so the registry/CLI contract assertions never execute.
Suggested direction
Add the standard ESM __dirname shim near the imports, or replace these joins with URL-based fixture reads.
For Agents
In packages/cli/test/integrations.test.ts, define __dirname using dirname(fileURLToPath(import.meta.url)) or build fixture paths from new URL(...). Keep the vendored fixture assertions intact and run the integration test file to prove they execute.
There was a problem hiding this comment.
The change is worth making and is in 4b7f60145. The stated reason is not correct, and since it is a 🔴 the distinction matters.
The claim is that the tests "fail at runtime instead of providing validation evidence" and that parses every vendored live registry entry hits ReferenceError "before it reads any fixture, so the registry/CLI contract assertions never execute". I ran that test in isolation against the current head:
✓ test/integrations.test.ts (65 tests | 64 skipped) 16ms
Tests 1 passed | 64 skipped (65)
It executes and passes. Vitest's transform supplies __dirname even though the package is "type": "module", so the fixture assertions have been running — including in the CI runs on every head of this PR. The contract evidence was not missing.
That said, your underlying point stands on its own: relying on the runner to paper over an ESM/CJS mismatch is a latent trap for anyone executing the file another way, and the fix costs two lines. Paths now derive from dirname(fileURLToPath(import.meta.url)).
Flagging the reasoning because a 🔴 that says "these tests never run" is a much stronger claim than "these tests depend on a runner shim", and only the second one was true.
There was a problem hiding this comment.
🔴 Bug: The npm-global fallback test can pass even if the generic failure path also runs
What's wrong
The test is meant to prove the new graceful fallback for schema-valid npm-global services with no npmGlobal metadata, but its assertions do not catch the main regression it describes: falling through into the generic install failure path after the graceful message.
Example
If the break after process.exit(2) in the npm-global/no-metadata branch were accidentally removed, the command would first print the graceful no npmGlobal.package/version message, then fall into installService, hit the catch block, and print Install failed: .... This test would still keep exit === 2 and err would still contain no npmGlobal.package/version, so it could pass while the real CLI output regressed.
Suggested direction
Assert that the generic catch-all error is absent for svc-bare, or change the exit spy so tests cannot continue past a simulated process exit.
For Agents
In packages/cli/test/integrations.test.ts, strengthen the svc-bare install-dispatch test or the shared runInstall helper. Preserve the intended behavior that schema-valid npm-global service entries without package metadata exit through the graceful non-automated path only. Add an assertion mirroring the binary-runtime case, such as expect(r.err).not.toContain('Install failed'), or make the mocked process.exit abort command execution after recording the code.
There was a problem hiding this comment.
Correct, and this is the best catch of the review — a test I wrote that could not detect the regression it was named for. Fixed in a6143effa.
Your reasoning is exactly right. The exit spy records rather than throws (throwing is caught by the command's own try/catch and converted to exit(1), which collapses every asserted code to 1 and hides which branch ran), so execution continues past a simulated exit. Remove the break and the command prints the graceful message, falls into installService, throws, and prints Install failed — while exit still reported the first code and err still contained the graceful string.
What makes it worse is that the assertion which catches this was already present eight lines below, on the svc-binary case, and I simply did not apply it to svc-bare in the same commit. Same shape as the array-container bug last round: a rule written in one place and not the adjacent one.
I took your second suggested direction rather than the first, because a per-test string assertion only protects the test that remembers it. The spy now records every exit code and the fallback tests assert exits is exactly [2]. More than one entry means the real process would already have terminated and the test is observing code that could never run — so every future test on this harness inherits the check.
Verified by mutation rather than assumed, and worth showing because my first attempt at the mutation silently failed to apply and "66 passed" proved nothing. Deleting the break:
FAIL … > `install` falls back gracefully for an npm-global service with no package metadata
AssertionError: expected [ 2, 1 ] to deeply equal [ 2 ]
[2, 1] is precisely the fall-through you described. The previous exit === 2 assertion passes against that same mutant.
| kind: 'mcp'; | ||
| command: string; | ||
| args: string[]; | ||
| // Optional per the registry schema. An entry without args is readable |
There was a problem hiding this comment.
🔵 Nit: MCP args comment contradicts the installer
Why it matters
This is small, but it documents the opposite invariant from the implementation and tests, which invites someone to reintroduce a refusal path later.
Suggestion
Update the comment to say args are optional and installMcp emits args: [], or remove the installability claim entirely.
There was a problem hiding this comment.
Right, and fixed in 5aa58e026. The comment documented a refusal path that does not exist — installMcp normalises a missing args to [] (install-mcp.ts), it does not reject the entry.
Fixed in both places it appeared: schema.ts on the InstallMcp.args field, and the same stale claim on the mcp (args absent) row in the contract suite. As you say, the risk was not the wrong sentence but that someone would later "restore" the invariant it described.
There was a problem hiding this comment.
🔵 Nit: Update the stale args-less MCP comment
Why it matters
The comment is now the opposite of the implementation at a schema boundary where future maintainers will look for invariants.
Suggestion
Change the comment to say missing args is both readable and installable, with installMcp normalizing it to [], or remove the installer-behavior claim entirely.
There was a problem hiding this comment.
This one is already fixed — I think you are reading a pre-5aa58e026 state.
On the current head the comment reads:
// Optional per the registry schema, and genuinely optional here: a server
// launched by a binary already on PATH needs none, so installMcp normalises
// a missing value to `args: []` rather than refusing the entry. Judging
// whether a given command needs arguments is the entry author's call.
args?: string[];Which is your suggested wording — readable and installable, with installMcp normalizing to [], and no installer-behaviour claim left to contradict. I also fixed the same stale claim on the mcp (args absent) row in the contract suite at the time, since it repeated the refusal wording.
Verified against the pushed ref rather than my working tree: GET /repos/OriginTrail/dkg/contents/packages/cli/src/integrations/schema.ts?ref=387bd7a5d shows the corrected text. No change needed.
There was a problem hiding this comment.
🔵 Nit: The MCP args comment contradicts the installer behavior
Why it matters
This comment points maintainers toward the opposite invariant from the one implemented in install-mcp.ts, making a future cleanup likely to reintroduce the wrong guard.
Suggestion
Update the comment to say args are optional and installMcp emits args: [], or remove the installability claim entirely.
There was a problem hiding this comment.
This is the second time this has been raised and it was already fixed — in 5aa58e026, before the previous round. I verified it against the pushed ref rather than my working tree both times.
Current text at schema.ts, InstallMcp.args:
// Optional per the registry schema, and genuinely optional here: a server
// launched by a binary already on PATH needs none, so installMcp normalises
// a missing value to `args: []` rather than refusing the entry. Judging
// whether a given command needs arguments is the entry author's call.That is the suggested wording: optional, installable, installMcp normalises to [], no installer-refusal claim left to contradict. The matching stale comment on the mcp (args absent) contract row was fixed at the same time.
No change made. If it surfaces again it may be worth checking which ref the review is reading, since the fix predates the last two heads.
There was a problem hiding this comment.
🔵 Nit: Fix the stale MCP args comment
Why it matters
The code and comment now say opposite things about an important registry boundary. That kind of stale invariant comment makes future schema or installer changes harder to reason about.
Suggestion
Update the comment to match the new invariant: args is schema-optional and installable, with installMcp emitting args: [] when omitted.
There was a problem hiding this comment.
Third time raised, and it has been fixed since 5aa58e026 — several heads ago. Verified against the pushed ref again rather than my worktree, at 2044ed803:
// Optional per the registry schema, and genuinely optional here: a server
// launched by a binary already on PATH needs none, so installMcp normalises
// a missing value to `args: []` rather than refusing the entry. Judging
// whether a given command needs arguments is the entry author's call.
args?: string[];That is your suggested wording — optional, installable, installMcp normalises to [], and no installer-refusal claim left to contradict. The matching stale comment on the mcp (args absent) contract row was fixed in the same commit.
No change made. Since this has now surfaced three times against text that has not existed for several heads, it is worth checking which ref the review is reading — I would rather flag that than keep replying to a resolved thread.
… unknown rendering
Fixing the npm probe left the same defect in two other places. All three are
the same mistake: a failed inspection returned the value that means "we looked
and found nothing".
1. readRegisteredServerKeys returned [] for a missing config, an unreadable
one, an unparseable one, and a readable-but-empty one alike. Its own doc
comment said "absence of evidence, not evidence of absence" and then shipped
exactly that conflation. A malformed ~/.cursor/mcp.json therefore made
`dkg integration installed` report an MCP entry as "not installed" even when
the server block was sitting in the file it could not parse. It now returns
a ServerKeyProbe; a missing file stays a real answer (nothing registered),
while unreadable/malformed becomes ok:false. detectInstalled maps a slug
absent from every readable config to 'unknown' when any config was
unreadable, and keeps positive evidence authoritative when one client
registers it and another cannot be read.
2. The installed command rendered every 'unknown' row as "install kinds the
CLI does not perform". After the npm fix that is actively wrong: a cli entry
on a machine without npm is a supported detectable kind whose probe failed,
and the blanket message sends the user to fix the wrong thing. Rows now
render their own `detail`.
3. The schema-contract suite's npm-global row carried full npmGlobal data, so a
validator regressing to require it would still pass. Added the schema-minimal
`{ kind: 'service', runtime: 'npm-global' }` and docker equivalents, which
are the rows that actually pin the boundary.
Also corrected two comments claiming installMcp refuses an args-less entry; it
normalises to `args: []`.
The rewritten probe tests assert both directions per case. The previous
'without conflating it with a parse error' test compared both outcomes to []
and so could not have detected the conflation it was named for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every existing test stopped at a helper. The wiring between them lives only in
commands.ts — argument parsing, tier defaults, and which envelope key each verb
prints — so the exact regressions review called out would have left the whole
suite green: a `search` that ignored its keyword, or an `installed` that
printed `{ entries }` and looked like a registry listing to any consuming
script.
These drive the real Commander tree against the real local registry server
already used elsewhere in this file, redirected through the environment
because commands.ts resolves its config from process.env. Fixtures are
`manual` entries so detectInstalled performs no npm or filesystem I/O and the
assertions stay deterministic while still running the true command path.
Covered: search keyword filtering (with the unfiltered control, so a search
returning nothing cannot pass vacuously), list's unchanged { entries, failures }
envelope, installed's distinct { installed, failures }, and the deliberate
difference in default tier — verified for browsing, community for "what is on
my machine", which must not hide a community-tier install the user has.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| * that as "not installed": it would tell a user an integration is missing when | ||
| * the truth is that their config could not be read. | ||
| */ | ||
| export function readRegisteredServerKeys(target: ClientTarget): ServerKeyProbe { |
There was a problem hiding this comment.
🟡 Issue: Move MCP client config discovery out of the 2k-line setup command module
What's wrong
This adds a new public detection API to an already oversized command implementation file. The result is architectural drift: a registry integration feature now reaches into mcp-setup.ts internals because the reusable concept, MCP client config discovery/parsing, does not have its own home. That makes future client-format changes harder to reason about and keeps growing a file that is already well past a healthy size boundary.
Example
Adding another MCP client format now means touching the same large mcp-setup.ts command module and relying on detection code to import from that command-oriented file. That makes setup, parsing, writing, and install detection more coupled than they need to be.
Suggested direction
Use a dedicated MCP client config module as the canonical layer for client discovery and config parsing. mcp-setup.ts should orchestrate setup, while integration detection should reuse the same config module without importing from the setup command file.
For Agents
Extract the MCP client target/config primitives from packages/cli/src/mcp-setup.ts into a focused module such as mcp-client-configs.ts. Move ClientTarget, detectClients, readConfigBody, path helpers, and readRegisteredServerKeys there, then have both mcp-setup.ts and integrations/detect-installed.ts depend on that module. Preserve existing setup output and the new installed-detection behavior; the existing MCP config tests should continue to prove JSON/TOML/container handling.
There was a problem hiding this comment.
Third time you have raised this, so let me be concrete about the disposition rather than repeating "deferred".
I agree with the diagnosis, including the framing that the reusable concept has no home and that is what pulled a registry feature into a command module. The behavioural half you identified — the probe collapsing failure into absence — is fixed and tested. What remains is pure code motion: ClientTarget, detectClients, readConfigBody, the path helpers, and readRegisteredServerKeys into mcp-client-configs.ts, with mcp-setup.ts and detect-installed.ts both importing it.
My reason for not doing it in this PR is risk asymmetry, not disagreement. mcp-setup.ts is ~2k lines with substantial internal coupling between those helpers and the setup/write paths; moving readConfigBody and the path helpers touches code this PR otherwise does not go near, in a change whose diff would be almost entirely motion. Mixing that into a schema-conformance fix makes both harder to review, and a mistake there breaks dkg mcp setup — a surface with no relation to the bug being fixed.
Concretely: I would rather land this and do the extraction as its own PR where the diff is reviewable as motion and mcp setup behaviour can be verified in isolation. If maintainers prefer it here I will do it — it is mechanical, and I would rather carry the noise than leave a thread genuinely unresolved. Your call; I will not raise the deferral again.
There was a problem hiding this comment.
🟡 Issue: Do not make the setup action module the shared MCP config library
What's wrong
The new helper is generic MCP config inventory logic, but it is being added to and exported from a very large command/action file. That couples dkg integration installed to the setup command’s implementation details and keeps growing an already oversized module instead of separating the reusable MCP client-config abstraction.
Example
A maintainer trying to change how installed MCP integrations are detected now has to open the full setup command implementation and understand unrelated setup orchestration just to modify client-config inventory parsing.
Suggested direction
Move ClientTarget, detectClients, entry-path helpers, config body parsing, and readRegisteredServerKeys behind a dedicated MCP client config module, then import that module from both setup and installed detection.
For Agents
Extract MCP client target discovery and config read/write primitives from packages/cli/src/mcp-setup.ts into a focused module such as mcp-client-configs.ts. Keep behavior identical: mcpSetupAction should still register the same targets, and detectInstalled should still read the same containers and preserve unreadable-vs-empty probe states. Existing mcp-registered-server-keys tests should move with the new module.
There was a problem hiding this comment.
My answer is unchanged from the thread above, where I said I would not re-raise the deferral — so, briefly, and then I will stop arguing it.
I agree with the diagnosis. The blocker is scope, and I checked it rather than assumed: detectClients pulls in claudeDesktopPaths, clineMcpPaths, vscodeMcpPaths, isWSL, and resolveWslWindowsEnvPath, so the extraction is not five symbols — it is the whole client-target and WSL path-resolution surface, moved out of a ~2k-line file whose setup and write paths depend on the same helpers. That is a change whose diff should be reviewable as motion, with dkg mcp setup verified in isolation, and mixing it into a schema-conformance fix makes both harder to review and risks a surface unrelated to the bug.
The behavioural half you cared about is done and tested: the probe distinguishes unreadable from empty, and now also distinguishes ours from foreign. The remaining ask is purely where the code lives.
Standing offer, unchanged: if a maintainer says do it here, I will. Absent that I will land this and do the extraction as its own PR, carrying mcp-registered-server-keys.test.ts with it as you suggest.
There was a problem hiding this comment.
🟡 Issue: MCP integration detection is bolted onto an oversized setup module
What's wrong
readRegisteredServerKeys is a general MCP config parser, but it is being added as another export on mcp-setup.ts. That widens a command-oriented, already very large file instead of creating a clean shared boundary for MCP client config handling.
Example
Adding or changing a client config format now requires touching a module that mixes CLI setup orchestration, client discovery, JSON/TOML parsing, registration writing, skill delivery, and installed-integration detection support.
Suggested direction
Split MCP client config discovery/parsing into a small canonical module used by both mcp setup and integration detection.
Confidence note
This is a structural recommendation; the current behavior may be correct, but the added public API worsens an already oversized module boundary.
For Agents
Extract the shared MCP client config model and IO helpers into a focused module, for example mcp-client-config.ts. Move ClientTarget, detectClients, entry-path helpers, read/write config-body dispatch, and registered-server reading there; keep mcp-setup.ts as setup orchestration and have detect-installed.ts import the shared config module.
There was a problem hiding this comment.
Eleventh raise; position unchanged and the offer still stands unconditionally on a maintainer's word. Every behavioural defect on this helper is fixed; what remains is which file it lives in. Follow-up PR unless a maintainer says otherwise.
There was a problem hiding this comment.
🟡 Issue: Extract MCP client config parsing instead of expanding mcp-setup.ts
What's wrong
The new helper is useful, but it further turns an already oversized setup/orchestration file into a shared MCP config parsing library. That couples integration detection to a 2k+ line command module and makes future client-format changes harder to reason about.
Example
Adding a new MCP client format/container now requires understanding detectClients, classify, readConfigBody, readRegisteredServerKeys, and write paths inside one very large command module.
Suggested direction
Move the shared MCP config model and parsing helpers behind a dedicated module, then let both setup and integration detection reuse that smaller boundary.
For Agents
Extract MCP client config primitives from packages/cli/src/mcp-setup.ts into a focused module: ClientTarget, entry-path helpers, config body read/write, classification, and registered-server enumeration. Keep mcp-setup.ts as orchestration and have detect-installed.ts import the focused reader. Existing mcp setup and registered-server tests should continue to pass.
There was a problem hiding this comment.
🟡 Issue: Move MCP config inspection out of the monolithic setup module
What's wrong
This change adds a general-purpose integration-detection reader inside the already sprawling MCP setup file. That makes a new feature depend on a command orchestration module and exposes internal setup concepts as public API, which worsens module boundaries and makes future MCP config changes harder to reason about.
Example
detect-installed.ts imports detectClients, readRegisteredServerKeys, ClientTarget, RegisteredMcpServer, and ServerKeyProbe from ../mcp-setup.js, even though its dependency is really MCP client config discovery/parsing, not the setup action.
Suggested direction
Make MCP client config parsing the shared abstraction, rather than exporting more surface from mcp-setup.ts. That would reduce the size pressure on an already oversized file and put the reusable concept in the layer that actually owns it.
For Agents
Extract MCP client target/config parsing into a focused module, for example mcp-client-configs.ts, containing ClientTarget, target detection, readConfigBody, entry-path traversal, and registered-server enumeration. Have both mcp-setup.ts and integrations/detect-installed.ts depend on that module while preserving existing setup behavior and detection outputs.
There was a problem hiding this comment.
🟡 Issue: Extract MCP config inspection instead of growing the setup module
What's wrong
The new detection helper is reusable config parsing, not setup orchestration, but it is added inside an already very large setup module and exported from there. That deepens the module’s “god file” role and couples integration detection to setup internals rather than a canonical MCP config boundary.
Example
A future change to MCP config containers now has to reason through a 2k+ line setup command file even when the caller only needs to inspect registered servers for dkg integration installed.
Suggested direction
Pull the reusable MCP config primitives out of mcp-setup.ts and have both setup and integration detection consume that smaller abstraction.
For Agents
Move MCP client config modeling/parsing into a focused module, for example src/mcp-config.ts, containing ClientTarget, entry-path helpers, body read/parse, and registered-server extraction. Keep mcp-setup.ts as orchestration/write behavior and update detect-installed.ts to depend on the focused config module. Existing MCP setup behavior should remain unchanged.
There was a problem hiding this comment.
Twelfth raise; unchanged. Every behavioural defect on this helper is fixed in this PR — unreadable-vs-empty, unlaunchable blocks, foreign blocks, unfilled placeholders, per-client judgement, malformed args, and array containers. What remains is which file it lives in.
Scope is the blocker and it is measured: detectClients pulls the four client-path builders plus isWSL and resolveWslWindowsEnvPath, and readConfigBody shares format dispatch with the write path. It wants a diff reviewable as motion with dkg mcp setup verified in isolation.
Offer unchanged and unconditional on a maintainer's word.
| console.log(`Run \`dkg integration info ${entry.slug}\` for the full entry.`); | ||
| break; | ||
| } | ||
| case 'service': { |
There was a problem hiding this comment.
🟡 Issue: Centralize install-kind/runtime policy instead of scattering service special cases
What's wrong
The PR adds service support by bolting the service/npm-global distinction into several unrelated places. This keeps the behavior correct locally, but the install model is now spread across command wiring, detection, validation, and display. That is exactly the kind of branching growth that makes the next install runtime harder to add cleanly.
Example
To add automated docker service support, a contributor would need to find and update at least the command dispatcher, service installer assertion, detection package mapping, human formatter, schema validator, and tests. Missing any one of those leaves a partial model.
Suggested direction
Create a canonical install capability layer, even if it is just a small table or a set of shared helpers like npmGlobalPackageForInstall, installHandlerFor, and formatInstallSpec. The top-level command should dispatch to that layer instead of carrying runtime-specific branching itself.
Confidence note
The current behavior is small enough to work, but the diff establishes the pattern for every future install kind/runtime, so the maintainability risk is visible now.
For Agents
Look across packages/cli/src/integrations/commands.ts, detect-installed.ts, install-cli.ts, install-service.ts, and schema.ts. Introduce a small install-kind/runtime model or handler table that owns installability, display, npm-global package extraction, and detection support per kind/runtime. Preserve the existing CLI output and install behavior; add a focused test proving service npm-global still installs and detects through the shared path.
There was a problem hiding this comment.
Agreed as a direction, and your confidence note frames the tradeoff correctly: the behaviour is small enough to work today, and the concern is that this diff sets the pattern for every future kind/runtime.
Your list of touch points is accurate — command dispatcher, service installer assertion, detection package mapping, human formatter, schema validator, tests. Worth noting one of them is already centralized: globalNpmPackageFor in detect-installed.ts is exactly the npmGlobalPackageForInstall helper you describe, and it is why adding npm-global service detection was a two-line change rather than a sixth branch. So the model you are proposing is partly there; what is missing is installHandlerFor and formatInstallSpec.
I am not doing it in this PR, for the same reason as the discriminated-union thread and with the same caveat that it should be one pass rather than three partial ones: the handler table wants to be cut against the schema's real per-runtime payloads, and those types are currently wrong in about ten places (catalogued on the schema.ts thread). Building a dispatch table on top of types that misdescribe the contract would bake the divergence into the abstraction — the more expensive mistake.
The sequence I would propose: land this PR's behavioural fixes, do type conformance against the published schema, then introduce the capability layer on top of types that are actually correct. Happy to file that as a tracked follow-up so it does not evaporate.
There was a problem hiding this comment.
🟡 Issue: Service install policy is split between the command layer and installer
What's wrong
The new service case makes commands.ts understand the service runtime matrix and the schema-valid-but-not-automatable npmGlobal shape. That same invariant also exists inside install-service.ts. This spreads install-kind policy across layers and makes future service runtimes or metadata changes harder to reason about.
Example
The service branch rejects non-npm-global runtimes and missing npmGlobal.package/version, then installService.assertNpmGlobalService repeats those gates. Adding docker automation would require coordinating the dispatcher branch, the installer assertion, and command output handling.
Suggested direction
Centralize the service runtime/package-metadata decision behind the service installer boundary, then have the Commander switch delegate instead of peeking into service internals.
For Agents
Move service installability into packages/cli/src/integrations/install-service.ts as a typed plan/result, such as installable npm-global vs not-automated with a message. Keep commands.ts responsible for invoking and rendering outcomes while preserving current exit codes, dry-run behavior, and graceful fallback tests.
There was a problem hiding this comment.
🟡 Issue: Non-npm service install fallback is not covered at the command layer
What's wrong
The schema change makes docker and binary service entries readable, and this branch is now the public install behavior for those entries. The tests cover parser acceptance and npm-global service installs, but they do not drive dkg integration install through the docker/binary service path, so a regression from the intended graceful fallback to a generic failed install would still be green.
Example
A fixture like { install: { kind: 'service', runtime: 'binary', binary: { url: 'https://example.com/svc' } } } should make dkg integration install <slug> exit with code 2 and print the non-automated runtime guidance. If the dispatcher accidentally routed that entry into installService and surfaced generic Install failed, the current tests would still pass.
Suggested direction
Extend the command dispatch tests with at least one non-npm service runtime fixture so the newly reachable graceful fallback path cannot regress unnoticed.
For Agents
Add a Commander-layer install test in packages/cli/test/integrations.test.ts for a service entry with runtime: 'binary' or runtime: 'docker'; keep the schema-readable behavior, assert exit code 2, and assert the message names the runtime and points to the integration instructions without the generic install failure.
There was a problem hiding this comment.
Same answer as the classifier thread, and the gap has narrowed since it was first raised.
Install dispatch now gates on complete npmGlobal.package/version rather than runtime alone (edfff9a07), and assertNpmGlobalService type-checks the same fields (4b7f60145), so the command layer and the installer no longer disagree about what "installable" means — they now enforce the same predicate, just at two depths, which is defence in depth rather than split policy.
What is still true is that the predicate is written twice instead of living in one classifier. That is the installHandlerFor piece I want to do against corrected types rather than the ~10 divergent ones catalogued above, for the reason given there: a dispatch table built on types that misdescribe the registry bakes the divergence into the abstraction.
Sequence unchanged — behavioural fixes, then schema conformance, then the capability layer.
There was a problem hiding this comment.
🟡 Issue: Centralize service installability instead of duplicating it in the dispatcher
What's wrong
The command dispatcher is accumulating install-kind policy that the installer already owns. That makes the public command path more branchy and gives the codebase two subtly different definitions of which service entries are installable.
Example
The command layer says "automatable npm-global service" means truthy npmGlobal.package/version; installService says it means non-empty strings after trimming. A future service metadata rule has to be changed in both places, and TypeScript cannot carry the invariant through the dispatcher.
Suggested direction
Move the runtime/package-metadata classification behind a single classifyServiceInstall or resolveNpmGlobalService helper that returns either a typed installable spec or an unsupported reason. Then commands.ts can render the reason or call installService without knowing the internal payload rules.
For Agents
Look at the service branch in commands.ts and assertNpmGlobalService in install-service.ts. Preserve the current graceful non-automated paths, but expose a typed classifier/resolver from the service installer layer so the Commander action dispatches on a single result instead of rechecking nested schema fields.
There was a problem hiding this comment.
Taken, and your example was a live bug rather than only a design concern — thanks for making it concrete enough to check. Fixed in 73e54aad7 and 3b614bf2b.
You were exactly right that the two layers disagreed: the dispatcher gated on truthiness, assertNpmGlobalService required non-empty strings after trimming. That gap is reachable:
{ package: ' ', version: '1.0.0' }
parses as a valid entry: true
passes the dispatcher's gate: true
passes the installer's trim gate: false
So a registry-valid entry slipped past the dispatcher, threw inside installService, and surfaced as a generic Install failed with exit 1 — the precise failure the dispatcher gate was added to prevent, arriving through the seam between two checks that were supposed to mean the same thing.
I took the minimal form of your suggested direction rather than the full classifier: resolveNpmGlobalService(install) is now exported from the installer and returns the trimmed payload or null, and both the Commander branch and assertNpmGlobalService call it. One definition, and TypeScript carries the resolved payload into the install path.
Two things worth noting. First, re-reading it I found a residual you had not flagged: installService still destructured the raw payload, so the resolver's trimming never reached npm — "@acme/svc " would have been installed with the space intact, making the trim decorative exactly where it has to hold. Both the npm spec and the start-command fallback now derive from the resolver's output.
Second, mutation-verified rather than assumed: disabling the trim check fails the new command-level test with expected [ 1 ] to deeply equal [ 2 ] — the exit-code sequence showing the fall-through into the generic path.
This does not build the full capability layer, and I still think that wants correct types underneath it. But it removes the duplication you have been pointing at for four rounds instead of only documenting it.
There was a problem hiding this comment.
🟡 Issue: The changed integration info display path has no regression test
What's wrong
This PR makes previously unreadable schema-valid entries parse and updates printEntryHuman to display them safely, but the public info command path is not exercised. That leaves a user-facing crash or stale output in info undetected while helper tests still pass.
Example
A regression back to e.install.args.join(' ') would still pass the new schema and installMcp tests, but dkg integration info <slug> would throw for a schema-valid MCP entry like { kind: 'mcp', command: 'my-mcp-server', supportedClients: ['cursor'] } instead of printing (no args declared).
Suggested direction
Drive dkg integration info <slug> through the real Commander command for the schema shapes whose display changed, so schema-compatible entries remain readable in the public info view.
For Agents
Add a Commander-level integration info human-output test in packages/cli/test/integrations.test.ts using the local registry server. Include at least an MCP entry without args; ideally also cover the changed manual docs and service runtime display branches. Assert the command exits cleanly and the expected text is printed.
There was a problem hiding this comment.
Valid as a coverage gap, and I want to be precise that it is a gap rather than a live defect — the distinction matters for whether it blocks.
printEntryHuman already handles the args-less case safely:
` command: ${e.install.command}${e.install.args?.length ? ` ${e.install.args.join(' ')}` : ' (no args declared)'}`Optional chaining, so { kind: 'mcp', command: 'my-mcp-server', supportedClients: ['cursor'] } prints (no args declared) rather than throwing. Your regression scenario — someone reverting to e.install.args.join(' ') — is real and would not be caught, which is the actual point.
So: no crash today, no test guarding it tomorrow. That is worth closing, and info is the one public verb this PR touched without command-level coverage (search, list, installed, and install all have it). Going into the follow-up alongside the extractions, covering the args-less MCP entry plus the changed manual and service display branches.
There was a problem hiding this comment.
🔴 Bug: --api-url is ignored for npm-global service installs
What's wrong
This PR adds automated npm-global service installs and a service-specific apiUrl option so post-install instructions can point the service at the selected DKG node. The Commander dispatch never passes that option, so users who choose a non-default node get instructions for the wrong endpoint.
Example
Run dkg integration install svc-ok --api-url http://10.0.0.5:9200 for an npm-global service with envRequired: ['DKG_API_URL']. Current behavior installs the package, then prints DKG_API_URL — default http://127.0.0.1:9200; expected behavior is to print http://10.0.0.5:9200, matching the user-selected node.
Suggested direction
Forward the parsed opts.apiUrl into installService in the service branch, the same way the MCP branch forwards it to installMcp.
For Agents
Look in packages/cli/src/integrations/commands.ts service install dispatch. Preserve the existing dry-run/provenance behavior, pass opts.apiUrl through to installService, and add a Commander-level test proving install <npm-global service> --dry-run --api-url <custom> renders the custom DKG_API_URL.
Centralize install dispatch instead of hand-wiring another special-case branch
What's wrong
The install command is accumulating feature-specific branches and duplicated orchestration as new install kinds are added. That structure makes the command file the owner of too many policies and forces every shared install option or output convention to be remembered in each arm.
Example
The command-level install options include apiUrl, and InstallServiceOptions also has apiUrl, but the new service branch manually forwards only entry, dryRun, and skipProvenance. That is a symptom of each switch arm owning its own option plumbing.
Suggested direction
Collapse the per-kind switch bodies behind focused install handlers or a typed outcome model so option forwarding, not-automated handling, dry-run messaging, and post-instruction rendering are defined once.
Confidence note
This is a structural concern inferred from the new service branch and the new InstallServiceOptions.apiUrl boundary; the behavior itself should be evaluated by the business-logic lens.
For Agents
Introduce a small install-kind dispatcher with handlers that accept a common InstallContext (entry, dryRun, apiUrl, provenance setting, logger) and return a typed outcome such as installed/manual/notAutomated plus post-instructions. Keep commands.ts responsible for fetching, trust gating, and rendering outcomes; keep installability checks in the handlers or shared resolvers.
Service --api-url behavior is only verified below the public command
What's wrong
The added test gives false confidence about --api-url: it proves the helper can render a supplied URL, but not that the shipped dkg integration install command passes the option through. Since this is user-facing CLI behavior, a green suite can still miss the real command printing the default node URL.
Example
A regression test should drive the public path: dkg integration install svc-ok --dry-run --api-url http://10.0.0.5:9200 for a service with envRequired: ['DKG_API_URL'] should assert the output contains http://10.0.0.5:9200. That test would currently expose that the command wiring never forwards opts.apiUrl.
Suggested direction
Add a command-level regression test for install <npm-global service> --dry-run --api-url ... and assert the selected URL reaches the service post-install guidance.
For Agents
In packages/cli/src/integrations/commands.ts, check the service branch of install <slug>. Preserve the helper behavior in install-service.ts, forward the command option, and add a Commander-layer test in packages/cli/test/integrations.test.ts for service --api-url guidance.
…utomatable services
Three findings from review, all on surfaces this PR introduced.
1. False POSITIVE in MCP detection, the mirror of the false negatives fixed
last commit. readRegisteredServerKeys returned Object.keys() verbatim, so a
key counted as a registration whatever its value: `{ mcpServers: { x: null } }`
reported x as installed though nothing there can launch. The module had
already settled this ten lines away — classify() treats `{ dkg: null }` as
not-registered, with a comment recording that reading it as `stale` was
wrong — so the reader contradicted an invariant its own file established.
Keys are now filtered to non-null, non-array objects. Deliberately
'not installed' rather than 'unknown': the config read fine and the entry is
explicitly not a registration, which is the same conclusion classify reaches.
2. `install` dispatched to the automated service path on runtime alone. The
schema requires only kind + runtime for a service, so
`{ kind: 'service', runtime: 'npm-global' }` is valid, readable, and not
automatable — it reached installService, threw, and surfaced as a generic
"install failed", worse than the docker/binary path which exits cleanly
pointing at the integration's docs. The gate now also requires a complete
npmGlobal.package/version and falls back with its own message, since the
runtime IS supported here and only the metadata is missing. Fixed by
gating rather than by tightening the validator, which would have made the
CLI stricter than the published schema — the defect this PR removes.
3. npmGlobal.binary is now optional in the type, matching the schema and
resolveBinary's long-standing fallback. The invariant previously lived in a
comment, the fallback, and test casts — everywhere except the type.
Commander coverage extended to the install branches: manual prints its docs
link without exiting, an npm-global service dry-run reaches the installer with
its pin, and the unautomatable service exits 2 with no `undefined` in the
message. The service fixture omits npmGlobal.binary so the optional-binary path
is exercised through the command, not only in the installer unit test.
Note on the harness: stubbing process.exit to THROW is wrong here — the command
wraps its switch in try/catch, so the throw is caught and converted to exit(1),
collapsing every asserted exit code to 1. It records the first code instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nd tree The header still listed only list/info/install and claimed "only cli, mcp are implemented" — written before this PR added search, installed, manual, and npm-global service support. Same defect class review flagged on the MCP `args` comment: prose describing an invariant the code no longer holds, which invites someone to restore the invariant rather than the prose. Now states automation per kind, including the condition that makes an npm-global service automatable at all (package metadata the schema does not require). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Detection treated any object stored under the entry's slug as proof the
integration was wired in. A user with `{ "mcpServers": { "buzz-dkg": { "command":
"npx", "args": ["-y", "other-package"] } } }` was told buzz-dkg was installed
while their client would launch something else entirely — a substituted or
stale server hidden behind a reassuring green row. An empty object under the
slug counted too.
The probe now carries the blocks rather than only their names, so the caller
can compare command and args against what the entry declares. A block with no
`command` string is not a registration at all and is dropped alongside the
null/scalar/array cases fixed last commit.
A slug registered with a DIFFERENT command reports 'unknown', not
'not installed'. Neither of the confident answers is honest: 'installed' would
start the wrong server, and 'not installed' would hide a name collision the
user needs to know about. The detail says which client holds the conflicting
registration.
This is the third distinct way this one surface reported something it had not
established — unreadable configs read as absent, unlaunchable blocks read as
present, and now foreign blocks read as ours. The probe type is what makes each
of them expressible rather than collapsed into a bare list.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…; flag unfilled MCP env
Three review findings, plus one refuted.
1. isValidInstallSpec and assertNpmGlobalService accepted npmGlobal by
TRUTHINESS, so `{ package: { name: '@acme/svc' }, version: '1.0.0' }` parsed
and the installer would have run `npm install --global [object Object]@1.0.0`.
Both now type-check the fields the CLI consumes. This is schema-CONSISTENT
strictness, not the stricter-than-schema drift this PR removes: payload
objects stay optional (a bare `{ kind, runtime }` still parses), but the
schema marks their own fields required when the object is present. Docker,
binary, envRequired and portsOpened get the same treatment.
2. resolveBinary fell back to the full package name, so a scoped package
printed `Start it with: @acme/svc` — not a runnable command, since a global
install of `@acme/svc` puts `svc` on PATH. The same class of unusable
guidance as the `undefined` this fallback was added to fix. It now drops the
scope, and an explicit `binary` still wins.
3. MCP detection reported a matching block as installed even when it still
carried installMcp's `<NAME>` placeholders — registered, but it will start
unauthenticated. That now reports 'unknown' naming the unfilled keys.
Deliberately NOT requiring every envRequired key to be present: MCP clients
can supply env from the parent process, so a user keeping secrets out of the
config file is correctly installed. An unfilled placeholder is unambiguous;
an absent key is not, and demanding presence would manufacture the false
negative the last four rounds removed.
Also replaced `__dirname` with an import.meta.url-derived path. Review called
this a runtime failure that stopped the fixture assertions executing; that part
is not correct — vitest supplies `__dirname` and the test passes today, verified
by running it in isolation. But the package is `"type": "module"` and relying on
the runner's shim is a real trap for anyone executing the file another way, so
the hardening is worth taking on its own merits.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e malformed args Both of these are regressions in the previous commit, caught in review. 1. The placeholder check aggregated across every matching client, so one stale block hid a real install: a filled Cursor config plus an old Windsurf block still carrying `<DKG_AUTH_TOKEN>` reported the whole entry as 'unknown'. That is the same false negative the four preceding rounds removed, reintroduced by the fix for the round before. Matching registrations are now split into complete and incomplete: one complete block means installed, and the incomplete siblings are named in the detail rather than overriding positive evidence. 2. Filtering non-string elements out of `args` rewrote `args: [123]` into `[]`, which then matched an args-less registry entry — manufacturing an install from a block that does not declare the entry's launch shape. `args` is now `string[] | null`: `[]` means the key was absent, which is a legitimate args-less server, and `null` means present-but-not-a-string-array, which cannot match anything. Normalizing evidence before comparing it was the underlying mistake; the type now makes the two cases inexpressible as one. Also adds the Commander-layer test for a non-npm service runtime. Making docker and binary services readable turned that branch into their public install behaviour, so a regression routing them into installService would surface a generic "Install failed" (exit 1) rather than the graceful runtime-not-automated message, and no helper test would have noticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // $defs (minimal shapes the registry can merge); the vendored cases are | ||
| // verbatim copies of live entries. The first set is what catches drift. | ||
|
|
||
| describe('registry ↔ CLI contract', () => { |
There was a problem hiding this comment.
🟡 Issue: Split this test file before it grows past 1k lines
What's wrong
The PR pushes an already broad test file over the 1,000-line threshold by adding several unrelated suites. That makes the test surface harder to navigate and raises the cost of future changes around integrations.
Example
A reader looking for existing installMcp tests now has to scan registry-schema compatibility, fixture parsing, Commander JSON envelopes, process.exit interception, and install dispatch cases in one file.
Suggested direction
Move the newly added registry contract and Commander-layer suites into dedicated test modules instead of appending another large section to the existing integration test file.
For Agents
Split the new coverage into focused files such as integrations-registry-contract.test.ts, integrations-commands.test.ts, and the existing MCP installer tests. Share only small fixture builders where needed, and keep the current behavioral assertions unchanged.
There was a problem hiding this comment.
Reasonable, and I am not going to do it in this PR — with a reason specific to timing rather than a general objection.
The navigability point is fair: that file now holds registry-schema compatibility, fixture parsing, Commander envelopes, exit-code interception, and install dispatch, and your proposed split (integrations-registry-contract, integrations-commands, MCP installer) is the right seam.
Two things argue for doing it after this lands rather than inside it. First, it is a pure-motion change across ~1k lines of test at the point where a human reviewer is being asked to check behavioural fixes; a diff that moves everything makes the actual changes much harder to see. Second, this PR is on its seventh review round and the file is still being edited — three of the last four rounds added or changed suites in it, so splitting now means splitting a moving target and then rebasing motion against further edits.
Worth noting one structural constraint for whoever does it: the Commander-layer suites depend on the local registry HTTP server and the registryRoutes/localRegistryCfg harness defined in that file, so the split needs a shared fixture module rather than three independent copies — otherwise the "one broken entry must not hide the others" behaviour gets tested against three drifting servers. Flagging it because it is the part most likely to go wrong.
Happy to open it as a follow-up alongside the other two extractions; they are all the same kind of change and would review well together.
There was a problem hiding this comment.
🟡 Issue: This PR pushes integrations.test.ts past the 1k-line decomposition boundary
What's wrong
The PR adds roughly 400 lines to an already broad integration test file, taking it from under 1k lines to over 1k. That is exactly the sprawl threshold where discoverability and local ownership start degrading, especially because the added suites cover distinct concerns.
Example
This single test file now contains registry client tests, schema contract tests, MCP installer tests, args-less MCP tests, and real Commander wiring tests. A future change to integration command behavior has to scan a thousand-line file to find the relevant suite.
Suggested direction
Decompose the new suites before merging so each file has one reviewable concern.
For Agents
Split the newly added suites into focused files, for example integrations-registry-contract.test.ts and integrations-commands.test.ts, with shared fixture builders moved to a small test helper if needed. Preserve the existing assertions and local registry setup behavior.
There was a problem hiding this comment.
Unchanged from the previous round: agreed on the seam, declining on timing, because it is a ~1k-line motion diff placed in front of a human reviewer who is being asked to check behavioural fixes — and the file gained another suite this round, so it is still moving.
Repeating the one constraint for whoever does it, since it is the part most likely to break: the Commander-layer suites depend on the local registry HTTP server and the registryRoutes/localRegistryCfg harness defined in that file. The split needs a shared fixture module, not three independent servers, or "one broken entry must not hide the others" ends up asserted against three drifting harnesses.
Happy to bundle it with the other two extractions as a single follow-up; they are the same kind of change and would review well together.
There was a problem hiding this comment.
🟡 Issue: Split this before letting the test file cross 1k lines
What's wrong
This PR takes a file from well under 1k lines to over 1k lines, and the added tests cover several different ownership areas. That makes the file harder to navigate and turns unrelated future changes into edits against the same sprawling test module.
Example
A future change to only the Commander wiring now has to scan past registry validator fixtures and MCP args-less installer cases in the same 1k+ line file, even though those are separable concerns.
Suggested direction
Decompose the newly added test sections into focused files instead of growing this already broad integration test file past the 1000-line boundary.
For Agents
Split the new sections out of packages/cli/test/integrations.test.ts: put registry/schema contract coverage in a registry-contract test file, Commander command wiring in an integrations-commands test file, and keep MCP installer cases near the existing MCP installer tests. Preserve the same assertions and shared fixture behavior.
There was a problem hiding this comment.
Unchanged — agreed on the seam, deferring on timing, bundled with the other extractions in the follow-up. The shared registryRoutes/localRegistryCfg harness is the constraint to respect when splitting; three independent servers would quietly weaken the "one broken entry must not hide the others" assertion.
There was a problem hiding this comment.
🟡 Issue: Split the new tests before this file crosses 1k lines
What's wrong
The PR appends roughly 440 lines to a test file that was already about 620 lines, pushing it past the 1k-line threshold. The added suites cover distinct concerns, so keeping them together creates avoidable sprawl rather than a cohesive test module.
Example
This file now contains registry client tests, installMcp tests, registry/schema contract tests, args-less MCP tests, and Commander wire tests in one place.
Suggested direction
Move the registry contract and Commander-layer suites into dedicated test files so each file has a clear ownership boundary.
For Agents
Split the added sections into focused files, for example integrations-registry-contract.test.ts and integrations-commands.test.ts, leaving the existing installMcp tests where they are or moving the new args-less cases next to them. Preserve shared fixtures through a small test helper if needed.
There was a problem hiding this comment.
🟡 Issue: Split the added integration tests before this file stays over 1k lines
What's wrong
This PR pushes a previously sub-1k test file past the 1k-line threshold by appending several unrelated test concerns. Even though the tests are useful, keeping all of them in one fixture-heavy file makes the integration test surface harder to scan and turns this file into the default dumping ground for future registry work.
Example
The file now contains registry-client tests, schema validation tests, installCli/installMcp tests, vendored registry fixture checks, Commander command wiring, and install-dispatch tests in one 1k+ line module.
Suggested direction
Decompose the new suites by responsibility instead of continuing to grow integrations.test.ts; this keeps future registry, installer, and Commander changes from all landing in the same large file.
For Agents
Split the added suites into focused files such as integrations-schema-contract.test.ts, integrations-commands.test.ts, and an MCP installer-specific test file. Move shared baseEntry/registry-server fixtures into a small helper so behavior stays unchanged while each file owns one concern.
There was a problem hiding this comment.
🟡 Issue: Split the integration tests before letting the file sprawl past 1k lines
What's wrong
This PR pushes an integration test file past the 1k-line threshold by appending several unrelated suites. That makes future changes harder to scan and increases the chance that command, registry, and installer fixtures become tangled together.
Example
The new registry-contract tests, args-less MCP tests, and Commander command tests are independent concerns, but they now share one sprawling file with local registry setup, command helpers, installer fixtures, and parser contract cases all interleaved.
Suggested direction
Move the newly added registry-contract and Commander-layer suites into focused test modules, reusing shared fixtures/helpers where needed.
For Agents
Split the added suites into focused files such as integrations-registry-contract.test.ts, integrations-mcp-install.test.ts, and integrations-commands.test.ts. Preserve the same assertions and shared fixture setup, but keep each file under a single concern.
There was a problem hiding this comment.
Unchanged — agreed on the seam, deferring on timing, and the file gained two more suites this round (the --api-url command coverage), so it is still moving.
The constraint for whoever splits it: the Commander suites depend on the local registry HTTP server and the registryRoutes/localRegistryCfg harness in that file. It needs a shared fixture module, not three independent servers.
Bundled with the extractions in the follow-up.
…alidates Review caught an inconsistency the previous commit created rather than inherited: adding docker.version validation to the runtime guard while InstallService.docker never declared `version`. A type that disagrees with its own validator is worse than either being wrong alone — a future docker installer would be told the field exists by one and not the other, and the resolution would be a cast, which is how the original drift got in. InstallService.docker now mirrors installService.docker exactly: image and version required, digest and composeUrl optional. `ports` and `env` are removed; they had no readers anywhere in src or test, and the schema sets additionalProperties: false, so no registry entry could ever have carried them. The same invented `env` is removed from npmGlobal. Typecheck passes with no new casts, which is the evidence nothing depended on the fictional fields. Also narrows the divergence list this PR has been tracking by three entries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… registry failures in `installed` 1. `typeof [] === 'object'`, so an array server container slipped past the malformed-container check and fell through to Object.entries([]), reporting "readable, nothing registered" for a container we cannot interpret. The comment immediately below already listed arrays among the values that cannot be a registration — that rule was applied at the entry level and missed at the container level. This is the fourth distinct way this surface reported a config it had failed to read as a config it had read. 2. `dkg integration installed` destructured `failures`, reported them under --json, and never printed them in human mode, so the summary counted only the entries it could parse and gave no hint any were skipped. `list` and `search` already warn about these; the same evidence now appears in both places, noting that skipped entries were not considered for detection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found that the npm-global fallback test could pass while the behaviour it describes regressed. The exit spy records rather than throws — throwing is caught by the command's own try/catch and converted to exit(1), collapsing every asserted code to 1 — but the cost is that execution CONTINUES past a simulated exit. Remove the `break` after process.exit(2) and the command prints the graceful message, falls into installService, throws, and prints "Install failed"; the test still saw exit === 2 and the graceful string, and passed. The negative assertion that catches this was already present eight lines below, on the svc-binary case, and simply not applied to svc-bare. Fixed structurally rather than per test: the spy now records EVERY exit code and the fallback tests assert `exits` is exactly [2]. More than one entry means the real process would already have terminated and the test is observing code that could never run, so any future test using this harness inherits the check instead of relying on someone remembering a string assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…service
The Commander dispatcher and the installer had two predicates for the same
question and they disagreed. The dispatcher gated on truthiness; the installer
required non-empty strings after trimming. A registry-valid
`{ package: ' ', version: '1.0.0' }` therefore parsed, passed the dispatcher,
threw inside installService, and surfaced as a generic "Install failed" with
exit 1 — the exact failure the dispatcher gate was added to prevent, reachable
through the gap between two checks that were meant to mean the same thing.
`resolveNpmGlobalService` is now the single definition, returning the trimmed
payload or null, and both layers call it. Review has flagged this duplication
for several rounds; taking the minimal form of their suggestion fixes a live
inconsistency and removes the divergence rather than only documenting it.
Covered by a command-level test asserting the whitespace case exits exactly
[2] with the graceful message and no generic failure.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…load installService destructured entry.install.npmGlobal directly, so the trimming resolveNpmGlobalService performs never reached npm: `"@acme/svc "` would have been installed with the trailing space intact. The trim was decorative at the one place it has to hold. Both the npm spec and the start-command fallback now derive from the resolver's output. Found while re-reading the previous commit rather than reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // `installed` reports something different from `list`/`search`, so it prints a | ||
| // different key. Printing `{ entries }` here would silently look like a | ||
| // registry listing to any script consuming it. | ||
| it('`installed --json` prints { installed, failures }, never { entries }', async () => { |
There was a problem hiding this comment.
🔴 Bug: installed --json test can pass with raw registry entries instead of detection rows
What's wrong
The new command’s JSON contract is that installed contains detection results from detectInstalled. The current tests do not verify the row shape or state, so they give false confidence that the command is wired to detection rather than just echoing registry entries under a different key.
Example
If installed --json accidentally printed JSON.stringify({ installed: candidates, failures }, null, 2), these tests would still pass: the top-level keys are still installed and failures, installed is still an array, and gamma-tool is still present. The regression would only show up for consumers expecting detection fields like state, kind, and detail.
Suggested direction
Assert the installed rows’ behavioral contract, not only the envelope key and array-ness.
For Agents
Strengthen the Commander-layer installed --json tests in packages/cli/test/integrations.test.ts to assert detection row shape and behavior, for example that manual rows have { slug, kind: 'manual', state: 'unknown', detail: ... } and do not look like raw registry entries with install/name. Preserve the no-npm setup by using manual entries or by making the command dependency injectable if npm-backed rows are needed.
There was a problem hiding this comment.
🟡 Issue: installed command tests avoid the detectable paths
What's wrong
The new installed subcommand is mostly about detecting cli/npm-global/MCP integrations on the local machine, but the command-layer tests intentionally keep all detectable kinds out of the registry index. The helper is well covered in isolation, yet the public command wiring can regress while these tests still pass, giving false confidence in the behavior users and scripts actually call.
Example
A regression in installed that still printed { installed, failures } but failed to pass detectable npm/MCP candidates through to detectInstalled would leave the current Commander tests green, because every indexed row is manual and expected to be unknown. A useful failing-test sketch is to put a cli or npm-global service fixture in /index, prepend a fake npm executable that returns {"dependencies":{"@acme/cli":{"version":"1.0.0"}}}, then assert dkg integration installed --json contains that slug with state: "installed".
Suggested direction
Exercise one detectable cli/service or MCP row through the real Commander command instead of indexing only manual entries.
For Agents
In packages/cli/test/integrations.test.ts, add at least one Commander-level installed --json case whose registry index includes a detectable install kind. Keep it hermetic by using a temp PATH fake for npm ls -g and/or temp HOME/DKG_HOME MCP config, then assert the public command returns the installed state and detail. Preserve the helper-level detectInstalled unit cases; this should cover the wiring boundary they currently bypass.
There was a problem hiding this comment.
Valid, and it is the highest-value item left — I want to say why rather than just deferring it.
You are right, and I documented the tradeoff myself in that fixture: "Only the manual entries are indexed … no test here ever shells out to npm." Keeping npm out kept the suite hermetic, and the cost is exactly what you describe — a regression that stopped passing detectable candidates to detectInstalled would leave every Commander row unknown and the tests green.
This matters more than a normal coverage nit because the same class produced a live bug on this PR an hour ago: --api-url was accepted, rendered correctly by installService, unit-tested, and never passed through by the command. The helper test passed; the CLI was broken. Below-the-boundary green is precisely what I have been caught by, so I am not going to argue this one down.
Not doing it in this PR, for a reason specific to how it has to be built. There is no injection seam: the installed action calls detectInstalled(candidates) with no deps, so covering the wiring needs either a PATH fake for npm ls -g (platform-sensitive — a shell shim on Linux CI and an npm.cmd on Windows, where I am running) or a temp HOME with a client config, which drags in detectClients' WSL and %APPDATA% path resolution. Adding platform-specific test scaffolding as the last act of a twelve-round review is how a flaky test gets shipped, and this PR has already had two CI flakes from unrelated timing-sensitive tests.
So: recorded as the FIRST follow-up, ahead of the extractions, with your failing-test sketch as the spec — index a cli or npm-global fixture, fake npm ls -g returning {"dependencies":{"@acme/cli":{"version":"1.0.0"}}}, assert installed --json reports that slug installed. That is a better description of the test than I would have written, and it goes in the issue verbatim.
There was a problem hiding this comment.
Filed as #1998, with your failing-test sketch as the spec — index a detectable fixture, fake the npm ls -g probe, assert installed --json reports the slug as installed.
The issue records why it was not done here (no injection seam; it needs either a platform-sensitive PATH fake or a temp HOME that drags in detectClients' WSL and %APPDATA% resolution), and carries an acceptance box for a mutation check — stop passing detectable candidates to detectInstalled and confirm the new test fails. It also picks up the integration info command-level gap from the sibling thread, since both are the same boundary.
Flagged there as the first follow-up ahead of the extractions, for the reason on this thread: the identical class produced a live --api-url bug on this PR, so below-the-boundary green is the failure mode with a track record here.
…e resolver install uses Two review findings, both live on the current head. 1. `--api-url` is documented as "wire into integrations" and installMcp honours it, but the service dispatch never passed it and buildPostInstructions hardcoded the default. `dkg integration install svc --api-url http://10.0.0.5:9200` therefore printed setup guidance pointing at 127.0.0.1 — the same flag working for mcp entries and silently ignored for services. 2. Detection read the raw npmGlobal payload while install used the trimmed resolver, so the two disagreed about the same entry: `" @acme/svc "` installs as `@acme/svc` but detection probed for the padded key and reported "not installed", and a whitespace-only package took the graceful not-automatable path on install while detection still claimed it checked npm and found nothing. resolveNpmGlobalService moves to schema.ts, which is where it should have gone when it was first extracted. Three callers need it — the Commander dispatcher, the installer, and detection — and every time it has lived in only one of them the others have drifted. This is the third bug from that duplication in three commits: dispatcher-vs-installer, installer-vs-its-own-raw-payload, and now detector-vs-installer. Fixing instances one caller at a time was the mistake. Detection now returns 'unresolvable' for a service the installer would refuse and reports 'unknown' rather than asserting an npm check that could never match. Five tests, each paired with a control: api-url echoed vs default rendered, padded package installing the trimmed spec, padded service detected by its trimmed name, and an unresolvable service reported unknown rather than absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit claimed this fix and did not contain it. installService gained the apiUrl option and rendered it, and its unit test passed — but the Commander action never passed the flag through, so the real CLI still printed the default node. The multi-line edit that was supposed to add it silently no-opped: this file is CRLF, and the search string was \n-joined. The script reported success because a separate single-line import edit in the same run did apply. Two lessons encoded here rather than remembered: - The unit test could not have caught it. It calls installService directly with apiUrl, which is below the boundary where the omission lives. The new test drives `integration install svc-ok --dry-run --api-url http://10.0.0.5:9200` through the real command tree and would have failed against the previous head. postInstructions print on dry-run, so the guidance is reachable without spawning npm. - The control test caught a second thing: Commander gives --api-url a default, so opts.apiUrl is always populated and the command path always echoes the EFFECTIVE node. The literal "default …" wording is only reachable by direct installService callers passing no apiUrl. The control now asserts what the command genuinely prints. Also reports `Installed <pkg>@<version>` from the resolver's trimmed values rather than the raw payload, which is not what the npm spec was built from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
validate.mjsupstream were silently dropped as "unreadable" — indkg integrationand in the node dashboard sidebar, which share the parser viafetchAllEntries().manualrequired aninstall.stepsfield the schema forbids (additionalProperties: false) while ignoring the requireddocsUrl;stepshas never existed in the schema, so nomanualentry could ever be read.mcprequired the schema-optionalargs;servicerejected the schema'sbinaryruntime. Fourmanualentries are affected today, including the merged,verified-tierbuzz-dkg.cliandmcp— precisely the two compatible kinds. This adds a contract test (one schema-valid fixture per install kind + vendored live entries + a check that every schema-defined kind is covered) and records the invariant in-code: the CLI validator must never be stricter than the published schema. It may be more lenient, so an older CLI can still read a newer registry.manualinstalls now hand off instead of erroring. Per CONTRIBUTING §2manualmeans "the installer links out to your docs" — a success path — so it printsdocsUrl+oneLiner+ the security declaration and exits 0. It was previously grouped with genuinely unimplemented kinds and exited 2.infoalso rendersmanualand theservicebinaryruntime, both of which printed nothing.search/listsplit, matching the registry README and npm/apt convention:searchdiscovers the registry (whatlistused to do, plus a keyword filter);listreports what is installed here, derived from the install targets rather than a ledger.installMcpwrites nothing — it prints a block for the user to paste — so a ledger would claim installs that never happened; detection reports what is true. Undetectable kinds reportunknown, never "not installed".installServiceforruntime: npm-global, mirroringinstallCliincluding the provenance gate, unblocking the one pendingserviceentry (dkg-integrations#9).docker/binarykeep the explicit not-implemented message.Related
publicInterfacesUsedguidance, drops the redundantcursor-mcp-dkgentry. Independent; needs no CLI release.tracabot, the only pendingserviceentry)agent-docs/plans/2026-07-28-dkg-integrations-cli-contract.mdDiagrams
Reading a registry entry
Before — a schema-valid
manualentry never survives the parser:sequenceDiagram participant User participant CLI participant Registry participant Parser as isIntegrationEntry User->>CLI: dkg integration list CLI->>Registry: GET integrations/*.json Registry-->>CLI: buzz-dkg (kind: manual, docsUrl) CLI->>Parser: validate Parser-->>CLI: false (no install.steps) CLI-->>User: "Skipped 1 unreadable registry entry"After — the parser matches the schema:
sequenceDiagram participant User participant CLI participant Registry participant Parser as isIntegrationEntry User->>CLI: dkg integration search CLI->>Registry: GET integrations/*.json Registry-->>CLI: buzz-dkg (kind: manual, docsUrl) CLI->>Parser: validate Parser-->>CLI: true CLI-->>User: buzz-dkg [verified] Buzz DKG IntegrationInstalling a
manualintegrationBefore — exits 2 pointing at the repo root:
sequenceDiagram participant User participant CLI participant Entry User->>CLI: dkg integration install buzz-dkg CLI->>Entry: read install.kind Entry-->>CLI: manual CLI-->>User: "not yet supported by the CLI" (exit 2)After — hands off to the entry's own docs:
sequenceDiagram participant User participant CLI participant Entry User->>CLI: dkg integration install buzz-dkg CLI->>Entry: read install.kind Entry-->>CLI: manual + docsUrl + oneLiner CLI-->>User: docs URL, one-liner, security declaration (exit 0)Files changed
packages/cli/src/integrations/schema.tsisValidInstallSpecto the published schema (manual→docsUrl,mcp.argsoptional,servicebinaryruntime); fix the matching TS types; record the never-stricter invariantpackages/cli/src/integrations/install-mcp.tsargskeyJSON.stringifydropspackages/cli/src/integrations/install-service.tsinstallServiceforruntime: npm-global, mirroringinstallCliincl. the provenance gate, with daemon-oriented post-install guidancepackages/cli/src/integrations/detect-installed.tsnpm ls -g; MCP client configs keyed on slug); injectable deps so tests spawn nothingpackages/cli/src/integrations/commands.tssearch(registry + keyword) andlist(installed here);manualhand-off exits 0;inforendersmanualandservice.binary; dispatchservice→installServicepackages/cli/test/integrations.test.tsinstallMcpargs guardpackages/cli/test/fixtures/registry/*Test plan
npx vitest run test/integrations.test.ts→ 54 passednpx tsc -p tsconfig.json --noEmit→ no errors insrc/integrations/manualrule toisStringArray(v.steps)→ 3 contract tests fail (manual (docsUrl only),manual (+ oneLiner),parses every vendored live registry entry), mutation confirmed still on disk at exit; restored and re-verified greendkg integration search→buzz-dkg [verified]now listed (was "Skipped 1 unreadable registry entry")dkg integration search buzz→ 1 matchdkg integration info buzz-dkg→ rendersdocs:andsummary:(previously printed nothing formanual)dkg integration install buzz-dkg; echo $?→ docs hand-off, exit 0dkg integration list→ reportsbuzz-dkgasunknown (install kind the CLI does not perform), not "not installed"GET /api/integrations/registry) now returnsbuzz-dkg— same parser, no code change expected on that pathDeferred deliberately:
upgrade/uninstall(must undo an install, incl. MCP client configs the CLI never wrote),agent-pluginandservicedocker/binary installers, and tighteningmcp.argsto required in schema v0.2.0.🤖 Generated with Claude Code