fix: validate project name to reject path separators in apm init#724
fix: validate project name to reject path separators in apm init#724edenfunf wants to merge 2 commits intomicrosoft:mainfrom
Conversation
edenfunf
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
apm init passed the project_name argument directly into Path(), causing '/' and '\' to be silently interpreted as filesystem path separators. On Windows this produced a low-level WinError instead of a clear message. Add _validate_project_name() in _helpers.py that rejects names containing '/' or '\', call it in init() before any Path usage, and re-prompt in interactive mode when the user enters an invalid name. Fixes microsoft#723 Closes microsoft#718 (discussion)
edenfunf
force-pushed
the
fix/init-project-name-validation
branch
from
0b99acd to
0205f29
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds explicit validation for apm init project names to reject path separator characters (/ and \) and provide a clearer error than low-level filesystem exceptions (notably on Windows).
Changes:
- Added
_validate_project_name()helper to reject names containing/or\. - Validated
project_namebefore constructingPath(project_name)and re-prompted in interactive flows on invalid input. - Added unit + CLI-level tests covering rejection/acceptance cases.
Show a summary per file
| File | Description |
|---|---|
tests/unit/test_init_command.py |
Adds unit tests for the validator and CLI integration tests for invalid/valid project names. |
src/apm_cli/commands/init.py |
Calls _validate_project_name pre-Path() and re-prompts in interactive mode when invalid. |
src/apm_cli/commands/_helpers.py |
Introduces _validate_project_name() helper and its docstring. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 4
sergio-sisternes-epam
left a comment
sergio-sisternes-epam
left a comment
There was a problem hiding this comment.
Good fix -- catching path separators before Path() prevents the confusing WinError on Windows. Three items need attention before this can merge (see inline comments).
- Fix f-string bug: second string in click.echo was not an f-string so
{RESET} was printed literally instead of applying the ANSI reset code
- Extend _validate_project_name to also reject '..', which would resolve
to the parent directory via Path(); update all three error messages to
mention '..' alongside the separator check
- Update docstring to accurately describe what the guard does (interprets
as a filesystem path vs. traversal wording)
- Refactor TestInitProjectNameValidation to use CliRunner.isolated_filesystem()
removing the manual os.chdir / try-finally bookkeeping
- Add test_invalid_dotdot and test_init_rejects_dotdot covering 'apm init ..'
- Add test_init_interactive_reprompts_on_invalid_name_click and
test_init_interactive_reprompts_on_dotdot_click covering the while-True
re-prompt loop in interactive mode
- Fix test_invalid_backslash to use chr(92) to construct backslash strings
unambiguously, avoiding Python escape sequence confusion
|
@sergio-sisternes-epam Thanks for the detailed review! I've addressed all your feedback in the latest commit: f-string bug: fixed the click.echo call so {RESET} is now part of an f-string and interpolates correctly |
Description
apm initpassed theproject_nameargument directly intoPath(project_name), so/and\were silently treated as filesystem path separators. On Windows,apm init 4/15produced a low-level OS error:instead of a clear validation message.
Fixes #723
Closes discussion #718
Changes
_helpers.py— adds_validate_project_name(name): returnsFalseif the name contains/or\, making it clear these are invalid in a project name context.init.py— calls_validate_project_namebeforePath(project_name)is constructed and exits with a descriptive error. Also re-prompts in interactive mode when the user types an invalid name.tests/unit/test_init_command.py— addsTestProjectNameValidation(unit tests for the helper) andTestInitProjectNameValidation(integration tests verifying CLI exit code and output for/and\inputs).Type of change
Testing