build: review pass 4 — address HIGH + MED findings from PR #177

HIGH:
  #1 PruneResult.FreedBytes now reflects bytes actually reclaimed
     (sum over result.Removed after the loop), not the optimistic
     plan total. Honors the build-prune.schema.json contract under
     partial failure.

  #2 Prune now holds the FR-015 per-key flock around each entry's
     removal — same lock builder.Run() acquires — so a concurrent
     `trond build` cannot interleave with our manifest+artifact
     delete. New non-blocking TryAcquireCacheLock variant
     (posix: LOCK_EX|LOCK_NB; windows: TryLock) so a background
     prune never stalls an interactive build; locked entries are
     skipped with an info-level stderr line.

MEDIUM:
  #3 Sha256IfExists's *ssh.ExitError classification is verified
     against the actual wrap shape Exec emits (fmt.Errorf with %w)
     via 3 new sub-tests in internal/target/ssh_sha256_classify_test.go.
     Documents the invariant: changing Exec's wrap verb breaks
     this and the test catches it.

  #4 resolveBuild omits JDKVersion from CacheKey when Builder=host
     — the BuilderImageDigest (sha256 of `java -version`) already
     captures the actual JVM, so including --jdk would fragment
     the cache across two invocations that produce identical
     artifacts.

  #5 findLargestFatJAR matches `find -path '*/build/libs/*.jar'`
     exactly (parent dir must be `build/libs/`), not just any
     `/build/libs/` substring. Pins cross-builder cache reuse:
     host and docker builders pick the same fat JAR even in
     unusual layouts (`build/libs/sub/x.jar`, `libs-archive/`,
     wrong-order segments).

  #6 TestRunner interface widened to pass outDir through so test
     fakes can populate image-snapshot files. Adapter updated;
     two existing test callers (apply, cmd) updated to the new
     5-arg signature.

  #7 build_prune footgun guard: `--keep-last N --confirm` with
     no scoping filter is now rejected at both CLI and MCP
     layers. Operator must either add --all (explicit acknowledge
     of the near-wipe) or narrow with --orphan/--older-than.
     Dry-run is exempt — the plan output is the affordance for
     "what would keep_last remove".

Tests added (all green):
  - TestPrune_FreedBytesOnlyCountsActuallyRemoved
  - TestPrune_FreedBytesOnDryRunMatchesPlan
  - TestPrune_AcquiresCacheLock
  - TestFindLargestFatJAR_NestedAndDecoyPaths
  - TestResolveBuild_HostBuilder_JDKVersionDoesNotFragmentCacheKey
  - TestBuildPrune_KeepLastFootgunGuard (cobra, 3 sub-cases,
    properly isolated via paths.SetBaseDir so the test never
    walks the dev's real ~/.trond cache)
  - TestBuildPrune_KeepLastAloneConfirmRejected (MCP, 3 sub-cases)
  - TestSha256_ExitErrorClassification (3 sub-cases)

Author: barbatos2011

cmd/build_flags_test.go

@@ -24,7 +24,7 @@ type flagsCaptureRunner struct {
 
 func (f *flagsCaptureRunner) RunDockerBuild(
 	_ context.Context,
-	sourcePath, _ string,
+	sourcePath, _ /* outDir */, _ /* outTmpPath */ string,
 	gradleTask string,
 	gradleArgs []string,
 	env map[string]string,

cmd/build_mgmt_test.go

@@ -1,10 +1,12 @@
 package cmd
 
 import (
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/tronprotocol/tron-deployment/internal/build"
+	"github.com/tronprotocol/tron-deployment/internal/paths"
 )
 
 // Cobra-layer tests for `trond build list / inspect / prune`. The
@@ -82,6 +84,71 @@ func TestSortEntries(t *testing.T) {
 	})
 }
 
+// TestBuildPrune_KeepLastFootgunGuard pins the review-pass-4 CLI
+// guard: --keep-last with --confirm but no scoping filter would
+// wipe everything-but-N — required either --all (explicit
+// acknowledge) or a scoping filter (--orphan / --older-than). Dry-
+// run is exempt; the plan output is the affordance.
+//
+// Isolates paths.BaseDir to a TempDir so the sub-tests that pass the
+// guard and reach the real Prune call don't walk the developer's
+// actual ~/.trond cache — a footgun that would have deleted real
+// build artifacts during test runs without this isolation.
+func TestBuildPrune_KeepLastFootgunGuard(t *testing.T) {
+	dir := t.TempDir()
+	paths.SetBaseDir(dir)
+	t.Cleanup(func() { paths.SetBaseDir("") })
+
+	// Reset flag state between sub-tests. Cobra StringVars/BoolVars
+	// persist between Execute() calls because they're package vars,
+	// so explicitly clear before each scenario.
+	reset := func() {
+		buildPruneAll = false
+		buildPruneOlderThan = 0
+		buildPruneKeepLast = 0
+		buildPruneOrphan = false
+		buildPruneConfirm = false
+	}
+
+	t.Run("--keep-last --confirm alone is rejected", func(t *testing.T) {
+		reset()
+		buildPruneKeepLast = 1
+		buildPruneConfirm = true
+		err := runBuildPrune(buildPruneCmd, nil)
+		if err == nil {
+			t.Fatal("expected validation error")
+		}
+		if !strings.Contains(err.Error(), "would wipe everything except") {
+			t.Errorf("error %q should mention the footgun", err)
+		}
+	})
+
+	t.Run("--keep-last (dry-run) is allowed", func(t *testing.T) {
+		reset()
+		buildPruneKeepLast = 1
+		// confirm=false → dry-run path; should NOT trigger the guard.
+		err := runBuildPrune(buildPruneCmd, nil)
+		// May succeed or fail at the actual Prune call (empty cache
+		// dir in test) — what we're pinning is that the guard does
+		// not fire. So the error, if any, must NOT mention the
+		// footgun string.
+		if err != nil && strings.Contains(err.Error(), "would wipe everything except") {
+			t.Errorf("dry-run keep_last triggered the footgun guard; got %q", err)
+		}
+	})
+
+	t.Run("--keep-last --all --confirm is allowed (explicit)", func(t *testing.T) {
+		reset()
+		buildPruneKeepLast = 1
+		buildPruneAll = true
+		buildPruneConfirm = true
+		err := runBuildPrune(buildPruneCmd, nil)
+		if err != nil && strings.Contains(err.Error(), "would wipe everything except") {
+			t.Errorf("explicit --all+keep-last+confirm triggered the footgun guard; got %q", err)
+		}
+	})
+}
+
 // TestHumanBytes pins the table formatter. The numbers feed both
 // 'trond build list' (table mode) and 'trond build prune' (dry-run
 // output), so a regression here is loudly visible.

cmd/build_prune.go

@@ -96,6 +96,27 @@ func runBuildPrune(cmd *cobra.Command, _ []string) error {
 				"To remove entries older than a week: trond build prune --older-than 168h --confirm",
 			)
 	}
+	// Footgun guard: `--keep-last N --confirm` with NO other filter
+	// is equivalent to "delete every entry except the N newest" —
+	// a near-wipe operation that looks small at a glance. Require
+	// either an explicit second filter (--orphan / --older-than)
+	// to scope what gets pruned, OR an explicit --all to acknowledge
+	// the near-wipe intent. Dry-run is exempt: the plan output
+	// shows exactly what would be deleted, which IS the obvious
+	// affordance an interactive operator wants.
+	if buildPruneConfirm && buildPruneKeepLast > 0 &&
+		!buildPruneAll && !buildPruneOrphan && buildPruneOlderThan == 0 {
+		return output.NewError("VALIDATION_ERROR", output.ExitValidationError,
+			"--keep-last alone with --confirm would wipe everything except "+
+				"the N newest entries; combine with --all to acknowledge, OR "+
+				"narrow with --orphan / --older-than").
+			WithSuggestions(
+				"Preview first: trond build prune --keep-last "+
+					fmt.Sprintf("%d", buildPruneKeepLast)+" (dry-run shows the plan)",
+				"To genuinely wipe-all-but-N: trond build prune --all --keep-last "+
+					fmt.Sprintf("%d", buildPruneKeepLast)+" --confirm",
+			)
+	}
 
 	opts := build.PruneOptions{
 		All:        buildPruneAll,

internal/apply/build_test.go

@@ -25,7 +25,7 @@ type fakeBuilderRunner struct {
 
 func (f *fakeBuilderRunner) RunDockerBuild(
 	_ context.Context,
-	_ /* sourcePath */, outTmp string,
+	_ /* sourcePath */, _ /* outDir */, outTmp string,
 	_ /* gradleTask */ string,
 	_ /* gradleArgs */ []string,
 	_ /* env */ map[string]string,

internal/build/builder.go

@@ -268,11 +268,23 @@ func resolveBuild(ctx context.Context, req Request) (*resolved, error) {
 			)
 	}
 
+	// For host builds, BuilderImageDigest already captures the exact
+	// JVM in use (sha256 of `java -version` output). Including
+	// req.JDKVersion in the key on top of that would fragment the
+	// cache pointlessly: two `--builder host` invocations with the
+	// same actual JDK but different --jdk flags (e.g. --jdk 8 vs
+	// --jdk 17, both falling back to whatever the host has)
+	// rebuild identically. Drop JDKVersion from the host-builder
+	// key so cache hits work as expected.
+	keyJDK := req.JDKVersion
+	if req.Builder == "host" {
+		keyJDK = ""
+	}
 	key := CacheKey{
 		GitRevision:        src.ResolvedRevision,
 		PatchHash:          src.PatchHash,
 		BuilderImageDigest: imageDigest,
-		JDKVersion:         req.JDKVersion,
+		JDKVersion:         keyJDK,
 		ArtifactKind:       req.ArtifactKind,
 		GradleTask:         req.GradleTask,
 		GradleArgs:         append([]string(nil), req.GradleArgs...),

internal/build/cache_ops.go

@@ -244,8 +244,10 @@ type PruneOptions struct {
 // PruneResult is the structured output the CLI/MCP layer renders.
 // Plan is what WOULD be removed; Removed is what was actually
 // removed (nil when DryRun). FreedBytes is the sum of SizeBytes for
-// Removed entries (DryRun: sum over Plan, since we know what we
-// would have freed).
+// successfully-removed entries (or, on DryRun, the sum over Plan).
+// Schema contract: schemas/output/build-prune.schema.json describes
+// freed_bytes as "Total bytes reclaimed"; the post-loop recomputation
+// below honors that even when one entry's removal partially failed.
 type PruneResult struct {
 	Plan       []*Entry `json:"plan"`
 	Removed    []*Entry `json:"removed,omitempty"`
@@ -254,10 +256,25 @@ type PruneResult struct {
 }
 
 // Prune evaluates the cache against opts, builds a deletion plan, and
-// (unless DryRun) executes it. Image artifacts get a best-effort
-// `docker rmi <tag>` so the docker storage actually reclaims layers;
-// failures there don't abort the prune (the trond cache files still
-// get cleaned).
+// (unless DryRun) executes it under per-entry flocks so concurrent
+// `trond build` runs against the same cache key cannot interleave
+// with our manifest/artifact deletion. Image artifacts also get a
+// best-effort `docker rmi <tag>` so the docker storage actually
+// reclaims layers; failures there don't abort the prune (the trond
+// cache files still get cleaned).
+//
+// Concurrency invariants:
+//
+//   - The flock per cache key matches the one builder.go acquires in
+//     Run() (AcquireCacheLock). A concurrent build of the same key
+//     either finishes first (we then prune the produced artifact,
+//     unsurprising) or blocks until we release (the build then sees
+//     no manifest and rebuilds, also unsurprising). No race window
+//     where Prune deletes a half-written manifest mid-build.
+//   - Two concurrent Prune invocations on the same entry race only
+//     on the lock acquisition; the second sees the manifest already
+//     gone in removeEntry and treats it as best-effort (the
+//     errors.Is(..., os.ErrNotExist) branch).
 func Prune(ctx context.Context, opts PruneOptions) (*PruneResult, error) {
 	all, err := ListEntries(ctx, IncludeOrphans())
 	if err != nil {
@@ -269,25 +286,48 @@ func Prune(ctx context.Context, opts PruneOptions) (*PruneResult, error) {
 		Plan:   plan,
 		DryRun: opts.DryRun,
 	}
-	for _, e := range plan {
-		result.FreedBytes += e.SizeBytes
-	}
 	if opts.DryRun {
+		// Dry-run reports what WOULD be freed — the plan's full
+		// size, since nothing is removed.
+		for _, e := range plan {
+			result.FreedBytes += e.SizeBytes
+		}
 		return result, nil
 	}
 
 	for _, e := range plan {
-		if err := removeEntry(ctx, e); err != nil {
+		// FR-015 lock — same key the builder grabs in Run(). We use
+		// the non-blocking try-variant: if a build holds this key
+		// right now, prune skips the entry rather than waiting (a
+		// background prune should NEVER stall an interactive build).
+		release, ok, lockErr := TryAcquireCacheLock(CacheDir(), e.CacheKey)
+		if lockErr != nil {
+			fmt.Fprintf(os.Stderr,
+				"warning: prune skip %s — lock open failed: %v\n",
+				e.CacheKey, lockErr)
+			continue
+		}
+		if !ok {
+			fmt.Fprintf(os.Stderr,
+				"info: prune skip %s — build in progress for this key\n",
+				e.CacheKey)
+			continue
+		}
+		err := removeEntry(ctx, e)
+		release()
+		if err != nil {
 			// Don't abort the whole prune — one wedged docker rmi
 			// shouldn't block the rest of the cleanup. Surface to
-			// stderr; the result.Removed list reflects what
-			// succeeded.
+			// stderr; result.Removed + FreedBytes reflect only what
+			// actually succeeded so the JSON contract holds even
+			// after partial failures.
 			fmt.Fprintf(os.Stderr,
 				"warning: prune partial failure for %s: %v\n",
 				e.CacheKey, err)
 			continue
 		}
 		result.Removed = append(result.Removed, e)
+		result.FreedBytes += e.SizeBytes
 	}
 	return result, nil
 }

internal/build/cache_ops_test.go

@@ -318,6 +318,123 @@ func TestPrune_OrphanCleansManifestEvenWithoutArtifact(t *testing.T) {
 	}
 }
 
+// TestPrune_AcquiresCacheLock pins the review-pass-4 race fix: Prune
+// MUST hold the same per-key flock that builder.Run() acquires, so a
+// concurrent build of the entry being pruned cannot interleave with
+// our manifest+artifact deletion. Verified by holding the flock
+// externally and observing Prune's "lock unavailable" skip path.
+//
+// (We can't easily reproduce the data corruption directly in a unit
+// test — too much process orchestration. This test pins the
+// contract: "if the lock can't be acquired, the entry is skipped",
+// which is the post-condition that protects the cache.)
+func TestPrune_AcquiresCacheLock(t *testing.T) {
+	withTempBaseDir(t)
+	if err := EnsureCacheDirs(); err != nil {
+		t.Fatalf("EnsureCacheDirs: %v", err)
+	}
+	locked := seedJARManifest(t, "locked-by-build", time.Now(), 100)
+
+	// Simulate a concurrent build: hold the flock for this entry's
+	// key. Prune must observe the lock and skip without touching
+	// anything.
+	release, err := AcquireCacheLock(CacheDir(), locked.CacheKey)
+	if err != nil {
+		t.Fatalf("test AcquireCacheLock: %v", err)
+	}
+	t.Cleanup(release)
+
+	res, err := Prune(context.Background(), PruneOptions{All: true})
+	if err != nil {
+		t.Fatalf("Prune: %v", err)
+	}
+	// Plan still lists the entry (we never released the lock until
+	// AFTER Prune returns), but Removed must NOT contain it because
+	// Prune couldn't acquire the lock.
+	if len(res.Removed) != 0 {
+		t.Errorf("Removed should be empty when lock is held externally; got %v", keys(res.Removed))
+	}
+	// And the JAR + manifest are still on disk — no partial
+	// deletion.
+	if _, statErr := os.Stat(locked.ArtifactPath); statErr != nil {
+		t.Errorf("locked JAR should be untouched; stat err = %v", statErr)
+	}
+	if _, statErr := os.Stat(manifestPath(locked.CacheKey)); statErr != nil {
+		t.Errorf("locked manifest should be untouched; stat err = %v", statErr)
+	}
+}
+
+// TestPrune_FreedBytesOnlyCountsActuallyRemoved is the review-pass-4
+// regression guard: PruneResult.FreedBytes MUST reflect bytes
+// actually reclaimed, not the plan's optimistic total. Otherwise an
+// MCP agent surfacing "freed N bytes" would report a number that
+// doesn't match the bytes the OS actually got back.
+//
+// Constructed scenario: two entries in the plan, simulate partial
+// failure by pre-deleting one entry's manifest under our feet so
+// removeEntry's os.Remove(manifestPath) fails. The successful entry
+// contributes its bytes; the failed entry does not.
+func TestPrune_FreedBytesOnlyCountsActuallyRemoved(t *testing.T) {
+	withTempBaseDir(t)
+	good := seedJARManifest(t, "good", time.Now(), 500)
+	bad := seedJARManifest(t, "bad", time.Now().Add(-time.Hour), 1000)
+
+	// Sabotage the "bad" entry's artifact: swap the JAR file for a
+	// non-empty directory at the same path. removeEntry's
+	// os.Remove(e.ArtifactPath) refuses to delete a non-empty dir
+	// → entry removal fails. ListEntries still parses the (intact)
+	// manifest, so the entry IS in the plan but won't end up in
+	// Removed. Faithfully models real failures the reviewer flagged
+	// (docker rmi wedged, fs permissions, etc.).
+	if err := os.Remove(bad.ArtifactPath); err != nil {
+		t.Fatalf("setup: remove jar for sabotage: %v", err)
+	}
+	if err := os.MkdirAll(bad.ArtifactPath, 0o755); err != nil {
+		t.Fatalf("setup: mkdir jar path for sabotage: %v", err)
+	}
+	if err := os.WriteFile(filepath.Join(bad.ArtifactPath, "wedge"), []byte("x"), 0o600); err != nil {
+		t.Fatalf("setup: wedge file: %v", err)
+	}
+
+	res, err := Prune(context.Background(), PruneOptions{All: true})
+	if err != nil {
+		t.Fatalf("Prune: %v", err)
+	}
+	// Plan has both entries; Removed has only the good one.
+	if len(res.Plan) != 2 {
+		t.Errorf("Plan should list 2 entries; got %d", len(res.Plan))
+	}
+	if len(res.Removed) != 1 || res.Removed[0].CacheKey != "good" {
+		t.Fatalf("Removed should contain only 'good'; got %v", keys(res.Removed))
+	}
+	// THE FIX: FreedBytes must equal Removed-only sum, NOT Plan sum.
+	if res.FreedBytes != 500 {
+		t.Errorf("FreedBytes = %d; want 500 (bytes actually reclaimed, not plan total of 1500)", res.FreedBytes)
+	}
+	// And good's artifact should be gone.
+	if _, statErr := os.Stat(good.ArtifactPath); !os.IsNotExist(statErr) {
+		t.Errorf("good JAR should be deleted; stat err = %v", statErr)
+	}
+}
+
+// TestPrune_FreedBytesOnDryRunMatchesPlan: on dry-run, FreedBytes
+// reflects what WOULD be freed (== plan sum), since Removed is
+// empty by design. Without this branch in Prune, the post-Removed-
+// loop accumulation would report 0 freed for dry-runs.
+func TestPrune_FreedBytesOnDryRunMatchesPlan(t *testing.T) {
+	withTempBaseDir(t)
+	seedJARManifest(t, "a", time.Now(), 100)
+	seedJARManifest(t, "b", time.Now(), 200)
+
+	res, err := Prune(context.Background(), PruneOptions{All: true, DryRun: true})
+	if err != nil {
+		t.Fatalf("Prune: %v", err)
+	}
+	if res.FreedBytes != 300 {
+		t.Errorf("DryRun FreedBytes = %d; want 300 (full plan total)", res.FreedBytes)
+	}
+}
+
 // --- helpers ---
 
 func keys(entries []*Entry) []string {