Skip to content

fix(push): SniffFamily mirrors the walk (symlinked/mis-cased/lone markers) - #223

Merged
saadqbal merged 3 commits into
developfrom
fix/sniff-mirror-walk
Jul 10, 2026
Merged

fix(push): SniffFamily mirrors the walk (symlinked/mis-cased/lone markers)#223
saadqbal merged 3 commits into
developfrom
fix/sniff-mirror-walk

Conversation

@saadqbal

@saadqbalsaadqbal commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

What

SniffFamily inferred layout markers from the directory listing (DirEntry.IsDir() + a case-sensitive name compare) instead of mirroring the walk's os.Lstat-based, symlink-rejecting, case-following resolution. Several layouts therefore sniffed confident tabular and got silently ingested as a table when the walk would refuse them — or the data was actually image/text with its media dropped:

  • Symlinkedimages//texts//sequences/IsDir()==false → confident tabular (image/text ingested as a table). (This was the open Bugbot round-3 finding's more dangerous sibling — it happens even when the media dir is present, as a symlink.)
  • A plain file named images → same.
  • A lone labels.csv with no media dir → confident tabular, though labels.csv is the image/text manifest name (usually an incomplete media set). (The round-3 Bugbot finding.)
  • labels.csv matched case-sensitively while the walk resolves it case-insensitively on macOS/Windows.
  • A dir whose only CSV is a symlink was counted, though DiscoverTabular rejects a symlinked CSV.

Fix

Media markers are probed via markerResolves (the walk's literal-lowercase Lstat); labels.csv via a new regularFileResolves (regular-file Lstat — tracks the walk's case behavior, rejects a symlink/dir); symlinked CSVs are skipped from the count; any marker-named entry the walk can't use is flagged ambiguous with a fix hint; a lone labels.csv stays ambiguous. Generalises the mis-cased-marker fix (#203) to its symlink/file/lone-manifest siblings.

Tests

Added subtests for lone labels.csv, symlinked marker, plain-file marker, and symlinked-lone-CSV; existing #203 mis-cased and confident-image/text/tabular tests still pass.

Scope

Resolves the open Bugbot finding on #219 + three sibling cases surfaced in a full adversarial review. Rolls up under release ticket #220; joins the v0.8.0 payload.

🤖 Generated with Claude Code


Note

Medium Risk
Changes only pre-ingest sniffing and CLI hints, but wrong sniffing previously routed datasets into the wrong family silently—behavior shifts for several real folder layouts.

Overview
SniffFamily no longer infers layout from ReadDir name/casing checks. It probes images / texts / sequences and labels.csv the same way Discover* does (Lstat, real directories/regular files, symlink-aware), and treats marker-named entries the walk cannot use as ambiguous with an advisory hint instead of confident tabular.

The guided flow gains clearer guardrails: lone labels.csv without a media folder stays ambiguous (with a missing-folder hint); symlinked or file “markers”, mis-cased markers on Linux, and CSV layouts the tabular walk rejects (sole symlinked CSV, or regular + symlinked CSV counted as multiple) no longer auto-sniff as tables. Bad-marker hints now say “fix it and ingest again” and mention symlinks/files, not only rename.

Tests cover these layouts plus an interactive assertion update for the new hint wording.

Reviewed by Cursor Bugbot for commit 827d264. Bugbot is set up for automated code reviews on this repo. Configure here.

…markers
SniffFamily inferred layout markers from the directory listing (e.type/
IsDir + a case-sensitive name compare), which diverged from the walk's
os.Lstat-based, symlink-rejecting, case-following resolution — so several
layouts sniffed "confident tabular" and got silently ingested as a table when
the walk would have refused them or the data was actually image/text:
- A symlinked images/ / texts/ / sequences/ (IsDir()==false) fell through to
confident tabular → an image/text dataset ingested as a table, media dropped.
- A plain file named images/… did the same.
- A lone labels.csv with no media dir sniffed confident tabular, though
labels.csv is the image/text manifest name (usually an incomplete media set).
- labels.csv was matched case-sensitively while the walk resolves it
case-insensitively on macOS/Windows (under-claimed there).
- A dir whose only CSV was a symlink was counted, though DiscoverTabular
rejects a symlinked CSV.
Now media markers are probed with markerResolves (the walk's literal-lowercase
Lstat), labels.csv with a new regularFileResolves (regular-file Lstat, so it
tracks the walk's case behavior and rejects a symlink/dir), symlinked CSVs are
skipped from the count, any marker-named entry the walk can't use is flagged
ambiguous with a fix hint, and a lone labels.csv stays ambiguous. Generalises
the mis-cased-marker fix (#203) to its symlink/file/lone-manifest siblings and
resolves the open Bugbot finding on #219.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@saadqbalsaadqbal self-assigned this Jul 10, 2026

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit db3b5a6. Configure here.

Comment threadinternal/push/preview.go
The mis-cased-marker hint became a general badMarker hint covering symlinks
and files too, where "rename" isn't the right verb — the message now says
"fix it and ingest again". Update the resolveFamily consumer test's assertion
to match (it fired on the case-sensitive-FS branch in Linux CI).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bugbot follow-up on #223: SniffFamily skipped symlinked .csv entries from
csvCount, but findSingleCSV counts every non-dir .csv (symlinks included)
toward its exactly-one rule. So a directory with one regular CSV plus a
symlinked CSV sniffed confident tabular while DiscoverTabular rejects it as
multi-CSV. Count symlinks too, and track csvSymlink so a lone symlinked CSV
(which DiscoverTabular rejectSymlinks) stays ambiguous rather than confident.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@saadqbal

Copy link
Copy Markdown
CollaboratorAuthor

Good catch — fixed in 827d264. findSingleCSV counts symlinked CSVs toward its exactly-one rule, so the sniff now counts them the same way (and stays ambiguous when the sole CSV is a symlink, which the walk rejectSymlinks). Added a regular+symlink test with a DiscoverTabular parity assertion.

@LukasWodka

Copy link
Copy Markdown
Contributor

Reviewed this (adversarial pass + reproduced against the branch in a worktree). The direction is right — mirroring the walk is the correct fix for the symlink/mis-case/lone-marker cases. One real gap re-opens the same class of bug, plus a test note.

[Medium] csvCount skips symlinked .csv, but findSingleCSV counts them → the sniff over-claims confident-tabular for a directory the walk rejectsinternal/push/preview.go:142

The count does:

ife.IsDir() ||e.Type()&os.ModeSymlink!=0 { continue }
ifisCSV(name) { csvCount++ }

but findSingleCSV (internal/push/tabular.go:143-150) skips only e.IsDir(), so a symlinked .csvdoes count toward its exactly-one rule (rejectSymlink runs only on the single chosen CSV, i.e. the len==1 case). So:

  • data.csv (regular) + backup.csv (symlink to a .csv), no media dir, no labels.csv → sniff csvCount==1confident tabular, but DiscoverTabular errors found 2 .csv files … expects exactly one. Same shape when labels.csv itself is the symlink beside one regular CSV.

That's the exact "sniff claims more than the walk accepts" over-claim this PR eliminates elsewhere, re-introduced here (reproduced empirically on db3b5a6).

The subtlety: neither "skip symlinks" nor "count symlinks" alone mirrors the walk — the walk accepts iff (#.csv-by-name == 1) AND (that one is not a symlink). The lone-symlinked-CSV case needs the skip (else it'd over-claim), but the 1-regular-+-1-symlink case needs the count. So the count for the "exactly one" test should match findSingleCSV (skip only IsDir), with a separate regular-file gate on the single CSV — rather than folding the symlink check into the count.

[Low] Test gapinternal/push/preview_test.go:280

The new symlink-CSV coverage only exercises the lone symlinked CSV (csvCount 0); it doesn't cover a regular CSV beside a symlinked CSV, which is the case that trips the divergence above. Adding that case would pin it.

(Minor, likely intentional: a plain file named texts/images beside a genuine one-CSV table now trips badMarker → the image/text hint, though DiscoverTabular accepts that dir as a valid table. preview_test.go:271 locks this in, so noting only in case the "fix it and ingest again" wording for an already-valid table isn't intended.)

@saadqbal
saadqbal merged commit b38d736 into developJul 10, 2026
16 checks passed
@saadqbal
saadqbal deleted the fix/sniff-mirror-walk branch July 10, 2026 16:32
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@saadqbal@LukasWodka