fix(cli): unbreak the interactive pickers and Go timestamp parsing on 3.10 - #824
Open
guill wants to merge 2 commits into
Open
fix(cli): unbreak the interactive pickers and Go timestamp parsing on 3.10#824guill wants to merge 2 commits into
guill wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
This was referenced Aug 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack of 7 — review bottom-up (this is the top)
feat(build): packaging, sync and Builder client primitives — basemainfeat(build)!: replace the legacy build and distribution surface (breaking)feat(deploy): the deployment control planefeat(deploy): the asset and job data-plane clientscomfy deploy run#805 —feat(deploy):comfy deploy runfix(build): a pull no longer deletes definition data silentlyfix(cli): unbreak the interactive pickers and Go timestamp parsing on 3.10 ← this PREach PR is based on the branch below it, so its own diff is only its commits. Merge in order.
TL;DR: Two independent defects found while exercising the stack by hand, neither caught by the suite. Every interactive picker in the CLI raised
ValueErrorbefore it could draw, and roughly one Go-issued timestamp in ten was unparsable on Python 3.10 — the version CI actually runs. Stacked on #810.1.
ce93092— every interactive picker raised before drawingprompt_selectandprompt_autocompletedefaulteddefaultto""and passed it straight to questionary. questionary validates thatdefaultis one ofchoices, and""never is:This fires on construction, before any prompt renders, so no caller with a real choice list worked. That is every picker in the CLI:
build.py:2029comfy build pullopens when given no--iddeploy_compute.py:26deploy_compute.py:43uv.py:227cmdline.py:1853,:1858The fix is the correct sentinel:
default: ChoiceType | None = None. questionary treatsNoneas "no default" and selects the first choice, which is what the""was reaching for.Worth noting for #810's reviewer: the broken picker is the same one the adopt flow depends on —
comfy build pullwith no--idlists the workspace's builds and asks you to choose one.2.
f40f519— Go's trimmed fractional seconds are unparsable on Python 3.10Our Go services marshal
time.Time, which encodes as RFC 3339 Nano — and that trims trailing zeros from the fractional part. A timestamp holding437450microseconds ships as...:09.43745Z: five fractional digits.datetime.fromisoformaton Python 3.10 —requires-python = ">=3.10", and the CI matrix runs 3.10 only — accepts exactly 3 or 6 fractional digits and rejects a bareZ. So that value raises. Trailing-zero trimming makes this roughly a one-in-ten event, and since acreatedAtnever changes, an affected row was permanently unreadable rather than intermittently so.New
utils.parse_rfc3339parses any sub-second precision and always returns an aware datetime: it pads so.1is a tenth rather than a microsecond, truncates so Go's nanosecond precision degrades instead of failing, and reads a missing zone as UTC because callers order these against each other and comparing naive to aware raisesTypeError.Three call sites move onto it, and each had its own failure shape:
deploy_resolve.deployment_selection_key— raiseddeploy_server_erroron an affected deployment. The status and timestamp checks are also split apart now: oneexcept (KeyError, ValueError)covered both and reportedstatus=..., so a validprovisioningwas printed as the evidence while the timestamp that actually failed went unmentioned. Each is now reported separately, naming the value it rejected.build._release_order— comparedcreatedAtas text, so a whole-second...:16Zsorted above the strictly later...:16.5Z(.precedesZin ASCII), andcomfy build releasepicked the wrong newest release. Now ordered on the parsed instant, with an_UNDATEDfloor so a badly dated release loses the tiebreak instead of winning it.jobs._parse_epoch— swallowed the failure and returned0.0, sorting an affected row below every dated one, precisely wherejobs lsslices[:limit]and drops a fresh completion.Validation
test_utils.py,test_deploy_resolve.py,test_jobs.py,test_build_release.py: 314 passed.ruff check ./ruff format --check .clean at CI's pin (0.15.15).createdAtshape and that a dated terminal row never sinks to the epoch-zero floor.default=""raises,default=Noneconstructs.