diff --git a/internal/cli/interactive.go b/internal/cli/interactive.go index 70c1e279..fda85562 100644 --- a/internal/cli/interactive.go +++ b/internal/cli/interactive.go @@ -208,10 +208,17 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo // prompts for the task afterward, so resolveFamily needn't report whether // it prompted the family question.) func resolveFamily(p *ui.Printer, pr prompter, path string) (push.Family, error) { - if s := push.SniffFamily(path); s.Confident { + s := push.SniffFamily(path) + if s.Confident { p.Successf("%s", s.Echo) return s.Family, nil } + if s.Hint != "" { + // Advisory only — e.g. a mis-cased media folder the walk won't see + // (#203). We still ask the family question; the hint just tells the + // user what looks off so they can fix the layout. + p.Warnf("%s", s.Hint) + } p.PromptHint("We couldn't tell the data type from what's there — which is it?") opts := push.FamilyNouns() ans, err := pr.Select("What kind of data is this?", diff --git a/internal/cli/interactive_test.go b/internal/cli/interactive_test.go index c15e35a0..6e36d42a 100644 --- a/internal/cli/interactive_test.go +++ b/internal/cli/interactive_test.go @@ -212,6 +212,57 @@ func TestRunInteractive_SniffIsHintNotLock(t *testing.T) { } } +// TestResolveFamily_SurfacesMiscasedHint pins the PR's headline behavior: an +// ambiguous sniff that carries an advisory hint (a mis-cased media folder next +// to labels.csv the walk can't see) has that hint surfaced through the printer +// before the family question — instead of silently ingesting the tree as a +// table (#203). The sniff tracks the walk, so behavior is FS-dependent; this +// asserts whichever branch applies on the machine it runs on. On a +// case-sensitive FS (Linux CI) the hint fires, so deleting resolveFamily's +// Warnf branch fails this test there. +func TestResolveFamily_SurfacesMiscasedHint(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "labels.csv"), + []byte("image_id,label\n001.jpg,cat\n"), 0o644); err != nil { + t.Fatalf("write labels.csv: %v", err) + } + if err := os.Mkdir(filepath.Join(dir, "Images"), 0o755); err != nil { + t.Fatalf("mkdir Images: %v", err) + } + + var buf bytes.Buffer + p := ui.New(&buf, ui.WithColor(false)) + f := &fakePrompter{answers: map[string]string{"What kind of data is this?": "image"}} + fam, err := resolveFamily(p, f, dir) + if err != nil { + t.Fatalf("resolveFamily: %v", err) + } + + if push.SniffFamily(dir).Hint != "" { + // Case-sensitive FS: the walk can't see Images/, so the sniff stays + // ambiguous with a rename hint. resolveFamily must print it, and still + // ask the family plainly (the hint is advisory, not a lock). + if !strings.Contains(buf.String(), "rename it and ingest again") { + t.Errorf("resolveFamily must surface the mis-cased rename hint; got:\n%s", buf.String()) + } + if !contains(f.asked, "What kind of data is this?") { + t.Errorf("hint is advisory — the family question must still be asked; asked=%v", f.asked) + } + } else { + // Case-insensitive FS: the walk resolves Images/, so the sniff is + // confident image — no false rename hint, no family question. + if fam != push.FamilyImage { + t.Errorf("family = %v, want image (walk resolves the mis-cased folder here)", fam) + } + if strings.Contains(buf.String(), "rename it and ingest again") { + t.Errorf("no false rename hint when the walk sees the folder; got:\n%s", buf.String()) + } + if contains(f.asked, "What kind of data is this?") { + t.Errorf("a confident sniff must not ask the family question; asked=%v", f.asked) + } + } +} + // TestRunInteractive_ExplicitTaskSkipsSniff: an explicit --task wins — no // sniff echo, no family question, no task picker. func TestRunInteractive_ExplicitTaskSkipsSniff(t *testing.T) { diff --git a/internal/push/preview.go b/internal/push/preview.go index f1248524..259925d0 100644 --- a/internal/push/preview.go +++ b/internal/push/preview.go @@ -25,6 +25,11 @@ type FamilySniff struct { // e.g. "Found a CSV table — this is tabular data." Empty when not // confident. Echo string + // Hint is an optional plain-language note for the ambiguous case, e.g. + // pointing out a mis-cased media folder ("Images/") that the walk won't + // see. Purely advisory — it never makes the sniff confident; the caller + // still asks the family question. Empty when there's nothing to add. + Hint string } // SniffFamily previews the family of the dataset at path by looking for @@ -34,12 +39,15 @@ type FamilySniff struct { // those (tabular), or a bare .csv file (tabular). It reads directory // entries only; it opens no files and validates nothing. // -// It never claims more than the matching Discover* would accept: the -// marker directories (images/, texts/, sequences/) and labels.csv are -// matched with the SAME literal, case-sensitive names the walk joins and -// Lstats — a mis-cased "Images/" is not the walk's marker, so it is not -// sniffed as confident image. Image / text are confident only when BOTH -// labels.csv AND the subdir are present, mirroring Discover / DiscoverText. +// It never claims more than the matching Discover* would accept: the marker +// directories (images/, texts/, sequences/) and labels.csv are probed with +// the SAME literal lowercase names the walk joins and Lstats. A mis-cased +// "Images/" is treated as the marker only when the walk's own +// os.Lstat(/images) resolves it — true on a case-insensitive filesystem +// (macOS, Windows), false on a case-sensitive one (Linux), so the sniff +// tracks the walk on the machine it runs on rather than guessing from the +// on-disk casing. Image / text are confident only when BOTH labels.csv AND +// the subdir are present, mirroring Discover / DiscoverText. // Tabular is confident on EXACTLY ONE CSV in a directory, mirroring // DiscoverTabular's findSingleCSV count rule — a directory with two or more // CSVs is a layout the tabular walk refuses, so the sniff must not @@ -95,13 +103,21 @@ func SniffFamily(path string) FamilySniff { // DiscoverTabular's EqualFold. var hasImages, hasTexts, hasSequences, hasLabels bool // miscasedMarker flags a subdir that matches a marker name only - // case-insensitively (e.g. "Images", "Texts") — a likely mis-cased - // media folder. The walk keys on the literal lowercase name, so such a - // dir is NOT a marker to it; but its lone labels.csv would otherwise - // fall through to the confident-tabular branch and get silently - // ingested as a table, images/texts dropped. When we see one, stay - // ambiguous and ask the family plainly. + // case-insensitively (e.g. "Images", "Texts") AND that the walk can't + // actually see. Whether the walk sees it is filesystem-dependent, so we + // don't guess from the on-disk casing — we probe the literal lowercase + // path the walk keys on (markerResolves, below). On a case-sensitive FS + // (Linux) a mis-cased dir is invisible to the walk, so its lone + // labels.csv would otherwise fall through to the confident-tabular + // branch and get silently ingested as a table, images/texts dropped — + // that's the footgun we flag. On a case-insensitive FS (macOS, Windows) + // the walk DOES resolve the mis-cased dir, so it's a real marker and we + // never set this. When set, stay ambiguous and ask the family plainly. miscasedMarker := false + // miscasedName / miscasedCanonical hold the first mis-cased dir we see + // and the lowercase marker it resembles, so the ambiguous return can + // name the likely rename (e.g. "Images" → "images") in its Hint. + var miscasedName, miscasedCanonical string csvCount := 0 for _, e := range entries { name := e.Name() @@ -114,8 +130,34 @@ func SniffFamily(path string) FamilySniff { case "sequences": hasSequences = true default: - if isMarkerFold(name) { - miscasedMarker = true + // The exact lowercase markers are matched by the cases above, so + // anything that EqualFolds a marker here is case-insensitive but + // NOT exact — e.g. "Images". Whether the walk actually SEES it is + // filesystem-dependent, so mirror the walk rather than guess: + // Discover keys on the literal lowercase name via + // os.Lstat(filepath.Join(dir, "images")). On a case-insensitive + // FS (macOS APFS, Windows) that resolves the mis-cased dir, so + // the walk accepts it — treat it as the real marker, exactly as + // Discover will (no false rename hint). Only when the lowercase + // literal does NOT resolve (a case-sensitive FS, e.g. Linux) is + // this a genuine footgun the walk can't see: flag it ambiguous + // and name the likely rename. + if m := markerFold(name); m != "" { + if markerResolves(abs, m) { + switch m { + case "images": + hasImages = true + case "texts": + hasTexts = true + case "sequences": + hasSequences = true + } + } else { + miscasedMarker = true + if miscasedName == "" { + miscasedName, miscasedCanonical = name, m + } + } } } continue @@ -160,22 +202,47 @@ func SniffFamily(path string) FamilySniff { // whose media folder was mis-cased must not masquerade as a table. return FamilySniff{Family: FamilyTabular, Confident: true, Echo: "Found a CSV table — this is tabular data."} + case miscasedMarker: + // A subdir matches a media marker case-insensitively but not exactly + // (e.g. "Images/"), AND markerResolves already confirmed the walk's + // literal lowercase path does NOT resolve on this filesystem — so the + // walk genuinely won't see this folder. An image/text layout here would + // otherwise look like a lone-CSV table and be silently ingested as one + // (#203). Stay ambiguous, but point at the likely rename so the user can + // fix the layout rather than just being asked a blind question. + return FamilySniff{Hint: fmt.Sprintf( + "Found a %q folder — image and text data use a lowercase folder like %q. "+ + "If that's your data folder, rename it and ingest again.", + miscasedName, miscasedCanonical)} default: return FamilySniff{} } } -// isMarkerFold reports whether name is one of the media-folder markers -// (images / texts / sequences) ignoring case. Used only to detect a -// mis-cased marker dir; the confident image/text branches still require an -// EXACT match, mirroring the walk's literal os.Lstat. -func isMarkerFold(name string) bool { +// markerResolves reports whether the walk's literal lowercase marker path +// (canonical is one of images / texts / sequences) resolves to a directory +// under dir — exactly the os.Lstat(filepath.Join(dir, canonical)) probe +// Discover / DiscoverText run. On a case-insensitive filesystem this resolves +// a mis-cased on-disk name (e.g. "Images"), so the sniff agrees with the walk +// that the folder IS the marker instead of emitting a false rename hint; on a +// case-sensitive filesystem it doesn't, exposing the real #203 footgun. +func markerResolves(dir, canonical string) bool { + fi, err := os.Lstat(filepath.Join(dir, canonical)) + return err == nil && fi.IsDir() +} + +// markerFold returns the media-folder marker (images / texts / sequences) +// that name matches ignoring case, or "" if none. Used only to detect a +// mis-cased marker dir and name the correct lowercase form in the hint; the +// confident image/text branches still require an EXACT match, mirroring the +// walk's literal os.Lstat. +func markerFold(name string) string { for _, m := range []string{"images", "texts", "sequences"} { if strings.EqualFold(name, m) { - return true + return m } } - return false + return "" } // PreviewLabelHeaders returns the column names of the CSV a label column diff --git a/internal/push/preview_test.go b/internal/push/preview_test.go index dba65827..d479ce6d 100644 --- a/internal/push/preview_test.go +++ b/internal/push/preview_test.go @@ -3,6 +3,7 @@ package push import ( "os" "path/filepath" + "strings" "testing" ) @@ -91,34 +92,66 @@ func TestSniffFamily(t *testing.T) { } }) - t.Run("mis-cased Images/ + labels.csv is ambiguous, NOT confident tabular", func(t *testing.T) { - // Discover Lstats the literal "images"; a mis-cased "Images/" is not - // its marker. The sniff must not claim confident image — but it must - // ALSO not fall through to confident tabular, or the lone labels.csv - // of a mis-cased image layout would be silently ingested as a table - // (cli#203). It stays ambiguous so the flow asks the family plainly. - dir := t.TempDir() - writePrev(t, filepath.Join(dir, "labels.csv"), "image_id,label\n1.jpg,c\n") - if err := os.Mkdir(filepath.Join(dir, "Images"), 0o755); err != nil { - t.Fatal(err) - } - if s := SniffFamily(dir); s.Confident { - t.Fatalf("mis-cased Images/ + labels.csv must be ambiguous, got %+v", s) - } - }) + // The mis-cased-media footgun (#203): labels.csv next to a media folder + // whose name matches a marker case-insensitively but not exactly. The + // sniff mirrors the walk, which keys on the literal lowercase name via + // os.Lstat — so behavior is filesystem-dependent, and each subtest asserts + // the branch that actually applies on the FS it runs on: + // - case-SENSITIVE FS (Linux CI): the walk can't see the folder, so the + // lone labels.csv would otherwise fall through to confident tabular and + // the media be silently ingested away. The sniff must stay ambiguous + // (NOT confident — that covers the confident-tabular footgun) and carry + // a rename hint naming both the folder and its lowercase form. + // - case-INSENSITIVE FS (macOS APFS, Windows): the walk resolves the + // mis-cased folder under its lowercase name, so the layout is valid. + // The sniff must AGREE — confident media, no false rename hint telling + // the user to fix a layout that already works (#203 cross-platform). + for _, tc := range []struct { + folder, canonical string + wantFamily Family + }{ + {"Images", "images", FamilyImage}, + {"Texts", "texts", FamilyText}, + {"Sequences", "sequences", FamilyText}, + } { + tc := tc + t.Run("mis-cased "+tc.folder+"/ + labels.csv tracks the walk", func(t *testing.T) { + dir := t.TempDir() + writePrev(t, filepath.Join(dir, "labels.csv"), "id,label\n1,c\n") + if err := os.Mkdir(filepath.Join(dir, tc.folder), 0o755); err != nil { + t.Fatal(err) + } + // The walk's own probe: does the literal lowercase marker resolve? + fi, err := os.Lstat(filepath.Join(dir, tc.canonical)) + walkSeesIt := err == nil && fi.IsDir() - t.Run("mis-cased Texts/ + labels.csv is ambiguous, NOT confident tabular", func(t *testing.T) { - dir := t.TempDir() - writePrev(t, filepath.Join(dir, "labels.csv"), "text_id,label\n1.txt,c\n") - if err := os.Mkdir(filepath.Join(dir, "Texts"), 0o755); err != nil { - t.Fatal(err) - } - if s := SniffFamily(dir); s.Confident { - t.Fatalf("mis-cased Texts/ + labels.csv must be ambiguous, got %+v", s) - } - }) + s := SniffFamily(dir) + if walkSeesIt { + // Case-insensitive FS: the folder IS the marker to the walk. + if !s.Confident || s.Family != tc.wantFamily { + t.Fatalf("case-insensitive FS: mis-cased %s/ should sniff confident %v, got %+v", tc.folder, tc.wantFamily, s) + } + if s.Hint != "" { + t.Fatalf("no false rename hint when the walk resolves %s/, got %q", tc.folder, s.Hint) + } + return + } + // Case-sensitive FS: the real #203 footgun. Not confident at all — + // that single check covers the confident-tabular masquerade — plus a + // rename hint that names both the folder and its lowercase form. + if s.Confident { + t.Fatalf("case-sensitive FS: mis-cased %s/ + labels.csv must be ambiguous, got %+v", tc.folder, s) + } + if s.Hint == "" { + t.Fatalf("mis-cased %s/ should carry a rename hint, got %+v", tc.folder, s) + } + if !strings.Contains(s.Hint, tc.folder) || !strings.Contains(s.Hint, tc.canonical) { + t.Fatalf("hint should name both %q and %q, got %q", tc.folder, tc.canonical, s.Hint) + } + }) + } - t.Run("single csv + unrelated subdir stays confident tabular", func(t *testing.T) { + t.Run("single csv + unrelated subdir stays confident tabular, no hint", func(t *testing.T) { // The mis-cased guard must be narrow: a subdir that is NOT a marker // name (case-insensitively) — a stray backup/ etc. — must not derail // the confident-tabular sniff, since DiscoverTabular ignores it too. @@ -131,6 +164,9 @@ func TestSniffFamily(t *testing.T) { if !s.Confident || s.Family != FamilyTabular { t.Fatalf("single csv + unrelated subdir should stay confident tabular, got %+v", s) } + if s.Hint != "" { + t.Fatalf("an unrelated subdir must not trigger a mis-cased hint, got %q", s.Hint) + } }) t.Run("images/ without labels.csv is not confident image", func(t *testing.T) {