fix(checks): restore the max_retries type guard - #414
Conversation
The guard went out disabled with Hebbian-Robotics#410, so bools and floats were accepted as max_retries. Restore the isinstance check and add a regression test covering True and 2.5, both of which slipped through while it was off.
The shared **kwargs dict inferred as dict[str, str], so splatting it into endpoint, model and max_retries produced seven ty errors. Parametrized instead, with the positive case as its own test, and ty's own suppression comment rather than the mypy-style one, which ty does not honour.
kstonekuan
left a comment
There was a problem hiding this comment.
Merging. This regression was mine, not yours, and I want that on the record before anything else.
The if False: came from my fixup commit on your #410, not from your work. I had been mutation-testing that exact guard while scoping #407, the script restores the source in a finally block, and the run hit a ten-minute timeout that killed the process before the restore ran. The mutated line sat in my working tree, I then ran git add -A while preparing the FORMAT.md fixup, and it went out inside a commit I described as cosmetic. Your PR was documentation only, and the merge commit attributing a source change to it is my doing.
Two things made it invisible, and both are uncomfortable:
- I ran the full gate on that PR and reported 1531 passing. It did pass. Nothing tests that guard, which is exactly what #407 says, so my own mutation was invisible to the suite I was using to check my own work.
- I described the change as docs-only in the review. The
+2/-1on a source file was right there in the diff stat and I did not read it.
I have swept main for other leaked mutations and there is one and only one, this line. rg "if False:" src/ packages/ tests/ comes back with nothing now.
On the fix itself: restoring the guard verbatim is right, and covering both True and 2.5 is the right pair, since they fail for different reasons and only the first is the trap. The regression test does fail on main before the change:
FAILED test_openai_compatible_execution_refuses_non_integer_max_retries
I pushed one fixup, 286a860. The shared **kwargs dict inferred as dict[str, str], so splatting it into endpoint, model, and max_retries produced seven ty errors on the merged result. Parametrized instead, with the positive case as its own test, and swapped # type: ignore[arg-type] for # ty: ignore, which is what this repo uses and what ty actually honours. Worth knowing for next time: the mypy-style code is silently ignored here.
Gate on the merged result: ruff check, ruff format --check, ty check clean, 1540 passed / 6 skipped.
Fixes #413. Two PRs merged in your first day, one of them cleaning up after me. Thank you for catching it rather than assuming the maintainer knew what he was doing.
Restores the max_retries guard in OpenAICompatibleExecution.post_init, which went out disabled in #410. With the guard off, True and 2.5 were accepted as max_retries (details in #413).
The regression test covers both values and fails on current main. Note that #412 adds wider validation coverage to the same test file, so the two may conflict there; happy to rebase if needed.
Fixes #413