diff --git a/millbun/integration/resources/invalid-bun-specifier/build.mill b/millbun/integration/resources/invalid-bun-specifier/build.mill new file mode 100644 index 0000000..3699c20 --- /dev/null +++ b/millbun/integration/resources/invalid-bun-specifier/build.mill @@ -0,0 +1,24 @@ +//| mill-version: 1.1.5 +//| mill-jvm-version: system +//| mvnDeps: +//| - com.tjclp::mill-bun_mill1:0.0.0-NIGHTLY + +package build + +import mill.* +import mill.bun.bun +import mill.scalajslib.* +import mill.scalajslib.api.* +import mill.scalajslib.bun.* + +object app extends BunScalaJSModule { + override def moduleDir = build.moduleDir + def scalaVersion = "3.8.2" + def scalaJSVersion = "1.22.0" + + override def moduleKind = Task { ModuleKind.CommonJSModule } + override def sources = Task.Sources(moduleDir / "src") + // Empty *specifier*: the name half is valid, so the macro used to accept this and let + // parseDependency throw at task time instead. + override def bunDeps = Task { Seq(bun"react@") } +} diff --git a/millbun/integration/resources/invalid-bun-specifier/src/App.scala b/millbun/integration/resources/invalid-bun-specifier/src/App.scala new file mode 100644 index 0000000..047460e --- /dev/null +++ b/millbun/integration/resources/invalid-bun-specifier/src/App.scala @@ -0,0 +1 @@ +object App diff --git a/millbun/integration/src/mill/bun/BunDependencyManifestIntegrationTests.scala b/millbun/integration/src/mill/bun/BunDependencyManifestIntegrationTests.scala index 532cd82..5d72c45 100644 --- a/millbun/integration/src/mill/bun/BunDependencyManifestIntegrationTests.scala +++ b/millbun/integration/src/mill/bun/BunDependencyManifestIntegrationTests.scala @@ -27,6 +27,14 @@ object BunDependencyManifestIntegrationTests extends TestSuite { assert(!res.isSuccess) } + test("invalid bun specifier fails in build definitions") { + // The macro only checked the package-name half, so bun"react@" compiled cleanly and then + // threw from parseDependency during the install — defeating the interpolator's purpose. + val tester = this.tester("invalid-bun-specifier") + val res = tester.eval("app.bunDeps") + assert(!res.isSuccess) + } + test("published dev-only modules do not emit runtime manifests") { val tester = this.tester("scalajs-dependency-manifests") val res = tester.eval("publishedDevOnlyLib.jar") diff --git a/millbun/src/mill/bun/BunDep.scala b/millbun/src/mill/bun/BunDep.scala index e4a62b7..450e194 100644 --- a/millbun/src/mill/bun/BunDep.scala +++ b/millbun/src/mill/bun/BunDep.scala @@ -20,6 +20,18 @@ extension (sc: StringContext) inline def bun(inline args: Any*): String = ${ BunDepMacro.validateImpl('sc, 'args) } +object BunDep: + /** + * Validate a dependency the macro could not check at compile time, returning it unchanged. + * + * Used for interpolated forms like `bun"react@$version"`, where the value only exists once the + * build evaluates. Failing here still beats failing inside `bun install`. + */ + def validate(dep: String): String = + BunToolchainModule.parseDependency(dep) match + case Right(_) => dep + case Left(message) => throw new IllegalArgumentException(s"Invalid bun dependency: $message") + private object BunDepMacro: import scala.quoted.* @@ -30,8 +42,9 @@ private object BunDepMacro: Expr(literal) case _ => // Has interpolated parts, a non-literal StringContext, or runs inside another - // macro-generated context — build at runtime and skip compile-time validation. - '{ $sc.s($args*) } + // macro-generated context, so the value is not known at compile time. Validate when + // the build evaluates it instead, which still fails before any install runs. + '{ BunDep.validate($sc.s($args*)) } private def literalParts(sc: Expr[StringContext])(using Quotes): Option[Seq[String]] = import quotes.reflect.* @@ -74,30 +87,15 @@ private object BunDepMacro: extractRepeatedArgs(args.asTerm.underlyingArgument).contains(Nil) + /** + * Validate against the same parser the build uses at task time. + * + * This deliberately delegates rather than re-deriving the rules: a second, weaker parser here + * meant `bun"react@"` compiled cleanly and then threw from `parseDependency` during the install, + * which is exactly what the interpolator exists to prevent. + */ private def validateLiteral(dep: String)(using Quotes): Unit = import quotes.reflect.* - if dep.isEmpty then - report.errorAndAbort("bun dependency cannot be empty. Use bun\"package@version\" format.") - // Validate package name format - val name = if dep.startsWith("@") then - // Scoped: @scope/name or @scope/name@version - val afterScope = dep.drop(1) - if !afterScope.contains('/') then - report.errorAndAbort( - s"Invalid scoped package: '$dep'. Expected @scope/name or @scope/name@version" - ) - val slashIdx = afterScope.indexOf('/') - val scopeName = afterScope.take(slashIdx) - if scopeName.isEmpty then - report.errorAndAbort(s"Invalid scoped package: '$dep'. Scope name is empty.") - val afterSlash = afterScope.drop(slashIdx + 1) - val nameOnly = if afterSlash.contains('@') then afterSlash.take(afterSlash.indexOf('@')) else afterSlash - if nameOnly.isEmpty then - report.errorAndAbort(s"Invalid scoped package: '$dep'. Package name is empty after scope.") - dep - else - // Unscoped: name or name@version - val nameOnly = if dep.contains('@') then dep.take(dep.indexOf('@')) else dep - if nameOnly.isEmpty then - report.errorAndAbort(s"Invalid package: '$dep'. Package name cannot be empty.") - dep + BunToolchainModule.parseDependency(dep).left.foreach { message => + report.errorAndAbort(s"Invalid bun dependency: $message") + } diff --git a/millbun/src/mill/bun/BunVendoredNodeModules.scala b/millbun/src/mill/bun/BunVendoredNodeModules.scala index 623d487..23050c0 100644 --- a/millbun/src/mill/bun/BunVendoredNodeModules.scala +++ b/millbun/src/mill/bun/BunVendoredNodeModules.scala @@ -44,9 +44,19 @@ object BunVendoredNodeModules: FileVisitResult.CONTINUE ) - /** Merge vendored node_modules from a classpath entry into a destination. */ + /** Merge vendored node_modules from a classpath entry into a destination. + * + * Refuses a symlinked destination: in workspace mode `node_modules` is a link into the shared + * workspace install's `Task.dest`, and merging through it would silently mutate another task's + * output — shared by every other member of the workspace. + */ def mergeFromClasspathEntry(entry: os.Path, destNodeModules: os.Path): Boolean = - if os.isDir(entry) then mergeFromDir(entry, destNodeModules, entry.toString) + if os.isLink(destNodeModules) then + throw new RuntimeException( + s"Refusing to merge vendored Bun runtime into $destNodeModules: it is a symlink to " + + s"${os.readLink.absolute(destNodeModules)}, which belongs to another task." + ) + else if os.isDir(entry) then mergeFromDir(entry, destNodeModules, entry.toString) else if os.exists(entry) && entry.ext == "jar" then mergeFromJar(entry, destNodeModules) else false diff --git a/millbun/src/mill/scalajslib/bun/BunScalaJSModule.scala b/millbun/src/mill/scalajslib/bun/BunScalaJSModule.scala index 1b86361..fbb8800 100644 --- a/millbun/src/mill/scalajslib/bun/BunScalaJSModule.scala +++ b/millbun/src/mill/scalajslib/bun/BunScalaJSModule.scala @@ -254,8 +254,21 @@ trait BunScalaJSModule extends ScalaJSModule with BunToolchainModule with BunPac transitiveUnmanagedDeps().nonEmpty || bunPackageJsonExtras().value.nonEmpty + val ownResourceRoots = resources().map(_.path).toSet + val vendoredEntries = runClasspath().map(_.path).filterNot(ownResourceRoots.contains) + bunWorkspaceInstall() match case Some(workspaceInstall) => + // Vendored trees must not be merged here: node_modules is a link into the workspace + // install's dest, so merging would mutate a directory shared by every workspace member. + val vendored = vendoredEntries.filter(BunVendoredNodeModules.hasVendoredNodeModules) + if vendored.nonEmpty then + Task.fail( + s"Bun workspace members cannot consume vendored runtime dependencies: " + + s"${vendored.map(_.last).mkString(", ")}. Depend on the manifest-only artifact, or " + + "install this module outside the workspace." + ) + val installed = workspaceInstall.path if os.exists(installed / "node_modules") then os.symlink(dest / "node_modules", installed / "node_modules") @@ -281,9 +294,7 @@ trait BunScalaJSModule extends ScalaJSModule with BunToolchainModule with BunPac env = bunEnv() ) - val ownResourceRoots = resources().map(_.path).toSet - val vendoredEntries = runClasspath().map(_.path).filterNot(ownResourceRoots.contains) - mergeVendoredNodeModules(vendoredEntries, dest / "node_modules") + mergeVendoredNodeModules(vendoredEntries, dest / "node_modules") PathRef(dest) } diff --git a/millbun/test/src/mill/bun/BunDepTests.scala b/millbun/test/src/mill/bun/BunDepTests.scala index 1ecd98f..7233b97 100644 --- a/millbun/test/src/mill/bun/BunDepTests.scala +++ b/millbun/test/src/mill/bun/BunDepTests.scala @@ -50,7 +50,32 @@ object BunDepTests extends TestSuite { assert(deps.head.startsWith("@anthropic-ai")) } - // Invalid literal coverage lives in integration tests so the interpolator + test("the interpolator and the task-time parser accept the same inputs") { + // The macro used to run its own weaker parser, so bun"react@" compiled and then threw + // during the install. Both must now agree, in both directions. + Seq("react@^19.0.0", "@types/node", "zod", "lodash@~4.17.0", "@scope/pkg@1.0.0").foreach { + valid => + assert(BunDep.validate(valid) == valid) + assert(BunToolchainModule.parseDependency(valid).isRight) + } + + Seq("", "react@", "@types", "@types/bun@", "@/pkg").foreach { invalid => + val error = intercept[IllegalArgumentException](BunDep.validate(invalid)) + assert(error.getMessage.contains("Invalid bun dependency")) + assert(BunToolchainModule.parseDependency(invalid).isLeft) + } + } + + test("interpolated forms are validated when the build evaluates them") { + // Not knowable at compile time, so this is the runtime backstop. + val version = "^19.0.0" + assert(bun"react@$version" == "react@^19.0.0") + + val empty = "" + intercept[IllegalArgumentException](bun"react@$empty") + } + + // Invalid *literal* coverage lives in integration tests so the interpolator // is compiled in a normal build.mill context rather than inside another macro. } } diff --git a/millbun/test/src/mill/bun/BunVendoredNodeModulesTests.scala b/millbun/test/src/mill/bun/BunVendoredNodeModulesTests.scala index 54caa89..ac34238 100644 --- a/millbun/test/src/mill/bun/BunVendoredNodeModulesTests.scala +++ b/millbun/test/src/mill/bun/BunVendoredNodeModulesTests.scala @@ -100,6 +100,25 @@ object BunVendoredNodeModulesTests extends TestSuite { assert(err.getMessage.contains("Vendored Bun bundle conflict")) } + + test("merging through a symlinked destination is refused") { + // In workspace mode node_modules links into the shared install's Task.dest. Merging + // through it would silently mutate a directory every workspace member depends on. + val source = os.temp.dir() + writeVendoredPackage(source, "react", "19.1.1") + + val shared = os.temp.dir() / "shared-node-modules" + os.makeDir.all(shared) + val member = os.temp.dir() + os.symlink(member / "node_modules", shared) + + val err = intercept[RuntimeException] { + BunVendoredNodeModules.mergeFromClasspathEntry(source, member / "node_modules") + } + assert(err.getMessage.contains("belongs to another task")) + // The critical assertion: nothing leaked into the shared directory. + assert(os.list(shared).isEmpty) + } } private def writeVendoredPackage(root: os.Path, name: String, version: String): Unit = {