From 9aaa404397062715bc560874bcca2a8c94eb1f9a Mon Sep 17 00:00:00 2001 From: Snider Date: Tue, 17 Mar 2026 08:50:17 +0000 Subject: [PATCH] fix(dx): audit coding standards and add tests for untested paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CLAUDE.md: document coreerr.E() error handling and go-io exclusion - server_test.go: replace fmt.Errorf with coreerr.E() in test fixtures - gguf_test.go: add tests for v2 format, skipValue (all type branches), readTypedValue uint64 path, unsupported version, truncated file - discover_test.go: add test for corrupt GGUF file skipping - vram_test.go: add tests for invalid/empty sysfs content Coverage: 65.8% → 79.2% (+13.4%) Co-Authored-By: Virgil --- CLAUDE.md | 2 + discover_test.go | 20 ++++ internal/gguf/gguf_test.go | 201 +++++++++++++++++++++++++++++++++++++ server_test.go | 8 +- vram_test.go | 18 ++++ 5 files changed, 245 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 41afd1c..69f4334 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -68,6 +68,8 @@ sudo cp build/bin/llama-server /usr/local/bin/llama-server - UK English - Tests: testify assert/require - Build tags: `linux && amd64` for GPU code, `rocm` for integration tests +- Errors: `coreerr.E("pkg.Func", "what failed", err)` via `go-log`, never `fmt.Errorf` or `errors.New` +- File I/O: `os` package used directly — `go-io` not imported (its transitive deps are too heavy for a GPU inference module) - Conventional commits - Co-Author: `Co-Authored-By: Virgil ` - Licence: EUPL-1.2 diff --git a/discover_test.go b/discover_test.go index c4cd5dd..9a6ce1a 100644 --- a/discover_test.go +++ b/discover_test.go @@ -127,3 +127,23 @@ func TestDiscoverModels_NotFound(t *testing.T) { require.NoError(t, err) assert.Empty(t, models) } + +func TestDiscoverModels_SkipsCorruptFile(t *testing.T) { + dir := t.TempDir() + + // Create a valid GGUF file. + writeDiscoverTestGGUF(t, dir, "valid.gguf", [][2]any{ + {"general.architecture", "llama"}, + {"general.name", "Valid Model"}, + {"general.file_type", uint32(15)}, + }) + + // Create a corrupt .gguf file (not valid GGUF binary). + require.NoError(t, os.WriteFile(filepath.Join(dir, "corrupt.gguf"), []byte("not gguf data"), 0644)) + + models, err := DiscoverModels(dir) + require.NoError(t, err) + // Only the valid model should be returned; corrupt one is silently skipped. + require.Len(t, models, 1) + assert.Equal(t, "Valid Model", models[0].Name) +} diff --git a/internal/gguf/gguf_test.go b/internal/gguf/gguf_test.go index fe298f3..5afb7b0 100644 --- a/internal/gguf/gguf_test.go +++ b/internal/gguf/gguf_test.go @@ -59,11 +59,56 @@ func writeKV(t *testing.T, f *os.File, key string, val any) { // Type: 4 (uint32) require.NoError(t, binary.Write(f, binary.LittleEndian, uint32(4))) require.NoError(t, binary.Write(f, binary.LittleEndian, v)) + case uint64: + // Type: 10 (uint64) + require.NoError(t, binary.Write(f, binary.LittleEndian, uint32(10))) + require.NoError(t, binary.Write(f, binary.LittleEndian, v)) default: t.Fatalf("writeKV: unsupported value type %T", val) } } +// writeRawKV writes a key with a specific GGUF type and raw byte payload. +// Used to test skipValue for types not used in interesting keys. +func writeRawKV(t *testing.T, f *os.File, key string, valType uint32, rawVal []byte) { + t.Helper() + + require.NoError(t, binary.Write(f, binary.LittleEndian, uint64(len(key)))) + _, err := f.Write([]byte(key)) + require.NoError(t, err) + require.NoError(t, binary.Write(f, binary.LittleEndian, valType)) + _, err = f.Write(rawVal) + require.NoError(t, err) +} + +// writeTestGGUFV2 creates a synthetic GGUF v2 file (uint32 tensor/kv counts). +func writeTestGGUFV2(t *testing.T, kvs [][2]any) string { + t.Helper() + + dir := t.TempDir() + path := filepath.Join(dir, "test_v2.gguf") + + f, err := os.Create(path) + require.NoError(t, err) + defer f.Close() + + // Magic + require.NoError(t, binary.Write(f, binary.LittleEndian, uint32(0x46554747))) + // Version 2 + require.NoError(t, binary.Write(f, binary.LittleEndian, uint32(2))) + // Tensor count (uint32 for v2): 0 + require.NoError(t, binary.Write(f, binary.LittleEndian, uint32(0))) + // KV count (uint32 for v2) + require.NoError(t, binary.Write(f, binary.LittleEndian, uint32(len(kvs)))) + + for _, kv := range kvs { + key := kv[0].(string) + writeKV(t, f, key, kv[1]) + } + + return path +} + func TestReadMetadata_Gemma3(t *testing.T) { path := writeTestGGUFOrdered(t, [][2]any{ {"general.architecture", "gemma3"}, @@ -153,3 +198,159 @@ func TestFileTypeName(t *testing.T) { assert.Equal(t, "F16", FileTypeName(1)) assert.Equal(t, "type_999", FileTypeName(999)) } + +func TestReadMetadata_V2(t *testing.T) { + // GGUF v2 uses uint32 for tensor and KV counts (instead of uint64 in v3). + path := writeTestGGUFV2(t, [][2]any{ + {"general.architecture", "llama"}, + {"general.name", "V2 Model"}, + {"general.file_type", uint32(15)}, + {"llama.context_length", uint32(2048)}, + {"llama.block_count", uint32(16)}, + }) + + m, err := ReadMetadata(path) + require.NoError(t, err) + + assert.Equal(t, "llama", m.Architecture) + assert.Equal(t, "V2 Model", m.Name) + assert.Equal(t, uint32(15), m.FileType) + assert.Equal(t, uint32(2048), m.ContextLength) + assert.Equal(t, uint32(16), m.BlockCount) +} + +func TestReadMetadata_UnsupportedVersion(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "bad_version.gguf") + + f, err := os.Create(path) + require.NoError(t, err) + + require.NoError(t, binary.Write(f, binary.LittleEndian, uint32(0x46554747))) // magic + require.NoError(t, binary.Write(f, binary.LittleEndian, uint32(99))) // invalid version + f.Close() + + _, err = ReadMetadata(path) + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported GGUF version") +} + +func TestReadMetadata_SkipsUnknownValueTypes(t *testing.T) { + // Tests skipValue for uint8, int16, float32, uint64, bool, and array types. + // These are stored under uninteresting keys so ReadMetadata skips them. + dir := t.TempDir() + path := filepath.Join(dir, "skip_types.gguf") + + f, err := os.Create(path) + require.NoError(t, err) + + // Header: magic, v3, 0 tensors + require.NoError(t, binary.Write(f, binary.LittleEndian, uint32(0x46554747))) + require.NoError(t, binary.Write(f, binary.LittleEndian, uint32(3))) + require.NoError(t, binary.Write(f, binary.LittleEndian, uint64(0))) + // 8 KV pairs: 6 skip types + 2 interesting keys + require.NoError(t, binary.Write(f, binary.LittleEndian, uint64(8))) + + // 1. uint8 (type 0) — 1 byte + raw := make([]byte, 1) + raw[0] = 42 + writeRawKV(t, f, "custom.uint8_val", 0, raw) + + // 2. bool (type 7) — 1 byte + raw = []byte{1} + writeRawKV(t, f, "custom.bool_val", 7, raw) + + // 3. int16 (type 3) — 2 bytes + raw = make([]byte, 2) + binary.LittleEndian.PutUint16(raw, 1234) + writeRawKV(t, f, "custom.int16_val", 3, raw) + + // 4. float32 (type 6) — 4 bytes + raw = make([]byte, 4) + binary.LittleEndian.PutUint32(raw, 0x3F800000) // 1.0 + writeRawKV(t, f, "custom.float32_val", 6, raw) + + // 5. uint64 (type 10) — 8 bytes + raw = make([]byte, 8) + binary.LittleEndian.PutUint64(raw, 9999) + writeRawKV(t, f, "custom.uint64_val", 10, raw) + + // 6. array of uint8 (type 9, element type 0, count 3) + var arrBuf []byte + b4 := make([]byte, 4) + binary.LittleEndian.PutUint32(b4, 0) // element type: uint8 + arrBuf = append(arrBuf, b4...) + b8 := make([]byte, 8) + binary.LittleEndian.PutUint64(b8, 3) // count: 3 + arrBuf = append(arrBuf, b8...) + arrBuf = append(arrBuf, 10, 20, 30) // 3 uint8 values + writeRawKV(t, f, "custom.array_val", 9, arrBuf) + + // 7-8. Interesting keys to verify parsing continued correctly. + writeKV(t, f, "general.architecture", "llama") + writeKV(t, f, "general.name", "Skip Test Model") + + f.Close() + + m, err := ReadMetadata(path) + require.NoError(t, err) + + assert.Equal(t, "llama", m.Architecture) + assert.Equal(t, "Skip Test Model", m.Name) +} + +func TestReadMetadata_Uint64ContextLength(t *testing.T) { + // context_length stored as uint64 that fits in uint32 — readTypedValue + // should downcast it to uint32. + path := writeTestGGUFOrdered(t, [][2]any{ + {"general.architecture", "llama"}, + {"llama.context_length", uint64(8192)}, + {"llama.block_count", uint64(32)}, + }) + + m, err := ReadMetadata(path) + require.NoError(t, err) + + assert.Equal(t, uint32(8192), m.ContextLength) + assert.Equal(t, uint32(32), m.BlockCount) +} + +func TestReadMetadata_TruncatedFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "truncated.gguf") + + // Write only the magic — no version or counts. + f, err := os.Create(path) + require.NoError(t, err) + require.NoError(t, binary.Write(f, binary.LittleEndian, uint32(0x46554747))) + f.Close() + + _, err = ReadMetadata(path) + require.Error(t, err) + assert.Contains(t, err.Error(), "reading version") +} + +func TestReadMetadata_SkipsStringValue(t *testing.T) { + // Tests skipValue for string type (type 8) on an uninteresting key. + dir := t.TempDir() + path := filepath.Join(dir, "skip_string.gguf") + + f, err := os.Create(path) + require.NoError(t, err) + + require.NoError(t, binary.Write(f, binary.LittleEndian, uint32(0x46554747))) + require.NoError(t, binary.Write(f, binary.LittleEndian, uint32(3))) + require.NoError(t, binary.Write(f, binary.LittleEndian, uint64(0))) + require.NoError(t, binary.Write(f, binary.LittleEndian, uint64(2))) + + // Uninteresting string key (exercises skipValue for typeString). + writeKV(t, f, "custom.description", "a long description value") + // Interesting key to confirm parsing continued. + writeKV(t, f, "general.architecture", "gemma3") + + f.Close() + + m, err := ReadMetadata(path) + require.NoError(t, err) + assert.Equal(t, "gemma3", m.Architecture) +} diff --git a/server_test.go b/server_test.go index 75a157f..0ecbc57 100644 --- a/server_test.go +++ b/server_test.go @@ -4,12 +4,12 @@ package rocm import ( "context" - "fmt" "os" "strings" "testing" "forge.lthn.ai/core/go-inference" + coreerr "forge.lthn.ai/core/go-log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -90,7 +90,7 @@ func TestServerAlive_Running(t *testing.T) { func TestServerAlive_Exited(t *testing.T) { exited := make(chan struct{}) close(exited) - s := &server{exited: exited, exitErr: fmt.Errorf("process killed")} + s := &server{exited: exited, exitErr: coreerr.E("test", "process killed", nil)} assert.False(t, s.alive()) } @@ -99,7 +99,7 @@ func TestGenerate_ServerDead(t *testing.T) { close(exited) s := &server{ exited: exited, - exitErr: fmt.Errorf("process killed"), + exitErr: coreerr.E("test", "process killed", nil), } m := &rocmModel{srv: s} @@ -124,7 +124,7 @@ func TestChat_ServerDead(t *testing.T) { close(exited) s := &server{ exited: exited, - exitErr: fmt.Errorf("process killed"), + exitErr: coreerr.E("test", "process killed", nil), } m := &rocmModel{srv: s} diff --git a/vram_test.go b/vram_test.go index 012399d..36c8eca 100644 --- a/vram_test.go +++ b/vram_test.go @@ -26,6 +26,24 @@ func TestReadSysfsUint64_NotFound(t *testing.T) { assert.Error(t, err) } +func TestReadSysfsUint64_InvalidContent(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "bad_value") + require.NoError(t, os.WriteFile(path, []byte("not-a-number\n"), 0644)) + + _, err := readSysfsUint64(path) + assert.Error(t, err) +} + +func TestReadSysfsUint64_EmptyFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "empty_value") + require.NoError(t, os.WriteFile(path, []byte(""), 0644)) + + _, err := readSysfsUint64(path) + assert.Error(t, err) +} + func TestGetVRAMInfo(t *testing.T) { info, err := GetVRAMInfo() if err != nil { -- 2.45.3