MLD-1307 - Auto-run plugin mcp.json align - #50
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
| @@ -0,0 +1,356 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
This file (plus claude-align-plugin-mcps.test.mjs, claude-register-align-watch-paths.mjs, and the skills/jfrog-ai-catalog/* additions) is hand-committed into directories that VENDOR.md documents as wholesale-vendored: modules/ from jfrog-agent-hooks, skills/jfrog-ai-catalog-skills/ from jfrog/jfrog-skills. Both directories get wholesale-replaced by their sync scripts on the next pin bump (sync-modules-vendor.json / sync-skills-vendor.json weren't touched here). This exact mistake was already made and reverted once, with the lesson written down in commit 8aa1671 ("Drop hand-committed MCP skills; they arrive via vendored sync ... committing them here is redundant and would be overwritten on the next pin bump"). Recent history shows both syncs happen routinely (module syncs fc235ef/d899b22/5d6ba15, skill syncs f2088f7/8aa1671/320a558/427d2e6) — the next one will silently delete this PR's work unless it lands upstream (in jfrog-agent-hooks / jfrog-skills) first.
| const HARNESS_ID = "claude_code"; | ||
| const log = createLogger("align-plugin-mcps"); | ||
|
|
||
| export const AGENT_GUARD_PACKAGE = "@jfrog/agent-guard"; |
There was a problem hiding this comment.
@jfrog/agent-guard is installed via unpinned npx --yes, from a registry fully controllable via JFROG_AGENT_GUARD_REPO, on every single SessionStart. This is trust-on-first-use repeated every run rather than once — whoever controls that package tag/registry gets code execution in the Claude Code process on every session, on every machine with this plugin installed. Worth a version pin (or shipping this disabled by default) rather than relying on operators to discover the README's mirroring advice.
| stdio: /** @type {const} */ (["pipe", "pipe", "pipe"]), | ||
| env, | ||
| // Resolve npx.cmd via cmd.exe; bare spawn("npx") often ENOENTs on Windows. | ||
| shell: isWin, |
There was a problem hiding this comment.
On Windows, spawn(command, args, {shell:true}) is used with args built from env-derived values (JF_PROJECT, CLAUDE_CONFIG_DIR, JFROG_AGENT_GUARD_REPO) with no escaping/quoting layer. Node's spawn+shell:true on Windows joins array args into a single command-line string without per-argument quoting for cmd.exe, so a value containing a space — e.g. a completely normal Windows path like C:\Users\John Smith\.claude — gets corrupted/split rather than passed through intact. This will affect ordinary Windows users with spaces in their profile path, not just a theoretical attacker-controlled input.
| } | ||
|
|
||
| try { | ||
| child?.kill?.("SIGTERM"); |
There was a problem hiding this comment.
On Windows, killAlignChildTree falls through to child.kill('SIGTERM') on the single spawned process. Because shell:true makes cmd.exe the immediate child on Windows (unlike the POSIX branch's own process group), killing cmd.exe does not cascade-terminate the npx.cmd/node.exe grandchildren it launched — Windows doesn't auto-terminate child processes the way POSIX process groups do. A timeout-triggered kill on Windows can leave the real hung npx/node process running as an orphan indefinitely.
|
|
||
| if (timeoutMs > 0) { | ||
| timer = setTimeout(() => { | ||
| killAlignChildTree(child, { |
There was a problem hiding this comment.
killAlignChildTree only ever sends SIGTERM — no escalation to SIGKILL if the child doesn't die promptly, and no grace-period wait before finish() resolves and the parent process exits. This is exactly the failure mode the timeout exists to prevent: a genuinely stuck npx (e.g. a stalled network download) may not respond to SIGTERM at all, and can survive as an orphan after the Claude Code hook has already exited.
| const env = deps.env ?? process.env; | ||
| const writeStdout = deps.writeStdout ?? ((s) => process.stdout.write(s)); | ||
| const readStdinFn = deps.readStdinFn ?? readStdin; | ||
| const format = MODES[modeArg]; |
There was a problem hiding this comment.
const format = MODES[modeArg] is a plain bracket lookup on a frozen plain object with no Object.hasOwn guard. If modeArg ever named an inherited Object.prototype member (e.g. "toString", "constructor"), MODES[modeArg] would return that inherited value (truthy), silently bypassing the if (!format) unknown-mode guard below. Unreachable today since modeArg only ever comes from the two hardcoded hooks.json literals, but worth tightening (Object.hasOwn(MODES, modeArg) ? MODES[modeArg] : undefined, or a Map) so it stays correct if this function is ever invoked another way.
| }); | ||
|
|
||
| try { | ||
| child.stdin?.end(stdin); |
There was a problem hiding this comment.
For file-changed mode, the raw Claude hook stdin — read via the shared readStdin(), which settles after a ~50ms idle gap with no new data — is forwarded verbatim as agent-guard's own subprocess stdin. If the payload is large enough or arrives with any inter-chunk gap at or above that idle window, readStdin() can resolve with a truncated string, which then gets handed to agent-guard as if it were the complete FileChanged payload. This forwarding is new in this PR; previously this value was only used locally (e.g. parseSessionId, which tolerates parse failures gracefully).
| import { pathToFileURL } from "node:url"; | ||
|
|
||
| import { createLogger, setLogContext } from "./core/logger.mjs"; | ||
| import { readStdin, parseSessionId } from "./core/io.mjs"; |
There was a problem hiding this comment.
This adapter (and claude-register-align-watch-paths.mjs) doesn't call detectHarness(), unlike the existing adapters (claude-session-start.mjs, cursor-session-start.mjs), which use it specifically to avoid double-firing when Cursor and Claude Code read overlapping hook config. This particular hooks.json lives in this plugin's own directory rather than a shared global config file, so the practical double-fire risk is lower than for the adapters that do guard — but it's an inconsistency with the established defensive pattern in this codebase worth a conscious call rather than an omission.
| ], | ||
| "FileChanged": [ | ||
| { | ||
| "matcher": "installed_plugins.json|known_marketplaces.json", |
There was a problem hiding this comment.
This FileChanged matcher contains literal . characters. Per Claude Code's hooks documentation, any matcher character outside [A-Za-z0-9_|] forces evaluation as an unanchored JavaScript regex — so . means "match any single character" here, not a literal dot, and the whole pattern is an unanchored substring match. This means it would also match unintended filenames like installed_pluginsXjson (any char for X) or installed_plugins.json.bak, not just the two intended exact filenames. Escaping the dots (installed_plugins\.json|known_marketplaces\.json) would fix this.
| "${CLAUDE_PLUGIN_ROOT}/modules/claude-align-plugin-mcps.mjs", | ||
| "session-start" | ||
| ], | ||
| "timeout": 45, |
There was a problem hiding this comment.
This SessionStart hook's timeout is 45s, and the sibling FileChanged hook is the same. The only prior timeout history in this file is a deliberate reduction (10s → 7s, commit 6d443ba) aimed at keeping session start snappy. 45s is over 6x that previous ceiling, and three SessionStart hooks now stack up (7s + 5s + 45s worst-case) on every session start. The 45s figure is justified by this being a network-bound operation with its own internal 40s kill-switch, so it's not necessarily wrong — but it's a large enough shift from this file's established direction that it's worth a deliberate, discussed sign-off rather than landing as part of a larger PR.
2 participants
Summary
Wire Claude Code SessionStart / FileChanged hooks so installed-plugin
.mcp.jsonfiles are auto-aligned through@jfrog/agent-guard --align-plugin-mcps.Also expands the vendored AI Catalog skill for agent plugins (rename + references) and adds a light entitlement gate on plugin download that reuses the existing MCP Agent Guard check.
Align hooks
claude-align-plugin-mcps.mjs— Claude adapter: spawnnpx @jfrog/agent-guard --align-plugin-mcps --format hook-session-start|hook-file-changed, passthrough stdout (watchPaths/systemMessage/additionalContextowned by agent-guard), never fail the session.claude-register-align-watch-paths.mjs— fastSessionStartcompanion: emitFileChangedwatchPathsforinstalled_plugins.json/known_marketplaces.jsonbefore the slower npx align finishes, so mid-session plugin installs are watched while Agent Guard is still downloading.hooks/hooks.json— register both on SessionStart (exec form command + args); re-run align onFileChangedwhen those metadata files change.modules/claude-align-plugin-mcps.test.mjs+ workflow / CONTRIBUTING coverage.READMEdocumentsSessionStart/FileChangedalign, kill switch, and unpinned npx behavior.Discovery + rewrite stay in agent-guard (
--align-plugin-mcps). This PR only owns Claude hook UX, env forwarding, soft-fail, early watchPath registration, and related docs/CI.AI Catalog skill
jfrog-ai-catalog-skills→jfrog-ai-catalog(skills + plugins).discover/install/manage/publish.agent-guard-activation.md. No duplicated check script in this skill (simplified after an earlier draft). Listing installed plugins and remove remain local-only / ungated.Goal
SessionStartstill registerswatchPaths).Assumptions
This adapter does not rewrite
mcp.jsonitself and does not invent Claude hook payload shapes. Agent Guard owns discovery, transform, and hook stdout. The Claude plugin must:--yes --registry <...> @jfrog/agent-guard --align-plugin-mcps --format hook-...JF_PROJECT→--project, CLAUDE_CONFIG_DIR→--claude-config-dir,JFROG_AGENT_GUARD_REPO→ npx registry + agent-guard--registrySessionStartfailure / empty stdout, still emit fallbackwatchPathsJF_AGENT_ALIGN_PLUGIN_MCPS_DISABLE=1(no-op, no stdout)Happy flow
SessionStart:
claude-register-align-watch-paths.mjsprints ClaudeSessionStartJSON with watchPaths for~/.claude/plugins/installed_plugins.jsonandknown_marketplaces.json(or$CLAUDE_CONFIG_DIR/plugins/...).claude-align-plugin-mcps.mjssession-start runs agent-guard with--format hook-session-start..mcp.jsonpaths, rewrites stdio MCPs to launch via Agent Guard, and prints Claude hook stdout (watchPaths+ optional reload guidance).FileChanged (matcher:
installed_plugins.json|known_marketplaces.json):claude-align-plugin-mcps.mjsfile-changed runs with--format hook-file-changed.Idempotency / drift (owned by agent-guard; adapter just re-invokes):
rewritten: 0(idempotent).Kill switch:
JF_AGENT_ALIGN_PLUGIN_MCPS_DISABLE=1→ both hooks exit 0 with no stdout / no spawn.Plugin install (skill): before
jf agent plugins install/update, run the shared Agent Guard activation check; proceed only on exit 0.Non-happy flows
Expected behavior / exit (adapter always exit 0):
watchPathsso FileChanged still works (exit 0).watchPaths(exit 0).JF_PROJECT→ still invoke agent-guard (no--project); agent-guard decides whether project is required for rewrite (exit 0).JFROG_AGENT_GUARD_REPO→ used for bothnpx --registryand agent-guard--registry.watchPaths(exit 0).