serve: integration harness + fixes for --trackDeps over-reporting and JGit fetch failures

.github/workflows/serve-harness.yml

@@ -0,0 +1,45 @@
+name: Serve Harness
+
+# Integration validation for the `bazel-diff serve` query service. Runs tools/serve_harness.py,
+# which builds //cli:bazel-diff and drives the real binary over HTTP against a live git:// remote
+# (git daemon) across both git engines x full/shallow/partial clones -- coverage the in-process
+# E2ETest cannot reach. See the harness module docstring for the scenario matrix.
+
+# Cron-only for now while we confirm the harness is stable on CI runners. Once it has a healthy
+# track record, add `pull_request` / `push` triggers (and/or make it a required status check) to
+# gate merges. `workflow_dispatch` allows on-demand runs from the Actions tab in the meantime.
+# Note: scheduled + dispatched workflows run from the default branch, so this must land on master
+# before the cron fires.
+on:
+  workflow_dispatch:
+  schedule:
+    - cron: "0 */12 * * *"
+
+jobs:
+  ServeHarness:
+    runs-on: ubuntu-latest
+    timeout-minutes: 30
+    strategy:
+      fail-fast: false
+      matrix:
+        bazel: [ '8.x', '9.x' ]
+    steps:
+      - name: Setup Java JDK
+        uses: actions/setup-java@v4
+        with:
+          distribution: 'temurin'
+          java-version: '21'
+      - name: Setup Go environment
+        uses: actions/setup-go@v5
+        with:
+          go-version: ^1.17
+        id: go
+      - name: Setup Bazelisk
+        run: go install github.com/bazelbuild/bazelisk@latest
+      - uses: actions/checkout@v4
+      - name: Run serve harness
+        env:
+          USE_BAZEL_VERSION: ${{ matrix.bazel }}
+        # git daemon ships with git (preinstalled on the runner); python3 is preinstalled.
+        # --bazel points the harness (and the serve subprocesses it spawns) at bazelisk.
+        run: python3 tools/serve_harness.py --bazel "$HOME/go/bin/bazelisk"

README.md

@@ -186,6 +186,13 @@ Notes and current limitations:
   `--gitEngine=subprocess` to shell out to the `git` binary at `--gitPath` instead -- useful for
   workspaces that depend on checkout filters or hooks that JGit does not run. Note that JGit only
   moves the git plumbing in-process; the working tree is still materialized on disk for `bazel query`.
+  JGit cannot fetch some clone shapes native git handles fine -- notably shallow (`--depth`) and
+  partial (`--filter=blob:none`) clones, whose thin packs are delta-compressed against objects the
+  clone does not have ("Missing delta base"). When an in-process JGit fetch fails, the service
+  automatically falls back to the native `git` binary (at `--gitPath`) for that fetch and logs a
+  warning; point `--gitEngine=subprocess` at such workspaces to skip the in-process attempt (a
+  partial clone in particular needs the subprocess engine to *serve* queries, since JGit cannot
+  lazily fetch the missing blobs a checkout needs).
 * Hashes are cached on local disk via `--cacheDir` and survive restarts. The cache layer is
   pluggable behind a byte-oriented interface so a remote backend (e.g. S3) can be added without
   touching callers.

cli/src/main/kotlin/com/bazel_diff/cli/ServeCommand.kt

@@ -237,7 +237,7 @@ class ServeCommand : Callable<Int> {
    */
   fun createGitClient(): GitClient =
       when (gitEngine.lowercase()) {
-        "jgit" -> JGitClient(workspacePath)
+        "jgit" -> JGitClient(workspacePath, gitPath)
         "subprocess" -> ProcessGitClient(workspacePath, gitPath)
         else ->
             throw CommandLine.ParameterException(

cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt

@@ -80,7 +80,7 @@ class CalculateImpactedTargetsInteractor : KoinComponent {
       from: Map<String, TargetHash>,
       to: Map<String, TargetHash>
   ): Set<String> {
-    val difference = Maps.difference(to, from)
+    val difference = Maps.difference(hashIdentities(to), hashIdentities(from))
     val onlyInEnd: Set<String> = difference.entriesOnlyOnLeft().keys
     val changed: Set<String> = difference.entriesDiffering().keys
     val impactedTargets =
@@ -91,6 +91,21 @@ class CalculateImpactedTargetsInteractor : KoinComponent {
     return impactedTargets
   }
 
+  /**
+   * Projects each target to its hash identity (`type#hash~directHash`) for impactedness comparison.
+   *
+   * Impactedness is defined purely by a target's hash, so the comparison must not include
+   * [TargetHash.deps]: that field is auxiliary data used only to derive distance metrics. It is
+   * populated on freshly generated hashes (under `--trackDeps`) but is always null on hashes
+   * deserialised from cache/JSON ([TargetHash.fromJson] does not restore it). Diffing the full
+   * [TargetHash] data class -- whose generated `equals` includes `deps` -- would therefore report
+   * every dep-carrying target as changed whenever one side was generated and the other read from
+   * cache. That is precisely a query-service cache hit under `--trackDeps`, which spuriously
+   * inflated the impacted set for the cached `from` revision.
+   */
+  private fun hashIdentities(hashes: Map<String, TargetHash>): Map<String, String> =
+      hashes.mapValues { it.value.hashWithType }
+
   fun executeWithDistances(
       from: Map<String, TargetHash>,
       to: Map<String, TargetHash>,
@@ -188,7 +203,9 @@ class CalculateImpactedTargetsInteractor : KoinComponent {
       to: Map<String, TargetHash>,
       depEdges: Map<String, List<String>>
   ): Map<String, TargetDistanceMetrics> {
-    val difference = Maps.difference(to, from)
+    // Diff by hash identity, not the full TargetHash -- see [hashIdentities]. The keys still index
+    // the original `from`/`to` maps below for the directHash (DIRECT vs INDIRECT) check.
+    val difference = Maps.difference(hashIdentities(to), hashIdentities(from))
 
     val newLabels = difference.entriesOnlyOnLeft().keys
     val existingImpactedLabels = difference.entriesDiffering().keys

cli/src/main/kotlin/com/bazel_diff/server/JGitClient.kt

@@ -1,10 +1,12 @@
 package com.bazel_diff.server
 
 import com.bazel_diff.log.Logger
+import java.io.File
 import java.nio.file.Path
 import org.eclipse.jgit.api.Git
 import org.eclipse.jgit.errors.IncorrectObjectTypeException
 import org.eclipse.jgit.errors.MissingObjectException
+import org.eclipse.jgit.lib.Repository
 import org.eclipse.jgit.revwalk.RevWalk
 import org.eclipse.jgit.storage.file.FileRepositoryBuilder
 import org.eclipse.jgit.transport.RemoteConfig
@@ -23,9 +25,20 @@ import org.koin.core.component.inject
  * these are cheap relative to the `bazel query` that follows, and it keeps the client stateless and
  * thread-safe (the workspace itself is still serialized by [HashService]'s lock).
  */
-class JGitClient(private val workspacePath: Path) : GitClient, KoinComponent {
+class JGitClient(
+    private val workspacePath: Path,
+    private val gitPath: String = "git",
+    private val nativeFetchFallback: Boolean = true,
+) : GitClient, KoinComponent {
   private val logger: Logger by inject()
 
+  // Built lazily and only when a JGit fetch actually fails. Native git negotiates clone shapes
+  // JGit 5.13 cannot fetch (shallow, partial/promisor, thin packs whose delta base is absent),
+  // scoped to the same workspace and configured git binary.
+  private val nativeGit: ProcessGitClient by lazy { ProcessGitClient(workspacePath, gitPath) }
+
+  @Volatile private var warnedCloneShape = false
+
   private fun open(): Git {
     val repository =
         FileRepositoryBuilder()
@@ -38,6 +51,7 @@ class JGitClient(private val workspacePath: Path) : GitClient, KoinComponent {
 
   override fun fetch() {
     open().use { git ->
+      warnOnUnsupportedCloneShapeOnce(git.repository)
       val remotes = RemoteConfig.getAllRemoteConfigs(git.repository.config)
       if (remotes.isEmpty()) {
         logger.i { "No git remotes configured; skipping fetch" }
@@ -48,12 +62,68 @@ class JGitClient(private val workspacePath: Path) : GitClient, KoinComponent {
         try {
           git.fetch().setRemote(remote.name).setRemoveDeletedRefs(true).call()
         } catch (e: Exception) {
-          throw GitClientException("JGit fetch ${remote.name} failed: ${e.message}", e)
+          // JGit cannot fetch some clone shapes it otherwise reads fine -- notably shallow and
+          // partial (blob:none) clones, where the remote's thin pack is delta-compressed against a
+          // base object absent from this clone ("Missing delta base <sha>"). Native git negotiates
+          // these correctly, so fall back to it rather than lame-ducking the whole service on a
+          // fetch the machine is perfectly capable of.
+          fetchViaNativeGitOrThrow(remote.name, e)
+          return // native `git fetch --all` already covered every remote; don't re-attempt them.
         }
       }
     }
   }
 
+  /**
+   * Retries a failed JGit fetch with the native `git` binary (unless [nativeFetchFallback] is off,
+   * in which case the original JGit failure is surfaced). If native git also fails the two failures
+   * are combined so neither cause is hidden.
+   */
+  private fun fetchViaNativeGitOrThrow(remoteName: String, jgitError: Exception) {
+    if (!nativeFetchFallback) {
+      throw GitClientException("JGit fetch $remoteName failed: ${jgitError.message}", jgitError)
+    }
+    logger.w {
+      "JGit fetch $remoteName failed (${jgitError.message}); retrying with native git ('$gitPath'). " +
+          "This usually means the workspace is a shallow or partial clone, which JGit cannot fetch; " +
+          "pass --gitEngine=subprocess to skip the in-process attempt entirely."
+    }
+    try {
+      nativeGit.fetch()
+    } catch (nativeError: Exception) {
+      throw GitClientException(
+              "JGit fetch $remoteName failed (${jgitError.message}) and the native git fallback " +
+                  "('$gitPath') also failed: ${nativeError.message}",
+              jgitError)
+          .apply { addSuppressed(nativeError) }
+    }
+  }
+
+  /**
+   * Logs a one-time warning if the workspace is a shallow or partial (promisor) clone -- shapes
+   * JGit 5.13 cannot fetch, so every fetch will take the native-git fallback path. Surfacing it up
+   * front (rather than only on the first failed fetch) makes the misconfiguration obvious.
+   */
+  private fun warnOnUnsupportedCloneShapeOnce(repo: Repository) {
+    if (warnedCloneShape) return
+    warnedCloneShape = true
+    val gitDir = repo.directory ?: return
+    val shallow = File(gitDir, "shallow").exists()
+    val partial =
+        repo.config.getString("extensions", null, "partialClone") != null ||
+            repo.config.getSubsections("remote").any {
+              repo.config.getBoolean("remote", it, "promisor", false)
+            }
+    if (shallow || partial) {
+      val kinds = listOfNotNull("shallow".takeIf { shallow }, "partial".takeIf { partial })
+      logger.w {
+        "workspace clone is ${kinds.joinToString("+")}; JGit cannot reliably fetch these " +
+            "(thin-pack delta bases may be absent). Fetches will fall back to native git " +
+            "('$gitPath'); consider --gitEngine=subprocess for this workspace."
+      }
+    }
+  }
+
   override fun resolveSha(revision: String): String {
     open().use { git ->
       val objectId = git.repository.resolve(revision) ?: throw MissingRevisionException(revision)

cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt

@@ -41,6 +41,50 @@ class CalculateImpactedTargetsInteractorTest : KoinTest {
     assertThat(impacted).containsExactlyInAnyOrder("1", "3")
   }
 
+  @Test
+  fun computeSimpleImpactedTargetsIgnoresDepsAsymmetry() {
+    // Impactedness is defined by a target's hash, not its `deps` metadata. A query-service cache
+    // hit deserialises the `from` hashes (TargetHash.deps == null, fromJson never restores it),
+    // while a freshly generated `to` under --trackDeps has deps populated. A target whose hash is
+    // unchanged must therefore NOT be reported impacted just because its deps field differs
+    // (null vs a list). Regression test for the --trackDeps query-service over-reporting bug.
+    val from =
+        mapOf(
+            "//:unchanged" to TargetHash("Rule", "h", "h", deps = null),
+            "//:changed" to TargetHash("Rule", "old", "old", deps = null),
+        )
+    val to =
+        mapOf(
+            "//:unchanged" to TargetHash("Rule", "h", "h", deps = listOf("//:dep")),
+            "//:changed" to TargetHash("Rule", "new", "new", deps = listOf("//:dep")),
+        )
+
+    val impacted = CalculateImpactedTargetsInteractor().computeSimpleImpactedTargets(from, to)
+
+    assertThat(impacted).containsExactlyInAnyOrder("//:changed")
+  }
+
+  @Test
+  fun computeAllDistancesIgnoresDepsAsymmetry() {
+    // Same deps-asymmetry guard on the distance path (/impacted_targets_with_distances): an
+    // unchanged target must not surface merely because its deps field differs between a
+    // deserialised `from` and a freshly generated `to`.
+    val from =
+        mapOf(
+            "//:unchanged" to TargetHash("Rule", "h", "h", deps = null),
+            "//:changed" to TargetHash("Rule", "old", "old", deps = null),
+        )
+    val to =
+        mapOf(
+            "//:unchanged" to TargetHash("Rule", "h", "h", deps = listOf("//:changed")),
+            "//:changed" to TargetHash("Rule", "new", "new", deps = emptyList()),
+        )
+
+    val distances = CalculateImpactedTargetsInteractor().computeAllDistances(from, to, emptyMap())
+
+    assertThat(distances.keys).containsExactlyInAnyOrder("//:changed")
+  }
+
   @Test
   fun testExecuteSortsByKindThenLabel() {
     val startHashes =

cli/src/test/kotlin/com/bazel_diff/server/JGitClientTest.kt

@@ -159,4 +159,67 @@ class JGitClientTest : KoinTest {
     client.fetch()
     assertThat(client.resolveSha(sha2)).isEqualTo(sha2)
   }
+
+  @Test
+  fun fetchWithoutFallbackSurfacesJGitFailure() {
+    initRepoWithTwoCommits()
+    // origin points at a directory that is not a git repository, so the in-process JGit fetch
+    // fails. With the native fallback disabled the failure surfaces directly (legacy behaviour).
+    val bogus = File(temp.root, "not-a-repo").apply { mkdirs() }
+    git("remote", "add", "origin", bogus.absolutePath)
+    val client = JGitClient(temp.root.toPath(), gitPath = "git", nativeFetchFallback = false)
+    assertThrows(GitClientException::class.java) { client.fetch() }
+  }
+
+  @Test
+  fun fetchFallbackErrorNamesNativeGitWhenBothFail() {
+    initRepoWithTwoCommits()
+    // JGit fetch fails (origin is not a repo) AND the native fallback fails (bogus git binary).
+    // The surfaced error must name the native fallback so neither cause is hidden.
+    val bogus = File(temp.root, "not-a-repo").apply { mkdirs() }
+    git("remote", "add", "origin", bogus.absolutePath)
+    val client =
+        JGitClient(
+            temp.root.toPath(), gitPath = "/nonexistent/git", nativeFetchFallback = true)
+    val ex = assertThrows(GitClientException::class.java) { client.fetch() }
+    assertThat(ex).messageContains("native git fallback")
+  }
+
+  @Test
+  fun fetchBringsInNewCommitForShallowClone() {
+    // A shallow clone is a shape JGit 5.13 may be unable to fetch (the remote's thin pack can be
+    // delta-compressed against a base object below the shallow boundary). The client must fall
+    // back to native git so a just-landed commit still arrives; whichever path succeeds, the end
+    // state must be the new commit present and resolvable.
+    val origin = File(temp.root, "origin").apply { mkdirs() }
+    runGit(origin, "init", "-q")
+    runGit(origin, "config", "user.email", "test@example.com")
+    runGit(origin, "config", "user.name", "test")
+    File(origin, "file.txt").writeText("one")
+    runGit(origin, "add", ".")
+    runGit(origin, "commit", "-q", "-m", "first")
+    File(origin, "file.txt").writeText("two")
+    runGit(origin, "add", ".")
+    runGit(origin, "commit", "-q", "-m", "second")
+
+    val workspace = File(temp.root, "workspace")
+    runGit(
+        temp.root,
+        "clone",
+        "-q",
+        "--depth=1",
+        "file://${origin.absolutePath}",
+        workspace.absolutePath)
+
+    // A new commit lands after the shallow clone; the workspace has not fetched it yet.
+    File(origin, "file.txt").writeText("three")
+    runGit(origin, "add", ".")
+    runGit(origin, "commit", "-q", "-m", "third")
+    val sha3 = runGit(origin, "rev-parse", "HEAD")
+
+    val client = JGitClient(workspace.toPath())
+    assertThrows(MissingRevisionException::class.java) { client.resolveSha(sha3) }
+    client.fetch()
+    assertThat(client.resolveSha(sha3)).isEqualTo(sha3)
+  }
 }

tools/BUILD

@@ -54,3 +54,8 @@ py_test(
     python_version = "PY3",
     deps = [":coverage_check_lib"],
 )
+
+# Note: the serve integration harness (serve_harness.py) is intentionally NOT wrapped in a
+# py_binary. It shells out to `bazel build //cli:bazel-diff`, which would deadlock on the output-base
+# lock if it were itself launched via `bazel run` (nested bazel, same output base). Run it directly:
+# `python3 tools/serve_harness.py` (see its module docstring). It uses only the Python stdlib.

tools/readme_template.md

@@ -186,6 +186,13 @@ Notes and current limitations:
   `--gitEngine=subprocess` to shell out to the `git` binary at `--gitPath` instead -- useful for
   workspaces that depend on checkout filters or hooks that JGit does not run. Note that JGit only
   moves the git plumbing in-process; the working tree is still materialized on disk for `bazel query`.
+  JGit cannot fetch some clone shapes native git handles fine -- notably shallow (`--depth`) and
+  partial (`--filter=blob:none`) clones, whose thin packs are delta-compressed against objects the
+  clone does not have ("Missing delta base"). When an in-process JGit fetch fails, the service
+  automatically falls back to the native `git` binary (at `--gitPath`) for that fetch and logs a
+  warning; point `--gitEngine=subprocess` at such workspaces to skip the in-process attempt (a
+  partial clone in particular needs the subprocess engine to *serve* queries, since JGit cannot
+  lazily fetch the missing blobs a checkout needs).
 * Hashes are cached on local disk via `--cacheDir` and survive restarts. The cache layer is
   pluggable behind a byte-oriented interface so a remote backend (e.g. S3) can be added without
   touching callers.