AX-1644 - add jfrog mcp#26
Conversation
YoniMelki
left a comment
There was a problem hiding this comment.
Review — AX-1644: built-in always-on JFrog MCP (Cursor)
The change is coherent: mcp.json switches the jfrog server from a direct remote URL to npx @jfrog/agent-guard with _JF_ARGS=mcp=jfrog-mcp, and the README + jfrog-mcp-management.md hard-rules clearly explain the always-on, catalog-exempt model and the jfrog vs jfrog-catalog coexistence story. The template guidance (scope: plugin (jfrog), never run catalog flows against it, the admin-allowlist troubleshooting) is genuinely good.
Two things to resolve before merge:
1. ${VAR:-default} registry expansion in Cursor (functional risk). The bundled mcp.json depends on Cursor expanding a bash-style default inside args. If Cursor doesn't implement :-default semantics, npx gets the literal ${...} string as --registry and package resolution fails. This is a new pattern (the catalog flow writes a concrete URL, not a :-default). Please confirm the server launches with JFROG_AGENT_GUARD_REPO both set and unset. (inline)
2. JFROG_PLATFORM_URL → JFROG_URL is a breaking, format-changing rename. Needs a migration note + an explicit "must include https://" caveat. (inline)
Also heads-up (no change required here): the agent-guard-hook validates --registry as a literal http(s) URL and reads the raw mcp.json, so the ${VAR:-default} form would be rejected by a deployed hook — I've flagged that on the agent-guard PR for coordination.
YoniMelki
left a comment
There was a problem hiding this comment.
Re-review — both findings resolved ✅
Re-reviewed at head 37bdf19 (was CHANGES_REQUESTED). Both blocking findings are addressed:
mcp.jsonregistry expansion — now a hardcodedhttps://releases.jfrog.io/artifactory/api/npm/coding-agents-npm/(no${JFROG_AGENT_GUARD_REPO:-default}). I verified this exact entry against agent-guard'sagent-guard-hook.mjs:validateAgentGuard→null(passes); the old${VAR:-default}string returned "not an http(s) URL" and would have denied every tool call on the built-injfrogserver. The Cursor${env:VAR}-interpolation + hook-rejection risk is gone. ✅JFROG_PLATFORM_URL→JFROG_URLbreaking rename — now flagged with a Breaking: bullet in "What's new" and a dedicated upgrade blockquote in Authentication, both spelling out the host-only → full-URL-with-scheme change and the silent-breakage failure mode. ✅
Docs are notably thorough (Prerequisites add Node ≥14/npx, new "JFrog MCP" section, troubleshooting entry that correctly frames the admin MCP-allowlist case as an enterprise-policy issue).
One non-blocking nit: the breaking note lives under "What's new in v0.5.0" / "pre-v0.5.0" but plugin.json is now 0.5.5 — worth aligning the version label. Not a blocker.
Switching to APPROVE.
No description provided.