OpenAICompatibleExecution stopped type-checking max_retries in #410

[chiruu12]

OpenAICompatibleExecution.__post_init__ stopped type-checking max_retries in #410. The change is on main now, at 224dc6f.

#410 is a documentation change to FORMAT.md. Its title, its description and the rest of its diff are all about the LeRobot importer's episode/v1 keys. Alongside that it carries one line in src/hflow/build_ai_vlm_checks.py:

-        if not isinstance(self.max_retries, int) or isinstance(self.max_retries, bool):
+        if False:
             raise ValueError("max_retries must be an integer")

if False: is the shape a guard takes while it is being neutered to check whether a test notices, which is the experiment #407 is built on. It looks like one of those was left in rather than backed out.

Verification

Against main at 224dc6f, constructing the dataclass directly:

max_retries=True   accepted, stored as True
max_retries=2.5    accepted, stored as 2.5
max_retries="5"    TypeError: '<' not supported between instances of 'str' and 'int'
max_retries=None   TypeError: '<' not supported between instances of 'NoneType' and 'int'

True and 2.5 should raise ValueError("max_retries must be an integer"). True is the case the deleted clause named on its own: bool subclasses int, so it survives an isinstance(x, int) check and then compares equal to 1.

"5" and None still fail, but from the self.max_retries < 0 comparison on the next line rather than from the guard. The caller gets a raw TypeError out of a comparison instead of the message that says what was wrong.

The full suite is green with the guard disabled. That is the subject of #407: this is one of the eight refusals in this class with no test behind it, so nothing failed when it went.

What to do

Restore the clause, and land a case that fails without it. #407 covers this guard among the other seven, so it can go there instead if you would rather keep it in one change.

On scope

Worth saying separately from the fix: a code change to a validation path travelled in a PR whose title, description and remaining diff are all documentation. Nothing about the PR as described would lead a reviewer to look at build_ai_vlm_checks.py, which is how a disabled guard reached main on a docs merge. Keeping a PR to what its description claims is what makes review load match review risk.

[kstonekuan]

Fixed on main by #414. Good catch, and the attribution needs correcting: this was not #410's author's doing.

I introduced the if False: myself. I had been mutation-testing that exact guard while scoping #407; the script restores the source in a finally, the run hit a ten-minute timeout that killed the process before the restore ran, and I then swept the mutated line into a fixup commit on #410 with git add -A. #410 was a documentation PR. The source hunk in that merge is mine.

The part worth keeping from this, since it is the same lesson your issue is about: I ran the full gate on that PR and it passed 1531 tests. It passed because nothing tests that guard, which is precisely what #407 says. A mutation I made to demonstrate that a guard is untested was then hidden by the guard being untested.

Main has been swept for other leaked mutations; this was the only one.

[chiruu12]

Separate from this issue.

I have noticed the same pattern a few times now: I finish something, open the PR, and then wait rather than pick up the next issue, because pr-limit.yml caps outside authors at one open PR. That is fine when review is quick. When a PR sits for a week the queue here keeps moving and the issues I screened get claimed by someone else in the meantime.

Would two be reasonable? I would keep them in unrelated files so neither review depends on the other. Entirely your judgement, and I will work within whatever the limit is.

[chiruu12]

Thanks for the quick fix, and sorry for the misattribution. I read the blame and stopped at the merge, so I pointed the scope paragraph at #410's author for a hunk that was yours. That paragraph should not have named a PR before I knew how the line got there.

The git add -A after a killed cleanup is a good failure to have written down. I have hit the same shape myself, which is why I now name paths on every add rather than sweeping the tree.

[kstonekuan]

Keeping the cap at one, because in both cases the cap is not what cost you the issue.

On #407 you commented with a plan at 06:02 and #412 was already open over the same eight refusals. A second slot would not have helped there; nothing reserves an issue against a PR that already exists.

On #408 you claimed it at 08:02 with a plan that was right, and you had no open PR at that moment. The cap was not binding. What lost it was that I did not reply and did not assign you, and #419 landed six hours later. That is the whole failure.

So the fix is assignment, not a second slot. From here, when you say you are taking something, it gets assigned, and an assignee is the one thing that reserves an issue in this repo. You can screen and claim without holding a PR slot open to do it.

Worth saying separately: you are nineteen PRs in, so the starter pool is not your lane. The advanced backlog has no race in it and can be assigned to you on request. #320 is still yours if you want it, but GitHub will not let me assign you until you have commented on that issue, so drop a line there and I will assign it.

If review latency does become the thing holding you up, say so and we will look at the cap again with that as the reason.