From b972f58bbf3bfa93ace6fe7d2aa3159b80cbe3b0 Mon Sep 17 00:00:00 2001 From: "jiaxin.loh" Date: Fri, 28 Aug 2026 16:29:56 +0800 Subject: [PATCH 1/3] feat: prevent overwriting existing downloads --- app/commands/download.py | 168 ++++++++++++++++++---------- tests/e2e/commands/test_download.py | 68 ++++++++++- tests/e2e/conftest.py | 11 ++ 3 files changed, 185 insertions(+), 62 deletions(-) diff --git a/app/commands/download.py b/app/commands/download.py index be902d4..ee20425 100644 --- a/app/commands/download.py +++ b/app/commands/download.py @@ -1,5 +1,8 @@ +import json import os +import shutil import sys +import tempfile from datetime import datetime from pathlib import Path from typing import Dict, Optional @@ -32,6 +35,27 @@ ) from app.utils.gitmastery import ExercisesRepo, Namespace +BASE_FILES = [".gitmastery-exercise.json", "README.md"] + + +def _read_existing_config(path: Path) -> Optional[ExerciseConfig]: + """ + Reads the exercise config of an already downloaded exercise, if it is readable. + + A folder that is about to be overwritten may not hold a valid exercise config, e.g. + a partial download or a folder the student created themselves, so a failure to read + it is not fatal. + + :param path: folder of the already downloaded exercise + :type path: Path + :return: the existing exercise config, or None if it cannot be read + :rtype: Optional[ExerciseConfig] + """ + try: + return ExerciseConfig.read(path, 0) + except (FileNotFoundError, PermissionError, OSError, json.JSONDecodeError, KeyError): + return None + def _download_exercise( exercise: str, formatted_exercise: str, download_time: datetime @@ -49,53 +73,60 @@ def _download_exercise( ) old_config: Optional[ExerciseConfig] = None - if os.path.isdir(exercise): - warn(f"You already have {exercise}, removing it to download again") - old_config = ExerciseConfig.read(Path(exercise), 0) - rmtree(exercise) - os.makedirs(exercise) - os.chdir(exercise) + # The base files are staged outside of the exercise folder so that the Git and + # Github requirement checks can run before any existing folder is removed. A + # failed check must never leave the student without their work. + with tempfile.TemporaryDirectory() as staging_dir: + staging_path = Path(staging_dir) - info("Downloading base files...") - base_files = [".gitmastery-exercise.json", "README.md"] - for file in base_files: - repo.download_file( - f"{formatted_exercise}/{file}", - f"./{file}", - False, - ) - config = ExerciseConfig.read(Path("./"), 0) - - # Check if the exercise requires Git to operate, if so, error if not present - if config.requires_git: - try: - info("Exercise requires Git, checking if you have it setup") - invoke_command(git) - except SystemExit as e: - if e.code == 1: - # Exited because of missing Github configuration - # Rollback the download and remove the folder - warn("Git is not setup. Rolling back the download") - os.chdir("..") - rmtree(exercise) - warn("Setup Git before downloading this exercise") - sys.exit(1) + info("Downloading base files...") + for file in BASE_FILES: + repo.download_file( + f"{formatted_exercise}/{file}", + staging_path / file, + False, + ) + staged_config = ExerciseConfig.read(staging_path, 0) + + # Check if the exercise requires Git to operate, if so, error if not present + if staged_config.requires_git: + try: + info("Exercise requires Git, checking if you have it setup") + invoke_command(git) + except SystemExit as e: + if e.code == 1: + # Exited because of missing Git configuration + # Nothing has been written yet, so there is nothing to roll back + warn("Git is not setup. Cancelling the download") + warn("Setup Git before downloading this exercise") + sys.exit(1) + + # Check if the exercise requires Github/Github CLI to operate, if so, error if not present + if staged_config.requires_github: + try: + info("Exercise requires Github, checking if you have it setup") + invoke_command(github) + except SystemExit as e: + if e.code == 1: + # Exited because of missing Github configuration + # Nothing has been written yet, so there is nothing to roll back + warn("Github is not setup. Cancelling the download") + warn("Setup Github and Github CLI before downloading this exercise") + sys.exit(1) + + if os.path.isdir(exercise): + # Only reachable with --force, download() blocks otherwise + warn(f"Removing your existing {exercise} folder") + old_config = _read_existing_config(Path(exercise)) + rmtree(exercise) + + os.makedirs(exercise) + for file in BASE_FILES: + shutil.move(str(staging_path / file), str(Path(exercise) / file)) - # Check if the exercise requires Github/Github CLI to operate, if so, error if not present - if config.requires_github: - try: - info("Exercise requires Github, checking if you have it setup") - invoke_command(github) - except SystemExit as e: - if e.code == 1: - # Exited because of missing Github configuration - # Rollback the download and remove the folder - warn("Github is not setup. Rolling back the download") - os.chdir("..") - rmtree(exercise) - warn("Setup Github and Github CLI before downloading this exercise") - sys.exit(1) + os.chdir(exercise) + config = ExerciseConfig.read(Path("./"), 0) if old_config and old_config.exercise_repo.repo_type == "remote" and old_config.exercise_repo.create_fork: pr_repo_full_name = old_config.exercise_repo.pr_repo_full_name @@ -147,13 +178,8 @@ def _download_hands_on(hands_on: str, formatted_hands_on: str) -> None: f"Downloading {hands_on} to {click.style(hands_on + '/', bold=True, italic=True)}" ) - if os.path.isdir(hands_on): - warn(f"You already have {hands_on}, removing it to download again") - rmtree(hands_on) - - os.makedirs(hands_on) - os.chdir(hands_on) - + # The requirement checks run before any existing folder is removed so that a + # failed check never leaves the student without their work. hands_on_namespace = Namespace.load_file_as_namespace( repo, f"hands_on/{hands_on_without_prefix}.py" ) @@ -166,11 +192,9 @@ def _download_hands_on(hands_on: str, formatted_hands_on: str) -> None: invoke_command(git) except SystemExit as e: if e.code == 1: - # Exited because of missing Github configuration - # Rollback the download and remove the folder - warn("Git is not setup. Rolling back the download") - os.chdir("..") - rmtree(hands_on) + # Exited because of missing Git configuration + # Nothing has been written yet, so there is nothing to roll back + warn("Git is not setup. Cancelling the download") warn("Setup Git before downloading this hands-on") sys.exit(1) @@ -181,13 +205,19 @@ def _download_hands_on(hands_on: str, formatted_hands_on: str) -> None: except SystemExit as e: if e.code == 1: # Exited because of missing Github configuration - # Rollback the download and remove the folder - warn("Github is not setup. Rolling back the download") - os.chdir("..") - rmtree(hands_on) + # Nothing has been written yet, so there is nothing to roll back + warn("Github is not setup. Cancelling the download") warn("Setup Github and Github CLI before downloading this hands-on") sys.exit(1) + if os.path.isdir(hands_on): + # Only reachable with --force, download() blocks otherwise + warn(f"Removing your existing {hands_on} folder") + rmtree(hands_on) + + os.makedirs(hands_on) + os.chdir(hands_on) + verbose = get_verbose() with create_repo_smith(verbose, null_repo=True) as repo_smith: hands_on_namespace.execute_function( @@ -281,9 +311,25 @@ def setup_exercise_folder( # TODO: Maybe store the random "keys" in config @click.command() @click.argument("exercise") +@click.option( + "--force", + "-f", + is_flag=True, + help="Delete the existing exercise folder, along with any work in it, and download the exercise again", +) @in_gitmastery_root(must=True) -def download(exercise: str) -> None: +def download(exercise: str, force: bool) -> None: """Download an exercise""" + if os.path.isdir(exercise) and not force: + error( + f"You already have {click.style(exercise, bold=True)} downloaded at " + f"{click.style(exercise + '/', bold=True, italic=True)}. Nothing was downloaded.\n" + f" To start the exercise over, run " + f"{click.style('gitmastery progress reset', bold=True, italic=True)} from inside it.\n" + f" To delete the folder, along with any work in it, and download it again, run " + f"{click.style(f'gitmastery download {exercise} --force', bold=True, italic=True)}." + ) + download_time = datetime.now(tz=pytz.UTC) formatted_exercise = exercise.replace("-", "_") diff --git a/tests/e2e/commands/test_download.py b/tests/e2e/commands/test_download.py index d99eeb4..d603624 100644 --- a/tests/e2e/commands/test_download.py +++ b/tests/e2e/commands/test_download.py @@ -1,7 +1,8 @@ import json from pathlib import Path -from ..constants import EXERCISE_NAME +from ..constants import EXERCISE_NAME, HANDS_ON_NAME +from ..runner import BinaryRunner def test_download_exercise(downloaded_exercise_dir: Path) -> None: @@ -18,3 +19,68 @@ def test_download_exercise(downloaded_exercise_dir: Path) -> None: def test_download_hands_on(downloaded_hands_on_dir: Path) -> None: """download creates the hands-on folder.""" assert downloaded_hands_on_dir.is_dir() + + +def test_download_blocks_when_already_downloaded( + runner: BinaryRunner, gitmastery_root: Path, downloaded_exercise_dir: Path +) -> None: + """download refuses to overwrite an existing exercise folder.""" + sentinel = downloaded_exercise_dir / "NOTES.md" + sentinel.write_text("local work") + + try: + res = runner.run(["download", EXERCISE_NAME], cwd=gitmastery_root) + + assert res.returncode != 0, ( + f"Expected a non-zero exit code, got {res.returncode}\n" + f"stdout:\n{res.stdout}" + ) + res.assert_stdout_contains("already have") + res.assert_stdout_contains("--force") + + assert sentinel.is_file() + assert sentinel.read_text() == "local work" + finally: + sentinel.unlink() + + +def test_download_hands_on_blocks_when_already_downloaded( + runner: BinaryRunner, gitmastery_root: Path, downloaded_hands_on_dir: Path +) -> None: + """download refuses to overwrite an existing hands-on folder.""" + sentinel = downloaded_hands_on_dir / "NOTES.md" + sentinel.write_text("local work") + + try: + res = runner.run(["download", HANDS_ON_NAME], cwd=gitmastery_root) + + assert res.returncode != 0, ( + f"Expected a non-zero exit code, got {res.returncode}\n" + f"stdout:\n{res.stdout}" + ) + res.assert_stdout_contains("--force") + + assert sentinel.is_file() + finally: + sentinel.unlink() + + +def test_download_force_overwrites( + runner: BinaryRunner, isolated_gitmastery_root: Path +) -> None: + """download --force wipes the existing exercise folder and downloads it again.""" + runner.run( + ["download", EXERCISE_NAME], cwd=isolated_gitmastery_root + ).assert_success() + + exercise_dir = isolated_gitmastery_root / EXERCISE_NAME + sentinel = exercise_dir / "NOTES.md" + sentinel.write_text("local work") + + runner.run( + ["download", EXERCISE_NAME, "--force"], cwd=isolated_gitmastery_root + ).assert_success() + + assert not sentinel.exists() + assert (exercise_dir / ".gitmastery-exercise.json").is_file() + assert (exercise_dir / "README.md").is_file() diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index 650bc32..2bd58e8 100644 --- a/tests/e2e/conftest.py +++ b/tests/e2e/conftest.py @@ -53,6 +53,17 @@ def setup_gitmastery_root( yield from _make_gitmastery_root(runner, tmp_path_factory) +@pytest.fixture +def isolated_gitmastery_root( + runner: BinaryRunner, tmp_path_factory: pytest.TempPathFactory +) -> Generator[Path, None, None]: + """ + A throwaway Git-Mastery root for tests that overwrite a downloaded exercise, + so that they do not disturb the session-scoped fixtures below. + """ + yield from _make_gitmastery_root(runner, tmp_path_factory) + + @pytest.fixture(scope="session") def downloaded_exercise_dir(runner: BinaryRunner, gitmastery_root: Path) -> Path: """ From 41df82d70ef3a72a7fa79db3dc49632fd79f6256 Mon Sep 17 00:00:00 2001 From: "jiaxin.loh" Date: Fri, 28 Aug 2026 16:54:04 +0800 Subject: [PATCH 2/3] feat: fix hands on message --- app/commands/download.py | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/app/commands/download.py b/app/commands/download.py index ee20425..2fe8fcc 100644 --- a/app/commands/download.py +++ b/app/commands/download.py @@ -320,21 +320,29 @@ def setup_exercise_folder( @in_gitmastery_root(must=True) def download(exercise: str, force: bool) -> None: """Download an exercise""" + download_time = datetime.now(tz=pytz.UTC) + + formatted_exercise = exercise.replace("-", "_") + is_hands_on = exercise.startswith("hp-") + if os.path.isdir(exercise) and not force: + # Hands-on practices are not tracked, so progress reset does not apply to them + reset_hint = ( + "" + if is_hands_on + else ( + f" To start the exercise over, run " + f"{click.style('gitmastery progress reset', bold=True, italic=True)} from inside it.\n" + ) + ) error( f"You already have {click.style(exercise, bold=True)} downloaded at " f"{click.style(exercise + '/', bold=True, italic=True)}. Nothing was downloaded.\n" - f" To start the exercise over, run " - f"{click.style('gitmastery progress reset', bold=True, italic=True)} from inside it.\n" + f"{reset_hint}" f" To delete the folder, along with any work in it, and download it again, run " f"{click.style(f'gitmastery download {exercise} --force', bold=True, italic=True)}." ) - download_time = datetime.now(tz=pytz.UTC) - - formatted_exercise = exercise.replace("-", "_") - is_hands_on = exercise.startswith("hp-") - if is_hands_on: _download_hands_on(exercise, formatted_exercise) else: From 100e13c33dfd431ebdcee37b6caffceda949d867 Mon Sep 17 00:00:00 2001 From: "jiaxin.loh" Date: Sat, 29 Aug 2026 10:59:55 +0800 Subject: [PATCH 3/3] fix: remove e2e tests for edge cases --- AGENTS.md | 1 + tests/e2e/commands/test_download.py | 68 +---------------------------- tests/e2e/conftest.py | 11 ----- 3 files changed, 2 insertions(+), 78 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index bc0a111..9de90d9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,6 +13,7 @@ You are an expert software developer handling this repo. **Note**: - Git CLI commands should be taken from `git.py`. - GitHub CLI commands should be taken from `github_cli.py`. +- E2E tests in `tests/e2e/` cover the happy path only. Do not add failure, edge case, or error handling tests there. **File Structure**: - app/: CLI entry, commands, utils, configs, logging, hooks, __init__.py, cli.py, version.py diff --git a/tests/e2e/commands/test_download.py b/tests/e2e/commands/test_download.py index d603624..d99eeb4 100644 --- a/tests/e2e/commands/test_download.py +++ b/tests/e2e/commands/test_download.py @@ -1,8 +1,7 @@ import json from pathlib import Path -from ..constants import EXERCISE_NAME, HANDS_ON_NAME -from ..runner import BinaryRunner +from ..constants import EXERCISE_NAME def test_download_exercise(downloaded_exercise_dir: Path) -> None: @@ -19,68 +18,3 @@ def test_download_exercise(downloaded_exercise_dir: Path) -> None: def test_download_hands_on(downloaded_hands_on_dir: Path) -> None: """download creates the hands-on folder.""" assert downloaded_hands_on_dir.is_dir() - - -def test_download_blocks_when_already_downloaded( - runner: BinaryRunner, gitmastery_root: Path, downloaded_exercise_dir: Path -) -> None: - """download refuses to overwrite an existing exercise folder.""" - sentinel = downloaded_exercise_dir / "NOTES.md" - sentinel.write_text("local work") - - try: - res = runner.run(["download", EXERCISE_NAME], cwd=gitmastery_root) - - assert res.returncode != 0, ( - f"Expected a non-zero exit code, got {res.returncode}\n" - f"stdout:\n{res.stdout}" - ) - res.assert_stdout_contains("already have") - res.assert_stdout_contains("--force") - - assert sentinel.is_file() - assert sentinel.read_text() == "local work" - finally: - sentinel.unlink() - - -def test_download_hands_on_blocks_when_already_downloaded( - runner: BinaryRunner, gitmastery_root: Path, downloaded_hands_on_dir: Path -) -> None: - """download refuses to overwrite an existing hands-on folder.""" - sentinel = downloaded_hands_on_dir / "NOTES.md" - sentinel.write_text("local work") - - try: - res = runner.run(["download", HANDS_ON_NAME], cwd=gitmastery_root) - - assert res.returncode != 0, ( - f"Expected a non-zero exit code, got {res.returncode}\n" - f"stdout:\n{res.stdout}" - ) - res.assert_stdout_contains("--force") - - assert sentinel.is_file() - finally: - sentinel.unlink() - - -def test_download_force_overwrites( - runner: BinaryRunner, isolated_gitmastery_root: Path -) -> None: - """download --force wipes the existing exercise folder and downloads it again.""" - runner.run( - ["download", EXERCISE_NAME], cwd=isolated_gitmastery_root - ).assert_success() - - exercise_dir = isolated_gitmastery_root / EXERCISE_NAME - sentinel = exercise_dir / "NOTES.md" - sentinel.write_text("local work") - - runner.run( - ["download", EXERCISE_NAME, "--force"], cwd=isolated_gitmastery_root - ).assert_success() - - assert not sentinel.exists() - assert (exercise_dir / ".gitmastery-exercise.json").is_file() - assert (exercise_dir / "README.md").is_file() diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index 2bd58e8..650bc32 100644 --- a/tests/e2e/conftest.py +++ b/tests/e2e/conftest.py @@ -53,17 +53,6 @@ def setup_gitmastery_root( yield from _make_gitmastery_root(runner, tmp_path_factory) -@pytest.fixture -def isolated_gitmastery_root( - runner: BinaryRunner, tmp_path_factory: pytest.TempPathFactory -) -> Generator[Path, None, None]: - """ - A throwaway Git-Mastery root for tests that overwrite a downloaded exercise, - so that they do not disturb the session-scoped fixtures below. - """ - yield from _make_gitmastery_root(runner, tmp_path_factory) - - @pytest.fixture(scope="session") def downloaded_exercise_dir(runner: BinaryRunner, gitmastery_root: Path) -> Path: """