Merge pull request #1816 from entireio/fix/explain-export-notfound

fix(explain): surface export-path failures instead of masking as not-found

Author: peyton-alt

cmd/entire/cli/explain_export.go

@@ -6,11 +6,13 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"log/slog"
 	"strings"
 	"time"
 
 	"github.com/entireio/cli/cmd/entire/cli/checkpoint"
 	"github.com/entireio/cli/cmd/entire/cli/checkpoint/id"
+	"github.com/entireio/cli/cmd/entire/cli/logging"
 	"github.com/entireio/cli/cmd/entire/cli/settings"
 	"github.com/entireio/cli/cmd/entire/cli/strategy"
 	"github.com/entireio/cli/cmd/entire/cli/trailers"
@@ -128,28 +130,68 @@ func resolveExplainCheckpointID(ctx context.Context, errW io.Writer, opts explai
 				_ = lookup.Close()
 				return cpID, freshLookup, nil
 			}
+			// Only "the target isn't a commit at all" is a genuine miss that
+			// keeps the checkpoint-not-found report below. Everything else —
+			// unreadable commit, missing trailer, ambiguous ref, repo or
+			// lookup failure — reflects a real failure, not a miss; masking
+			// those as not-found is this path's variant of the conflation
+			// PR #1812 fixes for the prose path in runExplainAuto (this
+			// path's fix: issue #1814).
+			if !errors.Is(commitErr, errExportTargetNotCommit) {
+				if freshLookup != nil {
+					// Defensive: resolveCheckpointFromCommitRef documents a
+					// nil lookup on error, but a leak here would be silent.
+					_ = freshLookup.Close()
+				}
+				return id.CheckpointID(""), lookup, commitErr
+			}
 		}
 		return id.CheckpointID(""), lookup, fmt.Errorf("%w: %s", checkpoint.ErrCheckpointNotFound, prefix)
 	default:
-		return id.CheckpointID(""), lookup, fmt.Errorf("%w: %s matches %d checkpoints", errAmbiguousCommitPrefix, prefix, len(matches))
+		ids := make([]string, len(matches))
+		for i, m := range matches {
+			ids[i] = m.String()
+		}
+		return id.CheckpointID(""), lookup, fmt.Errorf("%w: %s matches %d checkpoints (%s)", errAmbiguousCommitPrefix, prefix, len(matches), strings.Join(ids, ", "))
 	}
 }
 
+// errExportTargetNotCommit marks resolveCheckpointFromCommitRef's genuine
+// "target does not resolve to any commit" outcome, so
+// resolveExplainCheckpointID's positional commit fallback can fall through to
+// its checkpoint-not-found report only for that case and surface every other
+// failure verbatim.
+var errExportTargetNotCommit = errors.New("commit not found")
+
 // resolveCheckpointFromCommitRef opens the repo, resolves a git commit-ish,
 // and extracts the Entire-Checkpoint trailer. If the resolved checkpoint
 // isn't present in the local committed list, retries once after fetching
 // metadata from the remote — symmetry with the prefix path so
 // `--commit <sha>` and `--checkpoint <prefix>` share the same fetch
 // behavior.
+//
+// Invariant: on every error return the lookup is nil (any lookup created
+// along the way is closed internally), so callers may drop the lookup slot
+// without closing it when err != nil.
 func resolveCheckpointFromCommitRef(ctx context.Context, errW io.Writer, commitRef string) (id.CheckpointID, *explainCheckpointLookup, error) {
 	repo, err := openRepository(ctx)
 	if err != nil {
 		return id.CheckpointID(""), nil, fmt.Errorf("not a git repository: %w", err)
 	}
 	defer repo.Close()
-	hash, _, err := resolveCommitUnambiguous(repo, commitRef)
+	hash, ambiguousMatches, err := resolveCommitUnambiguous(repo, commitRef)
 	if err != nil {
-		return id.CheckpointID(""), nil, fmt.Errorf("commit not found: %s: %w", commitRef, err)
+		if errors.Is(err, errAmbiguousCommitPrefix) {
+			// The target IS commit-like (several commits match); reporting it
+			// as not-found would misdirect. Surface the ambiguity, naming the
+			// candidates so the user can disambiguate without rerunning git log.
+			candidates := make([]string, 0, len(ambiguousMatches))
+			for _, m := range buildAmbiguousCommitMatches(repo, ambiguousMatches) {
+				candidates = append(candidates, m.ShortID)
+			}
+			return id.CheckpointID(""), nil, fmt.Errorf("ambiguous commit ref %s (matches commits %s): %w", commitRef, strings.Join(candidates, ", "), err)
+		}
+		return id.CheckpointID(""), nil, fmt.Errorf("%w: %s: %w", errExportTargetNotCommit, commitRef, err)
 	}
 	commit, err := repo.CommitObject(hash)
 	if err != nil {
@@ -168,12 +210,21 @@ func resolveCheckpointFromCommitRef(ctx context.Context, errW io.Writer, commitR
 	// same remote-fetch retry the prefix path uses; otherwise downstream
 	// metadata reads would fail with an immediate "not found".
 	if !lookupHasCheckpoint(lookup, cpID) {
-		if matches, fresh := matchCheckpointPrefixWithRemoteFallback(ctx, errW, lookup, cpID.String()); len(matches) == 1 {
-			if fresh != lookup {
-				_ = lookup.Close()
-			}
+		matches, fresh := matchCheckpointPrefixWithRemoteFallback(ctx, errW, lookup, cpID.String())
+		if fresh != lookup {
+			_ = lookup.Close()
 			lookup = fresh
 		}
+		if len(matches) != 1 {
+			// The commit resolved and its trailer parsed; the checkpoint is
+			// simply not obtainable here. Failing now with the linkage beats
+			// succeeding and letting a downstream read die with a bare
+			// "checkpoint not found" that misdirects the user toward the
+			// checkpoint ID when the problem is availability (offline,
+			// unfetchable remote, or genuinely gone).
+			_ = lookup.Close()
+			return id.CheckpointID(""), nil, fmt.Errorf("commit %s references checkpoint %s, which is not available locally and could not be fetched from the remote", commitRef, cpID)
+		}
 	}
 	return cpID, lookup, nil
 }
@@ -202,11 +253,16 @@ func matchCheckpointPrefixWithRemoteFallback(ctx context.Context, errW io.Writer
 	// git-refs primary: there is no single metadata branch to fetch — each
 	// checkpoint is its own ref. When the prefix is a full checkpoint ID (the
 	// Entire-Checkpoint commit trailer always is), fetch that one ref directly,
-	// then re-list. Falls through to the v1-branch fetch below otherwise.
+	// then re-list. A shorter prefix cannot be fetched per-ref, and under a
+	// refs primary there is no v1 metadata branch to fetch either, so a
+	// short-prefix miss stays local-only.
 	if cpCfg, _ := settings.LoadCheckpointsConfig(ctx); checkpoint.PrimaryIsRefs(cpCfg) { //nolint:errcheck // fail-soft: bad config surfaces via Open elsewhere
-		if cid, err := id.NewCheckpointID(prefix); err == nil {
+		if cid, err := id.NewCheckpointID(prefix); err != nil {
+			logging.Debug(ctx, "explain: prefix is not a full checkpoint ID; refs-primary store cannot fetch by prefix, treating as no match",
+				slog.String("prefix", prefix))
+		} else {
 			// cid is already validated by NewCheckpointID above, so RefName can't
-			// error here; the guard is defensive — fall back to the v1-branch path
+			// error here; the guard is defensive — treat it as a local-only miss
 			// rather than fetch a malformed ref.
 			refName, refErr := checkpoint.RefName(cid)
 			if refErr != nil {
@@ -216,12 +272,23 @@ func matchCheckpointPrefixWithRemoteFallback(ctx context.Context, errW io.Writer
 			fetchErr := FetchCheckpointRef(ctx, refName)
 			stop(false)
 			if fetchErr == nil {
-				if fresh, freshErr := newExplainCheckpointLookup(ctx); freshErr == nil {
+				fresh, freshErr := newExplainCheckpointLookup(ctx)
+				if freshErr == nil {
 					if m := matchCheckpointPrefix(fresh, prefix); len(m) > 0 {
 						return m, fresh
 					}
 					_ = fresh.Close()
+				} else {
+					// The collapse to "no match" below reads to the user as
+					// "doesn't exist"; record what actually failed (issue #1815).
+					logging.Debug(ctx, "explain: lookup rebuild after checkpoint ref fetch failed; treating as no match",
+						slog.String("prefix", prefix),
+						slog.String("error", freshErr.Error()))
 				}
+			} else {
+				logging.Debug(ctx, "explain: on-demand checkpoint ref fetch failed; treating as no match",
+					slog.String("ref", refName.String()),
+					slog.String("error", fetchErr.Error()))
 			}
 		}
 		return nil, lookup
@@ -234,10 +301,16 @@ func matchCheckpointPrefixWithRemoteFallback(ctx context.Context, errW io.Writer
 	}
 	stop(false)
 	if v1Err != nil {
+		logging.Debug(ctx, "explain: metadata branch fetch failed; treating as no match",
+			slog.String("prefix", prefix),
+			slog.String("error", v1Err.Error()))
 		return nil, lookup
 	}
 	fresh, freshErr := newExplainCheckpointLookup(ctx)
 	if freshErr != nil {
+		logging.Debug(ctx, "explain: lookup rebuild after metadata fetch failed; treating as no match",
+			slog.String("prefix", prefix),
+			slog.String("error", freshErr.Error()))
 		return nil, lookup
 	}
 	return matchCheckpointPrefix(fresh, prefix), fresh

cmd/entire/cli/explain_export_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/entireio/cli/cmd/entire/cli/paths"
 	"github.com/entireio/cli/cmd/entire/cli/strategy"
 	"github.com/entireio/cli/cmd/entire/cli/testutil"
+	"github.com/entireio/cli/cmd/entire/cli/trailers"
 	"github.com/entireio/cli/redact"
 	"github.com/go-git/go-git/v6"
 	"github.com/go-git/go-git/v6/plumbing/object"
@@ -191,6 +192,150 @@ func TestRunExplainExport_JSONUsesMetadataOnlyReader(t *testing.T) {
 	require.Empty(t, envelope.Sessions[0].Error, "well-formed v1 read must not surface a per-session error")
 }
 
+// TestRunExplainExport_CommitWithoutTrailerSurfacesTrailerError (issue #1814):
+// a positional target that resolves to a real commit without an
+// Entire-Checkpoint trailer must surface that fact — not be masked as
+// `checkpoint not found: <sha>`, which reads as a typo and hides that the
+// commit was found. Same conflation class PR #1812 fixes for the prose path.
+func TestRunExplainExport_CommitWithoutTrailerSurfacesTrailerError(t *testing.T) {
+	repo := setupExportRepo(t)
+	head, err := repo.Head()
+	require.NoError(t, err)
+
+	var stdout, stderr bytes.Buffer
+	err = runExplainExport(context.Background(), &stdout, &stderr, explainExportOptions{
+		target:       head.Hash().String(),
+		json:         true,
+		sessionIndex: -1,
+	})
+
+	require.Error(t, err)
+	require.ErrorContains(t, err, "has no Entire-Checkpoint trailer",
+		"a trailer-less commit target must surface the trailer failure")
+	require.NotContains(t, err.Error(), "checkpoint not found",
+		"a resolved commit must not be masked as an unknown checkpoint")
+}
+
+// TestRunExplainExport_TrailerCheckpointUnavailableFailsWithCause: when a
+// commit's Entire-Checkpoint trailer references a checkpoint that is neither
+// local nor fetchable, the export path must fail naming the commit, the
+// checkpoint, and availability as the cause — not succeed and let a
+// downstream read die with a bare "checkpoint not found" that misdirects the
+// user toward the checkpoint ID instead of connectivity.
+func TestRunExplainExport_TrailerCheckpointUnavailableFailsWithCause(t *testing.T) {
+	repo := setupExportRepo(t)
+
+	cpID := id.MustCheckpointID("deadbeefcafe")
+	wt, err := repo.Worktree()
+	require.NoError(t, err)
+	cwd, err := os.Getwd()
+	require.NoError(t, err)
+	require.NoError(t, os.WriteFile(filepath.Join(cwd, "feature.txt"), []byte("feature"), 0o644))
+	_, err = wt.Add("feature.txt")
+	require.NoError(t, err)
+	commitHash, err := wt.Commit(trailers.AppendCheckpointTrailer("Implement feature", cpID.String()), &git.CommitOptions{
+		Author: &object.Signature{Name: exportTestAuthorName, Email: exportTestAuthorEmail, When: time.Now()},
+	})
+	require.NoError(t, err)
+
+	var stdout, stderr bytes.Buffer
+	err = runExplainExport(context.Background(), &stdout, &stderr, explainExportOptions{
+		commitRef:    commitHash.String(),
+		json:         true,
+		sessionIndex: -1,
+	})
+
+	require.Error(t, err)
+	require.ErrorContains(t, err, "not available locally",
+		"the failure must name availability as the cause")
+	require.ErrorContains(t, err, cpID.String())
+	require.ErrorContains(t, err, commitHash.String()[:7],
+		"the failure must name the commit the user typed")
+}
+
+// TestRunExplainExport_AmbiguousCommitPrefixNamesCandidates: an ambiguous
+// positional prefix must be reported as ambiguity — with the candidate
+// commits, so the user can disambiguate without rerunning git log — and must
+// not be masked as "checkpoint not found" (the pre-#1814 behavior).
+func TestRunExplainExport_AmbiguousCommitPrefixNamesCandidates(t *testing.T) {
+	repo := setupExportRepo(t)
+	cwd, err := os.Getwd()
+	require.NoError(t, err)
+	prefix := collidingShaPrefix(t, repo, cwd)
+
+	var stdout, stderr bytes.Buffer
+	err = runExplainExport(context.Background(), &stdout, &stderr, explainExportOptions{
+		target:       prefix,
+		json:         true,
+		sessionIndex: -1,
+	})
+
+	require.Error(t, err)
+	require.ErrorIs(t, err, errAmbiguousCommitPrefix)
+	require.ErrorContains(t, err, "ambiguous commit ref")
+	require.ErrorContains(t, err, "matches commits",
+		"the error must list the candidate commits")
+	require.NotContains(t, err.Error(), "checkpoint not found",
+		"ambiguity must not be masked as an unknown checkpoint")
+}
+
+// TestRunExplainExport_CommitFlagNotFoundMessage pins the --commit flag
+// path's user-visible message: the errExportTargetNotCommit sentinel's text
+// is part of the rendered error, so renaming it would silently change every
+// --commit failure message.
+func TestRunExplainExport_CommitFlagNotFoundMessage(t *testing.T) {
+	setupExportRepo(t)
+
+	var stdout, stderr bytes.Buffer
+	err := runExplainExport(context.Background(), &stdout, &stderr, explainExportOptions{
+		commitRef:    "nosuchref",
+		json:         true,
+		sessionIndex: -1,
+	})
+
+	require.Error(t, err)
+	require.ErrorContains(t, err, "commit not found: nosuchref")
+}
+
+// TestRunExplainExport_CheckpointFlagNeverFallsBackToCommit pins the
+// deliberate asymmetry: an explicit --checkpoint selector is never
+// reinterpreted as a commit ref, even when it would resolve as one.
+func TestRunExplainExport_CheckpointFlagNeverFallsBackToCommit(t *testing.T) {
+	repo := setupExportRepo(t)
+	head, err := repo.Head()
+	require.NoError(t, err)
+
+	var stdout, stderr bytes.Buffer
+	err = runExplainExport(context.Background(), &stdout, &stderr, explainExportOptions{
+		checkpointFlag: head.Hash().String(),
+		json:           true,
+		sessionIndex:   -1,
+	})
+
+	require.Error(t, err)
+	require.ErrorIs(t, err, checkpoint.ErrCheckpointNotFound)
+	require.NotContains(t, err.Error(), "trailer",
+		"--checkpoint must not be reinterpreted as a commit ref")
+}
+
+// TestRunExplainExport_UnknownTargetStillReportsNotFound pins the genuine-miss
+// contract around the #1814 fix: a target that is neither a checkpoint prefix
+// nor a commit keeps the plain not-found report.
+func TestRunExplainExport_UnknownTargetStillReportsNotFound(t *testing.T) {
+	setupExportRepo(t)
+
+	var stdout, stderr bytes.Buffer
+	err := runExplainExport(context.Background(), &stdout, &stderr, explainExportOptions{
+		target:       "abababababab",
+		json:         true,
+		sessionIndex: -1,
+	})
+
+	require.Error(t, err)
+	require.ErrorIs(t, err, checkpoint.ErrCheckpointNotFound)
+	require.ErrorContains(t, err, "checkpoint not found: abababababab")
+}
+
 func TestRunExplainExport_JSONNeverEmbedsTranscript(t *testing.T) {
 	repo := setupExportRepo(t)
 

cmd/entire/cli/git_operations.go

@@ -488,7 +488,9 @@ func FetchCheckpointRef(ctx context.Context, ref plumbing.ReferenceName) error {
 		RefSpecs: []string{refSpec},
 		NoTags:   true,
 	}); err != nil {
-		return fmt.Errorf("fetch checkpoint ref %s from %s: %w", ref, fetchTarget, err)
+		// Redact: fetchTarget can be a remote URL with embedded credentials
+		// (CI origin URLs), and this error is logged and shown to users.
+		return fmt.Errorf("fetch checkpoint ref %s from %s: %w", ref, remote.RedactURL(fetchTarget), err)
 	}
 	return nil
 }