fix(cli): report a missing pipeline file as bad input, not a crash - #95
Conversation
kstonekuan
left a comment
There was a problem hiding this comment.
Thank you @chiruu12. Merging.
This is the most thoroughly argued PR the repo has had. Three things stand out.
You fixed the raise sites instead of the symptom. Catching FileNotFoundError in _command_up and printing something nicer would have closed the issue. Moving all four sites to the three-argument form fixed deploy's message as a side effect, with no CLI change, because _command_deploy already caught it and returned 2. That is the smaller diff and the more correct one.
You changed the wording I suggested, and you were right to. The issue proposed up: importing <path> failed: ... to match manifest and stale. Those commands genuinely import the pipeline; up fails in render_bundle before importing anything, so up: [Errno 2] No such file or directory: '<path>' is accurate where my wording would have been a lie about what the command was doing. I did not notice that when I wrote the issue.
You answered the two questions a reviewer would ask, before being asked. I went to check whether the new handler could swallow a genuine post-start failure and found you had already traced it: _compose.py:49 converts a missing docker into ComposeError, so the handler only ever sees render_bundle. That saved me the trip.
The directory case is the right call. --pipeline <a-directory> now says "No such file or directory" about something that exists, and the honest fix is IsADirectoryError, which changes the exception type on a public API and breaks every existing except FileNotFoundError. Deferring that to the Discussions thread with the bare-versus-errno question is correct. Flagging it in the PR body rather than leaving me to find it is what made it a non-issue.
Same for the two-line stderr: the definition of done said one line, the narration is pre-existing on every up, and saying so up front is better than silently not matching.
What I validated on your branch:
ruff check,ruff format --check,ty checkclean; 394 passed, 3 skipped. Your branch was already on currentmain(ca17730).hflow up --pipeline <missing>: one error line, exit 2, no traceback.hflow deploylikewise exits 2 and now prints the errno text instead of the bare path.- Your revert-each-site claim holds: each of the four raise sites is pinned by exactly one test.
The #26 observation was a good catch and worth stating plainly: that issue was closed with only storage.py fixed, and the pattern it was closed for survived in four other places. That is the kind of thing worth reopening an old issue over.
Open the Discussions thread whenever you want. Two questions are now queued for it: whether the bare FileNotFoundError(path) construction should be centralized, and whether a directory should raise IsADirectoryError. Both are real, and the second one is a public-API decision, so it should not be settled in a review comment.
Summary
hflow up --pipeline <missing>printed a traceback and exited 1. It now prints one line and exits 2.Closes #90.
Why
1 and 2 are not interchangeable here. 1 means the runtime started and then failed, so the handler that owns it prints teardown advice and the caller is told containers may still be running. A missing pipeline file fails inside
render_bundle, before any container exists, so 1 sent a script after containers that were never created. That is why the new handler is its ownexceptblock rather than an addition to the(ComposeError, TimeoutError)tuple.The raise sites moved to the three-argument
FileNotFoundError, matchingstorage.py:656, sostr(error)carries the errno text instead of a bare path. That is four sites: the pipeline and requirements checks in_bundle.pyand the same pair in_deploy.py.deployneeded no CLI change._command_deployalready caughtFileNotFoundErrorand returned 2, so fixing the raise site fixed its message too. Its old output wasdeploy: /tmp/x.py, which is the bare-path form #26 was closed for and which onlystorage.pywas ever fixed for.Two questions a reviewer would reasonably ask, answered up front.
Can the new handler swallow a genuine runtime failure? No. The one plausible source after containers start is a missing
dockerbinary, and_compose.py:49already converts that intoComposeError. Nothing betweencompose_up_detachedand the return raisesFileNotFoundError, so the new handler only ever seesrender_bundle.Does it lie about a path that exists? Yes, in one case, and I left it.
is_file()is False for a directory, sohflow up --pipeline <a-directory>now says "No such file or directory" about something that is present. The old bare-path message was not wrong so much as useless, so this is a small regression in accuracy for an input this issue does not cover. Fixing it properly means raisingIsADirectoryError, which changes the exception type on a public API and breaks everyexcept FileNotFoundErrorthat currently catches it. That is a design call, and it belongs with the bare-versus-errno question already headed for Discussions rather than in this PR.Validation
The 5 failures are
test_ffmpeg.pyandtest_run_profiles.pyon macOS. They reproduce on unmodifiedca17730with no changes applied, so they are the local ffmpeg build and not this change.Reverting all three source files to
origin/mainand rerunning the CLI suite:All 29 pass with the change restored. Each of the four raise sites is pinned on its own: reverting any single one of them fails exactly one test.
Behaviour before and after, same command:
stderr carries two lines, not one:
up: rendering the runtime bundle at <dir>prints first, from the existing progress callback. The definition of done said one line, so flagging the difference. The error itself is one line and the narration is pre-existing behaviour on everyup.Checklist
uv run ruff check --fix,uv run ruff format, anduv run ty check.