2 files changed, 79 insertions(+), 2 deletions(-)

M browse/log.go
A browse/log_fromhash_test.go

@@ 82,6 82,11 @@ func DefaultBranch(branches []Branch) string {
 // first are fetched: pass the nextHash returned by the previous call. nextHash
 // is the hash of the first commit of the following page, or "" when the last
 // page was returned.
+//
+// A caller may rely on errors.Is(err, ErrRefNotFound) to hold whenever refStr
+// or fromHash names nothing this store can resolve — including a fromHash
+// that does not even parse as a hash. Any other error means the store could
+// not answer, not that the answer is "not found".
 func (db *DB) Log(ctx context.Context, refStr, fromHash string, limit int) ([]CommitInfo, string, error) {
 	if limit <= 0 {
 		return nil, "", fmt.Errorf("browse: log limit must be positive, got %d", limit)


@@ 91,7 96,7 @@ func (db *DB) Log(ctx context.Context, refStr, fromHash string, limit int) ([]Co
 	if fromHash != "" {
 		h, ok := hash.MaybeParse(fromHash)
 		if !ok {
-			return nil, "", fmt.Errorf("browse: invalid from hash %q", fromHash)
+			return nil, "", fmt.Errorf("%w: invalid from hash %q", ErrRefNotFound, fromHash)
 		}
 		start = h
 	} else {


@@ 106,6 111,11 @@ func (db *DB) Log(ctx context.Context, refStr, fromHash string, limit int) ([]Co
 	}
 
 	itr, err := commitwalk.GetTopologicalOrderIterator[context.Context](ctx, db.ddb, []hash.Hash{start}, nil)
+	if errors.Is(err, datas.ErrCommitNotFound) {
+		// A well-formed hash (parsed above, or the head of a resolved ref)
+		// that names no commit in this store is a miss, not a store failure.
+		return nil, "", fmt.Errorf("%w: %s", ErrRefNotFound, start.String())
+	}
 	if err != nil {
 		return nil, "", fmt.Errorf("browse: topological iterator: %w", err)
 	}


@@ 173,7 183,10 @@ func commitInfo(ctx context.Context, h hash.Hash, oc *doltdb.OptionalCommit, met
 }
 
 // resolveCommit resolves a ref string to a commit. It is tried first as a
-// branch name, then as a commit hash.
+// branch name, then as a commit hash. Every way refStr can fail to name a
+// commit — not a hash, a hash with no matching commit, a hash that resolves
+// only to a ghost — is reported via ErrRefNotFound; other errors mean the
+// store itself could not answer.
 func (db *DB) resolveCommit(ctx context.Context, refStr string) (*doltdb.Commit, error) {
 	if _, ok, err := db.ddb.HasBranch(ctx, refStr); err != nil {
 		return nil, fmt.Errorf("browse: check branch %q: %w", refStr, err)


@@ 187,6 200,9 @@ func (db *DB) resolveCommit(ctx context.Context, refStr string) (*doltdb.Commit,
 
 	if h, ok := hash.MaybeParse(refStr); ok {
 		oc, err := db.ddb.ResolveHash(ctx, h)
+		if errors.Is(err, datas.ErrCommitNotFound) {
+			return nil, fmt.Errorf("%w: %s", ErrRefNotFound, refStr)
+		}
 		if err != nil {
 			return nil, fmt.Errorf("browse: resolve hash %q: %w", refStr, err)
 		}

@@ 0,0 1,61 @@
+package browse
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestLogFromHashGarbage covers the MCP get_commit_log case: a caller-supplied
+// cursor that does not even parse as a dolt hash must be an ordinary "not
+// found" (errors.Is(err, ErrRefNotFound)), not an opaque internal error.
+func TestLogFromHashGarbage(t *testing.T) {
+	db := openFixture(t)
+	ctx := context.Background()
+
+	_, _, err := db.Log(ctx, "main", "not-a-hash", 10)
+	require.Error(t, err)
+	assert.ErrorIs(t, err, ErrRefNotFound)
+}
+
+// TestLogFromHashWellFormedButUnknown covers a fromHash that parses fine but
+// names no commit this store has: same classification as a garbage string.
+func TestLogFromHashWellFormedButUnknown(t *testing.T) {
+	db := openFixture(t)
+	ctx := context.Background()
+
+	// 32 'a' characters is a syntactically valid (but non-existent) dolt hash.
+	_, _, err := db.Log(ctx, "main", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", 10)
+	require.Error(t, err)
+	assert.ErrorIs(t, err, ErrRefNotFound)
+}
+
+// TestLogFromHashValid confirms the happy path is untouched: a real hash
+// still starts the walk there and pages as before.
+func TestLogFromHashValid(t *testing.T) {
+	db := openFixture(t)
+	ctx := context.Background()
+
+	commits, next, err := db.Log(ctx, "main", commitHash(t, msgInsert), 100)
+	require.NoError(t, err)
+	assert.Empty(t, next)
+
+	want := []string{msgInsert, msgAddUser, msgInitial}
+	require.Len(t, commits, len(want))
+	for i, w := range want {
+		assert.Equal(t, w, commits[i].Message)
+	}
+}
+
+// TestLogFromHashEmpty confirms "" still means "start from ref's head".
+func TestLogFromHashEmpty(t *testing.T) {
+	db := openFixture(t)
+	ctx := context.Background()
+
+	commits, _, err := db.Log(ctx, "main", "", 100)
+	require.NoError(t, err)
+	require.NotEmpty(t, commits)
+	assert.Equal(t, msgAddItem, commits[0].Message)
+}