Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion internal/cli/interactive.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -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?",
Expand Down
51 changes: 51 additions & 0 deletions internal/cli/interactive_test.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -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) {
Expand Down
109 changes: 88 additions & 21 deletions internal/push/preview.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand All@@ -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(<dir>/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
Expand DownExpand Up@@ -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()
Expand All@@ -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
Expand DownExpand Up@@ -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
Expand Down
88 changes: 62 additions & 26 deletions internal/push/preview_test.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -3,6 +3,7 @@ package push
import (
"os"
"path/filepath"
"strings"
"testing"
)

Expand DownExpand Up@@ -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.
Expand All@@ -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) {
Expand Down
Loading