fix(runtime): reject a non-int api_port at construction - #89
Conversation
kstonekuan
left a comment
There was a problem hiding this comment.
Thank you @chiruu12. Merging.
You made the call I asked you to make and argued it on the right axis. isinstance over type(...) is int is correct, and the reason it is correct is the one you gave: a config field set once and rendered into a .env that is never rewritten is not the same surface as a measurement value arriving mid-append. Reading test_append_accepts_non_json_measurement_scalars as a precedent that does not extend here, rather than one to either follow blindly or ignore, is the part that matters.
ValueError for both halves is right, for exactly the reason you gave. Classifying False with the wrong types rather than beside True is a nice distinction: the range check catches it either way, just for the wrong reason, and the test docstring says so.
What I validated on your branch against current main (f9ac687):
ruff check,ruff format --check,ty checkclean; 388 passed, 3 skipped.- Reproduced your 7-failures-on-
mainresult, so every new test is load-bearing.
Two notes, neither blocking:
The IntEnum test passes on main too and you said so in the PR body. Keeping a test that only guards against a later tightening is the right instinct, and putting that in the docstring instead of leaving me to work it out is what made the review fast.
Docs: agreed, nothing to change. The 1-65535 range was already stated and no int-passing caller sees a difference.
On #90: good split, and the issue is better for it. The design half about the bare FileNotFoundError(path) call sites belongs in Discussions under Ideas whenever you want to open it. Do not wait on me to fix #90 first, they are independent.
Summary
RuntimeConfig(api_port=True)no longer builds. The range check added in #84 cannot catch a bool, becauseTrue == 1satisfies it, and the value is only everstr()-ed after that, so it renderedAPI_PORT=Trueinto the bundle and nothing downstream objected. Construction now refuses a non-int first, with the sameValueErrorthe CLI already handles.Closes #85.
Why
The type test is
isinstance(self.api_port, int) and not isinstance(..., bool)rather thantype(...) is int. The issue suggested the stricter form, so here is the reasoning for the narrower one, and the precedent it follows.An
IntEnummember passes. It is an int by every test Python has,tycannot flag it because it really is an int subclass, and it stringifies to bare digits on 3.11+, so the.envit renders is byte-identical to the one a plain int renders. CONTRIBUTING asks for typed variants over bare literals, so rejecting one would refuse the style the project recommends.A numpy integer is refused. This is the half worth arguing, because
str(np.int64(8080))is also'8080', so it too would have rendered a correct bundle.test_append_accepts_non_json_measurement_scalarssets the opposite precedent for measurements: numpy scalars are user data arriving mid-append, andcatalog.pyfingerprints them withdefault=reprrather than crashing the whole episode over one value in a dict of many.api_portis not that kind of surface. It is set once by the caller, it is rendered into a.envthat is never rewritten, and it is interpolated intoapi_base_url, which the health wait dials andstarted_summaryprints. There is no partial-success path to protect: refusing costs the caller one line at the call site and happens before anything is written, whereas accommodating means carrying a non-int through every one of those uses on the strength ofstr()alone.ValueErrorfor both halves, including the cases whereTypeErroris the more classically correct type, because_command_upcatchesValueErrorand aTypeErrorescapes as a traceback.Two prior behaviors collapse into this one error. A float passed the range check and rendered
API_PORT=8080.0, which Compose will not take. A str or None failed the range check as aTypeErrorfrom the comparison, which nothing catches.Validation
Against unmodified
mainat f9ac687, 7 of the new tests fail:test_api_port_accepts_an_int_enum_memberpasses onmaintoo, sincemainhas no type check at all. It is a guard rail against a later tightening, not a proof of this change.Falseis in the wrong-type list rather than besideTrueon purpose. It is0, so the range check rejects it either way, just with the wrong reason.Checklist
--helptext and RUNTIME.md already state the 1-65535 range, and no documented behavior changes for a caller who was passing an int.uv run ruff check --fix,uv run ruff format, anduv run ty check.