Add detection for file paths mistakenly used as cluster names#9233
Merged
Conversation
…h -c flag Detect when the cluster name argument to -c looks like a file path (ends in .yaml/.yml/.json or is an existing file) and raise a helpful error suggesting the correct syntax, e.g.: sky launch -c <cluster-name> job.yaml Added checks in sky launch, sky exec, and sky jobs launch commands. https://claude.ai/code/session_0127tbsbgCE95NTZ5h7SKv9w
…warning Instead of rejecting cluster names that look like file paths, show a warning and ask the user to confirm they want to proceed. This matches the existing pattern used for entrypoint validation. https://claude.ai/code/session_0127tbsbgCE95NTZ5h7SKv9w
When the user passes -y/--yes, skip the confirmation prompt for cluster names that look like file paths, allowing the command to proceed without interruption. https://claude.ai/code/session_0127tbsbgCE95NTZ5h7SKv9w
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a validation check to prevent users from mistakenly providing a file path as a cluster or job name in the launch, exec, and jobs_launch commands. A new utility function, cluster_name_looks_like_file_path, was added along with unit tests to detect common file extensions or existing files on disk. Feedback suggests refactoring the duplicated confirmation prompt logic into a helper function to improve maintainability.
Comment on lines
+1221
to
+1226
| if not yes and common_utils.cluster_name_looks_like_file_path(cluster): | ||
| click.confirm( | ||
| f'Cluster name {cluster!r} looks like a file path. ' | ||
| f'Did you mean: sky launch -c <cluster-name> {cluster}\n' | ||
| 'Proceed anyway?', | ||
| abort=True) |
Contributor
There was a problem hiding this comment.
The confirmation prompt logic is duplicated across three commands. Consider creating a helper function to encapsulate this validation and prompt logic to improve maintainability and reduce code duplication.
…prompting With -y, print a yellow WARNING about the file-path-like cluster name but continue execution instead of suppressing the message entirely. https://claude.ai/code/session_0127tbsbgCE95NTZ5h7SKv9w
Deduplicate the warn-or-confirm logic into _warn_if_name_looks_like_file_path(), reused by both sky launch and sky jobs launch. https://claude.ai/code/session_0127tbsbgCE95NTZ5h7SKv9w
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
3 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.
Review-level: checked the code manually and tried locally.
Description
This PR adds a safety check to detect when users accidentally pass file paths as cluster names in CLI commands. This addresses a common user mistake where users type commands like
sky launch -c job.yamlinstead ofsky launch -c mycluster job.yaml.Changes
New utility function (
sky/utils/common_utils.py):cluster_name_looks_like_file_path()function that detects if a cluster name looks like a file path by checking for common file extensions (.yaml,.yml,.json) or if the path exists as a file on disk.CLI safeguards (
sky/client/cli/command.py):launch: Warns when cluster name looks like a file pathexec: Warns when cluster name looks like a file pathjobs_launch: Warns when job name looks like a file pathUnit tests (
tests/unit_tests/test_sky/utils/test_common_utils.py):TestClusterNameLooksLikeFilePathcovering:.yaml,.yml,.jsonextensions (case-insensitive)Noneinput handlingTesting
https://claude.ai/code/session_0127tbsbgCE95NTZ5h7SKv9w