Eight LeRobot importer metadata refusals have no test, including the pagination loop guards

[Sagar-024]

Current behavior

The importer's episodes path carries a wall of refusals whose job is to fail loud on broken Hugging Face metadata. The suite covers the fps refusals (test_info_json_refuses_non_finite_or_non_positive_fps), the commit-sha refusal, the UTF-8 and JSON-decode errors (#301), and the boolean-dimension refusals (#306). Eight branches have no test at all. The receipt is a grep, the same one #393 used:

grep -rn "<message fragment>" tests/   ->  0 hits for all eight:

"not a LeRobot v3 repository"                  lerobot.py:462
"is not a JSON object"                         lerobot.py:472
"must define a non-empty data_path template"   lerobot.py:550
"must define a non-empty video_path template"  lerobot.py:555
"no meta/episodes parquet files found"         lerobot.py:572
"is not a list of objects"                     lerobot.py:416
"repeated an already fetched pagination URL"   lerobot.py:388
"contains an invalid pagination URL"           lerobot.py:395

(The two existing "is not a JSON object" hits are the runtime client's, not the importer's.)

The sharpest two:

• The pagination loop guards (:388-395). visited_urls plus the invalid-URL parse are the only protections against a Hugging Face response (or a tampered Link header) looping the fetch forever or walking it somewhere unexpected. The origin-mismatch guard next to them is tested, which makes the two untested siblings stand out: half the loop's defenses are pinned, half are not.
• The template refusals (:550, :555). data_path and video_path are format strings that later go through .format() with caller-provided indexes. An empty template fails at conversion time with a confusing error instead of at the boundary with the real one. Both refusals sit on lines our recent diffs (feat(lerobot): publish imports into bucket-backed data roots #377, fix(lerobot): read episode metadata from every v3 shard (fixes #293) #383, feat(lerobot): resume multi-episode imports at episode boundaries (#303) #390, The LeRobot importer ignores the source's success label and hardcodes "true" for every episode #395) have each moved, so a pin also protects the refactors.

Every branch is reachable with the same stub pattern the #301 tests already use (stub urlopen, feed fixed bytes), so none of this needs new machinery.

What the tests should assert

1. Each refusal message, pinned byte-identical (they are user-facing).
2. The pagination-guard tests prove termination: a response whose Link header repeats the fetched URL raises instead of hanging.
3. No partial state: the refusals fire before any cache or output writes, matching the fail-loud contract of the covered refusals.

Definition of done

1. All eight branches reached through the public import path with the LeRobot Hugging Face responses lose their source on JSON decode failures #301 stub pattern.
2. Refusal messages pinned byte-identical; a regression changing any of them fails.
3. The pagination and invalid-URL guards proven to terminate and to reject, respectively.
4. No message text changes; no behavior changes.

Non-goals

• Adding new refusals
• Refactoring the fetch loop
• Changing any refusal message

Validation

uv run ruff check --fix
uv run ruff format
uv run ty check
uv run pytest -q tests/test_lerobot_converter.py
uv run pytest -q

[Sagar-024]

Taking this if accepted. The importer boundary is where my last three PRs live, so the eight tests follow the #301 stub pattern and pin every message byte-identical. The pagination guard is the one I would most like to see pinned: it is the loop's only termination defense that nothing exercises.

[kstonekuan]

Accepted, labelled bug / good first issue. Your grep receipt holds, all eight, and the contrast you drew is the best argument in it:

"not a LeRobot v3 repository"                  tests: 0
"is not a JSON object"                         tests: 0
"must define a non-empty data_path template"   tests: 0
"must define a non-empty video_path template"  tests: 0
"no meta/episodes parquet files found"         tests: 0
"is not a list of objects"                     tests: 0
"repeated an already fetched pagination URL"   tests: 0
"contains an invalid pagination URL"           tests: 0

test_hf_tree_rejects_next_link_with_different_origin   tests/test_lerobot_converter.py:893

Three guards sit in the same while loop, one has a test, two do not. Half a loop's defenses pinned is the shape that hides regressions, because the passing half makes the region look covered. That same pattern cost #390 a fixup this afternoon: six identity comparisons, one test that differed in two fields at once, four fields carrying no weight.

The pagination loop guard is the one I would write first. visited_urls is the only thing standing between a malformed or tampered Link header and a fetch that never terminates, and an untested termination guard is the worst kind: when it breaks, the symptom is a hang rather than a failure, so CI reports nothing and a human waits. DoD item 3 is right to insist the test proves termination rather than merely proving a raise.

Timing is good. I had this seam on my own candidate list this afternoon and skipped it because importers/lerobot.py had four PRs in flight (#383, #390, #400, #403). All four are merged and the file is quiet, so now is the moment. Do rebase before starting: _hf_tree's line numbers moved, and :462/:472/:550/:555/:572 will have shifted too.

On who takes it. I labelled it good first issue because it genuinely is one: eight branches, the #301 stub pattern already exists, no design judgment anywhere in it. That does not shut you out. You reported it, and a reporter fixing what they found is filling the pool rather than draining it, so it is yours if you want it. If you would rather spend your time on #404, which needs the judgment you already put into it, say so and I will leave this one for a newcomer.

[Sagar-024]

Leave it for a newcomer, it's a clean on-ramp. My time goes to #404, and I'll rebase before starting.
@kstonekuan

[Sagar-024]

Grep receipts from hflow at 8044159 (Python 3.12, WSL2 Ubuntu); the fragment greps are the repro, runnable as-is.

[msparsh13]

hi, if this is not taken by someone i would like to take it

[Sagar-024]

@msparsh13 yeah go on it yours bro

[kstonekuan]

Closed by #415, and @msparsh13 I owe you an apology for how that went.

@Sagar-024 handed this to you at 12:30 and @harshitethic opened a PR at 13:23. In this repo only an assignee reserves an issue, and I never assigned anyone, so the first PR stands. But the handoff was right here in the thread and I read it; assigning you then would have taken five seconds and would have made the position clear to everyone before anyone wrote code. That is my miss, not either of yours.

@msparsh13 #408 is unassigned and closest in shape to the #412 work you just landed: four refusals in the native overlay manifest parse stage, none tested, with a merged pattern in #394 to copy. Say the word and I will assign it to you before you start.

@harshitethic on the fix itself: the pagination case was the one worth getting right and it needed one more turn, which is on the PR.

[msparsh13]

Hi @kstonekuan, thank you for the consideration and for clarification.

For now, I’m just trying to understand the repo thoroughly. No need to assign #408 to me, since there’s a good chance I may go very slow at times while I’m getting familiar with everything (I haven’t looked at #408 yet).

I will take on issues at my own pace as I get more comfortable with the repo. Thanks again

[kstonekuan]

Understood, and going slow while you learn the repo is the right instinct.

When you do want something without a clock on it, #287 suits that shape well: point HFlow at a real corpus such as Egocentric-10K and report what breaks, what is slow, and what is awkward. There is nothing to finish and no one waiting on it, and reading the repo through a failure you hit yourself teaches it faster than any issue we can write.