-
Notifications
You must be signed in to change notification settings - Fork 14
feat: automatically find existing IdentityFile
#39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
haginara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds automatic discovery of existing SSH identity files instead of always defaulting to ~/.ssh/id_rsa. This enhancement helps users who have different key types (like ed25519, ecdsa, etc.) by automatically detecting and suggesting the appropriate key file.
Key changes:
- Added
get_identity_files()function to scan the .ssh directory for identity files matching theid_*naming pattern - Modified the
addcommand to use the discovered identity file as the default value instead of hardcodedid_rsa
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for file in os.listdir(fullpath): | ||
| # if not ends with .pub and starts with id_ | ||
| if file.startswith("id_") and not file.endswith(".pub") and os.path.isfile(os.path.join(fullpath, file)): | ||
| keys.append(os.path.join(path, file)) |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function does not handle potential permission errors when accessing the .ssh directory. If the user lacks read permissions for the directory, os.listdir() will raise a PermissionError. Consider adding error handling to gracefully fall back to the default identity file in such cases.
| for file in os.listdir(fullpath): | |
| # if not ends with .pub and starts with id_ | |
| if file.startswith("id_") and not file.endswith(".pub") and os.path.isfile(os.path.join(fullpath, file)): | |
| keys.append(os.path.join(path, file)) | |
| try: | |
| for file in os.listdir(fullpath): | |
| # if not ends with .pub and starts with id_ | |
| if file.startswith("id_") and not file.endswith(".pub") and os.path.isfile(os.path.join(fullpath, file)): | |
| keys.append(os.path.join(path, file)) | |
| except (PermissionError, FileNotFoundError): | |
| # Cannot access the directory or it does not exist; fall back to default key | |
| click.echo(f"[Warning] unable to access {fullpath}, using default identity file") | |
| return os.path.join(path, "id_rsa") |
| if 'IdentityFile' not in attrs: | ||
| attrs['IdentityFile'] = click.prompt("IdentityFile", | ||
| type=str, default="~/.ssh/id_rsa", | ||
| type=str, default=get_identity_files(), |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new get_identity_files() function is called during the add command flow, but there are no tests covering this new behavior. Since the repository has comprehensive test coverage for CLI commands (as seen in test_cli.py), tests should be added to verify the identity file discovery functionality, including edge cases like missing .ssh directory, multiple identity files, and no identity files found.
| elif len(keys) == 1: | ||
| return keys[0] | ||
| else: | ||
| print(f"[Warning] multiple identity files found: {keys}") |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function uses click.echo for the warning on line 80 but uses print for the warning on line 85. This is inconsistent. Since this is a Click-based CLI application, click.echo should be used consistently for all output.
| print(f"[Warning] multiple identity files found: {keys}") | |
| click.echo(f"[Warning] multiple identity files found: {keys}") |
| for file in os.listdir(fullpath): | ||
| # if not ends with .pub and starts with id_ | ||
| if file.startswith("id_") and not file.endswith(".pub") and os.path.isfile(os.path.join(fullpath, file)): | ||
| keys.append(os.path.join(path, file)) |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function does not handle the case where the .ssh directory does not exist. This will cause an unhandled FileNotFoundError when os.listdir(fullpath) is called. Consider wrapping the directory listing in a try-except block or checking if the directory exists before attempting to list its contents.
| keys.append(os.path.join(path, file)) | ||
| if len(keys) == 0: | ||
| # keys not found, return default key | ||
| click.echo(f"[Warning] identity file not found, using default") | ||
| return os.path.join(path, "id_rsa") |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an inconsistency in path handling. On line 72, fullpath is created using os.path.expanduser(path), but on line 77, keys are appended using the unexpanded path variable instead of fullpath. This means that if the function is called with the default "/.ssh", the returned paths will contain the literal "" instead of the expanded home directory path. Either use fullpath throughout, or ensure the returned paths use the same format as the input.
| keys.append(os.path.join(path, file)) | |
| if len(keys) == 0: | |
| # keys not found, return default key | |
| click.echo(f"[Warning] identity file not found, using default") | |
| return os.path.join(path, "id_rsa") | |
| keys.append(os.path.join(fullpath, file)) | |
| if len(keys) == 0: | |
| # keys not found, return default key | |
| click.echo(f"[Warning] identity file not found, using default") | |
| return os.path.join(fullpath, "id_rsa") |
Find existing keys in the default SSH config path and it can save some time if one doesn't have
id_rsakey