From 02a47c7d273463d636896588bfe485ad147a95ab Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Mon, 10 Aug 2026 07:31:09 +0530 Subject: [PATCH 1/6] fix(apps): refuse to reinstall an app from a different branch App.install short-circuits when the app is already in the bench, which silently discarded an explicitly requested branch: importing erpnext@version-16-hotfix over the existing version-16 clone reported success while the site got version-16. A bench holds one checkout per app, so the install now fails loudly, naming both branches. GetAppCommand also stops defaulting an unspecified branch to 'main'. clone() already resolves the remote's real default, and the injected 'main' would read as explicit intent to the new guard. --- pilot/commands/apps/download.py | 3 ++- pilot/core/app/__init__.py | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/pilot/commands/apps/download.py b/pilot/commands/apps/download.py index f3dc357f2..9eebfcecd 100644 --- a/pilot/commands/apps/download.py +++ b/pilot/commands/apps/download.py @@ -21,7 +21,8 @@ class GetAppCommand(Command): def __post_init__(self) -> None: from pilot.core.app import App - self.app = App.from_repo(self.bench, self.repo, self.branch or "main") + # No branch means the remote's default (see AppRepository.clone), not "main". + self.app = App.from_repo(self.bench, self.repo, self.branch) self.installed_dependencies: list[App] = [] def run(self) -> None: diff --git a/pilot/core/app/__init__.py b/pilot/core/app/__init__.py index 973fdd7d0..d4e306273 100644 --- a/pilot/core/app/__init__.py +++ b/pilot/core/app/__init__.py @@ -245,6 +245,20 @@ def build_assets(self) -> None: return run_command(["yarn", "--cwd", str(self.path), "build"]) + def _reject_branch_mismatch(self) -> None: + """A bench holds one checkout per app, so an install cannot deliver a + second branch. Refuse loudly instead of keeping the current one.""" + requested = self.config.branch + if not requested or self.is_commit_hash(requested): + return + current = self.bench.app(self.module_name).current_branch + if current and requested != current: + raise BenchError( + f"'{self.config.name}' is already installed from branch '{current}', " + f"so this install cannot deliver '{requested}'. Remove the app and " + f"add it again to change its branch." + ) + def _skip_already_installed( self, on_progress: Callable[[str], None], install_dependencies: bool = False ) -> AppInstallResult: @@ -262,6 +276,7 @@ def install( ) -> AppInstallResult: """Pinned commit based Clone, validate, install, register, and build app assets.""" if self.bench.is_app_installed(self.config.name): + self._reject_branch_mismatch() return self._skip_already_installed(on_progress, install_dependencies) existing_clone = self.existing_clone_path From 873447a190e70b9dde73967de48db6eb0c467fc0 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Mon, 10 Aug 2026 07:31:09 +0530 Subject: [PATCH 2/6] test(apps): cover reinstalling from a mismatched branch --- tests/pilot/commands/test_get_app.py | 38 ++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/pilot/commands/test_get_app.py b/tests/pilot/commands/test_get_app.py index e9de3ea0e..f06b4c978 100644 --- a/tests/pilot/commands/test_get_app.py +++ b/tests/pilot/commands/test_get_app.py @@ -2,15 +2,26 @@ from __future__ import annotations +import subprocess from pathlib import Path from unittest.mock import patch +import pytest + from pilot.commands.apps.download import GetAppCommand from pilot.core.app import App +from pilot.exceptions import BenchError from pilot.integrations.marketplace import Marketplace, Resolver from tests.pilot.commands.test_commands import make_bench +def register_app_on_branch(bench, name: str, branch: str) -> None: + app_dir = bench.apps_path / name + app_dir.mkdir(parents=True) + subprocess.run(["git", "init", "-b", branch, str(app_dir)], check=True, capture_output=True) + (bench.sites_path / "apps.txt").write_text(f"frappe\n{name}\n") + + def make_resolver(name: str, deps: dict[str, str] | None = None) -> Resolver: return Resolver( app=name, @@ -71,6 +82,33 @@ def test_run_short_circuits_when_app_already_registered(tmp_path: Path) -> None: mock_build.assert_not_called() +def test_reinstall_with_a_different_branch_fails_loudly(tmp_path: Path) -> None: + """Silently reusing the installed checkout delivered the wrong branch: + importing erpnext@version-16-hotfix over an existing version-16 clone + reported success while installing version-16.""" + bench = make_bench(tmp_path) + bench.create_directories() + register_app_on_branch(bench, "myapp", "version-16") + + cmd = GetAppCommand(bench, repo="https://github.com/frappe/myapp", branch="version-16-hotfix") + + with pytest.raises(BenchError, match="already installed from branch 'version-16'"): + cmd.run() + + +def test_reinstall_with_the_same_branch_still_short_circuits(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + register_app_on_branch(bench, "myapp", "version-16") + + cmd = GetAppCommand(bench, repo="https://github.com/frappe/myapp", branch="version-16") + + with patch.object(App, "clone") as mock_clone: + cmd.run() + + mock_clone.assert_not_called() + + def test_short_circuit_adopts_real_on_disk_app_path(tmp_path: Path) -> None: """Regression: short-circuit uses the normalized on-disk app path.""" bench = make_bench(tmp_path) From a73f57d95e687e7fda03591ab1c15f4a353ba739 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Tue, 25 Aug 2026 19:49:25 +0530 Subject: [PATCH 3/6] docs(apps): explain get-app branch selection --- docs/commands.md | 2 +- pilot/commands/apps/download.py | 1 - pilot/core/app/__init__.py | 2 -- tests/pilot/commands/test_get_app.py | 3 --- 4 files changed, 1 insertion(+), 7 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 0a6045313..6ec5f0b7d 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -27,7 +27,7 @@ Some runtime commands support all benches when invoked with the CLI option for a ## App Commands - `pilot new-app APP`: scaffold a new Frappe app under `apps/` and install it. Prompts for title, description, publisher, email, license, GitHub workflow, and branch; pass any of `--title/--description/--publisher/--email/--license/--branch/--github-workflow` to skip prompts (branch defaults to `develop`). -- `pilot get-app REPO_OR_NAME`: clone and install an app into the bench. +- `pilot get-app REPO_OR_NAME [--branch BRANCH]`: clone and install an app into the bench. Without `--branch`, Git uses the remote default branch. An explicit branch that differs from an existing checkout fails instead of reusing that checkout. - `pilot list-apps`: list apps present in the bench. - `pilot install-app APP --site SITE`: install apps on a site. An app the site only has disabled is enabled instead, bringing back anything it requires first. - `pilot uninstall-app APP --site SITE`: uninstall apps from a site, dropping their data. diff --git a/pilot/commands/apps/download.py b/pilot/commands/apps/download.py index 9eebfcecd..6937b352d 100644 --- a/pilot/commands/apps/download.py +++ b/pilot/commands/apps/download.py @@ -21,7 +21,6 @@ class GetAppCommand(Command): def __post_init__(self) -> None: from pilot.core.app import App - # No branch means the remote's default (see AppRepository.clone), not "main". self.app = App.from_repo(self.bench, self.repo, self.branch) self.installed_dependencies: list[App] = [] diff --git a/pilot/core/app/__init__.py b/pilot/core/app/__init__.py index d4e306273..5a1815a03 100644 --- a/pilot/core/app/__init__.py +++ b/pilot/core/app/__init__.py @@ -246,8 +246,6 @@ def build_assets(self) -> None: run_command(["yarn", "--cwd", str(self.path), "build"]) def _reject_branch_mismatch(self) -> None: - """A bench holds one checkout per app, so an install cannot deliver a - second branch. Refuse loudly instead of keeping the current one.""" requested = self.config.branch if not requested or self.is_commit_hash(requested): return diff --git a/tests/pilot/commands/test_get_app.py b/tests/pilot/commands/test_get_app.py index f06b4c978..75ede4b06 100644 --- a/tests/pilot/commands/test_get_app.py +++ b/tests/pilot/commands/test_get_app.py @@ -83,9 +83,6 @@ def test_run_short_circuits_when_app_already_registered(tmp_path: Path) -> None: def test_reinstall_with_a_different_branch_fails_loudly(tmp_path: Path) -> None: - """Silently reusing the installed checkout delivered the wrong branch: - importing erpnext@version-16-hotfix over an existing version-16 clone - reported success while installing version-16.""" bench = make_bench(tmp_path) bench.create_directories() register_app_on_branch(bench, "myapp", "version-16") From 954065ec7276a5418e680809e4e8f9c01124c77d Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Tue, 25 Aug 2026 20:02:32 +0530 Subject: [PATCH 4/6] test(e2e): stop setup redirect before login --- tests/e2e/flows/admin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/e2e/flows/admin.py b/tests/e2e/flows/admin.py index 27cec807b..0f4da1563 100644 --- a/tests/e2e/flows/admin.py +++ b/tests/e2e/flows/admin.py @@ -7,8 +7,8 @@ def login(page: Page, base_url: str, password: str) -> None: - # The wizard's own sign-in carries over in this shared browser context, so without - # clearing it the login form would never render. Drop it to exercise real login. + # Let the setup page finish its restart redirect before starting login. + page.wait_for_url(f"{base_url}/sites", timeout=30_000) page.context.clear_cookies() page.goto(f"{base_url}/") page.get_by_placeholder("Password").fill(password) From 33d991143df9b75852b7c36c5e82c82d6aa90611 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Tue, 25 Aug 2026 20:23:36 +0530 Subject: [PATCH 5/6] fix(apps): validate commit pins on reinstall --- docs/commands.md | 2 +- pilot/core/app/__init__.py | 23 ++++++-- tests/pilot/commands/test_get_app.py | 88 +++++++++++++++++++++++++++- 3 files changed, 105 insertions(+), 8 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 6ec5f0b7d..6ce44ca3b 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -27,7 +27,7 @@ Some runtime commands support all benches when invoked with the CLI option for a ## App Commands - `pilot new-app APP`: scaffold a new Frappe app under `apps/` and install it. Prompts for title, description, publisher, email, license, GitHub workflow, and branch; pass any of `--title/--description/--publisher/--email/--license/--branch/--github-workflow` to skip prompts (branch defaults to `develop`). -- `pilot get-app REPO_OR_NAME [--branch BRANCH]`: clone and install an app into the bench. Without `--branch`, Git uses the remote default branch. An explicit branch that differs from an existing checkout fails instead of reusing that checkout. +- `pilot get-app REPO_OR_NAME [--branch BRANCH_OR_COMMIT]`: clone and install an app into the bench. Without `--branch`, a fresh clone uses the remote default branch and an existing install is reused at its current revision. An explicit branch or commit is a revision constraint, so a mismatch with an existing checkout fails instead of reusing it. - `pilot list-apps`: list apps present in the bench. - `pilot install-app APP --site SITE`: install apps on a site. An app the site only has disabled is enabled instead, bringing back anything it requires first. - `pilot uninstall-app APP --site SITE`: uninstall apps from a site, dropping their data. diff --git a/pilot/core/app/__init__.py b/pilot/core/app/__init__.py index 5a1815a03..b2f1c118c 100644 --- a/pilot/core/app/__init__.py +++ b/pilot/core/app/__init__.py @@ -245,14 +245,25 @@ def build_assets(self) -> None: return run_command(["yarn", "--cwd", str(self.path), "build"]) - def _reject_branch_mismatch(self) -> None: + def _reject_revision_mismatch(self, commit: str = "") -> None: requested = self.config.branch - if not requested or self.is_commit_hash(requested): + installed = self.bench.app(self.module_name) + requested_commit = commit or (requested if self.is_commit_hash(requested) else "") + if requested_commit: + if not installed.is_on_revision(RevisionPin(kind="commit", ref=requested_commit)): + raise BenchError( + f"'{self.config.name}' is already installed at a different commit, " + f"so this install cannot deliver '{requested_commit}'. Remove the app and " + f"add it again to change its revision." + ) + return + if not requested: return - current = self.bench.app(self.module_name).current_branch - if current and requested != current: + current = installed.current_branch + if requested != current: + source = f"branch '{current}'" if current else "a detached commit" raise BenchError( - f"'{self.config.name}' is already installed from branch '{current}', " + f"'{self.config.name}' is already installed from {source}, " f"so this install cannot deliver '{requested}'. Remove the app and " f"add it again to change its branch." ) @@ -274,7 +285,7 @@ def install( ) -> AppInstallResult: """Pinned commit based Clone, validate, install, register, and build app assets.""" if self.bench.is_app_installed(self.config.name): - self._reject_branch_mismatch() + self._reject_revision_mismatch(commit) return self._skip_already_installed(on_progress, install_dependencies) existing_clone = self.existing_clone_path diff --git a/tests/pilot/commands/test_get_app.py b/tests/pilot/commands/test_get_app.py index 75ede4b06..af905e686 100644 --- a/tests/pilot/commands/test_get_app.py +++ b/tests/pilot/commands/test_get_app.py @@ -15,11 +15,35 @@ from tests.pilot.commands.test_commands import make_bench -def register_app_on_branch(bench, name: str, branch: str) -> None: +def register_app_on_branch(bench, name: str, branch: str) -> str: app_dir = bench.apps_path / name app_dir.mkdir(parents=True) subprocess.run(["git", "init", "-b", branch, str(app_dir)], check=True, capture_output=True) + (app_dir / "hooks.py").write_text("") + subprocess.run(["git", "-C", str(app_dir), "add", "hooks.py"], check=True, capture_output=True) + subprocess.run( + [ + "git", + "-C", + str(app_dir), + "-c", + "user.name=Pilot Tests", + "-c", + "user.email=pilot@example.com", + "commit", + "-m", + "Initial commit", + ], + check=True, + capture_output=True, + ) (bench.sites_path / "apps.txt").write_text(f"frappe\n{name}\n") + return subprocess.run( + ["git", "-C", str(app_dir), "rev-parse", "HEAD"], + check=True, + capture_output=True, + text=True, + ).stdout.strip() def make_resolver(name: str, deps: dict[str, str] | None = None) -> Resolver: @@ -106,6 +130,68 @@ def test_reinstall_with_the_same_branch_still_short_circuits(tmp_path: Path) -> mock_clone.assert_not_called() +def test_reinstall_with_a_branch_rejects_a_detached_checkout(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + current = register_app_on_branch(bench, "myapp", "version-16") + subprocess.run( + ["git", "-C", str(bench.apps_path / "myapp"), "checkout", "--detach", current], + check=True, + capture_output=True, + ) + + cmd = GetAppCommand(bench, repo="https://github.com/frappe/myapp", branch="version-16") + + with pytest.raises(BenchError, match="already installed from a detached commit"): + cmd.run() + + +def test_reinstall_with_a_different_commit_fails_loudly(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + register_app_on_branch(bench, "myapp", "version-16") + + requested = "0" * 40 + cmd = GetAppCommand(bench, repo="https://github.com/frappe/myapp", branch=requested) + + with pytest.raises(BenchError, match="already installed at a different commit"): + cmd.run() + + +def test_reinstall_with_the_same_commit_still_short_circuits(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + current = register_app_on_branch(bench, "myapp", "version-16") + + cmd = GetAppCommand(bench, repo="https://github.com/frappe/myapp", branch=current[:8]) + + with patch.object(App, "clone") as mock_clone: + cmd.run() + + mock_clone.assert_not_called() + + +def test_reinstall_with_a_different_marketplace_commit_fails_loudly(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + register_app_on_branch(bench, "myapp", "version-16") + app = App.from_repo(bench, "https://github.com/frappe/myapp", branch="version-16") + + with pytest.raises(BenchError, match="already installed at a different commit"): + app.install(commit="0" * 40) + + +def test_reinstall_with_the_same_marketplace_commit_still_short_circuits(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + current = register_app_on_branch(bench, "myapp", "version-16") + app = App.from_repo(bench, "https://github.com/frappe/myapp", branch="version-16") + + result = app.install(commit=current[:8]) + + assert result.already_installed is True + + def test_short_circuit_adopts_real_on_disk_app_path(tmp_path: Path) -> None: """Regression: short-circuit uses the normalized on-disk app path.""" bench = make_bench(tmp_path) From a2c09cb399f1b77b76691bcecb35ce92a472b03f Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Tue, 25 Aug 2026 21:41:11 +0530 Subject: [PATCH 6/6] fix(apps): reject repository mismatches --- pilot/core/app/__init__.py | 11 +++++++++++ tests/pilot/commands/test_get_app.py | 16 ++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/pilot/core/app/__init__.py b/pilot/core/app/__init__.py index b2f1c118c..66a4a0bd1 100644 --- a/pilot/core/app/__init__.py +++ b/pilot/core/app/__init__.py @@ -246,8 +246,19 @@ def build_assets(self) -> None: run_command(["yarn", "--cwd", str(self.path), "build"]) def _reject_revision_mismatch(self, commit: str = "") -> None: + from pilot.integrations.git.base import repo_host, same_repository + requested = self.config.branch installed = self.bench.app(self.module_name) + if ( + repo_host(self.config.repo) + and repo_host(installed.config.repo) + and not same_repository(self.config.repo, installed.config.repo) + ): + raise BenchError( + f"'{self.config.name}' is already installed from a different repository. " + "Remove the app and add it again to change its repository." + ) requested_commit = commit or (requested if self.is_commit_hash(requested) else "") if requested_commit: if not installed.is_on_revision(RevisionPin(kind="commit", ref=requested_commit)): diff --git a/tests/pilot/commands/test_get_app.py b/tests/pilot/commands/test_get_app.py index af905e686..009f13117 100644 --- a/tests/pilot/commands/test_get_app.py +++ b/tests/pilot/commands/test_get_app.py @@ -19,6 +19,11 @@ def register_app_on_branch(bench, name: str, branch: str) -> str: app_dir = bench.apps_path / name app_dir.mkdir(parents=True) subprocess.run(["git", "init", "-b", branch, str(app_dir)], check=True, capture_output=True) + subprocess.run( + ["git", "-C", str(app_dir), "remote", "add", "origin", f"https://github.com/frappe/{name}"], + check=True, + capture_output=True, + ) (app_dir / "hooks.py").write_text("") subprocess.run(["git", "-C", str(app_dir), "add", "hooks.py"], check=True, capture_output=True) subprocess.run( @@ -117,6 +122,17 @@ def test_reinstall_with_a_different_branch_fails_loudly(tmp_path: Path) -> None: cmd.run() +def test_reinstall_from_a_different_repository_fails_loudly(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + register_app_on_branch(bench, "myapp", "version-16") + + cmd = GetAppCommand(bench, repo="https://github.com/acme/myapp", branch="version-16") + + with pytest.raises(BenchError, match="already installed from a different repository"): + cmd.run() + + def test_reinstall_with_the_same_branch_still_short_circuits(tmp_path: Path) -> None: bench = make_bench(tmp_path) bench.create_directories()