build: review pass 5 — close MCP guard bypass + add ctx cancel

HIGH: MCP build_prune footgun guard was bypassed by older_than=
"0s" / "0h" / "-1h". The string-based check (args.OlderThan == "")
treated those values as "filter set"; ParseDuration accepted them;
selectForPrune then silently ignored the zero/negative result
(gates on OlderThan > 0). Net: keep_last=N + confirm=true +
older_than="0s" passed both guards and reached the near-wipe path
the guard exists to prevent.

Fix: parse older_than FIRST in buildPruneTool, derive a hasOlderThan
boolean from the parsed value (> 0), then run all guards against
that boolean. Wording also clarified: "older_than (>0)" in the
suggestion messages so an LLM doesn't try "0s" thinking it's valid.

MED: Prune's per-entry loop didn't honor ctx cancellation between
iterations. Each iteration may run a `docker image rm` (image
artifacts) that can take seconds; without an explicit ctx.Err()
check, Ctrl+C only takes effect at the next docker round-trip,
making the CLI feel unresponsive on long prunes. Added a single
ctx.Err() check at the top of each iteration; on cancel, returns
(result, context.Canceled) with Plan still populated so the caller
sees what would have been done.

Tests added:
  - TestBuildPrune_ZeroDurationDoesNotBypassFootgun (3 sub-cases:
    "0s", "0h", "-1h" all correctly rejected with the footgun
    message — the exact LLM mistake the guard exists to prevent)
  - TestPrune_HonorsContextCancellation (cancel before iteration
    one; assert context.Canceled propagates and Removed is empty)

Author: barbatos2011

internal/build/cache_ops.go

@@ -296,6 +296,15 @@ func Prune(ctx context.Context, opts PruneOptions) (*PruneResult, error) {
 	}
 
 	for _, e := range plan {
+		// Honor cancellation between entries — each iteration may
+		// include a `docker image rm` (image artifacts) that can
+		// run for tens of seconds; without this check, Ctrl+C only
+		// takes effect when the next docker call returns. Plan
+		// remains populated so the caller sees what WOULD have
+		// been removed at the cancellation point.
+		if err := ctx.Err(); err != nil {
+			return result, err
+		}
 		// 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

internal/build/cache_ops_test.go

@@ -3,6 +3,7 @@ package build
 import (
 	"context"
 	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
@@ -318,6 +319,39 @@ func TestPrune_OrphanCleansManifestEvenWithoutArtifact(t *testing.T) {
 	}
 }
 
+// TestPrune_HonorsContextCancellation is the review-pass-5 guard:
+// a cancelled ctx must stop the per-entry loop promptly, even mid-
+// plan. Without the check, Ctrl+C during a long prune (many image
+// entries with slow `docker image rm` calls) only takes effect at
+// the next docker round-trip. Plan stays populated so the caller
+// can still see what WOULD have been done.
+func TestPrune_HonorsContextCancellation(t *testing.T) {
+	withTempBaseDir(t)
+	for i := range 5 {
+		seedJARManifest(t, fmt.Sprintf("entry-%d", i), time.Now(), 100)
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel() // cancel BEFORE Prune to guarantee the first iteration trips the check
+
+	res, err := Prune(ctx, PruneOptions{All: true})
+	if err == nil {
+		t.Fatal("expected ctx.Err() to surface from Prune; got nil")
+	}
+	if !errors.Is(err, context.Canceled) {
+		t.Errorf("expected context.Canceled; got %v", err)
+	}
+	// Plan should still be populated (we want the caller to know
+	// what would have been removed at cancellation time).
+	if len(res.Plan) != 5 {
+		t.Errorf("Plan should still have 5 entries on cancel; got %d", len(res.Plan))
+	}
+	// Removed should be empty — we cancelled before any deletion.
+	if len(res.Removed) != 0 {
+		t.Errorf("Removed should be empty when cancelled before first iteration; got %d", len(res.Removed))
+	}
+}
+
 // 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

internal/mcp/tools_build.go

@@ -114,11 +114,35 @@ func buildInspectTool(ctx context.Context, _ *mcp.CallToolRequest, args buildIns
 }
 
 func buildPruneTool(ctx context.Context, _ *mcp.CallToolRequest, args buildPruneArgs) (*mcp.CallToolResult, any, error) {
+	// Parse older_than FIRST so subsequent guards see the actual
+	// effective duration, not the raw string. An LLM passing
+	// older_than="0s" / "0h" / "-1h" otherwise sneaks past the
+	// string-based "is this filter set" check while the parsed
+	// value ends up being silently ignored by selectForPrune
+	// (which gates on `OlderThan > 0`). Net: the footgun guard
+	// below would not fire and we'd wipe everything-but-keep_last
+	// — the exact LLM mistake the guard exists to prevent.
+	var olderThan time.Duration
+	if args.OlderThan != "" {
+		d, err := time.ParseDuration(args.OlderThan)
+		if err != nil {
+			return errResult(output.NewErrorf("VALIDATION_ERROR", output.ExitValidationError,
+				"older_than %q is not a valid Go duration: %s", args.OlderThan, err.Error()).
+				WithSuggestions("Use Go duration syntax: '24h', '168h' (= 7 days), '720h' (= 30 days)"))
+		}
+		olderThan = d
+	}
+	// Effective scoping flag: true only when older_than actually
+	// narrows the candidate set. Zero or negative durations are
+	// treated as "not set" so the guards below catch the bypass
+	// the previous review identified.
+	hasOlderThan := olderThan > 0
+
 	// Same friendly guard as the CLI: empty policy is almost always
 	// an LLM mistake, not an intent to no-op.
-	if !args.All && !args.OrphanOnly && args.OlderThan == "" && args.KeepLast == 0 {
+	if !args.All && !args.OrphanOnly && !hasOlderThan && args.KeepLast == 0 {
 		return errResult(output.NewError("VALIDATION_ERROR", output.ExitValidationError,
-			"prune needs at least one of: all, orphan_only, older_than, keep_last").
+			"prune needs at least one of: all, orphan_only, older_than (>0), keep_last").
 			WithSuggestions(
 				"To wipe everything: build_prune with all=true confirm=true",
 				"To remove orphans only: build_prune with orphan_only=true confirm=true",
@@ -131,28 +155,17 @@ func buildPruneTool(ctx context.Context, _ *mcp.CallToolRequest, args buildPrune
 	// down to recent". Require either all=true (explicit acknowledge)
 	// or a scoping filter (orphan_only / older_than).
 	if args.Confirm && args.KeepLast > 0 &&
-		!args.All && !args.OrphanOnly && args.OlderThan == "" {
+		!args.All && !args.OrphanOnly && !hasOlderThan {
 		return errResult(output.NewError("VALIDATION_ERROR", output.ExitValidationError,
 			"keep_last alone with confirm=true would wipe everything except "+
 				"the N newest entries; set all=true to acknowledge, OR "+
-				"narrow with orphan_only / older_than").
+				"narrow with orphan_only / older_than (>0)").
 			WithSuggestions(
 				"Preview first: omit confirm=true (the plan shows exactly what would be removed)",
 				"To genuinely wipe-all-but-N: all=true keep_last=N confirm=true",
 			))
 	}
 
-	var olderThan time.Duration
-	if args.OlderThan != "" {
-		d, err := time.ParseDuration(args.OlderThan)
-		if err != nil {
-			return errResult(output.NewErrorf("VALIDATION_ERROR", output.ExitValidationError,
-				"older_than %q is not a valid Go duration: %s", args.OlderThan, err.Error()).
-				WithSuggestions("Use Go duration syntax: '24h', '168h' (= 7 days), '720h' (= 30 days)"))
-		}
-		olderThan = d
-	}
-
 	res, err := build.Prune(ctx, build.PruneOptions{
 		All:        args.All,
 		OlderThan:  olderThan,

internal/mcp/tools_build_test.go

@@ -241,6 +241,51 @@ func TestBuildPrune_KeepLastAloneConfirmRejected(t *testing.T) {
 	})
 }
 
+// TestBuildPrune_ZeroDurationDoesNotBypassFootgun is the review-
+// pass-5 hardening for the MCP footgun guard. The previous check
+// used `args.OlderThan == ""` to detect "no scoping filter", but
+// `older_than: "0s"` (or "0h", or "-1h") parses fine, ends up as
+// duration zero / negative, then `selectForPrune`'s `> 0` gate
+// silently ignores it — bypassing the guard the LLM cannot have
+// realized existed. Fix parses older_than FIRST and treats only
+// positive durations as a real scoping filter.
+func TestBuildPrune_ZeroDurationDoesNotBypassFootgun(t *testing.T) {
+	session, cleanup := newConnectedPair(t)
+	defer cleanup()
+
+	cases := []struct {
+		name string
+		dur  string
+	}{
+		{"zero seconds", "0s"},
+		{"zero hours", "0h"},
+		{"negative", "-1h"},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			res, err := session.CallTool(context.Background(), &mcp.CallToolParams{
+				Name: "build_prune",
+				Arguments: map[string]any{
+					"keep_last":  1,
+					"confirm":    true,
+					"older_than": c.dur,
+				},
+			})
+			if err != nil {
+				t.Fatalf("CallTool: %v", err)
+			}
+			if !res.IsError {
+				t.Errorf("older_than=%q must NOT bypass the footgun guard; "+
+					"got IsError=false (would have wiped everything-but-newest)", c.dur)
+			}
+			body := extractText(t, res)
+			if !contains(body, "would wipe everything except") {
+				t.Errorf("error should mention the footgun; got %q", body)
+			}
+		})
+	}
+}
+
 // TestBuildPrune_BadDuration: invalid older_than surfaces a clear
 // validation error instead of silently being ignored.
 func TestBuildPrune_BadDuration(t *testing.T) {