Correct artifact file and empty input handling

internal/artifact/reader.go

@@ -16,10 +16,12 @@ import (
 
 var (
 	ErrUnsupportedRefType = errors.New("unsupported artifact reference type")
-	ErrMissingInlineBody  = errors.New("missing body for inline artifact")
 	ErrMissingFilePath    = errors.New("missing file path for file artifact")
+	ErrUnsupportedFile    = errors.New("file artifact path is not a regular file")
 )
 
+const fileReadChunkSize = 64 * 1024
+
 // Reader resolves artifact references into actual artifacts.
 type Reader interface {
 	Read(ctx context.Context, ref domain.ArtifactRef) (*domain.Artifact, error)
@@ -34,7 +36,7 @@ type CompositeReader struct {
 func NewCompositeReader() Reader {
 	return &CompositeReader{
 		inlineReader: &inlineReader{},
-		fileReader:   &fileReader{},
+		fileReader:   &fileReader{open: openArtifactFile},
 	}
 }
 
@@ -64,10 +66,6 @@ func (r *inlineReader) Read(ctx context.Context, ref domain.ArtifactRef) (*domai
 	default:
 	}
 
-	if ref.Body == "" {
-		return nil, ErrMissingInlineBody
-	}
-
 	body := []byte(ref.Body)
 	return &domain.Artifact{
 		ContentType: defaults.ContentTypeTextPlain,
@@ -78,7 +76,15 @@ func (r *inlineReader) Read(ctx context.Context, ref domain.ArtifactRef) (*domai
 	}, nil
 }
 
-type fileReader struct{}
+type artifactFile interface {
+	Read([]byte) (int, error)
+	Stat() (os.FileInfo, error)
+	Close() error
+}
+
+type fileReader struct {
+	open func(string) (artifactFile, error)
+}
 
 func (r *fileReader) Read(ctx context.Context, ref domain.ArtifactRef) (*domain.Artifact, error) {
 	select {
@@ -91,25 +97,71 @@ func (r *fileReader) Read(ctx context.Context, ref domain.ArtifactRef) (*domain.
 		return nil, ErrMissingFilePath
 	}
 
-	return readFileArtifact(ref.URI)
+	return readFileArtifact(ctx, ref.URI, r.open)
 }
 
-func readFileArtifact(path string) (*domain.Artifact, error) {
-	file, err := os.Open(path)
+func openArtifactFile(path string) (artifactFile, error) {
+	return os.Open(path)
+}
+
+func readFileArtifact(ctx context.Context, path string, open func(string) (artifactFile, error)) (*domain.Artifact, error) {
+	if err := ctx.Err(); err != nil {
+		return nil, err
+	}
+	info, err := os.Stat(path)
+	if err != nil {
+		return nil, fmt.Errorf("failed to read file %s: %w", path, err)
+	}
+	if !info.Mode().IsRegular() {
+		return nil, fmt.Errorf("%w: %s", ErrUnsupportedFile, path)
+	}
+	if err := ctx.Err(); err != nil {
+		return nil, err
+	}
+
+	file, err := open(path)
 	if err != nil {
 		return nil, fmt.Errorf("failed to read file %s: %w", path, err)
 	}
 	defer file.Close()
 
-	data, err := io.ReadAll(file)
+	openedInfo, err := file.Stat()
 	if err != nil {
-		return nil, fmt.Errorf("failed to read file %s: %w", path, err)
+		return nil, fmt.Errorf("failed to inspect opened file %s: %w", path, err)
+	}
+	if !openedInfo.Mode().IsRegular() {
+		return nil, fmt.Errorf("%w: %s", ErrUnsupportedFile, path)
+	}
+
+	data := make([]byte, 0)
+	chunk := make([]byte, fileReadChunkSize)
+	for {
+		if err := ctx.Err(); err != nil {
+			return nil, err
+		}
+		n, readErr := file.Read(chunk)
+		if n > 0 {
+			data = append(data, chunk[:n]...)
+		}
+		if err := ctx.Err(); err != nil {
+			return nil, err
+		}
+		if errors.Is(readErr, io.EOF) {
+			break
+		}
+		if readErr != nil {
+			return nil, fmt.Errorf("failed to read file %s: %w", path, readErr)
+		}
 	}
 
 	contentType := mime.TypeByExtension(filepath.Ext(path))
 	if contentType == "" {
 		contentType = defaults.ContentTypeTextPlain
 	}
+	hash := fmt.Sprintf("%x", sha256.Sum256(data))
+	if err := ctx.Err(); err != nil {
+		return nil, err
+	}
 
 	return &domain.Artifact{
 		Name:        filepath.Base(path),
@@ -117,6 +169,6 @@ func readFileArtifact(path string) (*domain.Artifact, error) {
 		Body:        data,
 		URI:         path,
 		Size:        int64(len(data)),
-		Hash:        fmt.Sprintf("%x", sha256.Sum256(data)),
+		Hash:        hash,
 	}, nil
 }

internal/artifact/reader_fifo_linux_test.go

@@ -0,0 +1,43 @@
+//go:build linux
+
+package artifact
+
+import (
+	"context"
+	"errors"
+	"path/filepath"
+	"syscall"
+	"testing"
+	"time"
+
+	"gitea.maximumdirect.net/eric/promptkit/internal/domain"
+)
+
+func TestFileReaderRejectsFIFOBeforeOpen(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "artifact.fifo")
+	if err := syscall.Mkfifo(path, 0o600); err != nil {
+		t.Fatalf("create fifo: %v", err)
+	}
+
+	type result struct {
+		artifact *domain.Artifact
+		err      error
+	}
+	done := make(chan result, 1)
+	go func() {
+		artifact, err := NewCompositeReader().Read(context.Background(), domain.ArtifactRef{
+			Type: domain.ArtifactRefFile,
+			URI:  path,
+		})
+		done <- result{artifact: artifact, err: err}
+	}()
+
+	select {
+	case got := <-done:
+		if got.artifact != nil || !errors.Is(got.err, ErrUnsupportedFile) {
+			t.Fatalf("artifact=%#v err=%v, want nil/ErrUnsupportedFile", got.artifact, got.err)
+		}
+	case <-time.After(time.Second):
+		t.Fatal("FIFO read blocked instead of rejecting the non-regular file")
+	}
+}

internal/artifact/reader_test.go

@@ -1,6 +1,7 @@
 package artifact
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"os"
@@ -11,54 +12,97 @@ import (
 	"gitea.maximumdirect.net/eric/promptkit/internal/domain"
 )
 
-func TestCompositeReader_Read(t *testing.T) {
+func TestCompositeReaderRejectsUnsupportedReferences(t *testing.T) {
+	_, err := NewCompositeReader().Read(context.Background(), domain.ArtifactRef{
+		Type: domain.ArtifactRefType("unsupported"),
+		URI:  "unsupported://bucket/key",
+	})
+	if !errors.Is(err, ErrUnsupportedRefType) {
+		t.Fatalf("expected ErrUnsupportedRefType, got %v", err)
+	}
+}
+
+func TestCompositeReaderSourceParityAndOpaqueHashes(t *testing.T) {
 	reader := NewCompositeReader()
-	ctx := context.Background()
+	hashes := make(map[string]string)
+	tests := []struct {
+		name    string
+		content string
+	}{
+		{name: "empty", content: ""},
+		{name: "ordinary", content: "same content"},
+		{name: "changed", content: "changed content"},
+	}
 
-	t.Run("inline artifact", func(t *testing.T) {
-		ref := domain.ArtifactRef{
-			Type: domain.ArtifactRefInline,
-			Body: "hello world",
-		}
-		art, err := reader.Read(ctx, ref)
-		if err != nil {
-			t.Fatalf("unexpected error: %v", err)
-		}
-		if string(art.Body) != "hello world" {
-			t.Errorf("expected 'hello world', got %s", string(art.Body))
-		}
-		if art.ContentType != "text/plain" {
-			t.Errorf("expected text/plain content type, got %q", art.ContentType)
-		}
-		if art.Hash != "b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9" {
-			t.Errorf("unexpected hash: %s", art.Hash)
-		}
-		if art.Size != int64(len(ref.Body)) {
-			t.Errorf("expected size %d, got %d", len(ref.Body), art.Size)
-		}
-	})
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			filePath := filepath.Join(t.TempDir(), "artifact.txt")
+			if err := os.WriteFile(filePath, []byte(tc.content), 0o600); err != nil {
+				t.Fatal(err)
+			}
 
-	t.Run("inline artifact missing body", func(t *testing.T) {
-		ref := domain.ArtifactRef{
-			Type: domain.ArtifactRefInline,
-			Body: "",
-		}
-		_, err := reader.Read(ctx, ref)
-		if !errors.Is(err, ErrMissingInlineBody) {
-			t.Errorf("expected ErrMissingInlineBody, got %v", err)
-		}
-	})
+			sources := []struct {
+				name    string
+				ref     domain.ArtifactRef
+				wantURI string
+			}{
+				{
+					name: "inline",
+					ref:  domain.ArtifactRef{Type: domain.ArtifactRefInline, Body: tc.content},
+				},
+				{
+					name:    "inline with uri",
+					ref:     domain.ArtifactRef{Type: domain.ArtifactRefInline, URI: "memory://input", Body: tc.content},
+					wantURI: "memory://input",
+				},
+				{
+					name:    "file",
+					ref:     domain.ArtifactRef{Type: domain.ArtifactRefFile, URI: filePath},
+					wantURI: filePath,
+				},
+			}
 
-	t.Run("unsupported ref type", func(t *testing.T) {
-		ref := domain.ArtifactRef{
-			Type: domain.ArtifactRefType("unsupported"),
-			URI:  "unsupported://bucket/key",
-		}
-		_, err := reader.Read(ctx, ref)
-		if !errors.Is(err, ErrUnsupportedRefType) {
-			t.Error("expected error for unsupported type")
-		}
-	})
+			var sourceHash string
+			for _, source := range sources {
+				t.Run(source.name, func(t *testing.T) {
+					first, err := reader.Read(context.Background(), source.ref)
+					if err != nil {
+						t.Fatalf("first read: %v", err)
+					}
+					second, err := reader.Read(context.Background(), source.ref)
+					if err != nil {
+						t.Fatalf("second read: %v", err)
+					}
+					if string(first.Body) != tc.content || first.Size != int64(len(tc.content)) {
+						t.Fatalf("body=%q size=%d, want %q/%d", first.Body, first.Size, tc.content, len(tc.content))
+					}
+					if first.URI != source.wantURI {
+						t.Fatalf("URI = %q, want %q", first.URI, source.wantURI)
+					}
+					if first.Hash == "" || first.Hash != second.Hash {
+						t.Fatalf("hashes are not non-empty and stable: %q/%q", first.Hash, second.Hash)
+					}
+					if sourceHash == "" {
+						sourceHash = first.Hash
+					} else if first.Hash != sourceHash {
+						t.Fatalf("equal content hashes differ: %q/%q", sourceHash, first.Hash)
+					}
+					if source.ref.Type == domain.ArtifactRefFile {
+						if first.Name != filepath.Base(filePath) || !strings.HasPrefix(first.ContentType, "text/plain") {
+							t.Fatalf("unexpected file metadata: %+v", first)
+						}
+					} else if first.ContentType != "text/plain" {
+						t.Fatalf("inline content type = %q", first.ContentType)
+					}
+				})
+			}
+			hashes[tc.name] = sourceHash
+		})
+	}
+
+	if hashes["empty"] == hashes["ordinary"] || hashes["ordinary"] == hashes["changed"] {
+		t.Fatalf("changed content did not change opaque hash: %#v", hashes)
+	}
 }
 
 func TestCompositeReaderCopiesInlineData(t *testing.T) {
@@ -87,94 +131,154 @@ func TestCompositeReaderCopiesInlineData(t *testing.T) {
 	}
 }
 
-func TestCompositeReaderHonorsCancellation(t *testing.T) {
-	ctx, cancel := context.WithCancel(context.Background())
-	cancel()
+func TestCompositeReaderHonorsPreCancellation(t *testing.T) {
+	filePath := filepath.Join(t.TempDir(), "artifact.txt")
+	if err := os.WriteFile(filePath, []byte("ignored"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	tests := []struct {
+		name string
+		ref  domain.ArtifactRef
+	}{
+		{name: "inline", ref: domain.ArtifactRef{Type: domain.ArtifactRefInline, Body: "ignored"}},
+		{name: "file", ref: domain.ArtifactRef{Type: domain.ArtifactRefFile, URI: filePath}},
+	}
 
-	_, err := NewCompositeReader().Read(ctx, domain.ArtifactRef{
-		Type: domain.ArtifactRefInline,
-		Body: "ignored",
-	})
-	if !errors.Is(err, context.Canceled) {
-		t.Fatalf("expected context cancellation, got %v", err)
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			ctx, cancel := context.WithCancel(context.Background())
+			cancel()
+
+			artifact, err := NewCompositeReader().Read(ctx, tc.ref)
+			if artifact != nil || !errors.Is(err, context.Canceled) {
+				t.Fatalf("artifact=%#v err=%v, want nil/context.Canceled", artifact, err)
+			}
+		})
 	}
 }
 
-func TestFileReader_Read(t *testing.T) {
-	content := []byte("test file content")
-	filePath := filepath.Join(t.TempDir(), "artifact.txt")
-	if err := os.WriteFile(filePath, content, 0o600); err != nil {
-		t.Fatal(err)
-	}
-
+func TestFileReaderFailuresAndMetadata(t *testing.T) {
 	reader := NewCompositeReader()
-	ctx := context.Background()
-
-	t.Run("file artifact loading", func(t *testing.T) {
-		ref := domain.ArtifactRef{
-			Type: domain.ArtifactRefFile,
-			URI:  filePath,
-		}
-		art, err := reader.Read(ctx, ref)
-		if err != nil {
-			t.Fatalf("unexpected error: %v", err)
-		}
-		if string(art.Body) != string(content) {
-			t.Errorf("expected %s, got %s", string(content), string(art.Body))
-		}
-		if art.Name != filepath.Base(filePath) {
-			t.Errorf("expected name %q, got %q", filepath.Base(filePath), art.Name)
-		}
-		if !strings.HasPrefix(art.ContentType, "text/plain") {
-			t.Errorf("expected text content type, got %q", art.ContentType)
-		}
-		if art.URI != filePath {
-			t.Errorf("expected URI %q, got %q", filePath, art.URI)
-		}
-		if art.Size != int64(len(content)) {
-			t.Errorf("expected size %d, got %d", len(content), art.Size)
-		}
-		if art.Hash != "60f5237ed4049f0382661ef009d2bc42e48c3ceb3edb6600f7024e7ab3b838f3" {
-			t.Errorf("unexpected hash: %s", art.Hash)
-		}
-	})
 
 	t.Run("missing file path", func(t *testing.T) {
-		ref := domain.ArtifactRef{
-			Type: domain.ArtifactRefFile,
-			URI:  "",
-		}
-		_, err := reader.Read(ctx, ref)
+		_, err := reader.Read(context.Background(), domain.ArtifactRef{Type: domain.ArtifactRefFile})
 		if !errors.Is(err, ErrMissingFilePath) {
-			t.Errorf("expected ErrMissingFilePath, got %v", err)
+			t.Fatalf("expected ErrMissingFilePath, got %v", err)
 		}
 	})
 
 	t.Run("missing file", func(t *testing.T) {
-		ref := domain.ArtifactRef{
+		_, err := reader.Read(context.Background(), domain.ArtifactRef{
 			Type: domain.ArtifactRefFile,
 			URI:  filepath.Join(t.TempDir(), "missing.txt"),
-		}
-		if _, err := reader.Read(ctx, ref); err == nil {
+		})
+		if err == nil {
 			t.Fatal("expected missing file error")
 		}
 	})
 
+	t.Run("directory rejected before open", func(t *testing.T) {
+		artifact, err := reader.Read(context.Background(), domain.ArtifactRef{
+			Type: domain.ArtifactRefFile,
+			URI:  t.TempDir(),
+		})
+		if artifact != nil || !errors.Is(err, ErrUnsupportedFile) {
+			t.Fatalf("artifact=%#v err=%v, want nil/ErrUnsupportedFile", artifact, err)
+		}
+	})
+
+	t.Run("non-regular opened target rejected", func(t *testing.T) {
+		filePath := filepath.Join(t.TempDir(), "artifact.txt")
+		if err := os.WriteFile(filePath, []byte("content"), 0o600); err != nil {
+			t.Fatal(err)
+		}
+		directoryInfo, err := os.Stat(t.TempDir())
+		if err != nil {
+			t.Fatal(err)
+		}
+		fileReader := &fileReader{open: func(path string) (artifactFile, error) {
+			file, err := os.Open(path)
+			if err != nil {
+				return nil, err
+			}
+			return &reportedInfoFile{artifactFile: file, info: directoryInfo}, nil
+		}}
+
+		artifact, err := fileReader.Read(context.Background(), domain.ArtifactRef{
+			Type: domain.ArtifactRefFile,
+			URI:  filePath,
+		})
+		if artifact != nil || !errors.Is(err, ErrUnsupportedFile) {
+			t.Fatalf("artifact=%#v err=%v, want nil/ErrUnsupportedFile", artifact, err)
+		}
+	})
+
 	t.Run("unknown extension uses text fallback", func(t *testing.T) {
-		path := filepath.Join(t.TempDir(), "artifact.unknownextension")
-		if err := os.WriteFile(path, content, 0o600); err != nil {
+		filePath := filepath.Join(t.TempDir(), "artifact.unknownextension")
+		if err := os.WriteFile(filePath, []byte("content"), 0o600); err != nil {
 			t.Fatal(err)
 		}
 
-		art, err := reader.Read(ctx, domain.ArtifactRef{
+		artifact, err := reader.Read(context.Background(), domain.ArtifactRef{
 			Type: domain.ArtifactRefFile,
-			URI:  path,
+			URI:  filePath,
 		})
 		if err != nil {
-			t.Fatalf("unexpected error: %v", err)
+			t.Fatalf("read artifact: %v", err)
 		}
-		if art.ContentType != "text/plain" {
-			t.Errorf("expected text/plain fallback, got %q", art.ContentType)
+		if artifact.ContentType != "text/plain" {
+			t.Fatalf("content type = %q", artifact.ContentType)
 		}
 	})
 }
+
+func TestFileReaderCancelsAfterReadProgress(t *testing.T) {
+	filePath := filepath.Join(t.TempDir(), "artifact.bin")
+	content := bytes.Repeat([]byte("x"), fileReadChunkSize*2)
+	if err := os.WriteFile(filePath, content, 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	var opened *cancelAfterProgressFile
+	reader := &fileReader{open: func(path string) (artifactFile, error) {
+		file, err := os.Open(path)
+		if err != nil {
+			return nil, err
+		}
+		opened = &cancelAfterProgressFile{artifactFile: file, cancel: cancel}
+		return opened, nil
+	}}
+
+	artifact, err := reader.Read(ctx, domain.ArtifactRef{Type: domain.ArtifactRefFile, URI: filePath})
+	if artifact != nil || !errors.Is(err, context.Canceled) {
+		t.Fatalf("artifact=%#v err=%v, want nil/context.Canceled", artifact, err)
+	}
+	if opened == nil || opened.reads != 1 {
+		t.Fatalf("read count = %v, want one progressing read", opened)
+	}
+}
+
+type reportedInfoFile struct {
+	artifactFile
+	info os.FileInfo
+}
+
+func (f *reportedInfoFile) Stat() (os.FileInfo, error) {
+	return f.info, nil
+}
+
+type cancelAfterProgressFile struct {
+	artifactFile
+	cancel context.CancelFunc
+	reads  int
+}
+
+func (f *cancelAfterProgressFile) Read(buffer []byte) (int, error) {
+	n, err := f.artifactFile.Read(buffer)
+	if n > 0 {
+		f.reads++
+		f.cancel()
+	}
+	return n, err
+}