diff --git a/internal/store/pg/memory_docs.go b/internal/store/pg/memory_docs.go index 9834d2b0..2b1c6037 100644 --- a/internal/store/pg/memory_docs.go +++ b/internal/store/pg/memory_docs.go @@ -464,7 +464,27 @@ func (s *PGMemoryStore) Close() error { return nil } // --- Helpers --- -func mustParseUUID(s string) uuid.UUID { +// parseUUID returns the parsed UUID or a descriptive error. +// Use for every INSERT/UPDATE/UPSERT/DELETE/SELECT-WHERE path where silent nil +// would either corrupt data (FK constraints reject it, but callers see cryptic +// PG code 23503 instead of a clean Go error) OR hide bugs as empty reads / +// zero-row updates. Phase 4 migration target for all 57 CRITICAL sites. +// See docs/agent-identity-conventions.md (Phase 6). +func parseUUID(s string) (uuid.UUID, error) { + id, err := uuid.Parse(s) + if err != nil { + return uuid.Nil, fmt.Errorf("parse uuid %q: %w", s, err) + } + return id, nil +} + +// parseUUIDOrNil returns the parsed UUID or uuid.Nil on failure, without +// raising an error. INTENTIONALLY silent — only use for read-only SELECT-WHERE +// paths where a no-match (empty result) is acceptable. Do NOT use for writes, +// updates, deletes, or any SELECT where empty result would hide a bug. +// Prefer parseUUID for any new code. +// See docs/agent-identity-conventions.md (Phase 6). +func parseUUIDOrNil(s string) uuid.UUID { id, err := uuid.Parse(s) if err != nil { return uuid.Nil @@ -472,6 +492,13 @@ func mustParseUUID(s string) uuid.UUID { return id } +// mustParseUUID is an alias for parseUUIDOrNil kept during the Phase 4 +// migration window. Will be removed in sub-step 4d (single-commit rename). +// TODO(Phase 4 4d): remove after all call sites migrate. +func mustParseUUID(s string) uuid.UUID { + return parseUUIDOrNil(s) +} + func vectorToString(v []float32) string { if len(v) == 0 { return "" diff --git a/internal/store/pg/parseuuid_test.go b/internal/store/pg/parseuuid_test.go new file mode 100644 index 00000000..0f6ab5da --- /dev/null +++ b/internal/store/pg/parseuuid_test.go @@ -0,0 +1,84 @@ +package pg + +import ( + "errors" + "strings" + "testing" + + "github.com/google/uuid" +) + +func TestParseUUID_Valid(t *testing.T) { + want := uuid.New() + got, err := parseUUID(want.String()) + if err != nil { + t.Fatalf("parseUUID(valid): unexpected error %v", err) + } + if got != want { + t.Errorf("parseUUID: got %v, want %v", got, want) + } +} + +func TestParseUUID_Invalid_ReturnsError(t *testing.T) { + got, err := parseUUID("goctech-leader") + if err == nil { + t.Fatal("parseUUID(agent_key): expected error, got nil") + } + if got != uuid.Nil { + t.Errorf("parseUUID(invalid): got %v, want uuid.Nil", got) + } + // Error must include both the offending value (for debuggability) and wrap + // the underlying uuid package error (for errors.Is assertions). + if !strings.Contains(err.Error(), "goctech-leader") { + t.Errorf("parseUUID error must include raw value, got: %v", err) + } +} + +func TestParseUUID_Empty_ReturnsError(t *testing.T) { + _, err := parseUUID("") + if err == nil { + t.Error("parseUUID(\"\"): expected error for empty input") + } +} + +func TestParseUUIDOrNil_Valid(t *testing.T) { + want := uuid.New() + got := parseUUIDOrNil(want.String()) + if got != want { + t.Errorf("parseUUIDOrNil: got %v, want %v", got, want) + } +} + +func TestParseUUIDOrNil_Invalid_ReturnsNil(t *testing.T) { + got := parseUUIDOrNil("not-a-uuid") + if got != uuid.Nil { + t.Errorf("parseUUIDOrNil(invalid): got %v, want uuid.Nil", got) + } +} + +// TestParseUUID_ErrorIsUnwrappable verifies the wrapped error chain so +// downstream callers can errors.Is/errors.As against the root uuid error. +func TestParseUUID_ErrorIsUnwrappable(t *testing.T) { + _, err := parseUUID("bad") + if err == nil { + t.Fatal("expected error") + } + // Unwrap should not be nil — the wrapped error chain is what lets callers + // decide whether to return a 400 vs 500. + if errors.Unwrap(err) == nil { + t.Error("parseUUID error should wrap the underlying uuid.Parse error") + } +} + +// TestMustParseUUID_AliasToParseUUIDOrNil verifies the transitional alias +// during Phase 4 migration — mustParseUUID must behave identically to +// parseUUIDOrNil so existing call sites are untouched until rename (4d). +func TestMustParseUUID_AliasToParseUUIDOrNil(t *testing.T) { + u := uuid.New() + if mustParseUUID(u.String()) != u { + t.Error("mustParseUUID should equal parseUUIDOrNil on valid input") + } + if mustParseUUID("garbage") != uuid.Nil { + t.Error("mustParseUUID should return uuid.Nil on invalid input (parseUUIDOrNil semantics)") + } +}