archive cross-harness-plugin-instaler spec#52
Merged
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughAdds and archives planning documents for a cross-harness NodeSource plugin installer. The documents define a shared core, GitHub-root distribution, Pi packaging, explicit OAuth setup, fallback installation, uninstall and health-check behavior, harness mappings, and phased implementation tasks. ChangesCross-harness installer
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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
`@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.md`:
- Around line 157-172: Update the public Credentials interface to match the
persisted authentication schema by adding the accountsUrl field with the same
requiredness and type used by storage. Ensure ensureAuthenticated and
loadCredentials expose this aligned schema consistently rather than leaving
accountsUrl unsupported.
- Around line 307-313: Update the MCP_URL placeholder expansion to use the
persisted credentials.mcpUrl as the canonical endpoint instead of deriving it
from consoleUrl. Ensure fallback/setup configuration and native wrapper runtime
resolution both use this stored value, while preserving the existing
missing-credentials guidance.
- Around line 582-587: Update the installation flow around the “Write tracking
file” step to persist successfully installed artifacts before MCP configuration
can fail. Record each successful skill incrementally or ensure the
partial-success path finalizes tracking, so retained skills remain removable
during later uninstall.
- Around line 688-700: Update the uninstall flow documentation to delete shared
~/.agents/skills/ only when no harness associations remain, preserving the
shared copy when another harness still references it. Apply this rule in
design.md lines 688-700, openspec/specs/installation-and-auth/spec.md lines
181-184, and
openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md
lines 181-184; the latter two must mirror the design behavior for targeted
uninstall.
- Around line 343-369: Update the tracking schema in design.md at lines 343-369
to persist ownership metadata, such as before/after values or fingerprints, for
installed skills and MCP servers. In
openspec/specs/installation-and-auth/spec.md lines 198-223 and
openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md
lines 198-223, require uninstall cleanup to verify that stored ownership
metadata matches and skip ambiguous name-only matches; apply the same
conservative rule in all three sites.
In
`@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md`:
- Around line 224-229: Resolve the contradiction in the “Uninstall preserves
user modifications” scenario by aligning its title and assertions: either
document destructive deletion without claiming preservation, or change uninstall
behavior to back up, restore, or confirm before deleting modified skills. Ensure
the scenario’s stated guarantee matches the required outcome.
In `@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/tasks.md`:
- Around line 60-67: Update the atomic write utility in
packages/core/src/utils/fs.ts to avoid unlinking the existing destination before
replacement. Use the design’s write-file-atomic strategy or an equivalent
same-volume atomic replacement that keeps the old file available until the new
content is committed, including on Windows.
- Around line 115-124: Update the skill-linker installation and tracking
contract to retain each .bak.<timestamp> backup path alongside the replacement
target. During skill uninstall, remove the installed replacement and restore the
tracked backup to its original target location, preserving existing behavior for
installations without backups.
- Around line 217-223: Update the install() orchestrator to remove the
ensure-authentication step and avoid browser launches, OAuth flows, or
credential writes during installation. Instead, load existing credentials
opportunistically and continue installation without authenticating when none are
available; keep authentication exclusively in the explicit setup/login flow.
- Around line 199-205: Update Task 19 and the Pi adapter design to expose a Pi
MCP configuration path/adapter targeting ~/.pi/agent/mcp.json, with setup
writing "auth": false. Keep package activation side-effect-free by skipping MCP
changes during activation, while allowing explicit setup to configure MCP;
update supportsMcp and related tests to reflect this capability.
In `@openspec/specs/installation-and-auth/spec.md`:
- Around line 224-229: Resolve the contradiction in the “Uninstall preserves
user modifications” scenario by aligning its title and expected behavior: either
rename it to describe destructive deletion and retain the no-warning removal, or
change the uninstall requirements to preserve modified skills through backup,
restoration, or confirmation. Ensure the scenario title, Given/When/Then steps,
and guarantee consistently describe the selected behavior.
🪄 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: 921bd737-534f-4b81-b526-c085d88057db
📒 Files selected for processing (6)
openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/.openspec.yamlopenspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.mdopenspec/changes/archive/2026-07-21-cross-harness-plugin-installer/proposal.mdopenspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.mdopenspec/changes/archive/2026-07-21-cross-harness-plugin-installer/tasks.mdopenspec/specs/installation-and-auth/spec.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 11
🤖 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
`@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.md`:
- Around line 157-172: Update the public Credentials interface to match the
persisted authentication schema by adding the accountsUrl field with the same
requiredness and type used by storage. Ensure ensureAuthenticated and
loadCredentials expose this aligned schema consistently rather than leaving
accountsUrl unsupported.
- Around line 307-313: Update the MCP_URL placeholder expansion to use the
persisted credentials.mcpUrl as the canonical endpoint instead of deriving it
from consoleUrl. Ensure fallback/setup configuration and native wrapper runtime
resolution both use this stored value, while preserving the existing
missing-credentials guidance.
- Around line 582-587: Update the installation flow around the “Write tracking
file” step to persist successfully installed artifacts before MCP configuration
can fail. Record each successful skill incrementally or ensure the
partial-success path finalizes tracking, so retained skills remain removable
during later uninstall.
- Around line 688-700: Update the uninstall flow documentation to delete shared
~/.agents/skills/ only when no harness associations remain, preserving the
shared copy when another harness still references it. Apply this rule in
design.md lines 688-700, openspec/specs/installation-and-auth/spec.md lines
181-184, and
openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md
lines 181-184; the latter two must mirror the design behavior for targeted
uninstall.
- Around line 343-369: Update the tracking schema in design.md at lines 343-369
to persist ownership metadata, such as before/after values or fingerprints, for
installed skills and MCP servers. In
openspec/specs/installation-and-auth/spec.md lines 198-223 and
openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md
lines 198-223, require uninstall cleanup to verify that stored ownership
metadata matches and skip ambiguous name-only matches; apply the same
conservative rule in all three sites.
In
`@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md`:
- Around line 224-229: Resolve the contradiction in the “Uninstall preserves
user modifications” scenario by aligning its title and assertions: either
document destructive deletion without claiming preservation, or change uninstall
behavior to back up, restore, or confirm before deleting modified skills. Ensure
the scenario’s stated guarantee matches the required outcome.
In `@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/tasks.md`:
- Around line 60-67: Update the atomic write utility in
packages/core/src/utils/fs.ts to avoid unlinking the existing destination before
replacement. Use the design’s write-file-atomic strategy or an equivalent
same-volume atomic replacement that keeps the old file available until the new
content is committed, including on Windows.
- Around line 115-124: Update the skill-linker installation and tracking
contract to retain each .bak.<timestamp> backup path alongside the replacement
target. During skill uninstall, remove the installed replacement and restore the
tracked backup to its original target location, preserving existing behavior for
installations without backups.
- Around line 217-223: Update the install() orchestrator to remove the
ensure-authentication step and avoid browser launches, OAuth flows, or
credential writes during installation. Instead, load existing credentials
opportunistically and continue installation without authenticating when none are
available; keep authentication exclusively in the explicit setup/login flow.
- Around line 199-205: Update Task 19 and the Pi adapter design to expose a Pi
MCP configuration path/adapter targeting ~/.pi/agent/mcp.json, with setup
writing "auth": false. Keep package activation side-effect-free by skipping MCP
changes during activation, while allowing explicit setup to configure MCP;
update supportsMcp and related tests to reflect this capability.
In `@openspec/specs/installation-and-auth/spec.md`:
- Around line 224-229: Resolve the contradiction in the “Uninstall preserves
user modifications” scenario by aligning its title and expected behavior: either
rename it to describe destructive deletion and retain the no-warning removal, or
change the uninstall requirements to preserve modified skills through backup,
restoration, or confirmation. Ensure the scenario title, Given/When/Then steps,
and guarantee consistently describe the selected behavior.
🪄 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: 921bd737-534f-4b81-b526-c085d88057db
📒 Files selected for processing (6)
openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/.openspec.yamlopenspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.mdopenspec/changes/archive/2026-07-21-cross-harness-plugin-installer/proposal.mdopenspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.mdopenspec/changes/archive/2026-07-21-cross-harness-plugin-installer/tasks.mdopenspec/specs/installation-and-auth/spec.md
🛑 Comments failed to post (11)
openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.md (5)
157-172: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align
Credentialswith the persisted schema.The storage contract includes
accountsUrlat Lines 326-339, but the publicCredentialsinterface cannot represent it. Add the field or explicitly remove it from the persisted contract so auth storage and consumers share one schema.🤖 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 `@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.md` around lines 157 - 172, Update the public Credentials interface to match the persisted authentication schema by adding the accountsUrl field with the same requiredness and type used by storage. Ensure ensureAuthenticated and loadCredentials expose this aligned schema consistently rather than leaving accountsUrl unsupported.
307-313: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the stored MCP URL as the canonical endpoint.
Credentials explicitly persist
mcpUrl, but expansion reconstructs it fromconsoleUrl. That can produce the wrong endpoint when the service supplies a nonstandard path or host. Usecredentials.mcpUrl, or document and validate the invariant that makes derivation safe.🤖 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 `@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.md` around lines 307 - 313, Update the MCP_URL placeholder expansion to use the persisted credentials.mcpUrl as the canonical endpoint instead of deriving it from consoleUrl. Ensure fallback/setup configuration and native wrapper runtime resolution both use this stored value, while preserving the existing missing-credentials guidance.
343-369: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make uninstall ownership verifiable instead of name-based.
The tracking schema lacks a before/after value or ownership fingerprint, while missing-tracking cleanup matches MCP names alone. A pre-existing user entry named
ncm,ns-benchmark, ornsolid-consolecould be deleted.
openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.md#L343-L369: store ownership metadata or a before/after snapshot/fingerprint.openspec/specs/installation-and-auth/spec.md#L198-L223: require fingerprints and skip ambiguous name-only matches.openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md#L198-L223: apply the same conservative cleanup rule to the archived specification.📍 Affects 3 files
openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.md#L343-L369(this comment)openspec/specs/installation-and-auth/spec.md#L198-L223openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md#L198-L223🤖 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 `@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.md` around lines 343 - 369, Update the tracking schema in design.md at lines 343-369 to persist ownership metadata, such as before/after values or fingerprints, for installed skills and MCP servers. In openspec/specs/installation-and-auth/spec.md lines 198-223 and openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md lines 198-223, require uninstall cleanup to verify that stored ownership metadata matches and skip ambiguous name-only matches; apply the same conservative rule in all three sites.
582-587: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Persist partial-install tracking before MCP failure can return.
The sequence writes tracking only after MCP configuration. If skills succeed but MCP writing fails, the retained skills are untracked, so later uninstall cannot reliably remove them. Record successful artifacts incrementally or finalize tracking in the partial-success path.
🤖 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 `@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.md` around lines 582 - 587, Update the installation flow around the “Write tracking file” step to persist successfully installed artifacts before MCP configuration can fail. Record each successful skill incrementally or ensure the partial-success path finalizes tracking, so retained skills remain removable during later uninstall.
688-700: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Respect multi-harness ownership of shared skills.
The design tracks harness associations but the uninstall flow still removes the shared
~/.agents/skills/copy unconditionally.
openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.md#L688-L700: remove shared skills only when no harness references remain.openspec/specs/installation-and-auth/spec.md#L181-L184: qualify shared-skill deletion for targeted uninstall.openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md#L181-L184: mirror the same multi-harness cleanup rule.📍 Affects 3 files
openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.md#L688-L700(this comment)openspec/specs/installation-and-auth/spec.md#L181-L184openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md#L181-L184🤖 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 `@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/design.md` around lines 688 - 700, Update the uninstall flow documentation to delete shared ~/.agents/skills/ only when no harness associations remain, preserving the shared copy when another harness still references it. Apply this rule in design.md lines 688-700, openspec/specs/installation-and-auth/spec.md lines 181-184, and openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md lines 181-184; the latter two must mirror the design behavior for targeted uninstall.openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md (1)
224-229: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve the contradictory uninstall guarantee.
The scenario is titled “preserves user modifications,” but the behavior deletes an edited skill without warning. Either rename this to document destructive removal or require backup/restore/confirmation so the stated guarantee is true.
🤖 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 `@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/specs/installation-and-auth.md` around lines 224 - 229, Resolve the contradiction in the “Uninstall preserves user modifications” scenario by aligning its title and assertions: either document destructive deletion without claiming preservation, or change uninstall behavior to back up, restore, or confirm before deleting modified skills. Ensure the scenario’s stated guarantee matches the required outcome.openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/tasks.md (4)
60-67: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not unlink the destination during an “atomic” write.
Deleting the existing file before
rename()creates a data-loss window if the process crashes or the replacement fails. Use the design’swrite-file-atomicstrategy or an equivalent same-volume replacement that preserves the old file until the new file is committed.🧰 Tools
🪛 LanguageTool
[style] ~67-~67: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...oth Unix-style and Windows-style paths. Test atomic write behavior when target file ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 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 `@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/tasks.md` around lines 60 - 67, Update the atomic write utility in packages/core/src/utils/fs.ts to avoid unlinking the existing destination before replacement. Use the design’s write-file-atomic strategy or an equivalent same-volume atomic replacement that keeps the old file available until the new content is committed, including on Windows.
115-124: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define backup restoration for skill uninstall.
The linker moves an existing file/directory to
.bak.<timestamp>before installing a replacement, but the tracking/uninstall contracts only delete the new path. Uninstall can therefore leave the user’s original skill missing from its original location; track the backup and restore it when the replacement is removed.🤖 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 `@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/tasks.md` around lines 115 - 124, Update the skill-linker installation and tracking contract to retain each .bak.<timestamp> backup path alongside the replacement target. During skill uninstall, remove the installed replacement and restore the tracked backup to its original target location, preserving existing behavior for installations without backups.
199-205: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep Pi setup MCP-capable while keeping activation side-effect-free.
Marking the Pi adapter as
supportsMcp: falseconflicts with the setup requirement to write~/.pi/agent/mcp.jsonwith"auth": false. Separate package activation from explicit setup, but provide a Pi MCP path/adapter for setup.🤖 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 `@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/tasks.md` around lines 199 - 205, Update Task 19 and the Pi adapter design to expose a Pi MCP configuration path/adapter targeting ~/.pi/agent/mcp.json, with setup writing "auth": false. Keep package activation side-effect-free by skipping MCP changes during activation, while allowing explicit setup to configure MCP; update supportsMcp and related tests to reflect this capability.
217-223: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not authenticate from
install().This task says the installer flow calls
ensure auth, but the current lifecycle contract requires native and fallback install to avoid browsers, OAuth, and credential writes.install()should load existing credentials opportunistically and leave authentication exclusively to explicitsetup/login.🤖 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 `@openspec/changes/archive/2026-07-21-cross-harness-plugin-installer/tasks.md` around lines 217 - 223, Update the install() orchestrator to remove the ensure-authentication step and avoid browser launches, OAuth flows, or credential writes during installation. Instead, load existing credentials opportunistically and continue installation without authenticating when none are available; keep authentication exclusively in the explicit setup/login flow.openspec/specs/installation-and-auth/spec.md (1)
224-229: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve the contradictory uninstall guarantee.
The scenario is titled “preserves user modifications,” but the behavior deletes an edited skill without warning. Either rename this to document destructive removal or require backup/restore/confirmation so the stated guarantee is true.
🤖 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 `@openspec/specs/installation-and-auth/spec.md` around lines 224 - 229, Resolve the contradiction in the “Uninstall preserves user modifications” scenario by aligning its title and expected behavior: either rename it to describe destructive deletion and retain the no-warning removal, or change the uninstall requirements to preserve modified skills through backup, restoration, or confirmation. Ensure the scenario title, Given/When/Then steps, and guarantee consistently describe the selected behavior.
mafesernaarboleda
approved these changes
Jul 21, 2026
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
2 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