Conversation
6a88ae3 to
2323a30
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds “project-aware” symbol lookup commands that derive a TASTyQuery context from the current project’s build tool (Mill/sbt/scala-cli), including classpath extraction/caching and new end-to-end integration tests.
Changes:
- Add project-aware CLI subcommands (
get,list,search) backed by build-tool detection and classpath provisioning/caching. - Refactor existing external-coordinate handlers to share “core” logic with the new project-aware handlers.
- Add process execution utilities and comprehensive integration tests (including CI setup for scala-cli + sbt).
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/test/src/cellar/ProjectAwareIntegrationTest.scala | New integration tests for build tool detection, classpath extraction/caching, and project-aware handlers |
| lib/test/src/cellar/IntegrationTest.scala | Removes inline CapturingConsole (moved to shared test helper) |
| lib/test/src/cellar/CapturingConsole.scala | Shared console capture helper for tests |
| lib/src/cellar/process/ProcessRunner.scala | New process execution helper used by build tool integrations |
| lib/src/cellar/handlers/SearchHandler.scala | Refactor to expose runCore for reuse from project-aware search |
| lib/src/cellar/handlers/ProjectSearchHandler.scala | New project-aware search handler |
| lib/src/cellar/handlers/ProjectListHandler.scala | New project-aware list handler |
| lib/src/cellar/handlers/ProjectHandler.scala | Shared project handler to create context/classpath for project-aware commands |
| lib/src/cellar/handlers/ProjectGetHandler.scala | New project-aware get handler |
| lib/src/cellar/handlers/ListHandler.scala | Refactor to expose runCore and simplify error handling |
| lib/src/cellar/handlers/GetHandler.scala | Refactor to expose runCore, support coord-less behavior, and simplify error handling |
| lib/src/cellar/build/ScalaCliBuildTool.scala | Build-tool adapter for scala-cli classpath extraction |
| lib/src/cellar/build/SbtBuildTool.scala | Build-tool adapter for sbt compile + classpath extraction + fingerprinting |
| lib/src/cellar/build/ProjectClasspathProvider.scala | Central provider for project classpath with optional caching flow |
| lib/src/cellar/build/MillBuildTool.scala | Build-tool adapter for Mill compile + classpath extraction + fingerprinting |
| lib/src/cellar/build/ClasspathOutputParser.scala | Parser for build tool classpath output formats |
| lib/src/cellar/build/ClasspathCache.scala | On-disk classpath cache under .cellar/cache |
| lib/src/cellar/build/BuildToolDetector.scala | Detect build tool kind via marker files |
| lib/src/cellar/build/BuildTool.scala | BuildTool trait + shared module requirement helper |
| lib/src/cellar/build/BuildFingerprint.scala | SHA-256 fingerprinting for cache keys |
| lib/src/cellar/JreClasspath.scala | Stream-based parsing for bundled JRE resource |
| lib/src/cellar/ContextResource.scala | Minor import cleanup |
| lib/src/cellar/CellarError.scala | Adds build-tool related errors and makes PartialResolution coord optional |
| cli/src/resources/META-INF/native-image/cellar/resource-config.json | Updates native-image resource inclusion list |
| cli/src/resources/META-INF/native-image/cellar/reflect-config.json | Removes JRT filesystem provider reflection config entry |
| cli/src/cellar/cli/CellarApp.scala | Reworks CLI: project-aware defaults + *-external coordinate-based commands |
| build.mill | Passes Mill binary path into tests via system property |
| CLAUDE.md | Updates usage documentation to reflect project-aware vs external commands |
| .gitignore | Ignores .cellar cache directory |
| .github/workflows/ci.yml | Installs scala-cli and sbt for CI integration tests |
Comments suppressed due to low confidence (1)
cli/src/resources/META-INF/native-image/cellar/resource-config.json:6
resource-config.jsonno longer includesjre.bin, butJreClasspathstill loadsjre.binwhen running as a native image. Without an include rule, the native binary will likely fail at runtime with "Bundled JRE not found". Re-add thejre.binresource include (or remove the runtime dependency on it).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val stderrDrainer = Thread.startVirtualThread { () => | ||
| stderr = new String(process.getErrorStream.readAllBytes()) | ||
| } |
There was a problem hiding this comment.
Thread.startVirtualThread requires JDK 21+, but this project compiles with --release 17 (see build.mill) and CI uses Java 17. This will not compile on the current toolchain; use a regular Thread/ExecutorService, or gate virtual threads behind a Java 21 build target.
| val stderrDrainer = Thread.startVirtualThread { () => | |
| stderr = new String(process.getErrorStream.readAllBytes()) | |
| } | |
| val stderrDrainer = new Thread(() => { | |
| stderr = new String(process.getErrorStream.readAllBytes()) | |
| }) | |
| stderrDrainer.start() |
| }.adaptError { case e: java.io.IOException => | ||
| new RuntimeException(s"Command not found: '${command.headOption.getOrElse("")}'. Ensure it is installed and on PATH.", e) |
There was a problem hiding this comment.
adaptError { case e: IOException => ... } rewrites all IOExceptions as "Command not found". ProcessBuilder.start() can throw IOException for other reasons (permission denied, invalid working directory, etc.), so this message can be misleading. Consider detecting the specific “executable not found” case (e.g., by inspecting the exception message/cause) or preserving the original message in the error text.
| }.adaptError { case e: java.io.IOException => | |
| new RuntimeException(s"Command not found: '${command.headOption.getOrElse("")}'. Ensure it is installed and on PATH.", e) | |
| }.adaptError { | |
| case e: java.io.FileNotFoundException => | |
| new RuntimeException(s"Command not found: '${command.headOption.getOrElse("")}'. Ensure it is installed and on PATH.", e) | |
| case e: java.io.IOException => | |
| val cmd = command.headOption.getOrElse("") | |
| val details = Option(e.getMessage).getOrElse("I/O error while starting the process") | |
| new RuntimeException(s"Failed to start process '$cmd': $details", e) |
|
|
||
| object BuildFingerprint: | ||
| def compute(files: List[Path], module: String): IO[String] = | ||
| IO.blocking { | ||
| val digest = MessageDigest.getInstance("SHA-256") | ||
| digest.update(module.getBytes) | ||
| files.sorted.foreach { path => | ||
| if Files.exists(path) then | ||
| digest.update(path.toString.getBytes) |
There was a problem hiding this comment.
The SHA-256 fingerprint uses String.getBytes without an explicit charset. That makes the cache key depend on the platform default charset, which can change across environments. Use a fixed charset (e.g., UTF-8) for module and path.toString bytes to keep fingerprints deterministic.
| object BuildFingerprint: | |
| def compute(files: List[Path], module: String): IO[String] = | |
| IO.blocking { | |
| val digest = MessageDigest.getInstance("SHA-256") | |
| digest.update(module.getBytes) | |
| files.sorted.foreach { path => | |
| if Files.exists(path) then | |
| digest.update(path.toString.getBytes) | |
| import java.nio.charset.StandardCharsets | |
| object BuildFingerprint: | |
| def compute(files: List[Path], module: String): IO[String] = | |
| IO.blocking { | |
| val digest = MessageDigest.getInstance("SHA-256") | |
| digest.update(module.getBytes(StandardCharsets.UTF_8)) | |
| files.sorted.foreach { path => | |
| if Files.exists(path) then | |
| digest.update(path.toString.getBytes(StandardCharsets.UTF_8)) |
| private def withTempDir(test: Path => IO[Unit]): IO[Unit] = | ||
| IO.blocking(Files.createTempDirectory("cellar-test-")).flatMap { dir => | ||
| test(dir).guarantee(IO.blocking { | ||
| Files.walk(dir).sorted(java.util.Comparator.reverseOrder()).forEach(Files.deleteIfExists(_)) |
There was a problem hiding this comment.
The temp-dir cleanup uses Files.walk(dir) without closing the returned java.util.stream.Stream. On some platforms this can leak file handles and prevent deletions. Wrap the walk in a try/finally (or Using.resource) and close it after deleting.
| Files.walk(dir).sorted(java.util.Comparator.reverseOrder()).forEach(Files.deleteIfExists(_)) | |
| val stream = Files.walk(dir) | |
| try | |
| stream.sorted(java.util.Comparator.reverseOrder()).forEach(Files.deleteIfExists(_)) | |
| finally | |
| stream.close() |
| private val getSubcmd: Opts[IO[ExitCode]] = | ||
| Opts.subcommand("get", "Fetch symbol info from the current project") { | ||
| (symbolArg, moduleOpt, javaHomeOpt, noCacheOpt).mapN { (fqn, module, javaHome, noCache) => | ||
| ProjectGetHandler.run(fqn, module, javaHome, noCache) | ||
| } | ||
| } | ||
|
|
||
| private val getSourceSubcmd: Opts[Option[Path] => IO[ExitCode]] = | ||
| Opts.subcommand("get-source", "Fetch the source code of a named symbol") { | ||
| (coordArg, symbolArg, extraReposOpt).mapN { (rawCoord, fqn, extraRepos) => | ||
| (javaHome: Option[Path]) => | ||
| parseAndResolve(rawCoord, extraRepos).flatMap { | ||
| case Left(err) => IO.blocking(System.err.println(err)).as(ExitCode.Error) | ||
| case Right(coord) => GetSourceHandler.run(coord, fqn, javaHome, extraRepos) | ||
| } | ||
| private val listSubcmd: Opts[IO[ExitCode]] = | ||
| Opts.subcommand("list", "List symbols in a package or class from the current project") { | ||
| (symbolArg, moduleOpt, limitOpt, javaHomeOpt, noCacheOpt).mapN { (fqn, module, limit, javaHome, noCache) => | ||
| ProjectListHandler.run(fqn, module, limit, javaHome, noCache) | ||
| } | ||
| } | ||
|
|
||
| private val listSubcmd: Opts[Option[Path] => IO[ExitCode]] = | ||
| Opts.subcommand("list", "List symbols in a package or class") { | ||
| (coordArg, symbolArg, limitOpt, extraReposOpt).mapN { (rawCoord, fqn, limit, extraRepos) => | ||
| (javaHome: Option[Path]) => | ||
| parseAndResolve(rawCoord, extraRepos).flatMap { | ||
| case Left(err) => IO.blocking(System.err.println(err)).as(ExitCode.Error) | ||
| case Right(coord) => ListHandler.run(coord, fqn, limit, javaHome, extraRepos) | ||
| } | ||
| private val searchSubcmd: Opts[IO[ExitCode]] = | ||
| Opts.subcommand("search", "Substring search for symbol names in the current project") { | ||
| (Opts.argument[String]("query"), moduleOpt, limitOpt, javaHomeOpt, noCacheOpt).mapN { | ||
| (query, module, limit, javaHome, noCache) => | ||
| ProjectSearchHandler.run(query, module, limit, javaHome, noCache) | ||
| } | ||
| } |
There was a problem hiding this comment.
The CLI semantics have changed so get/list/search now operate on the current project and the old coordinate-based behavior moved to *-external. README.md still documents cellar get <coordinate> <fqn> etc., which will now be incorrect for users. Please update the README (and any other user docs) to match the new command names/meaning.
| result.waitFor() | ||
| stdout.linesIterator.filter(_.nonEmpty).map(cwd.resolve).toList |
There was a problem hiding this comment.
fingerprintFromGit ignores the git ls-files exit code. If git fails, this can produce an empty/partial fingerprint and lead to stale or incorrect classpath cache hits. Consider validating waitFor() and falling back to fingerprintFromDisk() (or surfacing a clear error) when the command fails.
| result.waitFor() | |
| stdout.linesIterator.filter(_.nonEmpty).map(cwd.resolve).toList | |
| val exitCode = result.waitFor() | |
| if exitCode == 0 then | |
| stdout.linesIterator.filter(_.nonEmpty).map(cwd.resolve).toList | |
| else | |
| // Fall back to disk-based fingerprinting if git fails to avoid stale/incorrect cache hits | |
| fingerprintFromDisk() |
| val inner = trimmed.substring(1, trimmed.length - 1).trim | ||
| if inner.isEmpty then return Left("Build tool produced an empty classpath.") | ||
|
|
||
| val entries = inner.split(",").map(_.trim.stripPrefix("\"").stripSuffix("\"")).toList | ||
| val paths = entries.map { entry => |
There was a problem hiding this comment.
parseJsonArray is not actually parsing JSON: it splits on commas and strips quotes. This will fail for valid JSON strings containing commas or escaped characters, and may break if Mill ever changes formatting (whitespace/newlines). Consider using a minimal JSON parser for string arrays, or at least a safer tokenizer that handles quoted commas/escapes.
| val stdout = new String(result.getInputStream.readAllBytes()) | ||
| result.waitFor() | ||
| stdout.linesIterator.filter(_.nonEmpty).map(cwd.resolve).toList |
There was a problem hiding this comment.
fingerprintFromGit ignores the git ls-files exit code. If git fails (e.g., missing binary, non-repo state, permission issues), this will silently return an empty/partial fingerprint and can cause incorrect cache hits. Consider checking waitFor() and falling back to fingerprintFromDisk() (or surfacing a clear error) when the command fails.
| val stdout = new String(result.getInputStream.readAllBytes()) | |
| result.waitFor() | |
| stdout.linesIterator.filter(_.nonEmpty).map(cwd.resolve).toList | |
| val exitCode = result.waitFor() | |
| if exitCode != 0 then | |
| fingerprintFromDisk() | |
| else | |
| val stdout = new String(result.getInputStream.readAllBytes()) | |
| stdout.linesIterator.filter(_.nonEmpty).map(cwd.resolve).toList |
No description provided.