Fix plugin root resolution - #207
Conversation
- Resolve plugin root paths in hooks and MCP launchers without relying on placeholder expansion. - Keep marketplace plugin entries minimal while preserving marketplace-level metadata. - Bundle shared workflow and telemetry runtime files as physical plugin content instead of symlinks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Incorporate Power Pages 2.6.0 ALM updates from origin/main. - Preserve minimal marketplace entries and bundled shared-content policy after resolving conflicts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates plugin packaging and runtime path resolution so hooks/MCP launchers no longer depend on placeholder expansion, while also tightening marketplace metadata expectations and bundling previously-shared/symlinked assets as physical plugin content for better compatibility (notably Windows/plugin-host installs).
Changes:
- Make Open Plugins marketplace entries “minimal” (per-plugin entries only
name+source) and update legacy compatibility validation accordingly. - Update Power Pages hooks and MCP server launch configuration to resolve the plugin root via environment variables instead of
${...}placeholder expansion. - Bundle shared telemetry runtime (and referenced shared workflow guidance in docs) as copied, physical plugin content rather than symlinks; update docs/comments to match.
Show a summary per file
| File | Description |
|---|---|
| shared/telemetry/tests/emit-spawn.test.js | Comment update clarifying shared-source execution context for dispatcher/ikey forwarding. |
| shared/telemetry/README.md | Updates guidance from “symlink” to “copy” for adopting plugins’ telemetry library layout. |
| shared/telemetry/lib/emit-spawn.js | Comment update clarifying behavior when running from shared vs plugin copy. |
| scripts/validate-legacy-compatibility.js | Enforces minimal marketplace entry shape and validates plugin manifest name mirroring. |
| README.md | Documents new intent: marketplace entries minimal; per-plugin metadata controlled by .plugin/plugin.json. |
| plugins/power-pages/scripts/lib/telemetry/lib/user-config.js | Added per-user telemetry config read/write helpers (opt-out precedence, config persistence). |
| plugins/power-pages/scripts/lib/telemetry/lib/telemetry-config.js | Added CLI for toggling/inspecting telemetry setting status. |
| plugins/power-pages/scripts/lib/telemetry/lib/session.js | Added session-id caching + host session-id extraction helper. |
| plugins/power-pages/scripts/lib/telemetry/lib/scrubber.js | Added placeholder scrubbing seam for future PII/regex handling. |
| plugins/power-pages/scripts/lib/telemetry/lib/resolver-loader.js | Added optional resolver discovery loader (resolver.js next to ikey.json). |
| plugins/power-pages/scripts/lib/telemetry/lib/prompt-detector.js | Added prompt parsing helper to detect tracked slash commands. |
| plugins/power-pages/scripts/lib/telemetry/lib/pac-auth.js | Added cached PAC auth banner parsing for org/tenant/cloud context. |
| plugins/power-pages/scripts/lib/telemetry/lib/local-log.js | Added local diagnostic log writer with basic rotation. |
| plugins/power-pages/scripts/lib/telemetry/lib/events.js | Added schema/type-aware event builder + defensive allowlist picker. |
| plugins/power-pages/scripts/lib/telemetry/lib/emit-spawn.js | Added detached dispatcher spawn wrapper with minimal env forwarding. |
| plugins/power-pages/scripts/lib/telemetry/lib/emit-from-prompt.js | Added prompt-driven “skill_started” emission path with fast gates and resolver integration. |
| plugins/power-pages/scripts/lib/telemetry/lib/emit-dispatcher.js | Added standalone dispatcher process (sanitization, local log mirror, resolver + static fallback, opt-out gate). |
| plugins/power-pages/scripts/lib/telemetry/lib/agent-info.js | Added agent + PAC CLI version detection with multiple-host heuristics and caching. |
| plugins/power-pages/references/approval-gates.md | Refines explanation of cross-plugin shared prompt cataloging and TODO. |
| plugins/power-pages/hooks/run-skill-pretool-telemetry.js | Comment update clarifying ikey path forwarding behavior for shared vs bundled copies. |
| plugins/power-pages/hooks/hooks.json | Switches hook commands to inline node -e loader that resolves plugin root from env. |
| plugins/power-pages/AGENTS.md | Updates telemetry guidance from symlinked shared library to copied/bundled library. |
| plugins/power-pages/.mcp.json | Switches MCP launcher to env-based plugin root resolution via inline node -e. |
| plugins/model-apps/.mcp.json | Same as above for model-apps Playwright MCP launcher. |
| marketplace.json | Removes per-plugin override metadata; keeps only required name + source per plugin entry. |
| AGENTS.md | Updates repo-wide guidance: no symlinks for shared workflows/telemetry; copy + refresh per plugin. |
| .claude-plugin/marketplace.json | Mirrors minimalized marketplace.json for legacy compatibility. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 34/40 changed files
- Comments generated: 2
- Normalize malformed telemetry config objects before writing opt-out choices. - Clarify telemetry CLI wording to include optional org/tenant and version fields. - Sync the bundled Power Pages telemetry library copy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Resolve hook and MCP loader paths to absolute file paths before require(). - Add POSIX copy guidance for telemetry library adoption. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
T-Nid (tyaginidhi)
left a comment
There was a problem hiding this comment.
Code Review — Fix plugin root resolution
+2567 / −285 across 37 files · CI: ✅ all green
Overview
Fixes unreliable plugin-root resolution by removing two mechanisms that don't survive Windows / plugin-host installs:
${CLAUDE_PLUGIN_ROOT}placeholder in.mcp.json+hooks.json→ replaced with an inlinenode -ethat resolvesPLUGIN_ROOT || CLAUDE_PLUGIN_ROOTfrom the environment andrequire()s the target.- Git symlinks for shared content (telemetry runtime, shared workflow
.mds) → replaced with committed physical copies. - Bonus: marketplace
plugins[]entries trimmed to minimal (name+source), hardened telemetryuser-configagainst a malformedtelemetry: [], and corrected the privacy disclosure.
Correctness — verified sound ✅
- MCP launcher works:
launch-playwright-mcp.jsalready exportslaunchbehind arequire.main === moduleguard, sonode -e "...mod.launch()"correctly runs it (the guarded CLI block doesn't double-fire). - Hook launchers work: all three hook scripts execute at top-level (IIFE /
process.stdinhandlers), sorequire()runs them. Hooks fail-open (exit 0when root unset) — correct posture for telemetry/validation; MCP hard-fails (throw) — also correct. - Physical copies are byte-identical to the edited
shared/telemetry/libsource — copies were correctly refreshed after the shared edits (checkedemit-spawn,telemetry-config,user-config,events,agent-info). - Privacy disclosure is now accurate:
events.jsalready allowlistsorgId/tenantId/aiAgentVersion, so the old text ("never includes … hostnames") understated collection. The new wording fixes a real accuracy gap. 👍 - Convention docs (
AGENTS.md, andCLAUDE.md→ symlink to it) are updated consistently, including the why ("Windows and plugin-host installs can materialize symlinks as plain link files").
Issues & risks
-
🔶 Main concern — no drift protection for the physical copies. The symlink couldn't drift; a committed copy can.
AGENTS.mdnow says "refresh every adopting plugin's copy in the same change," but nothing enforces it — there's no test/CI assertingplugins/*/scripts/lib/telemetry/libequalsshared/telemetry/lib(same for bundled workflow.mds). The next person who editsshared/and forgets to re-copy ships stale plugin code with green CI.- Suggestion: add a cheap guard —
readdirSyncovershared/telemetry/liband assert each file is byte-identical to the adopting plugin's copy (and the workflow.mds). This directly catches the failure mode the new manual process invites. Highest-value addition.
- Suggestion: add a cheap guard —
-
🔹 Duplicated inline
node -eblob. The resolver one-liner is copy-pasted across 2.mcp.json+ 3hooks.jsonentries with no test. If the resolution logic ever changes, it's edited in many places. Acceptable as a workaround, but a small maintenance smell. -
🔹 Coordination with #202. This PR removes
versionfrommarketplace.jsonplugin entries entirely — which makes the marketplace↔plugin version-mirror sync that #202/#194 maintain moot. Whichever merges second needs a rebase: #202 still bumps aversionfield this PR deletes. Not a defect in #207, but flag the ordering so the legacy-compat validator doesn't fail on the loser. -
🔹 Strictness of
assertMinimalMarketplaceEntry. It enforces exactly['name','source']— intentional per the Open Plugins rationale, but it will hard-reject any future legitimate marketplace-level override field. Fine as a deliberate gate; just noting it's strict.
Conventions / tests / security
- Conventions: ✅ Docs and validator updated to match; no doc/code contradiction left behind.
- Tests: ✅ The
user-confighardening has a dedicated new test (telemetry: []normalization). ❌ Gap: no test for copy↔shared parity (see main concern). - Security/privacy: ✅ Net-positive — disclosure now matches the allowlist; org/tenant collection is pre-existing (not introduced here) and governed by the events allowlist/scrubber.
node -eruns only trusted committed paths.
Verdict
Solid, root-cause fix — approve once the drift concern is addressed. The launcher and copy mechanics are correct and verified, the privacy-disclosure fix is a genuine improvement, and the docs are kept honest. The one thing I'd ask for before merge is a copy-parity test so the de-symlinked content can't silently drift; the #202 version-field coordination is a heads-up rather than a blocker.
🤖 Generated with Claude Code
- Validate that plugin.json keeps version, description, author, homepage, repository, license, and keywords after marketplace entries were minimized. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Will do this in a later PR |
Uh oh!
There was an error while loading. Please reload this page.