Dispatch layup verbs from installed metadata, not PATH (#500) - #506
Merged
Conversation
find_layup_verbs() walked every PATH directory for executables named layup-*, and the dispatcher ran `layup-<verb>` as a bare name, leaving the choice of program to PATH order. With two layup installations on one machine, `layup comet` from one environment could execute the other's layup-comet. Worse, any executable named layup-<verb> in a writable directory earlier on PATH would be run in preference to the real one, with the user's privileges -- an empty PATH entry means the working directory, and shared scientific systems often carry group-writable bin directories. PATH could also inject verbs into layup's own help output. Verbs now come from the installed distribution's entry points, and the verb runs in this process. Nothing is resolved by name, so the code that runs is always this installation's. Also pins the three CLI subprocess tests to this environment's console script rather than PATH's, which is how the problem surfaced.
mschwamb
reviewed
Aug 31, 2026
| dropped in any writable directory earlier on ``PATH`` (an empty ``PATH`` | ||
| entry means the working directory) would be run in preference to the real | ||
| one, with the user's privileges. | ||
| """ |
Collaborator
There was a problem hiding this comment.
This whole docstring isn't what this code is doing adn is convoluted- it's what the AI thinks it fixed. I think it needs to be rewritten by a human for who is going to edit this code in 6 months or 6 years from now
Collaborator
Author
There was a problem hiding this comment.
Could you please take charge of that, @mschwamb ?
Collaborator
There was a problem hiding this comment.
I'd get rid of that function and that would solve the issue. As per my other feedback comment.
mschwamb
reviewed
Aug 31, 2026
| for ep in distribution("layup").entry_points: | ||
| if ep.group == "console_scripts" and ep.name.startswith("layup-"): | ||
| verbs[ep.name[len("layup-") :]] = ep | ||
| return verbs |
Collaborator
There was a problem hiding this comment.
Do we need this in a separate function? It's a bit of Russian doll to read though. These lines could easily be in find_layup_verbs() and it would say having to dive into a separate function
mschwamb
requested changes
Aug 31, 2026
mschwamb
left a comment
mschwamb
left a comment
Collaborator
There was a problem hiding this comment.
Some feedback to consider
Resolved in tests/test_comet.py and tests/test_predict.py: keep this branch's layup_cli() wrapper, which pins the call to the environment under test, and take main's post-#503 argument convention (--stem for the file stem, -o for the output directory).
Review feedback: the two-function form was a step to read through for no gain. find_layup_verbs() now returns the verb -> entry point mapping directly, main() reuses it instead of reading the distribution metadata a second time, and the argparse choices are sorted at the point of use so the help text keeps a stable order. Also corrects the layup_cli() docstring, which gave a broken assist/rebound link as the example failure. The failure actually seen is an older layup on PATH rejecting arguments the current code added.
Collaborator
Author
|
I think this addresses your concerns, @mschwamb |
Per review: the previous text was a changelog rather than a description. Two of its three sentences explained what the change fixed and instructed a future editor not to undo it, which is commit-message material -- someone meeting this function in six years needs to know what it returns and why it reads metadata rather than scanning PATH, and nothing else. Wording reviewed and adopted by M. J. Holman.
mschwamb
approved these changes
Sep 3, 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
2 participants
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.
layup <verb>chose which program to run byPATHorder.find_layup_verbs()walked everyPATHdirectory for executables namedlayup-*, and the dispatcher ranlayup-<verb>as a bare name, so resolutionwas left to the OS.
Two consequences. With two layup installations on one machine,
layup cometfrom one environment can execute the other's
layup-comet-- that is how thissurfaced, and it is what made the three CLI tests in #500 fail. And any
executable named
layup-<verb>in a writable directory earlier onPATHrunsin preference to the real one, with the user's privileges; an empty
PATHentry means the working directory, and shared systems often carry
group-writable bin directories.
Verbs now come from the installed distribution's entry points and run in this
process, so nothing is resolved by name.
Also pins the three CLI subprocess tests to this environment's console script.
With the conda installation first on
PATH-- the configuration that wasfailing -- all three now pass. Full suite: 488 passed, 1 skipped.
Closes #500. Suggest v1.0: this is a correctness problem in the shipped CLI
rather than the test hygiene the issue describes.