change structure for github install#46
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRepository-root plugin manifests and MCP wiring replace generated artifacts, core and Pi package names are renamed, canonical skills move to ChangesRoot plugin distribution and skill workflow update
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/plugin-generators.mjs (1)
151-173: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGenerated recovery commands still use the old package name.
This template still bakes in
@nodesource/nsolid-plugin, while the renamed CLI path now usesnsolid-plugin(packages/core/scripts/setup.mjs, Line 37).scripts/materialize-github-marketplace.mjsonly patchesSETUP_COMMAND, so the unreadable-credentials branch at Line 173 will still tell users to run the old logout command.Suggested fix
-const SETUP_COMMAND = 'npx -y `@nodesource/nsolid-plugin` setup --harness ${harness}' +const SETUP_COMMAND = 'npx -y nsolid-plugin setup --harness ${harness}' ... - fail(\`NodeSource credentials are unreadable. Run: npx -y `@nodesource/nsolid-plugin` logout && \${SETUP_COMMAND}. \${err.message}\`) + fail(\`NodeSource credentials are unreadable. Run: npx -y nsolid-plugin logout && \${SETUP_COMMAND}. \${err.message}\`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/plugin-generators.mjs` around lines 151 - 173, The recovery message in the credentials error path still references the old package name, so update the SETUP_COMMAND template in plugin-generators.mjs and the unreadable-credentials branch in readCredentials() to use the renamed nsolid-plugin CLI path consistently. Make sure the generated logout/setup guidance matches the current command shape used by the setup materialization flow, so both the missing-credentials and unreadable-credentials fail() messages point to the same updated package name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.mcp.json:
- Line 7: The wrapper selection logic in the `.mcp.json` server entries
currently falls back to the first discovered `scripts/mcp-wrapper.js`, which can
execute an untrusted local file from the workspace. Update the discovery flow
used by the affected server config blocks to require a trusted match (for
example, only the `nsolid-plugin` cache path) and fail closed with the existing
error message when no approved wrapper is found. Keep the change aligned across
the duplicated server entries so the same trusted-only selection applies
everywhere.
In `@packages/core/package.json`:
- Around line 25-26: The packaging hooks in package.json can run chmodSync on
dist/src/cli.js before dist exists because build is only in prepublishOnly,
which does not run for npm pack. Move the build step into prepack (or prepare)
alongside skills:sync and bundle:sync so packaging always generates fresh dist
output first, and keep the chmodSync command after the build in the same hook.
In `@packages/core/src/index.ts`:
- Line 50: The Pi plugin package name constant only recognizes the new package
name, so existing installs using the legacy alias will be missed during
discovery. Update the Pi setup/CLI flow and package resolution in the symbols
around PI_PLUGIN_PACKAGE_NAME and any discovery logic to accept or migrate
`@nodesource/pi-plugin` alongside nsolid-pi-plugin. If needed, add temporary alias
handling or rewrite persisted settings/metadata so old installations continue to
load correctly.
In `@packages/pi-plugin/test/manifest.test.ts`:
- Around line 29-30: The publishable-package test in manifest.test.ts is too
strict because it only accepts pkg.private being undefined, but a manifest with
private set to false is also publishable. Update the assertion in the manifest
test so it checks that pkg.private is not true, using the existing pkg.private
expectation in the publishable-package test.
In `@README.md`:
- Around line 97-98: The README npx examples are using the scoped package name,
but the published package is unscoped, so the setup commands will fail for fresh
installs. Update the examples that invoke npx to use the unscoped nsolid-plugin
package name instead, and make the same change anywhere the README references
the `@nodesource/nsolid-plugin` command so the examples match the package name
used by package.json and packages/core/package.json.
In `@scripts/materialize-github-marketplace.mjs`:
- Around line 65-77: The success log in materialize-github-marketplace.mjs is
using the wrong collection property: buildExpectedFiles() returns a Map, so
expectedFiles.length will be undefined. Update the log in the non-CHECK_MODE
branch to use the Map’s size instead, keeping the message in sync with the
expectedFiles value returned by buildExpectedFiles().
In `@scripts/mcp-wrapper.js`:
- Line 33: The unreadable-credentials recovery message in mcp-wrapper’s fail
path still references the old package name, so update the logout hint to use the
renamed nsolid-plugin command instead of `@nodesource/nsolid-plugin`. Make the
change in the fail call that builds the NodeSource credentials error so the
suggested recovery steps match the new package name and users can rerun setup
successfully.
- Line 11: The setup hint in SETUP_COMMAND is missing the Agents option, so
update the recovery guidance to include agents alongside claude, codex, and
antigravity. Keep the change localized to the setup command string in the
wrapper so the printed remediation message matches the supported harnesses and
the new Agents marketplace.
- Around line 108-127: Remove the runtime npx fallback in mcp-wrapper.js so
startup does not depend on network/npm availability when mcp-remote is missing
locally. Update the wrapper logic around createRequire,
require.resolve('mcp-remote/dist/proxy.js'), and the spawn('npx', ...) path so
MCP launch only uses an installed/bundled mcp-remote, and surface a clear
failure if it is absent rather than fetching mcp-remote@0.1.38 at runtime.
In `@scripts/plugin-generators.mjs`:
- Around line 119-120: The fallback in generateCodexBootstrap is too broad
because it uses candidates[0] when no nsolid-plugin path is found, which can
import another plugin’s mcp-wrapper.js. Update the wrapper selection logic so it
only proceeds when a candidate can be positively identified as this plugin’s
install root (for example, via the existing nsolid-plugin path check or another
explicit root validation), and otherwise fail with the existing error path
instead of guessing.
- Around line 119-120: The generated bootstrap string in generateCodexBootstrap
currently contains a raw ${path.sep} sequence that trips
no-template-curly-in-string. Update the string construction so it no longer
embeds a template-like placeholder inside a normal quoted literal, or apply a
very narrow lint disable comment at the specific spot with a clear codegen
justification. Use generateCodexBootstrap and the wrapper candidate selection
logic as the location reference when making the change.
In `@scripts/sync-plugin-assets.mjs`:
- Line 149: The sync-assets path list currently includes packages/core, which
conflicts with the new packages/core/skills contract and can cause the directory
to be deleted or recreated depending on script order. Update the sync logic in
sync-plugin-assets.mjs to stop targeting the entire packages/core tree and
instead exclude packages/core/skills from deletion/copy operations so it remains
stable alongside sync-shared-skill-assets.mjs. Keep the fix localized around the
path handling in the asset sync routine and the packages/core entry in the
existing list.
---
Outside diff comments:
In `@scripts/plugin-generators.mjs`:
- Around line 151-173: The recovery message in the credentials error path still
references the old package name, so update the SETUP_COMMAND template in
plugin-generators.mjs and the unreadable-credentials branch in readCredentials()
to use the renamed nsolid-plugin CLI path consistently. Make sure the generated
logout/setup guidance matches the current command shape used by the setup
materialization flow, so both the missing-credentials and unreadable-credentials
fail() messages point to the same updated package name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46233648-cd33-497a-b911-a866da7d4e4e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (79)
.agents/plugins/marketplace.json.claude-mcp.json.claude-plugin/marketplace.json.claude-plugin/plugin.json.codex-plugin/plugin.json.gitignore.mcp.jsonREADME.mddocs/antigravity-research.mddocs/pi-mcp-research.mddocs/plugin-marketplace-research.mddocs/qa-guide.mdmcp_config.jsonpackage.jsonpackages/core/README.mdpackages/core/package.jsonpackages/core/scripts/setup.mjspackages/core/scripts/sync-shared-skill-assets.mjspackages/core/src/cli.tspackages/core/src/index.tspackages/core/test/integration/cli-help.test.tspackages/core/test/integration/installer.test.tspackages/core/test/integration/plugin-assets.test.tspackages/core/test/unit/skills/fetch-asset.test.tspackages/pi-plugin/README.mdpackages/pi-plugin/index.jspackages/pi-plugin/package.jsonpackages/pi-plugin/test/manifest.test.tsplugin.jsonplugins/templates/antigravity/README.mdplugins/templates/antigravity/package.jsonplugins/templates/antigravity/scripts/install.jsplugins/templates/claude/README.mdplugins/templates/claude/package.jsonplugins/templates/codex/README.mdplugins/templates/codex/package.jsonscripts/build-plugin-artifacts.mjsscripts/materialize-github-marketplace.mjsscripts/mcp-wrapper.jsscripts/plugin-generators.mjsscripts/sync-plugin-assets.mjsscripts/test-marketplace-install.jsskill-assets/fetch-asset.cjsskill-assets/save-report.cjsskill-assets/wait.cjsskills/ns-advanced-memory-leak-hunter/SKILL.mdskills/ns-advanced-memory-leak-hunter/fetch-asset.cjsskills/ns-advanced-memory-leak-hunter/save-report.cjsskills/ns-advanced-memory-leak-hunter/wait.cjsskills/ns-analyze-asset/SKILL.mdskills/ns-analyze-asset/save-report.cjsskills/ns-analyze-asset/wait.cjsskills/ns-analyze-cpu/SKILL.mdskills/ns-analyze-cpu/fetch-asset.cjsskills/ns-analyze-cpu/save-report.cjsskills/ns-analyze-cpu/wait.cjsskills/ns-analyze-cpu/workspace-delta.cjsskills/ns-analyze-event/SKILL.mdskills/ns-analyze-event/save-report.cjsskills/ns-analyze-memory/SKILL.mdskills/ns-analyze-memory/fetch-asset.cjsskills/ns-analyze-memory/save-report.cjsskills/ns-analyze-memory/wait.cjsskills/ns-analyze-tracing/SKILL.mdskills/ns-analyze-vulnerabilities/SKILL.mdskills/ns-audit-dependencies/SKILL.mdskills/ns-audit-dependencies/collect-dependencies.cjsskills/ns-benchmark-run/SKILL.mdskills/ns-benchmark-run/save-report.cjsskills/ns-benchmark-run/wait.cjsskills/ns-benchmark-validate/SKILL.mdskills/ns-benchmark-validate/save-report.cjsskills/ns-benchmark-validate/wait.cjsskills/ns-generate-asset/SKILL.mdskills/ns-generate-sbom/SKILL.mdskills/ns-node-upgrade/SKILL.mdskills/ns-node-upgrade/fetch-node-releases.cjsskills/ns-replace-package/SKILL.mdskills/ns-upgrade-package/SKILL.md
💤 Files with no reviewable changes (8)
- plugins/templates/codex/README.md
- scripts/build-plugin-artifacts.mjs
- plugins/templates/antigravity/README.md
- plugins/templates/claude/README.md
- plugins/templates/antigravity/package.json
- plugins/templates/claude/package.json
- plugins/templates/antigravity/scripts/install.js
- plugins/templates/codex/package.json
Cesar-M-Diaz
force-pushed
the
cesar/github-install-test
branch
from
e191ec2 to
9b5ee88
Compare
Cesar-M-Diaz
force-pushed
the
cesar/github-install-test
branch
from
7262016 to
57d2d70
Compare
doctor only read the CLI tracking file (~/.agents/.nodesource-installed.json), written by the direct install path. For plugin/package-owned harnesses the recommended path is the native mechanism (codex/claude/agy plugin install, pi install), which records state in harness-specific locations the doctor never inspected — so it reported everything as missing even when `codex plugin list` showed installed, enabled. Add an optional detectNativePlugin() to HarnessAdapter and implement it per harness (codex config.toml, claude installed_plugins.json, antigravity staged dir, pi package). When a native plugin is present, skills/MCPs are reported green from the plugin and a new Plugin line shows status + install hint. Health still allows the direct fallback install on plugin-owned harnesses.
Cesar-M-Diaz
requested review from
dobleuber,
edsadr,
jdaison and
mafesernaarboleda
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/qa-guide.md`:
- Around line 162-173: The QA guide hardcodes the contributor branch reference
in the plugin marketplace install commands, which will become stale if that
branch changes. Update the affected command examples in the guide to use a
placeholder or environment variable instead of the literal
`cesar/github-install-test` ref, and keep the existing `claude plugin
marketplace add` and `claude plugin install` examples aligned so they all refer
to the same configurable symbol.
- Line 115: Fix the typo in the QA guide step text by changing the word “firts”
to “first” in the instruction about installing the opencode mcp and skills.
Update the affected sentence in the documentation content so it reads naturally
and correctly, and keep the rest of the step unchanged.
In `@packages/core/src/auth/auth-manager.ts`:
- Around line 76-82: The validationUnavailable fallback in auth-manager.ts is
returning existing credentials before any required-permissions enforcement,
which can let commands proceed during outages without local authz checks. Update
the auth-manager flow around validateToken and checkRequiredPermissions so the
cached existing credentials are only reused when existing.permissions satisfies
the required permissions, and fail closed if permissions are missing or
insufficient instead of returning early.
In `@packages/core/src/harnesses/claude-adapter.ts`:
- Around line 76-78: The install check in ClaudeAdapter is incorrectly treating
a marketplace clone as a native plugin install, which causes doctor to report
Claude as installed too early. Update the logic in the Claude adapter’s install
detection path so only a real native install sets status.installed, and do not
flip it to true just because the marketplace clone exists. Keep the marketplace
repository existence check separate from the native install status in the
relevant status-building code.
- Around line 85-100: The top-level array path in extractPluginIds currently
only preserves string entries, so arrays of objects with id fields are dropped.
Update extractPluginIds in claude-adapter.ts so the Array.isArray(data) branch
also accepts objects shaped like { id: string } and returns their ids, matching
the existing handling for obj.plugins and preventing false negatives from
installed_plugins.json.
In `@packages/core/src/harnesses/codex-adapter.ts`:
- Around line 47-72: The installation and enabled state handling in
codex-adapter is conflating marketplace registration with plugin installation,
and it also ignores the installed-but-disabled plugin case. Update the status
logic in the codex adapter so the plugin entry under PLUGIN_KEY drives
status.installed regardless of whether enabled is true, while status.enabled is
set only from the plugin’s enabled flag. Keep marketplace presence and the clone
fallback separate from the plugin install signal, using the existing
data.plugins, data.marketplaces, and MARKETPLACE_NAME checks in the codex
adapter flow.
In `@packages/core/src/index.ts`:
- Around line 613-641: The native plugin handling in the doctor/report flow is
treating any detected install as native-owned, even when the plugin is disabled.
Update the logic around adapter.detectNativePlugin() and nativeOwned in
packages/core/src/index.ts so the fast path only applies when the detected
plugin is actually enabled; if enabled is explicitly false, do not mark bundle
skills or MCP servers as satisfied and keep them on the normal tracking path.
In `@packages/core/src/utils/format.ts`:
- Around line 20-21: The Antigravity install hint in format.ts is inconsistent
with the CLI guidance in cli.ts because it omits the .git suffix. Update the
install string returned for the antigravity case in format.ts to match the exact
agy plugin install URL used by the CLI so Doctor and install both suggest the
same native command.
- Around line 47-51: The plugin status rendering in format.ts is treating every
non-error status as installed, which causes disabled plugins to appear active.
Update the logic in the plugin formatting path around the status check so that
`report.plugin.enabled === false` is rendered as a disabled/not active state
instead of reusing the `✓ installed` branch. Keep the existing behavior for
truly installed/enabled plugins in the `line('Plugin', ...)` formatting, and
make sure the `plugin` status output clearly distinguishes enabled from disabled
using the existing symbols and helpers.
In `@scripts/mcp-wrapper.js`:
- Around line 53-54: The MCP URL selection in the wrapper is using a derived
value first, which can override the persisted endpoint and break the
auth/install contract. Update the `mcp-wrapper` URL resolution so
`credentials.mcpUrl` is preferred whenever present, and only fall back to
`deriveMcpUrlFromConsoleUrl(credentials.consoleUrl)` if no stored MCP URL
exists. Keep the change localized to the `derivedUrl`/`url` assignment logic so
`nsolid-console` continues to use the canonical stored endpoint when available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4eaf2091-e7e9-4470-93ce-89ec0ecf458e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (91)
.agents/plugins/marketplace.json.claude-mcp.json.claude-plugin/marketplace.json.claude-plugin/plugin.json.codex-plugin/plugin.json.gitignore.mcp.jsonREADME.mddocs/antigravity-research.mddocs/pi-mcp-research.mddocs/plugin-marketplace-research.mddocs/qa-guide.mdmcp_config.jsonpackage.jsonpackages/core/README.mdpackages/core/package.jsonpackages/core/scripts/setup.mjspackages/core/scripts/sync-shared-skill-assets.mjspackages/core/src/auth/auth-manager.tspackages/core/src/cli.tspackages/core/src/harnesses/antigravity-adapter.tspackages/core/src/harnesses/claude-adapter.tspackages/core/src/harnesses/codex-adapter.tspackages/core/src/harnesses/harness-adapter.tspackages/core/src/harnesses/index.tspackages/core/src/harnesses/pi-adapter.tspackages/core/src/harnesses/pi-plugin-detector.tspackages/core/src/index.tspackages/core/src/types.tspackages/core/src/utils/format.tspackages/core/test/integration/auth/auth-manager.test.tspackages/core/test/integration/cli-help.test.tspackages/core/test/integration/installer.test.tspackages/core/test/integration/plugin-assets.test.tspackages/core/test/unit/skills/fetch-asset.test.tspackages/core/test/unit/utils/format.test.tspackages/pi-plugin/README.mdpackages/pi-plugin/index.jspackages/pi-plugin/package.jsonpackages/pi-plugin/test/manifest.test.tsplugin.jsonplugins/templates/antigravity/README.mdplugins/templates/antigravity/package.jsonplugins/templates/antigravity/scripts/install.jsplugins/templates/claude/README.mdplugins/templates/claude/package.jsonplugins/templates/codex/README.mdplugins/templates/codex/package.jsonscripts/build-plugin-artifacts.mjsscripts/materialize-github-marketplace.mjsscripts/mcp-wrapper.jsscripts/plugin-generators.mjsscripts/sync-plugin-assets.mjsscripts/test-marketplace-install.jsskill-assets/fetch-asset.cjsskill-assets/save-report.cjsskill-assets/wait.cjsskills/ns-advanced-memory-leak-hunter/SKILL.mdskills/ns-advanced-memory-leak-hunter/fetch-asset.cjsskills/ns-advanced-memory-leak-hunter/save-report.cjsskills/ns-advanced-memory-leak-hunter/wait.cjsskills/ns-analyze-asset/SKILL.mdskills/ns-analyze-asset/save-report.cjsskills/ns-analyze-asset/wait.cjsskills/ns-analyze-cpu/SKILL.mdskills/ns-analyze-cpu/fetch-asset.cjsskills/ns-analyze-cpu/save-report.cjsskills/ns-analyze-cpu/wait.cjsskills/ns-analyze-cpu/workspace-delta.cjsskills/ns-analyze-event/SKILL.mdskills/ns-analyze-event/save-report.cjsskills/ns-analyze-memory/SKILL.mdskills/ns-analyze-memory/fetch-asset.cjsskills/ns-analyze-memory/save-report.cjsskills/ns-analyze-memory/wait.cjsskills/ns-analyze-tracing/SKILL.mdskills/ns-analyze-vulnerabilities/SKILL.mdskills/ns-audit-dependencies/SKILL.mdskills/ns-audit-dependencies/collect-dependencies.cjsskills/ns-benchmark-run/SKILL.mdskills/ns-benchmark-run/save-report.cjsskills/ns-benchmark-run/wait.cjsskills/ns-benchmark-validate/SKILL.mdskills/ns-benchmark-validate/save-report.cjsskills/ns-benchmark-validate/wait.cjsskills/ns-generate-asset/SKILL.mdskills/ns-generate-sbom/SKILL.mdskills/ns-node-upgrade/SKILL.mdskills/ns-node-upgrade/fetch-node-releases.cjsskills/ns-replace-package/SKILL.mdskills/ns-upgrade-package/SKILL.md
💤 Files with no reviewable changes (8)
- plugins/templates/antigravity/package.json
- plugins/templates/antigravity/README.md
- plugins/templates/codex/README.md
- plugins/templates/claude/README.md
- plugins/templates/claude/package.json
- plugins/templates/antigravity/scripts/install.js
- scripts/build-plugin-artifacts.mjs
- plugins/templates/codex/package.json
✅ Files skipped from review due to trivial changes (10)
- plugin.json
- .codex-plugin/plugin.json
- .claude-plugin/plugin.json
- .claude-plugin/marketplace.json
- packages/pi-plugin/README.md
- .gitignore
- packages/core/test/integration/cli-help.test.ts
- docs/antigravity-research.md
- packages/core/README.md
- docs/plugin-marketplace-research.md
🚧 Files skipped from review as they are similar to previous changes (18)
- .claude-mcp.json
- packages/pi-plugin/index.js
- .agents/plugins/marketplace.json
- .mcp.json
- packages/core/test/unit/skills/fetch-asset.test.ts
- package.json
- docs/pi-mcp-research.md
- packages/pi-plugin/test/manifest.test.ts
- mcp_config.json
- packages/core/package.json
- packages/core/scripts/setup.mjs
- packages/core/scripts/sync-shared-skill-assets.mjs
- scripts/materialize-github-marketplace.mjs
- packages/core/test/integration/plugin-assets.test.ts
- packages/core/src/cli.ts
- scripts/test-marketplace-install.js
- scripts/plugin-generators.mjs
- scripts/sync-plugin-assets.mjs
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bundle.json`:
- Around line 66-70: The `ns-memory-spike-analysis` entry’s description is out
of sync with the skill’s actual workflow. Update the `description` field in the
`bundle.json` item for `ns-memory-spike-analysis` so it reflects baseline/peak
heap sampling and optional `track-heap-objects`, and no longer mentions taking
heap snapshots; keep the change localized to that skill metadata entry.
In `@packages/core/bundle.json`:
- Around line 66-70: The ns-memory-spike-analysis entry in bundle.json has an
outdated description that still refers to heap snapshots. Update the description
to match the current flow of baseline/peak heap sampling and optional
track-heap-objects, using the ns-memory-spike-analysis skill entry so users are
not directed to the wrong capture type.
In `@skills/ns-advanced-memory-leak-hunter/SKILL.md`:
- Around line 11-23: The polling steps in the baseline and peak workflow can
retry forever if asset-summary never becomes ready. Update the guidance around
the wait/retry loop in Phase 1 and Phase 3 to include a clear retry cap or
maximum wait threshold, and then surface the pending asset state instead of
continuing indefinitely. Use the existing baseline/peak polling instructions and
the asset-summary check to make the stop condition explicit.
In `@skills/ns-analyze-asset/SKILL.md`:
- Around line 36-39: The “No asset ID” guidance in SKILL.md is too strong about
resolving a full asset ID from the flat filename alone. Update the instructions
near the asset resolution flow to clarify that the filename pattern in
fetch-asset.cjs only provides an asset ID prefix, so full resolution must come
from .nsolid/assets/index.json when available; if index.json is missing and MCP
is unavailable, the user should be told the ID cannot be fully recovered. Refer
to the asset-summary and fetch-asset.cjs paths in the text so the recovery
behavior is unambiguous.
- Around line 19-34: The heap snapshot retry guidance in SKILL.md only treats a
few response states as not ready, but it should also cover pending/async states
from the companion capture flow. Update the heap snapshot summarization loop
under the async summarization section so the `asset-summary` check and retry
conditions in `assets-in-progress` treat `"pending"` and any async-style
response as not ready, and keep retrying with the existing `wait.cjs` loop
before analyzing. Ensure the instructions remain aligned with the existing
`asset-summary`, `assets-in-progress`, and `wait.cjs` workflow.
In `@skills/ns-analyze-tracing/SKILL.md`:
- Around line 9-16: Make the trace-data handling branch in SKILL.md exclusive:
when the user supplies trace tree/export or any host-provided trace data, do not
fall through to live enrichment via tracing or information-dashboard. Update the
logic around the trace-handling guidance and the connected-services discovery
steps so they are only used when no sufficient host data is present, keeping the
host-data path authoritative.
In `@skills/ns-audit-dependencies/SKILL.md`:
- Around line 9-10: The audit flow still allows N|Solid enrichment even when the
host has already provided grounded audit data. Update the instructions in the
audit step sequence so that the `Audit Results` path in `SKILL.md` explicitly
stops at the injected data and does not continue into any live enrichment or
re-fetch logic; keep the behavior aligned with the existing step 3/“skip to step
3” guidance and the related audit-analysis section.
In `@skills/ns-benchmark-run/reference/benchmark-inputs.md`:
- Around line 23-30: The benchmark example is still missing the `resp` input
even though `exampleFn` reads it via `JSON.parse(resp)`. Update the `args` and
`argSetupCode` in the benchmark input example to explicitly include `resp`
alongside `req`, `res`, and `arrExample`, so the transformed `exampleFn`
signature and setup match the actual identifiers used.
- Around line 60-71: The benchmark input is passing a string literal instead of
the array created in argSetupCode. Update the args for processEvents so it uses
the actual data array value defined in the setup, keeping the input shape
consistent with the function signature and the other setup symbols like
eventEmitter and callback.
In `@skills/ns-generate-asset/fetch-asset.cjs`:
- Around line 56-68: readAssetIndex() is swallowing all read/parse errors and
returning an empty array, which causes saveToAssetIndex() to overwrite a broken
or unreadable asset index. Update readAssetIndex() in fetch-asset.cjs to
distinguish a missing index from a failed read/parse, and only return [] when
index.json does not exist; on JSON parse or I/O errors, propagate or surface the
error so saveToAssetIndex() does not clobber existing entries. Use the
readAssetIndex() and saveToAssetIndex() flow to keep malformed
.nsolid/assets/index.json from being silently replaced.
In `@skills/ns-generate-asset/SKILL.md`:
- Around line 39-43: The CPU profile and heap sample readiness flow in Check
Readiness needs an explicit retry cap like the other branches. Update the
asset-summary retry logic for those asset types in SKILL.md to stop after a
fixed maximum (match the existing 12-retry pattern), then report the asset ID
and pending state instead of looping forever; use the existing readiness
sections and asset-summary handling as the place to make the change.
In `@skills/ns-memory-spike-analysis/fetch-asset.cjs`:
- Around line 56-68: readAssetIndex() is turning every missing/read/parse
failure into an empty array, which causes saveToAssetIndex() to overwrite a
broken .nsolid/assets/index.json with loss of data. Update readAssetIndex() and
the saveToAssetIndex() flow to distinguish “file missing” from “file present but
unreadable/malformed” using the existing symbols readAssetIndex,
saveToAssetIndex, and getAssetsDir; only return [] when the index truly does not
exist, and otherwise surface or preserve the failure so the bad index is not
silently clobbered.
In `@skills/ns-memory-spike-analysis/SKILL.md`:
- Around line 31-38: Add language tags to the fenced command examples in
SKILL.md so markdownlint MD040 passes; update the wait/fetch command blocks in
the `heap-sampling` and `snapshot` sections to use `bash` fences, and make the
same change in the other affected command block near the later example. Locate
the bare fences around the `wait.cjs` usage and convert them consistently
without changing the commands themselves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e6da098-a28d-4c97-b611-f48ca74cef88
📒 Files selected for processing (31)
.claude-plugin/plugin.jsonbundle.jsonpackages/core/bundle.jsonpackages/core/scripts/skill-assets.manifest.jsonpackages/pi-plugin/test/manifest.test.tsskills/ns-advanced-memory-leak-hunter/SKILL.mdskills/ns-analyze-asset/SKILL.mdskills/ns-analyze-asset/fetch-asset.cjsskills/ns-analyze-event/SKILL.mdskills/ns-analyze-tracing/SKILL.mdskills/ns-analyze-vulnerabilities/SKILL.mdskills/ns-audit-dependencies/SKILL.mdskills/ns-benchmark-run/SKILL.mdskills/ns-benchmark-run/reference/benchmark-inputs.mdskills/ns-cpu-spike-analysis/SKILL.mdskills/ns-cpu-spike-analysis/fetch-asset.cjsskills/ns-cpu-spike-analysis/wait.cjsskills/ns-cpu-spike-analysis/workspace-delta.cjsskills/ns-generate-asset/SKILL.mdskills/ns-generate-asset/fetch-asset.cjsskills/ns-generate-asset/wait.cjsskills/ns-generate-sbom/SKILL.mdskills/ns-memory-spike-analysis/SKILL.mdskills/ns-memory-spike-analysis/fetch-asset.cjsskills/ns-memory-spike-analysis/wait.cjsskills/ns-node-upgrade/SKILL.mdskills/ns-optimize-function/SKILL.mdskills/ns-replace-package/SKILL.mdskills/ns-upgrade-package/SKILL.mdskills/ns-validate-optimization/SKILL.mdskills/ns-validate-optimization/wait.cjs
✅ Files skipped from review due to trivial changes (4)
- skills/ns-validate-optimization/wait.cjs
- skills/ns-generate-sbom/SKILL.md
- skills/ns-upgrade-package/SKILL.md
- skills/ns-cpu-spike-analysis/workspace-delta.cjs
🚧 Files skipped from review as they are similar to previous changes (2)
- .claude-plugin/plugin.json
- packages/pi-plugin/test/manifest.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
skills/ns-cpu-spike-analysis/fetch-asset.cjs (1)
59-72: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake
writeAssetIndex()atomic before making corruption fatal.With the stricter reader, any partially written
index.jsonbecomes a hard failure on every later run.writeAssetIndex()still replaces the file in place, so an interrupted write can create exactly the malformed state this code now refuses to recover from.♻️ Proposed fix
function writeAssetIndex (workspaceRoot, records) { const indexPath = path.join(getAssetsDir(workspaceRoot), 'index.json') - fs.writeFileSync(indexPath, JSON.stringify(records, null, 2), 'utf-8') + const tempPath = `${indexPath}.tmp` + fs.writeFileSync(tempPath, JSON.stringify(records, null, 2), 'utf-8') + fs.renameSync(tempPath, indexPath) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/ns-cpu-spike-analysis/fetch-asset.cjs` around lines 59 - 72, `fetchAssetIndex()` now fails fast on malformed `index.json`, so `writeAssetIndex()` must stop writing in place and instead perform an atomic replace to avoid leaving a partially written file behind. Update `writeAssetIndex()` to write through a temporary file and then rename/replace it into `indexPath`, keeping the existing validation behavior in `fetchAssetIndex()` and preserving compatibility with `saveToAssetIndex()` callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/ns-advanced-memory-leak-hunter/fetch-asset.cjs`:
- Around line 59-72: `resolveExistingAsset()` is now blocked by the strict
`readAssetIndex()` behavior, so a malformed `index.json` prevents the legacy
fallback from being checked. Keep the strict validation for
`saveToAssetIndex()`, but change the lookup path to use a lenient read in
`fetch-asset.cjs` that treats unreadable/malformed index data as empty and
continues to the legacy asset probe. Use the existing `resolveExistingAsset()`
and `readAssetIndex()` symbols to separate read-only lookup behavior from
write-time enforcement.
---
Nitpick comments:
In `@skills/ns-cpu-spike-analysis/fetch-asset.cjs`:
- Around line 59-72: `fetchAssetIndex()` now fails fast on malformed
`index.json`, so `writeAssetIndex()` must stop writing in place and instead
perform an atomic replace to avoid leaving a partially written file behind.
Update `writeAssetIndex()` to write through a temporary file and then
rename/replace it into `indexPath`, keeping the existing validation behavior in
`fetchAssetIndex()` and preserving compatibility with `saveToAssetIndex()`
callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae1fa207-dfc4-4402-b1e6-3bee6ecbffe4
📒 Files selected for processing (15)
bundle.jsonpackages/core/bundle.jsonpackages/core/scripts/skill-assets.manifest.jsonskill-assets/fetch-asset.cjsskills/ns-advanced-memory-leak-hunter/SKILL.mdskills/ns-advanced-memory-leak-hunter/fetch-asset.cjsskills/ns-analyze-asset/SKILL.mdskills/ns-analyze-asset/fetch-asset.cjsskills/ns-analyze-tracing/SKILL.mdskills/ns-audit-dependencies/SKILL.mdskills/ns-benchmark-run/reference/benchmark-inputs.mdskills/ns-cpu-spike-analysis/fetch-asset.cjsskills/ns-generate-asset/SKILL.mdskills/ns-generate-asset/fetch-asset.cjsskills/ns-memory-spike-analysis/fetch-asset.cjs
✅ Files skipped from review due to trivial changes (2)
- skills/ns-benchmark-run/reference/benchmark-inputs.md
- skills/ns-analyze-tracing/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (8)
- skills/ns-audit-dependencies/SKILL.md
- bundle.json
- packages/core/bundle.json
- skills/ns-generate-asset/SKILL.md
- skills/ns-analyze-asset/SKILL.md
- skills/ns-memory-spike-analysis/fetch-asset.cjs
- skills/ns-generate-asset/fetch-asset.cjs
- skills/ns-advanced-memory-leak-hunter/SKILL.md
dobleuber
reviewed
Jun 30, 2026
| } | ||
|
|
||
| /** True when a nsolid-pi-plugin package root exists on disk. */ | ||
| export function piPluginInstalled (): boolean { |
There was a problem hiding this comment.
Only checks whether a candidate root has a package.json, but findPiPluginPackageRoots() includes every non-npm local package source from Pi settings. In an environment with any local Pi package installed (for example ./my-plugin) and valid NodeSource credentials, doctor('pi') can mark the native plugin as installed and then report all N|Solid skills/MCPs as OK even when nsolid-pi-plugin is not installed. This should validate package.json.name === PI_PLUGIN_PACKAGE_NAME, like findPiPluginSkillRoots() already does, before returning true.
Collaborator
Author
There was a problem hiding this comment.
fixed! thanks for the review
EHortua
reviewed
Jun 30, 2026
…onality in cli to uninstall plugins
jdaison
requested changes
Jul 2, 2026
| } | ||
|
|
||
| if (existing && !isExpired(existing)) { | ||
| if (existing.accountsUrl && existing.accountsUrl !== authConfig.accountsUrl) { |
There was a problem hiding this comment.
(around lines 58–72) — this change now returns any non-expired stored credentials without re-validating them or re-checking requiredPermissions. That makes setup idempotent, but it also means revoked tokens or insufficient permissions are silently accepted until a later MCP call fails. This is a behavioral regression in correctness/security terms. At minimum, I’d suggest preserving the requiredPermissions check against cached existing.permissions before returning, and consider a best-effort validation path that only skips network validation on transient errors rather than unconditionally trusting stored credentials.
Collaborator
Author
There was a problem hiding this comment.
you are right, I had the wrong url to begin with and then removed it, now the fix uses the correct api endpoint.
| case 'claude': | ||
| return ['claude', 'plugin', 'uninstall', id] | ||
| case 'codex': | ||
| return ['codex', 'plugin', 'uninstall', id] |
mafesernaarboleda
approved these changes
Jul 3, 2026
dobleuber
reviewed
Jul 3, 2026
dobleuber
left a comment
dobleuber
left a comment
There was a problem hiding this comment.
I think we have still an error with the permissions fallback of auth-manager.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
nsolid-plugin setup --harness <harness>.index.json.