diff --git a/fileglancer/alembic/versions/b8e4f1a92c37_canonicalize_github_urls.py b/fileglancer/alembic/versions/b8e4f1a92c37_canonicalize_github_urls.py new file mode 100644 index 00000000..6ad96b32 --- /dev/null +++ b/fileglancer/alembic/versions/b8e4f1a92c37_canonicalize_github_urls.py @@ -0,0 +1,94 @@ +"""canonicalize stored GitHub URLs + +Normalizes app/listing/job GitHub URLs to a single canonical form (no ".git" +suffix, no trailing slash, no redundant "/tree/main"; SSH folded to https), so +that an app's URL matches consistently across the catalog, a user's library and +the launch page. Mirrors fileglancer.giturls.canonical_github_url, inlined here +so the migration stays self-contained. + +Revision ID: b8e4f1a92c37 +Revises: e1a7c93d04f5 +Create Date: 2026-06-26 00:00:00.000000 + +""" +import re + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'b8e4f1a92c37' +down_revision = 'e1a7c93d04f5' +branch_labels = None +depends_on = None + + +_HTTPS_RE = re.compile(r"https?://github\.com/([^/]+)/([^/]+?)(?:\.git)?(?:/tree/(.+?))?/?$") +_SSH_SCP_RE = re.compile(r"git@github\.com:([^/]+)/([^/]+?)(?:\.git)?/?$") +_SSH_PROTO_RE = re.compile(r"ssh://git@github\.com/([^/]+)/([^/]+?)(?:\.git)?/?$") + + +def _canonical(url): + if not url: + return url + m = _HTTPS_RE.match(url) + if m: + owner, repo, branch = m.group(1), m.group(2), m.group(3) + else: + m = _SSH_SCP_RE.match(url) or _SSH_PROTO_RE.match(url) + if not m: + return url + owner, repo, branch = m.group(1), m.group(2), None + if branch and branch != "main": + return f"https://github.com/{owner}/{repo}/tree/{branch}" + return f"https://github.com/{owner}/{repo}" + + +def _canonicalize_unique_table(conn, table, owner_col): + """Canonicalize ``url`` for a table with a UNIQUE(owner, url, manifest_path) + constraint. Rows that would collide with an existing canonical row are + dropped (the canonical row wins) rather than triggering a constraint error.""" + rows = conn.execute(sa.text( + f"SELECT id, {owner_col} AS owner, url, manifest_path FROM {table}" + )).fetchall() + + # Pass 1: register rows already in canonical form so non-canonical rows that + # map onto them are treated as duplicates. + seen = {} + for r in rows: + if _canonical(r.url) == r.url: + seen[(r.owner, r.url, r.manifest_path)] = r.id + + # Pass 2: rewrite the rest, deleting any that collide with a canonical row. + for r in rows: + canonical = _canonical(r.url) + if canonical == r.url: + continue + key = (r.owner, canonical, r.manifest_path) + if key in seen: + conn.execute(sa.text(f"DELETE FROM {table} WHERE id = :id"), + {"id": r.id}) + else: + conn.execute(sa.text(f"UPDATE {table} SET url = :url WHERE id = :id"), + {"url": canonical, "id": r.id}) + seen[key] = r.id + + +def upgrade() -> None: + conn = op.get_bind() + _canonicalize_unique_table(conn, "user_apps", "username") + _canonicalize_unique_table(conn, "app_listings", "owner_username") + + # jobs has no uniqueness constraint on the URL; canonicalize in place. + for r in conn.execute(sa.text("SELECT id, app_url FROM jobs")).fetchall(): + canonical = _canonical(r.app_url) + if canonical != r.app_url: + conn.execute(sa.text("UPDATE jobs SET app_url = :url WHERE id = :id"), + {"url": canonical, "id": r.id}) + + +def downgrade() -> None: + # Canonicalization is lossy (the original ".git"/trailing-slash/"/tree/main" + # form can't be recovered), so there is nothing to undo. + pass diff --git a/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py b/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py new file mode 100644 index 00000000..bb8f8a70 --- /dev/null +++ b/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py @@ -0,0 +1,140 @@ +"""bake the cloned revision into app URLs + +Rewrites stored app/listing URLs so the canonical URL always carries the +revision actually cloned (e.g. ".../tree/master" for a repo whose default is +"master"; "main" still folds to the bare URL). The ``branch`` column flips to +mean the *requested* revision — "" when the app was added from a bare URL. The +revision is fixed at migration/add time; a bare stored URL means the fixed +"main" revision, not "whatever the default branch is now". + +Before this migration ``branch`` held the resolved revision and the bare/master +ambiguity meant a bare URL and an explicit "/tree/master" were stored as two +different rows for the same app. Baking the resolved revision into the URL +closes that gap, so colliding rows are de-duplicated here (the canonical row +wins), mirroring b8e4f1a92c37. + +The requested revision is recovered from the URL's shape: a stored "/tree/" +was an explicit pin (requested = x), while a bare URL was unpinned +(requested = ""). + +Rows whose ``branch`` is NULL are legacy entries migrated from +user_preferences whose resolved default was never recorded. We can't resolve it +here (no network), and assuming "main" would break a repo defaulting to e.g. +"master", so those rows are left untouched and keep tracking the default branch +until they are re-added. jobs carry no branch column and are historical, so +they are left untouched too. + +Revision ID: c1f9a4e7b2d8 +Revises: b8e4f1a92c37 +Create Date: 2026-06-29 00:00:00.000000 + +""" +import re + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'c1f9a4e7b2d8' +down_revision = 'b8e4f1a92c37' +branch_labels = None +depends_on = None + + +_HTTPS_RE = re.compile(r"https?://github\.com/([^/]+)/([^/]+?)(?:\.git)?(?:/tree/(.+?))?/?$") +_SSH_SCP_RE = re.compile(r"git@github\.com:([^/]+)/([^/]+?)(?:\.git)?/?$") +_SSH_PROTO_RE = re.compile(r"ssh://git@github\.com/([^/]+)/([^/]+?)(?:\.git)?/?$") + + +def _parse(url): + """Return (owner, repo, branch) or None if not a parseable GitHub URL.""" + if not url: + return None + m = _HTTPS_RE.match(url) + if m: + return m.group(1), m.group(2), m.group(3) + m = _SSH_SCP_RE.match(url) or _SSH_PROTO_RE.match(url) + if m: + return m.group(1), m.group(2), None + return None + + +def _at_branch(owner, repo, branch): + """Canonical URL for owner/repo at branch ("main" folds to the bare URL).""" + if branch and branch != "main": + return f"https://github.com/{owner}/{repo}/tree/{branch}" + return f"https://github.com/{owner}/{repo}" + + +def _target(row): + """Return (new_url, requested_branch) for a row, or None to leave it alone. + + A NULL branch is a legacy row (migrated from user_preferences) whose resolved + default was never recorded. We can't resolve it here without network, and + assuming "main" would break a repo defaulting to e.g. "master". Leave such + rows untouched (branch stays NULL) so they keep tracking the default until + re-added; only rows with a known resolved revision are rewritten. + """ + if row.branch is None: + return None + parsed = _parse(row.url) + if parsed is None: + return None + owner, repo, url_branch = parsed + requested = url_branch or "" + resolved = row.branch or url_branch or "main" + return _at_branch(owner, repo, resolved), requested + + +def _migrate_unique_table(conn, table, owner_col): + """Bake the resolved revision into ``url`` and set ``branch`` to the + requested revision, for a table with UNIQUE(owner, url, manifest_path). + Rows whose new URL collides with another row are dropped (canonical wins).""" + rows = conn.execute(sa.text( + f"SELECT id, {owner_col} AS owner, url, branch, manifest_path FROM {table}" + )).fetchall() + + targets = {r.id: _target(r) for r in rows} + + # Pass 1: rows that will remain at their current URL are the winners on + # collision. This includes NULL-branch legacy rows (target is None): they are + # deliberately left untouched, so another row that bakes to their URL must be + # dropped instead of violating the table's UNIQUE(owner, url, manifest_path) + # constraint. + seen = {} + for r in rows: + t = targets[r.id] + if t is None: + seen[(r.owner, r.url, r.manifest_path)] = r.id + elif t[0] == r.url: + seen[(r.owner, t[0], r.manifest_path)] = r.id + + # Pass 2: rewrite the rest, dropping any that collide with a claimed URL. + for r in rows: + t = targets[r.id] + if t is None: + continue + new_url, requested = t + key = (r.owner, new_url, r.manifest_path) + if new_url != r.url and key in seen: + conn.execute(sa.text(f"DELETE FROM {table} WHERE id = :id"), + {"id": r.id}) + continue + conn.execute( + sa.text(f"UPDATE {table} SET url = :url, branch = :branch WHERE id = :id"), + {"url": new_url, "branch": requested, "id": r.id}, + ) + seen[key] = r.id + + +def upgrade() -> None: + conn = op.get_bind() + _migrate_unique_table(conn, "user_apps", "username") + _migrate_unique_table(conn, "app_listings", "owner_username") + + +def downgrade() -> None: + # Recovering the pre-migration url/branch split would require re-resolving + # each repo's default branch over the network, so there is nothing to undo. + pass diff --git a/fileglancer/apps/__init__.py b/fileglancer/apps/__init__.py index 4e4064e6..ad92584b 100644 --- a/fileglancer/apps/__init__.py +++ b/fileglancer/apps/__init__.py @@ -3,9 +3,11 @@ from fileglancer.apps.manifest import ( # noqa: F401 MANIFEST_FILENAME, _ensure_repo_cache, + clone_url_for_stored_app, discover_app_manifests, fetch_app_manifest, get_app_branch, + canonical_app_url, get_or_load_manifest, refresh_cached_manifest, set_worker_exec, diff --git a/fileglancer/apps/jobs.py b/fileglancer/apps/jobs.py index 7c8c49ec..066d91ca 100644 --- a/fileglancer/apps/jobs.py +++ b/fileglancer/apps/jobs.py @@ -19,6 +19,7 @@ from fileglancer import database as db from fileglancer.apps.manifest import ( + clone_url_for_stored_app, _dispatch, _ensure_repo_cache, get_or_load_manifest, @@ -34,6 +35,7 @@ _URI_PREFIXES, ) from fileglancer.apps.jobfiles import _build_work_dir +from fileglancer.giturls import canonical_github_url from fileglancer.model import AppEntryPoint from fileglancer.settings import get_settings @@ -538,16 +540,30 @@ async def submit_job( if v is not None } + stored_app_url = app_url + # Not in the user's library: clone the URL as given (a bare URL resolves the + # current default). Overridden below with the pinned URL when installed. + app_clone_url = app_url + with db.get_db_session(settings.db_url) as session: # Read user's container cache dir preference cache_dir_pref = db.get_user_preference(session, username, "apptainerCacheDir") container_cache_dir = cache_dir_pref.get("value") if cache_dir_pref else None + # Prefer the name the user saved for this app (which may be a custom name + # chosen when adding it from the catalog) over the raw manifest name, so + # jobs are labeled consistently with the user's library. + user_app = db.get_user_app(session, username, app_url, manifest_path) + app_name = user_app.name if user_app is not None else manifest.name + if user_app is not None: + stored_app_url = user_app.url + app_clone_url = clone_url_for_stored_app(stored_app_url, user_app.branch) + db_job = db.create_job( session=session, username=username, app_url=app_url, - app_name=manifest.name, + app_name=app_name, entry_point_id=entry_point.id, entry_point_name=entry_point.name, entry_point_type=entry_point.type, @@ -576,7 +592,7 @@ async def submit_job( # Ensure the repo is cached in the user's cache (~username/.fileglancer/apps). # Pulling is never done here; updates are an explicit user action via the # "Update" app endpoint. The manifest read above already reflects the cache. - if manifest.repo_url and manifest.repo_url != app_url: + if manifest.repo_url and canonical_github_url(manifest.repo_url) != stored_app_url: # Manifest and tool code live in separate repos: cache the code repo # and run from its root. cached_repo_dir = await _ensure_repo_cache(manifest.repo_url, username=username) @@ -584,7 +600,7 @@ async def submit_job( else: # Manifest and tool code share one repo: cache it and run from the # subdirectory that contains the manifest. - cached_repo_dir = await _ensure_repo_cache(app_url, username=username) + cached_repo_dir = await _ensure_repo_cache(app_clone_url, username=username) cd_suffix = f"repo/{manifest_path}" if manifest_path else "repo" # Build environment variable export lines diff --git a/fileglancer/apps/manifest.py b/fileglancer/apps/manifest.py index 3ee6f777..6e4c8307 100644 --- a/fileglancer/apps/manifest.py +++ b/fileglancer/apps/manifest.py @@ -2,7 +2,7 @@ import asyncio import os -import re +import shutil from contextlib import suppress from pathlib import Path, PurePosixPath @@ -11,6 +11,15 @@ from fileglancer import database as db from fileglancer.apps.adapters import try_adapt +# GitHub URL parsing/canonicalization lives in fileglancer.giturls (which has no +# fileglancer deps) so the database layer can reuse it without an import cycle. +# Re-exported for the apps module's internal callers and existing imports. +from fileglancer.giturls import ( # noqa: F401 + _parse_github_url, + canonical_github_url, + github_url_at_branch, + github_url_with_branch, +) from fileglancer.model import AppManifest from fileglancer.settings import get_settings @@ -55,41 +64,6 @@ def _get_repo_lock(owner: str, repo: str, branch: str) -> asyncio.Lock: return _repo_locks[key] -def _parse_github_url(url: str) -> tuple[str, str, str | None]: - """Parse a GitHub repo URL into (owner, repo, branch). - - Branch is None when not specified in the URL. - Raises ValueError if not a valid GitHub repo URL. - """ - pattern = r"https?://github\.com/([^/]+)/([^/]+?)(?:\.git)?(?:/tree/(.+?))?/?$" - match = re.match(pattern, url) - if not match: - raise ValueError( - f"Invalid app URL: '{url}'. Only GitHub repository URLs are supported " - f"(e.g., https://github.com/owner/repo)." - ) - owner, repo, branch = match.groups() - - # Validate segments to prevent path traversal - for name, value in [("owner", owner), ("repo", repo)]: - if ".." in value or "\x00" in value: - raise ValueError( - f"Invalid app URL: {name} '{value}' contains invalid characters" - ) - if branch and ( - ".." in branch - or "\x00" in branch - or branch.startswith("/") - or branch.endswith("/") - or "//" in branch - ): - raise ValueError( - f"Invalid app URL: branch '{branch}' contains invalid characters" - ) - - return owner, repo, branch - - def validate_manifest_path(manifest_path: str) -> str: """Validate and normalize a user-supplied manifest path. @@ -141,13 +115,83 @@ def _safe_repo_subdir(repo_dir: Path, manifest_path: str) -> Path: return target -async def _run_git(args: list[str], timeout: int = 60) -> tuple[bytes, bytes]: +# When cloning over SSH, never prompt interactively (that would hang the +# worker under GIT_TERMINAL_PROMPT=0, which only governs git's own prompts, not +# ssh's). BatchMode disables passphrase/password prompts; accept-new trusts the +# GitHub host key on first use so a missing known_hosts entry isn't a hard fail. +_SSH_GIT_ENV = { + "GIT_SSH_COMMAND": "ssh -o BatchMode=yes -o StrictHostKeyChecking=accept-new", +} + +# Substrings (lowercased) that mark an HTTPS git failure as an auth/access +# problem — the signal to retry the same repo over SSH. Private and nonexistent +# repos both look like this over HTTPS (GitHub won't confirm a repo exists to an +# unauthenticated client). +_GIT_AUTH_ERROR_MARKERS = ( + "could not read username", + "authentication failed", + "terminal prompts disabled", + "repository not found", + "fatal: could not read", +) + + +def _is_git_auth_error(message: str) -> bool: + low = message.lower() + return any(marker in low for marker in _GIT_AUTH_ERROR_MARKERS) + + +def _github_remote_urls(owner: str, repo: str) -> tuple[str, str]: + """Return (https, ssh) clone URLs for a GitHub owner/repo.""" + return ( + f"https://github.com/{owner}/{repo}.git", + f"git@github.com:{owner}/{repo}.git", + ) + + +def _is_git_ref_not_found(message: str) -> bool: + """True if a git failure means the requested branch/tag doesn't exist + (as opposed to an auth/access problem) — i.e. the remote was reachable.""" + low = message.lower() + return ( + "not found in upstream" in low + or "could not find remote ref" in low + or ("remote branch" in low and "not found" in low) + ) + + +def _ref_not_found_error(owner: str, repo: str, branch: str) -> str: + """User-facing message for a revision that doesn't exist in the repo.""" + return ( + f"Revision '{branch}' was not found in repository {owner}/{repo}. " + f"Check that the tag or branch name is spelled correctly." + ) + + +def _repo_access_error(owner: str, repo: str, branch: str, + https_err: str, ssh_err: str) -> str: + """Build a user-facing message for a repo that couldn't be cloned either way.""" + return ( + f"Could not access the repository {owner}/{repo} (revision '{branch}'). " + f"If it is private, make sure it exists and that you have access. Fileglancer " + f"tried HTTPS and then SSH (git@github.com) as your user — for SSH access, your " + f"SSH key must be configured on this server and added to your GitHub account.\n" + f" HTTPS: {https_err}\n" + f" SSH: {ssh_err}" + ) + + +async def _run_git(args: list[str], timeout: int = 60, + extra_env: dict | None = None) -> tuple[bytes, bytes]: """Run a git command asynchronously. The timeout covers the command's full runtime, not just process creation. + extra_env is merged into the subprocess environment (e.g. GIT_SSH_COMMAND). Raises ValueError with a readable message on failure. """ env = {**os.environ, "GIT_TERMINAL_PROMPT": "0"} + if extra_env: + env.update(extra_env) proc = None async def _create_and_communicate() -> tuple[bytes, bytes]: @@ -184,24 +228,75 @@ async def _create_and_communicate() -> tuple[bytes, bytes]: return stdout, stderr -async def _resolve_default_branch(clone_url: str) -> str: +async def _resolve_default_branch(owner: str, repo: str) -> str: """Query a remote repo for its default branch (HEAD). - Falls back to 'main' if the remote cannot be queried. + Tries HTTPS first, then SSH if HTTPS fails for auth/access reasons (private + repos). Falls back to 'main' if the remote cannot be queried. + """ + https_url, ssh_url = _github_remote_urls(owner, repo) + for url, extra_env in ((https_url, None), (ssh_url, _SSH_GIT_ENV)): + try: + stdout, _ = await _run_git( + ["git", "ls-remote", "--symref", url, "HEAD"], + timeout=30, extra_env=extra_env, + ) + # Output: "ref: refs/heads/master\tHEAD\n..." + for line in stdout.decode().splitlines(): + if line.startswith("ref:"): + ref = line.split()[1] + return ref.removeprefix("refs/heads/") + # Reached the remote but found no symref line — stop, use the default. + break + except ValueError as e: + # Only fall through to the SSH attempt for auth/access failures. + if not _is_git_auth_error(str(e)): + break + except Exception: + break + return "main" + + +async def _clone_repo(owner: str, repo: str, branch: str, repo_dir: Path) -> None: + """Clone owner/repo at branch into repo_dir, trying HTTPS then SSH. + + On an HTTPS auth/access failure (e.g. a private repo), retries over SSH as + the current user. If both transports fail, raises ValueError with a + user-facing message describing both errors. """ + https_url, ssh_url = _github_remote_urls(owner, repo) try: - stdout, _ = await _run_git( - ["git", "ls-remote", "--symref", clone_url, "HEAD"], - timeout=30, + await _run_git( + ["git", "clone", "--branch", branch, https_url, str(repo_dir)], + timeout=120, ) - # Output: "ref: refs/heads/master\tHEAD\n..." - for line in stdout.decode().splitlines(): - if line.startswith("ref:"): - ref = line.split()[1] - return ref.removeprefix("refs/heads/") - except Exception: - pass - return "main" + return + except ValueError as https_err: + shutil.rmtree(repo_dir, ignore_errors=True) + # The remote was reachable over HTTPS but the branch/tag doesn't exist + # (public repo, mistyped revision) — authoritative, don't try SSH. + if _is_git_ref_not_found(str(https_err)): + raise ValueError(_ref_not_found_error(owner, repo, branch)) + # Anything other than an auth/access failure is surfaced as-is. + if not _is_git_auth_error(str(https_err)): + raise + logger.info( + f"HTTPS clone of {owner}/{repo} failed authentication; retrying over SSH" + ) + try: + await _run_git( + ["git", "clone", "--branch", branch, ssh_url, str(repo_dir)], + timeout=120, extra_env=_SSH_GIT_ENV, + ) + except ValueError as ssh_err: + shutil.rmtree(repo_dir, ignore_errors=True) + # SSH reached the repo (auth worked) but the revision is missing — + # report that plainly instead of the misleading "can't access" text. + if _is_git_ref_not_found(str(ssh_err)): + raise ValueError(_ref_not_found_error(owner, repo, branch)) + raise ValueError( + _repo_access_error(owner, repo, branch, str(https_err), str(ssh_err)) + ) async def _ensure_repo_cache(url: str, pull: bool = False, @@ -218,9 +313,8 @@ async def _ensure_repo_cache(url: str, pull: bool = False, the worker subprocess itself, or in single-user dev mode). """ owner, repo, branch = _parse_github_url(url) - clone_url = f"https://github.com/{owner}/{repo}.git" if not branch: - branch = await _resolve_default_branch(clone_url) + branch = await _resolve_default_branch(owner, repo) if username: lock = _get_repo_lock(owner, repo, branch) @@ -229,7 +323,8 @@ async def _ensure_repo_cache(url: str, pull: bool = False, return Path(result["repo_dir"]) # Running as the current user (worker subprocess or dev mode) - logger.debug(f"ensure_repo running in-process as euid={os.geteuid()}") + euid = os.geteuid() if hasattr(os, "geteuid") else "n/a" + logger.debug(f"ensure_repo running in-process as euid={euid}") cache_base = _repo_cache_base() repo_dir = (cache_base / owner / repo / branch).resolve() repo_dir.relative_to(cache_base.resolve()) @@ -240,15 +335,22 @@ async def _ensure_repo_cache(url: str, pull: bool = False, logger.debug(f"Repo cache hit: {owner}/{repo} ({branch})") if pull: logger.info(f"Pulling latest for {owner}/{repo} ({branch})") - await _run_git(["git", "-C", str(repo_dir), "fetch", "origin", branch]) - await _run_git(["git", "-C", str(repo_dir), "reset", "--hard", f"origin/{branch}"]) + # `branch` may be a tag or commit, not a branch, so there is no + # origin/ tracking ref to reset to. Fetch the ref and + # reset to FETCH_HEAD, which works for branches, tags and SHAs. + # Pass the SSH env so private repos with an SSH origin don't + # prompt (harmless for HTTPS origins, which ignore GIT_SSH_COMMAND). + await _run_git( + ["git", "-C", str(repo_dir), "fetch", "origin", branch], + extra_env=_SSH_GIT_ENV, + ) + await _run_git( + ["git", "-C", str(repo_dir), "reset", "--hard", "FETCH_HEAD"] + ) else: logger.info(f"Cloning {owner}/{repo} ({branch}) into {repo_dir}") repo_dir.parent.mkdir(parents=True, exist_ok=True) - await _run_git( - ["git", "clone", "--branch", branch, clone_url, str(repo_dir)], - timeout=120, - ) + await _clone_repo(owner, repo, branch, repo_dir) return repo_dir @@ -344,25 +446,35 @@ def _find_manifests_in_repo(repo_dir: Path) -> list[tuple[str, AppManifest]]: MANIFEST_FILENAME = _MANIFEST_FILENAME -async def discover_app_manifests(url: str, - username: str | None = None) -> list[tuple[str, AppManifest]]: +async def discover_app_manifests( + url: str, + username: str | None = None, +) -> tuple[str, list[tuple[str, AppManifest]]]: """Clone/pull a GitHub repo and discover all manifest files. - Returns a list of (relative_dir_path, AppManifest) tuples. - Raises ValueError if the URL is invalid or the clone fails. + Returns (resolved_branch, [(relative_dir_path, AppManifest), ...]). The + resolved branch is the revision actually cloned — resolved in the same + process that does the clone, so a private repo's real default branch is used + rather than a fallback. Raises ValueError if the URL is invalid or the clone + fails. When username is provided, the work is delegated to a worker subprocess - running as the target user. + running as the target user (which holds the user's SSH credentials). """ if username: result = await _dispatch(username, "discover_manifests", url=url) - return [ + manifests = [ (item["path"], AppManifest(**item["manifest"])) for item in result["manifests"] ] + return result["branch"], manifests repo_dir = await _ensure_repo_cache(url, pull=True) - return _find_manifests_in_repo(repo_dir) + owner, repo, _ = _parse_github_url(url) + branch = repo_dir.relative_to( + (_repo_cache_base() / owner / repo).resolve() + ).as_posix() + return branch, _find_manifests_in_repo(repo_dir) async def fetch_app_manifest(url: str, manifest_path: str = "", @@ -409,6 +521,8 @@ async def get_or_load_manifest(username: str, url: str, row = db.get_user_app(session, username, url, manifest_path) stored = row.manifest if row else None row_exists = row is not None + row_url = row.url if row is not None else url + row_branch = row.branch if row is not None else None if stored is not None: try: @@ -416,16 +530,17 @@ async def get_or_load_manifest(username: str, url: str, except ValidationError as e: logger.warning(f"Stored manifest schema mismatch for {url}: {e}") - manifest = await fetch_app_manifest(url, manifest_path, username=username) + fetch_url = clone_url_for_stored_app(row_url, row_branch) if row_exists else url + manifest = await fetch_app_manifest(fetch_url, manifest_path, username=username) if row_exists: - branch = await get_app_branch(url) + # branch=None: this is a cache refresh, so leave the requested revision + # (the branch column) untouched. with db.get_db_session(settings.db_url) as session: db.upsert_user_app( session, username, - url=url, manifest_path=manifest_path, + url=row_url, manifest_path=manifest_path, name=manifest.name, description=manifest.description, - branch=branch, manifest=manifest.model_dump(mode="json"), bump_updated_at=False, ) @@ -436,7 +551,7 @@ async def get_or_load_manifest(username: str, url: str, async def refresh_cached_manifest(username: str, url: str, manifest_path: str = "", bump_updated_at: bool = False - ) -> tuple[AppManifest, str]: + ) -> AppManifest: """Re-read the manifest from disk and sync the cache. Call this after any operation that mutates the on-disk YAML @@ -444,26 +559,31 @@ async def refresh_cached_manifest(username: str, url: str, No-op on the DB if (username, url, manifest_path) has no row — callers that need to insert a new row should use upsert_user_app - directly. + directly. Leaves the requested revision (the branch column) untouched. - Returns (manifest, branch). + Returns the refreshed manifest. """ - manifest = await fetch_app_manifest(url, manifest_path, username=username) - branch = await get_app_branch(url) - settings = get_settings() with db.get_db_session(settings.db_url) as session: - if db.get_user_app(session, username, url, manifest_path) is not None: + row = db.get_user_app(session, username, url, manifest_path) + row_exists = row is not None + row_url = row.url if row_exists else url + row_branch = row.branch if row_exists else None + + fetch_url = clone_url_for_stored_app(row_url, row_branch) if row_exists else url + manifest = await fetch_app_manifest(fetch_url, manifest_path, username=username) + + with db.get_db_session(settings.db_url) as session: + if row_exists: db.upsert_user_app( session, username, - url=url, manifest_path=manifest_path, + url=row_url, manifest_path=manifest_path, name=manifest.name, description=manifest.description, - branch=branch, manifest=manifest.model_dump(mode="json"), bump_updated_at=bump_updated_at, ) - return manifest, branch + return manifest async def get_app_branch(url: str) -> str: @@ -473,6 +593,45 @@ async def get_app_branch(url: str) -> str: """ owner, repo, branch = _parse_github_url(url) if not branch: - clone_url = f"https://github.com/{owner}/{repo}.git" - branch = await _resolve_default_branch(clone_url) + branch = await _resolve_default_branch(owner, repo) return branch + + +def canonical_app_url(url: str, resolved_branch: str) -> tuple[str, str]: + """Build the (canonical_url, requested_branch) pair to store for an app. + + Called once, at add time, to fix the app's revision. Pure string work — no + network: the resolved branch is supplied by the caller (resolved in whatever + process actually cloned the repo, e.g. the user's worker for private repos). + + The canonical URL carries the resolved revision being cloned, so a repo whose + default is "master" yields ".../tree/master" while "main" folds to the bare + URL. requested_branch is the revision the user asked for verbatim — "" means + they gave a bare URL and took whatever the default was at add time. The + revision is fixed from here on; the app does not re-resolve later (re-add it + to pick up a moved default). + """ + owner, repo, url_branch = _parse_github_url(url) + return github_url_at_branch(owner, repo, resolved_branch), (url_branch or "") + + +def clone_url_for_stored_app(url: str, branch: str | None) -> str: + """Return the explicit GitHub URL to clone/fetch for a stored app. + + Stored app URLs are canonical and intentionally fold the fixed "main" + revision to a bare URL for UI and de-dupe compatibility. A bare GitHub URL is + unsafe for operational git work, though, because git interprets it as "the + repo's current default branch", which can move. So for a *pinned* app, make + the revision explicit (a bare URL means the fixed "main"). + + branch is the row's recorded revision. None marks a legacy row migrated from + user_preferences whose default branch was never recorded: those historically + tracked the repo's default branch, so the URL is returned unchanged (a bare + URL keeps resolving the current default) rather than guessing "main", which + would break a repo that defaults to e.g. "master". Such rows get pinned the + next time the app is re-added. + """ + if branch is None: + return url + owner, repo, url_branch = _parse_github_url(url) + return github_url_with_branch(owner, repo, url_branch or "main") diff --git a/fileglancer/apps/pixi.py b/fileglancer/apps/pixi.py index da748300..14246ff9 100644 --- a/fileglancer/apps/pixi.py +++ b/fileglancer/apps/pixi.py @@ -169,30 +169,24 @@ def _task_to_entry_point(name: str, task: dict) -> AppEntryPoint | None: ) -def _get_git_repo_and_branch(directory: Path) -> tuple[str, str] | None: - """Get the repo name and branch from a git directory. +def _get_git_repo_name(directory: Path) -> str | None: + """Get the repo name from a git directory's origin remote. - Returns (repo_name, branch) or None if not a git repo. + Returns the repo name (e.g. "repo" for ".../owner/repo.git") or None if not + a git repo or the remote can't be read. """ try: repo_url = subprocess.run( ["git", "-C", str(directory), "remote", "get-url", "origin"], capture_output=True, text=True, timeout=5, ) - branch = subprocess.run( - ["git", "-C", str(directory), "rev-parse", "--abbrev-ref", "HEAD"], - capture_output=True, text=True, timeout=5, - ) - if repo_url.returncode != 0 or branch.returncode != 0: + if repo_url.returncode != 0: return None # Extract repo name from URL (e.g. "https://github.com/owner/repo.git" -> "repo") url = repo_url.stdout.strip() repo_name = url.rstrip("/").rsplit("/", 1)[-1].removesuffix(".git") - branch_name = branch.stdout.strip() - - if repo_name and branch_name: - return repo_name, branch_name + return repo_name or None except (subprocess.TimeoutExpired, FileNotFoundError): pass return None @@ -211,13 +205,14 @@ def convert(self, directory: Path) -> AppManifest: project = config.get("project", config.get("workspace", {})) description = project.get("description") - # Use repo_name/branch as the app name if in a git repo - git_info = _get_git_repo_and_branch(directory) - if git_info: - repo_name, branch_name = git_info - name = f"{repo_name}/{branch_name}" - else: - name = project.get("name", directory.name) + # Prefer the pixi project's declared name. Fall back to the git repo + # name, then the directory name. (directory is the cached repo, whose + # leaf is the branch/revision — a poor name, hence the fallbacks.) + name = ( + project.get("name") + or _get_git_repo_name(directory) + or directory.name + ) # Collect and convert tasks tasks = _collect_tasks(config) diff --git a/fileglancer/database.py b/fileglancer/database.py index cf351276..4d6bb9c3 100644 --- a/fileglancer/database.py +++ b/fileglancer/database.py @@ -12,6 +12,7 @@ from loguru import logger from cachetools import LRUCache +from fileglancer.giturls import canonical_github_url from fileglancer.model import FileSharePath from fileglancer.settings import get_settings from fileglancer.utils import slugify_path @@ -960,7 +961,7 @@ def create_job(session: Session, username: str, app_url: str, app_name: str, now = datetime.now(UTC) job = JobDB( username=username, - app_url=app_url, + app_url=canonical_github_url(app_url), app_name=app_name, manifest_path=manifest_path, entry_point_id=entry_point_id, @@ -1076,7 +1077,7 @@ def get_user_app(session: Session, username: str, url: str, """Get a single user app by (username, url, manifest_path).""" return session.query(UserAppDB).filter_by( username=username, - url=url, + url=canonical_github_url(url), manifest_path=manifest_path, ).first() @@ -1094,8 +1095,16 @@ def upsert_user_app(session: Session, username: str, url: str, On update, added_at is preserved. updated_at is bumped only when bump_updated_at is True (the default) — set False for invisible refreshes like a lazy manifest backfill. + + branch holds the user's *requested* revision ("" means no explicit revision + was requested). The fixed revision actually cloned is encoded in url; due to + canonical URL folding, a bare stored URL means the fixed "main" revision. + Pass branch=None to leave an existing row's branch untouched — + manifest-cache refreshes use this so they don't clobber the requested + revision with a resolved one. """ now = datetime.now(UTC) + url = canonical_github_url(url) row = get_user_app(session, username, url, manifest_path) if row is None: row = UserAppDB( @@ -1112,7 +1121,8 @@ def upsert_user_app(session: Session, username: str, url: str, else: row.name = name row.description = description - row.branch = branch + if branch is not None: + row.branch = branch row.manifest = manifest if bump_updated_at: row.updated_at = now @@ -1125,7 +1135,7 @@ def delete_user_app(session: Session, username: str, url: str, """Delete a user app row. Returns True if a row was deleted.""" deleted = session.query(UserAppDB).filter_by( username=username, - url=url, + url=canonical_github_url(url), manifest_path=manifest_path, ).delete() session.commit() @@ -1158,7 +1168,7 @@ def get_app_listing_for_app(session: Session, owner_username: str, url: str, """Get the listing (if any) that this user has published for a given app.""" return session.query(AppListingDB).filter_by( owner_username=owner_username, - url=url, + url=canonical_github_url(url), manifest_path=manifest_path, ).first() @@ -1168,6 +1178,7 @@ def create_app_listing(session: Session, owner_username: str, url: str, description: Optional[str] = None, branch: Optional[str] = None) -> AppListingDB: """Publish a new listing. Raises ValueError if a duplicate exists.""" + url = canonical_github_url(url) existing = get_app_listing_for_app(session, owner_username, url, manifest_path) if existing is not None: raise ValueError("This app is already shared") diff --git a/fileglancer/giturls.py b/fileglancer/giturls.py new file mode 100644 index 00000000..47d8ff65 --- /dev/null +++ b/fileglancer/giturls.py @@ -0,0 +1,103 @@ +"""GitHub URL parsing and canonicalization. + +Standalone (no fileglancer imports) so both the apps module and the database +layer can use it without an import cycle. Mirrors the frontend helpers in +frontend/src/utils/appUrls.ts. +""" + +import re + +_HTTPS_GITHUB_RE = r"https?://github\.com/([^/]+)/([^/]+?)(?:\.git)?(?:/tree/(.+?))?/?$" +# scp-style (git@github.com:owner/repo.git) and ssh:// forms. SSH URLs don't +# carry a branch (no /tree/...), so branch is always None for them. +_SSH_SCP_GITHUB_RE = r"git@github\.com:([^/]+)/([^/]+?)(?:\.git)?/?$" +_SSH_PROTO_GITHUB_RE = r"ssh://git@github\.com/([^/]+)/([^/]+?)(?:\.git)?/?$" + + +def _parse_github_url(url: str) -> tuple[str, str, str | None]: + """Parse a GitHub repo URL into (owner, repo, branch). + + Accepts HTTPS (https://github.com/owner/repo[/tree/branch]) and SSH + (git@github.com:owner/repo.git or ssh://git@github.com/owner/repo) forms. + Branch is None when not specified in the URL (always None for SSH forms). + Raises ValueError if not a valid GitHub repo URL. + """ + branch: str | None = None + match = re.match(_HTTPS_GITHUB_RE, url) + if match: + owner, repo, branch = match.groups() + else: + match = re.match(_SSH_SCP_GITHUB_RE, url) or re.match(_SSH_PROTO_GITHUB_RE, url) + if not match: + raise ValueError( + f"Invalid app URL: '{url}'. Only GitHub repository URLs are supported " + f"(e.g., https://github.com/owner/repo or git@github.com:owner/repo.git)." + ) + owner, repo = match.group(1), match.group(2) + + # Validate segments to prevent path traversal + for name, value in [("owner", owner), ("repo", repo)]: + if ".." in value or "\x00" in value: + raise ValueError( + f"Invalid app URL: {name} '{value}' contains invalid characters" + ) + if branch and ( + ".." in branch + or "\x00" in branch + or branch.startswith("/") + or branch.endswith("/") + or "//" in branch + ): + raise ValueError( + f"Invalid app URL: branch '{branch}' contains invalid characters" + ) + + return owner, repo, branch + + +def canonical_github_url(url: str) -> str: + """Normalize a GitHub URL to its canonical https form. + + Strips a ".git" suffix and trailing slash, folds SSH forms to https, and + treats "/tree/main" as the bare repo URL — so the same repo has one stored + representation. Any other branch is kept as "/tree/". + + Note the "main" folding is shorthand for "the default branch", which is only + accurate for repos whose default actually is "main": a bare URL means "the + default branch" while "/tree/main" means the main branch specifically, so for + a repo defaulting to e.g. "master" these are different refs that this + collapses together. Callers that need the real cloned revision in the URL + resolve the default branch first (see canonical_app_url, fed by the + worker-resolved branch) rather than relying on this. The frontend's + buildGithubUrl/canonicalGithubUrl fold "main" the same + way, and the "is it installed?" comparison depends on both sides matching. + + Returns the input unchanged if it isn't a parseable GitHub URL. + """ + try: + owner, repo, branch = _parse_github_url(url) + except ValueError: + return url + if branch and branch != "main": + return f"https://github.com/{owner}/{repo}/tree/{branch}" + return f"https://github.com/{owner}/{repo}" + + +def github_url_at_branch(owner: str, repo: str, branch: str) -> str: + """Build the canonical https URL for owner/repo at a specific branch. + + The default branch "main" folds away (a bare repo URL implies it), so a + non-default branch like "master" stays explicit as "/tree/master". Use this + to bake a *resolved* revision into a stored app URL. + """ + return canonical_github_url(f"https://github.com/{owner}/{repo}/tree/{branch}") + + +def github_url_with_branch(owner: str, repo: str, branch: str) -> str: + """Build an explicit https URL for owner/repo at branch. + + Unlike github_url_at_branch, this intentionally does *not* fold "main" to a + bare repo URL. Use it for operational clone/fetch calls where a bare URL + would mean "current default branch" and therefore could move over time. + """ + return f"https://github.com/{owner}/{repo}/tree/{branch}" diff --git a/fileglancer/model.py b/fileglancer/model.py index 56110b7f..5743ddc8 100644 --- a/fileglancer/model.py +++ b/fileglancer/model.py @@ -617,7 +617,7 @@ class UserApp(BaseModel): """A user's saved app reference""" url: str = Field(description="URL to the app manifest") manifest_path: str = Field(description="Relative directory path to the manifest within the repo", default="") - branch: Optional[str] = Field(description="Git branch used", default=None) + branch: Optional[str] = Field(description="Revision the user requested; empty means no explicit revision was requested. The fixed actually-cloned revision is baked into url; a bare stored URL means main.", default=None) name: str = Field(description="App name from manifest") description: Optional[str] = Field(description="App description from manifest", default=None) added_at: datetime = Field(description="When the app was added") @@ -635,7 +635,7 @@ class AppListing(BaseModel): owner_username: str = Field(description="The user who published this listing") url: str = Field(description="Git URL of the app repo") manifest_path: str = Field(description="Manifest path within the repo", default="") - branch: Optional[str] = Field(description="Git branch", default=None) + branch: Optional[str] = Field(description="Revision the user requested; empty means no explicit revision was requested. The fixed actually-cloned revision is baked into url; a bare stored URL means main.", default=None) name: str = Field(description="Display name for the catalog") description: Optional[str] = Field(description="Description for the catalog", default=None) published_at: datetime = Field(description="When this listing was published") diff --git a/fileglancer/server.py b/fileglancer/server.py index 6327d2cd..c277299b 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -34,6 +34,7 @@ from fileglancer import database as db from fileglancer import auth from fileglancer import apps as apps_module +from fileglancer.giturls import canonical_github_url from fileglancer.model import * from fileglancer.settings import get_settings from fileglancer.issues import create_jira_ticket, get_jira_ticket_details, delete_jira_ticket @@ -1664,7 +1665,7 @@ async def get_user_apps(username: str = Depends(get_current_user)): for idx in needs_backfill: snap = snapshots[idx] try: - manifest, branch = await apps_module.refresh_cached_manifest( + manifest = await apps_module.refresh_cached_manifest( username, snap["url"], snap["manifest_path"], ) except Exception as e: @@ -1675,19 +1676,28 @@ async def get_user_apps(username: str = Depends(get_current_user)): user_app.manifest = manifest user_app.name = manifest.name user_app.description = manifest.description - user_app.branch = branch return result @app.post("/api/apps", response_model=list[UserApp], description="Add an app by URL (discovers all manifests in the repo)") async def add_user_app(body: AppAddRequest, username: str = Depends(get_current_user)): - # Clone the repo and discover all manifests + # Clone the repo and discover all manifests. The worker resolves the + # branch as the user, so a private repo's real default is used. try: - discovered = await apps_module.discover_app_manifests(body.url, - username=username) + resolved_branch, discovered = await apps_module.discover_app_manifests( + body.url, username=username) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) + except HTTPException as e: + # The worker already produced a meaningful, user-facing message + # (e.g. a mistyped revision or a private-repo clone failure). Surface + # it directly instead of nesting it inside a generic "Failed to clone + # or scan repo: ..." wrapper. The worker's default 500 for an uncaught + # error is, for this endpoint, a problem with the requested repo, so + # present it as a 400 (preserving other codes like 503 worker-dead). + status = 400 if e.status_code == 500 else e.status_code + raise HTTPException(status_code=status, detail=e.detail) except Exception as e: raise HTTPException(status_code=400, detail=f"Failed to clone or scan repo: {str(e)}") @@ -1699,18 +1709,21 @@ async def add_user_app(body: AppAddRequest, f"Make sure a manifest exists in the repository.", ) - branch = await apps_module.get_app_branch(body.url) + # Bake the worker-resolved revision into the stored URL (so a repo whose + # default is e.g. "master" dedups against an explicit ".../tree/master"), + # and record the user's requested revision separately ("" = unpinned). + canonical_url, requested = apps_module.canonical_app_url(body.url, resolved_branch) new_apps: list[UserApp] = [] with db.get_db_session(settings.db_url) as session: for manifest_path, manifest in discovered: - if db.get_user_app(session, username, body.url, manifest_path) is not None: + if db.get_user_app(session, username, canonical_url, manifest_path) is not None: continue # silently skip duplicates row = db.upsert_user_app( session, username, - url=body.url, manifest_path=manifest_path, + url=canonical_url, manifest_path=manifest_path, name=manifest.name, description=manifest.description, - branch=branch, + branch=requested, manifest=manifest.model_dump(mode="json"), ) new_apps.append(UserApp( @@ -1746,20 +1759,31 @@ async def remove_user_app(url: str = Query(..., description="URL of the app to r description="Pull latest code and re-read the manifest for an app") async def update_user_app(body: ManifestFetchRequest, username: str = Depends(get_current_user)): + # The revision is fixed at add time and baked into body.url, so update + # just pulls that revision again and re-reads the manifest — it never + # re-resolves the default branch or moves the app to a new URL. + with db.get_db_session(settings.db_url) as session: + existing = db.get_user_app(session, username, body.url, body.manifest_path) + if existing is None: + raise HTTPException(status_code=404, detail="App not found") + stored_url = existing.url + stored_branch = existing.branch + + clone_url = apps_module.clone_url_for_stored_app(stored_url, stored_branch) try: - await apps_module._ensure_repo_cache(body.url, pull=True, username=username) + await apps_module._ensure_repo_cache(clone_url, pull=True, username=username) except Exception as e: raise HTTPException(status_code=400, detail=f"Failed to pull latest code: {str(e)}") try: - manifest = await apps_module.fetch_app_manifest(body.url, body.manifest_path, + manifest = await apps_module.fetch_app_manifest(clone_url, body.manifest_path, username=username) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) except Exception as e: raise HTTPException(status_code=400, detail=f"Failed to read manifest after update: {str(e)}") - if manifest.repo_url and manifest.repo_url != body.url: + if manifest.repo_url and canonical_github_url(manifest.repo_url) != stored_url: try: await apps_module._ensure_repo_cache( manifest.repo_url, @@ -1772,14 +1796,12 @@ async def update_user_app(body: ManifestFetchRequest, detail=f"Failed to pull latest app code: {str(e)}", ) - branch = await apps_module.get_app_branch(body.url) - with db.get_db_session(settings.db_url) as session: + # branch omitted (None) so the revision fixed at add time is preserved. row = db.upsert_user_app( session, username, - url=body.url, manifest_path=body.manifest_path, + url=stored_url, manifest_path=body.manifest_path, name=manifest.name, description=manifest.description, - branch=branch, manifest=manifest.model_dump(mode="json"), ) return UserApp( @@ -1890,27 +1912,29 @@ async def add_from_catalog(listing_id: int, ) listing_url = listing.url listing_manifest_path = listing.manifest_path + listing_name = listing.name + listing_description = listing.description + # Keep None (legacy "track default") distinct from "" (pinned main). + listing_branch = listing.branch + clone_url = apps_module.clone_url_for_stored_app(listing_url, listing_branch) try: manifest = await apps_module.fetch_app_manifest( - listing_url, listing_manifest_path, username=username, + clone_url, listing_manifest_path, username=username, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) except Exception as e: raise HTTPException(status_code=400, detail=f"Failed to fetch manifest: {str(e)}") - try: - branch = await apps_module.get_app_branch(listing_url) - except Exception as e: - raise HTTPException(status_code=400, detail=f"Failed to resolve branch: {str(e)}") - + # The listing already carries the canonical URL (resolved revision baked + # in) and the requested revision, so copy them straight over. with db.get_db_session(settings.db_url) as session: row = db.upsert_user_app( session, username, url=listing_url, manifest_path=listing_manifest_path, - name=manifest.name, description=manifest.description, - branch=branch, + name=listing_name, description=listing_description, + branch=listing_branch, manifest=manifest.model_dump(mode="json"), ) return UserApp( diff --git a/fileglancer/user_worker.py b/fileglancer/user_worker.py index efd7280f..34112abe 100644 --- a/fileglancer/user_worker.py +++ b/fileglancer/user_worker.py @@ -1035,18 +1035,33 @@ def _action_ensure_repo(request: dict, ctx: WorkerContext) -> dict: @action("discover_manifests") def _action_discover_manifests(request: dict, ctx: WorkerContext) -> dict: - """Clone/pull repo and discover all manifests.""" - from fileglancer.apps.manifest import _ensure_repo_cache, _find_manifests_in_repo + """Clone/pull repo and discover all manifests. + + Resolves the default branch here, in the user's worker, so a private repo's + real default (reachable via the user's SSH key) is used rather than the + server process's "main" fallback. + """ + from fileglancer.apps.manifest import ( + _ensure_repo_cache, + _find_manifests_in_repo, + _parse_github_url, + _repo_cache_base, + ) repo_dir = _run_async(_ensure_repo_cache( url=request["url"], pull=True, )) + owner, repo, _ = _parse_github_url(request["url"]) + branch = repo_dir.relative_to( + (_repo_cache_base() / owner / repo).resolve() + ).as_posix() results = _find_manifests_in_repo(repo_dir) return { + "branch": branch, "manifests": [ {"path": path, "manifest": manifest.model_dump(mode="json")} for path, manifest in results - ] + ], } diff --git a/frontend/src/__tests__/unitTests/appUrls.test.ts b/frontend/src/__tests__/unitTests/appUrls.test.ts index 327519ea..b86950c1 100644 --- a/frontend/src/__tests__/unitTests/appUrls.test.ts +++ b/frontend/src/__tests__/unitTests/appUrls.test.ts @@ -1,8 +1,11 @@ import { describe, expect, test } from 'vitest'; import { + buildAppUrl, buildLaunchPath, buildRelaunchPath, + canonicalGithubUrl, + isGithubRepoUrl, parseGithubUrl } from '@/utils/appUrls'; @@ -13,6 +16,69 @@ describe('app URL helpers', () => { ).toEqual({ owner: 'org', repo: 'tool', branch: 'feature/my-tool' }); }); + test('parses scp-style SSH URLs (with and without .git)', () => { + expect(parseGithubUrl('git@github.com:org/tool.git')).toEqual({ + owner: 'org', + repo: 'tool', + branch: 'main' + }); + expect(parseGithubUrl('git@github.com:org/tool')).toEqual({ + owner: 'org', + repo: 'tool', + branch: 'main' + }); + }); + + test('parses ssh:// URLs', () => { + expect(parseGithubUrl('ssh://git@github.com/org/tool.git')).toEqual({ + owner: 'org', + repo: 'tool', + branch: 'main' + }); + }); + + test('isGithubRepoUrl accepts HTTPS and SSH, rejects others', () => { + expect(isGithubRepoUrl('https://github.com/org/tool')).toBe(true); + expect(isGithubRepoUrl('git@github.com:org/tool.git')).toBe(true); + expect(isGithubRepoUrl('https://gitlab.com/org/tool')).toBe(false); + expect(isGithubRepoUrl('not a url')).toBe(false); + }); + + test('canonicalGithubUrl normalizes cosmetic URL variations', () => { + // These all refer to the same app and must canonicalize identically, so an + // installed-app lookup by URL doesn't wrongly miss (the "not in your + // library" bug). + const canonical = 'https://github.com/Org/Repo'; + expect(canonicalGithubUrl('https://github.com/Org/Repo')).toBe(canonical); + expect(canonicalGithubUrl('https://github.com/Org/Repo.git')).toBe( + canonical + ); + expect(canonicalGithubUrl('https://github.com/Org/Repo/')).toBe(canonical); + expect(canonicalGithubUrl('https://github.com/Org/Repo/tree/main')).toBe( + canonical + ); + expect(canonicalGithubUrl('git@github.com:Org/Repo.git')).toBe(canonical); + // Non-default branches are preserved. + expect(canonicalGithubUrl('https://github.com/Org/Repo/tree/dev')).toBe( + 'https://github.com/Org/Repo/tree/dev' + ); + // Unparseable input is returned unchanged. + expect(canonicalGithubUrl('not a url')).toBe('not a url'); + }); + + test('buildAppUrl normalizes SSH input and applies the revision', () => { + expect(buildAppUrl('git@github.com:org/tool.git', 'v0.1.0')).toBe( + 'https://github.com/org/tool/tree/v0.1.0' + ); + expect(buildAppUrl('https://github.com/org/tool', '')).toBe( + 'https://github.com/org/tool' + ); + // Revision overrides a branch embedded in the URL. + expect(buildAppUrl('https://github.com/org/tool/tree/dev', 'v1')).toBe( + 'https://github.com/org/tool/tree/v1' + ); + }); + test('builds launch paths with slash branches in the query string', () => { expect( buildLaunchPath('org', 'tool', 'feature/my-tool', 'run', 'apps/demo') diff --git a/frontend/src/components/AppJobs.tsx b/frontend/src/components/AppJobs.tsx index 3c932c60..e48c8c05 100644 --- a/frontend/src/components/AppJobs.tsx +++ b/frontend/src/components/AppJobs.tsx @@ -1,10 +1,11 @@ -import { useMemo } from 'react'; +import { useMemo, useState } from 'react'; import { useNavigate } from 'react-router'; import { Typography } from '@material-tailwind/react'; import toast from 'react-hot-toast'; import FgLink from '@/components/designSystem/atoms/FgLink'; +import CancelJobDialog from '@/components/ui/Dialogs/CancelJob'; import { TableCard } from '@/components/ui/Table/TableCard'; import { createAppsJobsColumns } from '@/components/ui/Table/appsJobsColumns'; import { buildRelaunchPath, parseGithubUrl } from '@/utils'; @@ -20,6 +21,7 @@ export default function AppJobs() { const jobsQuery = useJobsQuery(); const cancelJobMutation = useCancelJobMutation(); const deleteJobMutation = useDeleteJobMutation(); + const [jobToCancel, setJobToCancel] = useState(null); const handleViewJobDetail = (jobId: number) => { navigate(`/apps/jobs/${jobId}`); @@ -47,10 +49,17 @@ export default function AppJobs() { }); }; - const handleCancelJob = async (jobId: number) => { + const handleConfirmCancelJob = async () => { + if (!jobToCancel) { + return; + } + const job = jobToCancel; + setJobToCancel(null); try { - await cancelJobMutation.mutateAsync(jobId); - toast.success('Job cancelled'); + await cancelJobMutation.mutateAsync(job.id); + toast.success( + job.entry_point_type === 'service' ? 'Service stopped' : 'Job cancelled' + ); } catch (error) { const message = error instanceof Error ? error.message : 'Failed to cancel job'; @@ -74,7 +83,7 @@ export default function AppJobs() { createAppsJobsColumns({ onViewDetail: handleViewJobDetail, onRelaunch: handleRelaunch, - onCancel: handleCancelJob, + onCancel: setJobToCancel, onDelete: handleDeleteJob }), // eslint-disable-next-line react-hooks/exhaustive-deps @@ -96,6 +105,14 @@ export default function AppJobs() { gridColsClass="grid-cols-[2fr_2fr_1fr_2fr_1fr_1fr]" loadingState={jobsQuery.isPending} /> + + setJobToCancel(null)} + onConfirm={handleConfirmCancelJob} + open={jobToCancel !== null} + /> ); } diff --git a/frontend/src/components/AppLaunch.tsx b/frontend/src/components/AppLaunch.tsx index 37626736..2da22e74 100644 --- a/frontend/src/components/AppLaunch.tsx +++ b/frontend/src/components/AppLaunch.tsx @@ -15,7 +15,7 @@ import { import toast from 'react-hot-toast'; import AppLaunchForm from '@/components/ui/AppsPage/AppLaunchForm'; -import { buildGithubUrl } from '@/utils'; +import { buildGithubUrl, canonicalGithubUrl } from '@/utils'; import { useAppsQuery, useAddAppMutation, @@ -79,10 +79,16 @@ export default function AppLaunch() { const relaunchContainer = relaunchState?.container; const relaunchContainerArgs = relaunchState?.container_args; - // Check if app is in user's library - const isInstalled = appsQuery.data?.some( - a => a.url === appUrl && a.manifest_path === manifestPath + // Check if app is in user's library. Match by canonical URL identity rather + // than exact string: stored app URLs may carry cosmetic variations (a ".git" + // suffix, trailing slash, or "/tree/main") that don't survive the round-trip + // through the route, which would otherwise make an installed app — e.g. one + // added from the catalog — wrongly appear "not in your library". + const installedApp = appsQuery.data?.find( + a => + canonicalGithubUrl(a.url) === appUrl && a.manifest_path === manifestPath ); + const isInstalled = installedApp !== undefined; useEffect(() => { if (appUrl) { @@ -97,6 +103,10 @@ export default function AppLaunch() { const manifest = manifestMutation.data; + // Prefer the user's saved app name (which may be a custom name chosen when + // adding the app from the catalog) over the raw manifest name. + const displayName = installedApp?.name ?? manifest?.name; + // Auto-select entry point from URL param, or if there's only one useEffect(() => { if (!manifest) { @@ -229,6 +239,7 @@ export default function AppLaunch() { ) : manifest && selectedEntryPoint ? ( - {manifest.name} + {displayName} {manifest.description ? ( {manifest.description} diff --git a/frontend/src/components/JobDetail.tsx b/frontend/src/components/JobDetail.tsx index 185b9abf..11a971ab 100644 --- a/frontend/src/components/JobDetail.tsx +++ b/frontend/src/components/JobDetail.tsx @@ -19,7 +19,7 @@ import { import AnsiText from '@/components/ui/AppsPage/AnsiText'; import FgButton from '@/components/designSystem/atoms/FgButton'; import FgIcon from '@/components/designSystem/atoms/FgIcon'; -import FgDialog from '@/components/ui/Dialogs/FgDialog'; +import CancelJobDialog from '@/components/ui/Dialogs/CancelJob'; import FgTooltip from '@/components/ui/widgets/FgTooltip'; import type { JobFileInfo, @@ -203,13 +203,15 @@ function FileDownloadButton({ /** A titled card holding a small set of label/value rows. */ function InfoCard({ title, - children + children, + className }: { readonly title: string; readonly children: ReactNode; + readonly className?: string; }) { return ( -
+
{title} @@ -241,11 +243,19 @@ function InfoRow({ ); } -/** Best-effort GitHub repo link for an app manifest URL; null if not parseable. */ -function appRepoUrl(appUrl: string): string | null { +/** + * Best-effort GitHub repo link for an app manifest URL; null if not parseable. + * The label is "owner/repo", with "(branch)" appended when a non-default branch + * is specified. + */ +function appRepoLink(appUrl: string): { href: string; label: string } | null { try { const { owner, repo, branch } = parseGithubUrl(appUrl); - return buildGithubUrl(owner, repo, branch); + const label = + branch && branch !== 'main' + ? `${owner}/${repo} (${branch})` + : `${owner}/${repo}`; + return { href: buildGithubUrl(owner, repo, branch), label }; } catch { return null; } @@ -277,7 +287,7 @@ function JobOverview({ ? formatDuration(job.created_at, job.started_at) : null; const exitMeaning = exitCodeMeaning(job.exit_code); - const repoUrl = appRepoUrl(job.app_url); + const repoLink = appRepoLink(job.app_url); const stdoutTail = stdoutContent !== null && stdoutContent !== undefined @@ -291,8 +301,8 @@ function JobOverview({ return (
-
- +
+ @@ -314,18 +324,21 @@ function JobOverview({ /> - + + {job.app_name} - ) : ( - job.app_name - ) + repoLink ? ( + + {repoLink.label} + + ) : null } /> - setShowStopConfirm(false)} + onConfirm={() => { + cancelMutation.mutate(job.id); + setShowStopConfirm(false); + }} open={showStopConfirm} - > - - {isService ? 'Stop Service' : 'Cancel Job'} - - - {isService - ? 'Are you sure you want to stop this service? It will be terminated and the URL will no longer be accessible.' - : 'Are you sure you want to cancel this job? It will be terminated.'} - -
- setShowStopConfirm(false)} - variant="ghost" - > - Keep running - - { - cancelMutation.mutate(job.id); - setShowStopConfirm(false); - }} - > - {isService ? 'Stop Service' : 'Cancel Job'} - -
- + /> {/* Tabs */} diff --git a/frontend/src/components/ui/AppsPage/AddAppDialog.tsx b/frontend/src/components/ui/AppsPage/AddAppDialog.tsx index ad7fce01..35df07bf 100644 --- a/frontend/src/components/ui/AppsPage/AddAppDialog.tsx +++ b/frontend/src/components/ui/AppsPage/AddAppDialog.tsx @@ -5,20 +5,7 @@ import FgDialog from '@/components/ui/Dialogs/FgDialog'; import FgButton from '@/components/designSystem/atoms/FgButton'; import FgFormField from '@/components/designSystem/molecules/FgFormField'; import FgInput from '@/components/designSystem/atoms/formElements/FgInput'; - -const GITHUB_URL_PATTERN = /^https?:\/\/github\.com\/[^/]+\/[^/]+\/?$/; - -function isValidGitHubUrl(url: string): boolean { - return GITHUB_URL_PATTERN.test(url.trim()); -} - -function buildAppUrl(repoUrl: string, branch: string): string { - let url = repoUrl.trim().replace(/\/+$/, ''); - if (branch.trim()) { - url += `/tree/${branch.trim()}`; - } - return url; -} +import { buildAppUrl, isGithubRepoUrl } from '@/utils'; interface AddAppDialogProps { readonly open: boolean; @@ -42,8 +29,8 @@ export default function AddAppDialog({ setUrlError(''); return false; } - if (!isValidGitHubUrl(url)) { - setUrlError('Please enter a valid GitHub repository URL'); + if (!isGithubRepoUrl(url)) { + setUrlError('Please enter a valid GitHub repository URL (HTTPS or SSH)'); return false; } setUrlError(''); @@ -72,7 +59,7 @@ export default function AddAppDialog({ onClose(); }; - const urlIsValid = repoUrl.trim() !== '' && isValidGitHubUrl(repoUrl); + const urlIsValid = repoUrl.trim() !== '' && isGithubRepoUrl(repoUrl); return ( @@ -81,8 +68,9 @@ export default function AddAppDialog({ - Enter a GitHub repository URL containing a runnables.yaml{' '} - manifest. + Enter a GitHub repository URL (HTTPS or SSH) containing a{' '} + runnables.yaml manifest. Private repositories are accessed + over SSH using your configured SSH key. - + { setBranch(e.target.value); diff --git a/frontend/src/components/ui/AppsPage/AppCard.tsx b/frontend/src/components/ui/AppsPage/AppCard.tsx index 1fad099b..b214a059 100644 --- a/frontend/src/components/ui/AppsPage/AppCard.tsx +++ b/frontend/src/components/ui/AppsPage/AppCard.tsx @@ -8,6 +8,7 @@ import { HiOutlinePlay, HiOutlineTrash } from 'react-icons/hi'; +import { FaUsers, FaUsersSlash } from 'react-icons/fa6'; import AppInfoDialog from '@/components/ui/AppsPage/AppInfoDialog'; import FgIcon from '@/components/designSystem/atoms/FgIcon'; @@ -45,6 +46,7 @@ export default function AppCard({ }: AppCardProps) { const navigate = useNavigate(); const [infoOpen, setInfoOpen] = useState(false); + const [startInShareView, setStartInShareView] = useState(false); const isShared = app.listing_id !== undefined && app.listing_id !== null; @@ -52,6 +54,16 @@ export default function AppCard({ navigate(buildLaunchPathFromApp(app.url, app.manifest_path)); }; + const handleInfo = () => { + setStartInShareView(false); + setInfoOpen(true); + }; + + const handleShare = () => { + setStartInShareView(true); + setInfoOpen(true); + }; + return (
@@ -65,13 +77,38 @@ export default function AppCard({ setInfoOpen(true)} + onClick={handleInfo} size="sm" variant="ghost" > + {isShared ? ( + + + + + + ) : ( + + + + + + )} diff --git a/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx b/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx index 773e7217..66cb0968 100644 --- a/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx +++ b/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx @@ -12,6 +12,19 @@ import type { UserApp } from '@/shared.types'; import FgButton from '@/components/designSystem/atoms/FgButton'; import FgExternalLink from '@/components/designSystem/atoms/FgExternalLink'; import FgTooltip from '@/components/ui/widgets/FgTooltip'; +import { parseGithubUrl } from '@/utils/appUrls'; + +/** + * The revision actually cloned, parsed out of the canonical app URL (which + * always carries it). Falls back to the requested branch, then null. + */ +function appRevision(app: UserApp): string | null { + try { + return parseGithubUrl(app.url).branch; + } catch { + return app.branch || null; + } +} interface AppInfoDialogProps { readonly app: UserApp; @@ -31,12 +44,14 @@ interface AppInfoDialogProps { readonly removing: boolean; readonly sharing: boolean; readonly unsharing: boolean; + readonly startInShareView?: boolean; } function AppInfoTable({ app }: { readonly app: UserApp }) { const labelClass = 'text-foreground font-medium pr-4 py-1.5 align-top whitespace-nowrap'; const valueClass = 'text-foreground py-1.5'; + const revision = appRevision(app); return ( @@ -49,10 +64,10 @@ function AppInfoTable({ app }: { readonly app: UserApp }) { - {app.branch ? ( + {revision ? ( - - + + ) : null} {app.description ? ( @@ -86,7 +101,8 @@ export default function AppInfoDialog({ updating, removing, sharing, - unsharing + unsharing, + startInShareView = false }: AppInfoDialogProps) { const isShared = app.listing_id !== undefined && app.listing_id !== null; @@ -97,13 +113,6 @@ export default function AppInfoDialog({ const [description, setDescription] = useState(''); const [shareError, setShareError] = useState(''); - // Always return to the info view when the dialog is (re)opened. - useEffect(() => { - if (open) { - setShareView(false); - } - }, [open]); - const openShareForm = () => { setName(app.name); setDescription(app.description ?? ''); @@ -111,6 +120,19 @@ export default function AppInfoDialog({ setShareView(true); }; + // Open into the share form when requested (and shareable), otherwise always + // return to the info view when the dialog is (re)opened. + useEffect(() => { + if (open) { + if (startInShareView && !isShared) { + openShareForm(); + } else { + setShareView(false); + } + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [open]); + const handleShareSubmit = async () => { if (!name.trim()) { setShareError('Name is required'); diff --git a/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx b/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx index abfd921b..5dce41d5 100644 --- a/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx +++ b/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx @@ -64,6 +64,10 @@ interface AppLaunchFormProps { readonly initialPostRun?: string; readonly initialContainer?: string; readonly initialContainerArgs?: string; + // Display name for the app; defaults to the manifest name when omitted. Lets + // callers show a user's custom app name (e.g. chosen when adding from the + // catalog) instead of the raw manifest name. + readonly appName?: string; } type EnvVar = { key: string; value: string }; @@ -722,7 +726,8 @@ export default function AppLaunchForm({ initialPreRun, initialPostRun, initialContainer, - initialContainerArgs + initialContainerArgs, + appName }: AppLaunchFormProps) { const { defaultExtraArgs } = usePreferencesContext(); const { zonesAndFspQuery } = useZoneAndFspMapContext(); @@ -1245,7 +1250,7 @@ export default function AppLaunchForm({ {entryPoint.name} - {manifest.name} + {appName ?? manifest.name} {actionButtons} @@ -1274,7 +1279,11 @@ export default function AppLaunchForm({
- {hasSections ? ( + {allParams.length === 0 ? ( + + There are no parameters to set for this app. + + ) : hasSections ? ( @@ -39,10 +53,10 @@ function ListingInfoTable({ listing }: { readonly listing: AppListing }) { - {listing.branch ? ( + {revision ? (
- - + + ) : null} diff --git a/frontend/src/components/ui/Dialogs/CancelJob.tsx b/frontend/src/components/ui/Dialogs/CancelJob.tsx new file mode 100644 index 00000000..c5607a9b --- /dev/null +++ b/frontend/src/components/ui/Dialogs/CancelJob.tsx @@ -0,0 +1,49 @@ +import { Typography } from '@material-tailwind/react'; +import { HiOutlineStop } from 'react-icons/hi'; + +import FgButton from '@/components/designSystem/atoms/FgButton'; +import FgDialog from '@/components/ui/Dialogs/FgDialog'; + +type CancelJobDialogProps = { + readonly open: boolean; + readonly isService: boolean; + readonly isPending: boolean; + readonly onClose: () => void; + readonly onConfirm: () => void; +}; + +export default function CancelJobDialog({ + open, + isService, + isPending, + onClose, + onConfirm +}: CancelJobDialogProps) { + return ( + + + {isService ? 'Stop Service' : 'Cancel Job'} + + + {isService + ? 'Are you sure you want to stop this service? It will be terminated and the URL will no longer be accessible.' + : 'Are you sure you want to cancel this job? It will be terminated.'} + +
+ + Keep running + + + {isService ? 'Stop Service' : 'Cancel Job'} + +
+
+ ); +} diff --git a/frontend/src/components/ui/Table/appsJobsColumns.tsx b/frontend/src/components/ui/Table/appsJobsColumns.tsx index cdcba5a5..69f3ea54 100644 --- a/frontend/src/components/ui/Table/appsJobsColumns.tsx +++ b/frontend/src/components/ui/Table/appsJobsColumns.tsx @@ -37,7 +37,7 @@ function formatDuration(job: Job): string { type JobActionCallbacks = { onViewDetail: (jobId: number) => void; onRelaunch: (job: Job) => void; - onCancel: (jobId: number) => void; + onCancel: (job: Job) => void; onDelete: (jobId: number) => void; }; @@ -160,7 +160,7 @@ export function createAppsJobsColumns( }, { name: isService ? 'Stop Service' : 'Cancel', - action: props => props.onCancel(props.job.id), + action: props => props.onCancel(props.job), shouldShow: canCancel }, { diff --git a/frontend/src/utils/appUrls.ts b/frontend/src/utils/appUrls.ts index 17aa7abe..761cbe8e 100644 --- a/frontend/src/utils/appUrls.ts +++ b/frontend/src/utils/appUrls.ts @@ -5,21 +5,70 @@ const GITHUB_URL_PATTERN = /^https?:\/\/github\.com\/([^/]+)\/([^/]+?)(?:\.git)?(?:\/tree\/(.+?))?\/?$/; +// SSH forms: git@github.com:owner/repo(.git) and ssh://git@github.com/owner/repo(.git). +// SSH URLs don't carry a branch, so callers supply the revision separately. +const GITHUB_SSH_SCP_PATTERN = + /^git@github\.com:([^/]+)\/([^/]+?)(?:\.git)?\/?$/; +const GITHUB_SSH_PROTO_PATTERN = + /^ssh:\/\/git@github\.com\/([^/]+)\/([^/]+?)(?:\.git)?\/?$/; export function parseGithubUrl(url: string): { owner: string; repo: string; branch: string; } { - const match = url.match(GITHUB_URL_PATTERN); - if (!match) { - throw new Error(`Invalid GitHub URL: ${url}`); + const trimmed = url.trim(); + const httpsMatch = trimmed.match(GITHUB_URL_PATTERN); + if (httpsMatch) { + return { + owner: httpsMatch[1], + repo: httpsMatch[2], + branch: httpsMatch[3] || 'main' + }; } - return { - owner: match[1], - repo: match[2], - branch: match[3] || 'main' - }; + const sshMatch = + trimmed.match(GITHUB_SSH_SCP_PATTERN) ?? + trimmed.match(GITHUB_SSH_PROTO_PATTERN); + if (sshMatch) { + return { owner: sshMatch[1], repo: sshMatch[2], branch: 'main' }; + } + throw new Error(`Invalid GitHub URL: ${url}`); +} + +/** + * Normalize a GitHub URL to its canonical https form (no ".git" suffix, no + * trailing slash, no redundant "/tree/main"). Returns the input unchanged if it + * can't be parsed. Use this to compare URLs by repo identity rather than exact + * string, since stored app URLs may carry any of those cosmetic variations. + */ +export function canonicalGithubUrl(url: string): string { + try { + const { owner, repo, branch } = parseGithubUrl(url); + return buildGithubUrl(owner, repo, branch); + } catch { + return url; + } +} + +/** True if the string parses as a GitHub repo URL (HTTPS or SSH). */ +export function isGithubRepoUrl(url: string): boolean { + try { + parseGithubUrl(url); + return true; + } catch { + return false; + } +} + +/** + * Combine a user-entered repo URL (HTTPS or SSH) and an optional revision into + * a canonical https GitHub app URL. The revision overrides any branch in the + * URL; when empty, the URL's own branch (or the default) is kept. Throws if the + * repo URL can't be parsed. + */ +export function buildAppUrl(repoUrl: string, revision: string): string { + const { owner, repo, branch } = parseGithubUrl(repoUrl); + return buildGithubUrl(owner, repo, revision.trim() || branch); } export function buildGithubUrl( diff --git a/frontend/src/utils/index.ts b/frontend/src/utils/index.ts index bc65b08e..748ea97f 100644 --- a/frontend/src/utils/index.ts +++ b/frontend/src/utils/index.ts @@ -393,6 +393,9 @@ export type { // Re-export app URL utilities export { parseGithubUrl, + isGithubRepoUrl, + canonicalGithubUrl, + buildAppUrl, buildGithubUrl, buildLaunchPath, buildLaunchPathFromApp, diff --git a/tests/test_apps.py b/tests/test_apps.py index 4c100314..4ccc35a2 100644 --- a/tests/test_apps.py +++ b/tests/test_apps.py @@ -1063,6 +1063,8 @@ async def test_timeout_covers_command_runtime(self): validate_manifest_path, _safe_repo_subdir, _parse_github_url, + _is_git_auth_error, + _clone_repo, get_app_branch, ) @@ -1074,6 +1076,248 @@ def test_branch_name_may_contain_slashes(self): ) assert (owner, repo, branch) == ("org", "tool", "feature/my-tool") + @pytest.mark.parametrize( + "url", + [ + "git@github.com:org/tool.git", + "git@github.com:org/tool", + "ssh://git@github.com/org/tool.git", + "ssh://git@github.com/org/tool", + ], + ) + def test_parses_ssh_urls(self, url): + assert _parse_github_url(url) == ("org", "tool", None) + + +class TestCanonicalGithubUrl: + @pytest.mark.parametrize( + "url", + [ + "https://github.com/Org/Repo", + "https://github.com/Org/Repo.git", + "https://github.com/Org/Repo/", + "https://github.com/Org/Repo/tree/main", + "git@github.com:Org/Repo.git", + "ssh://git@github.com/Org/Repo", + ], + ) + def test_cosmetic_variations_canonicalize_identically(self, url): + from fileglancer.giturls import canonical_github_url + + assert canonical_github_url(url) == "https://github.com/Org/Repo" + + def test_non_default_branch_preserved(self): + from fileglancer.giturls import canonical_github_url + + assert ( + canonical_github_url("https://github.com/Org/Repo/tree/dev") + == "https://github.com/Org/Repo/tree/dev" + ) + + def test_unparseable_returned_unchanged(self): + from fileglancer.giturls import canonical_github_url + + assert canonical_github_url("not a url") == "not a url" + + @pytest.mark.parametrize( + "url", + [ + "https://gitlab.com/org/tool", + "git@gitlab.com:org/tool.git", + "not a url", + ], + ) + def test_rejects_non_github_urls(self, url): + with pytest.raises(ValueError): + _parse_github_url(url) + + +class TestGithubUrlAtBranch: + def test_default_branch_folds_to_bare(self): + from fileglancer.giturls import github_url_at_branch + + assert ( + github_url_at_branch("Org", "Repo", "main") + == "https://github.com/Org/Repo" + ) + + def test_non_default_branch_is_explicit(self): + from fileglancer.giturls import github_url_at_branch + + assert ( + github_url_at_branch("Org", "Repo", "master") + == "https://github.com/Org/Repo/tree/master" + ) + + +class TestCloneUrlForStoredApp: + def test_bare_stored_url_means_fixed_main(self): + from fileglancer.apps import clone_url_for_stored_app + + # A pinned app (branch recorded, "" = took the default which was main). + assert ( + clone_url_for_stored_app("https://github.com/Org/Repo", "") + == "https://github.com/Org/Repo/tree/main" + ) + + def test_non_main_revision_stays_explicit(self): + from fileglancer.apps import clone_url_for_stored_app + + assert ( + clone_url_for_stored_app("https://github.com/Org/Repo/tree/master", "master") + == "https://github.com/Org/Repo/tree/master" + ) + + def test_null_branch_legacy_row_tracks_default(self): + from fileglancer.apps import clone_url_for_stored_app + + # branch is None: a legacy row with an unknown default — return the URL + # unchanged so git resolves the current default, rather than guessing + # "main" and breaking a repo that defaults to e.g. "master". + assert ( + clone_url_for_stored_app("https://github.com/Org/Repo", None) + == "https://github.com/Org/Repo" + ) + + +class TestCloneFallback: + def test_auth_error_detection(self): + assert _is_git_auth_error( + "fatal: could not read Username for 'https://github.com': " + "terminal prompts disabled" + ) + assert _is_git_auth_error("remote: Repository not found.") + assert not _is_git_auth_error("fatal: Remote branch v9 not found") + + @pytest.mark.asyncio + async def test_falls_back_to_ssh_on_https_auth_error(self, tmp_path, monkeypatch): + from fileglancer.apps import manifest as m + + calls = [] + + async def fake_run_git(args, timeout=60, extra_env=None): + calls.append((args, extra_env)) + if "https://github.com/org/tool.git" in args: + raise ValueError( + "Git command failed: fatal: could not read Username for " + "'https://github.com': terminal prompts disabled" + ) + return (b"", b"") + + monkeypatch.setattr(m, "_run_git", fake_run_git) + monkeypatch.setattr(m.shutil, "rmtree", lambda *a, **k: None) + + await _clone_repo("org", "tool", "v0.1.0", tmp_path / "dest") + + used = [args for args, _ in calls] + assert any("https://github.com/org/tool.git" in a for a in used) + # The SSH retry was attempted with the SSH-specific environment. + ssh_call = next( + (env for args, env in calls if "git@github.com:org/tool.git" in args), + None, + ) + assert ssh_call is not None and "GIT_SSH_COMMAND" in ssh_call + + @pytest.mark.asyncio + async def test_raises_understandable_error_when_both_fail(self, tmp_path, monkeypatch): + from fileglancer.apps import manifest as m + + async def fake_run_git(args, timeout=60, extra_env=None): + raise ValueError( + "Git command failed: fatal: could not read Username for " + "'https://github.com': terminal prompts disabled" + ) + + monkeypatch.setattr(m, "_run_git", fake_run_git) + monkeypatch.setattr(m.shutil, "rmtree", lambda *a, **k: None) + + with pytest.raises(ValueError, match="Could not access the repository org/tool"): + await _clone_repo("org", "tool", "v0.1.0", tmp_path / "dest") + + @pytest.mark.asyncio + async def test_https_ref_not_found_is_not_retried(self, tmp_path, monkeypatch): + from fileglancer.apps import manifest as m + + calls = [] + + async def fake_run_git(args, timeout=60, extra_env=None): + calls.append(args) + raise ValueError( + "Git command failed: fatal: Remote branch v9 not found in upstream origin" + ) + + monkeypatch.setattr(m, "_run_git", fake_run_git) + monkeypatch.setattr(m.shutil, "rmtree", lambda *a, **k: None) + + with pytest.raises(ValueError, match="Revision 'v9' was not found"): + await _clone_repo("org", "tool", "v9", tmp_path / "dest") + # A reachable remote with a missing ref is authoritative — no SSH retry. + assert len(calls) == 1 + + @pytest.mark.asyncio + async def test_ssh_ref_not_found_reports_revision_not_access(self, tmp_path, monkeypatch): + """Private repo + mistyped tag: HTTPS auth-fails, SSH reaches the repo but + the revision is missing — report the revision, not a misleading + 'can't access / maybe private' message.""" + from fileglancer.apps import manifest as m + + async def fake_run_git(args, timeout=60, extra_env=None): + if "https://github.com/org/tool.git" in args: + raise ValueError( + "Git command failed: fatal: could not read Username for " + "'https://github.com': terminal prompts disabled" + ) + raise ValueError( + "Git command failed: fatal: Remote branch v1.0.1 not found in upstream origin" + ) + + monkeypatch.setattr(m, "_run_git", fake_run_git) + monkeypatch.setattr(m.shutil, "rmtree", lambda *a, **k: None) + + with pytest.raises(ValueError, match="Revision 'v1.0.1' was not found") as exc: + await _clone_repo("org", "tool", "v1.0.1", tmp_path / "dest") + assert "private" not in str(exc.value).lower() + + def test_ref_not_found_detection(self): + from fileglancer.apps.manifest import _is_git_ref_not_found + + assert _is_git_ref_not_found( + "fatal: Remote branch v1.0.1 not found in upstream origin" + ) + assert _is_git_ref_not_found("fatal: could not find remote ref refs/tags/v9") + assert not _is_git_ref_not_found( + "fatal: could not read Username for 'https://github.com'" + ) + + @pytest.mark.asyncio + async def test_pull_resets_to_fetch_head_not_origin_branch(self, tmp_path, monkeypatch): + """A cached tag/SHA has no origin/ tracking branch, so the pull must + reset to FETCH_HEAD (works for branches, tags and SHAs).""" + from fileglancer.apps import manifest as m + + monkeypatch.setattr(m, "_repo_cache_base", lambda username=None: tmp_path) + # Pre-create the cache dir so _ensure_repo_cache takes the pull branch. + repo_dir = tmp_path / "org" / "tool" / "v0.1.0" + repo_dir.mkdir(parents=True) + + calls = [] + + async def fake_run_git(args, timeout=60, extra_env=None): + calls.append(args) + return (b"", b"") + + monkeypatch.setattr(m, "_run_git", fake_run_git) + + await m._ensure_repo_cache( + "https://github.com/org/tool/tree/v0.1.0", pull=True + ) + + reset_calls = [a for a in calls if "reset" in a] + assert reset_calls, "expected a reset during pull" + # Must reset to FETCH_HEAD, never origin/. + assert "FETCH_HEAD" in reset_calls[0] + assert not any("origin/v0.1.0" in a for a in reset_calls) + @pytest.mark.asyncio async def test_get_app_branch_returns_slash_branch_without_remote_lookup(self): branch = await get_app_branch( @@ -1501,3 +1745,42 @@ def test_env_not_emitted_as_flags(self): def test_no_env_leaves_env_none(self): ep = _task_to_entry_point("build", {"cmd": "make"}) assert ep.env is None + + +class TestPixiAdapterName: + """The generated app name should come from the pixi project's name, not a + repo/branch combination (which produced ugly names like 'repo/HEAD').""" + + def _write_pixi(self, tmp_path, body: str): + (tmp_path / "pixi.toml").write_text(body) + + def test_uses_project_name_from_pixi_toml(self, tmp_path): + from fileglancer.apps.pixi import PixiAdapter + + self._write_pixi( + tmp_path, + '[project]\nname = "SmartSPIM"\n\n[tasks]\nrun = "echo hi"\n', + ) + manifest = PixiAdapter().convert(tmp_path) + assert manifest.name == "SmartSPIM" + + def test_falls_back_to_git_repo_name(self, tmp_path, monkeypatch): + from fileglancer.apps import pixi as pixi_mod + from fileglancer.apps.pixi import PixiAdapter + + # pixi.toml without a project name + self._write_pixi(tmp_path, '[tasks]\nrun = "echo hi"\n') + monkeypatch.setattr( + pixi_mod, "_get_git_repo_name", lambda d: "SmartSPIMGlancer" + ) + manifest = PixiAdapter().convert(tmp_path) + assert manifest.name == "SmartSPIMGlancer" + + def test_falls_back_to_directory_name(self, tmp_path, monkeypatch): + from fileglancer.apps import pixi as pixi_mod + from fileglancer.apps.pixi import PixiAdapter + + self._write_pixi(tmp_path, '[tasks]\nrun = "echo hi"\n') + monkeypatch.setattr(pixi_mod, "_get_git_repo_name", lambda d: None) + manifest = PixiAdapter().convert(tmp_path) + assert manifest.name == tmp_path.name diff --git a/tests/test_apps_endpoints.py b/tests/test_apps_endpoints.py index d0bd2ba5..9c62b1a9 100644 --- a/tests/test_apps_endpoints.py +++ b/tests/test_apps_endpoints.py @@ -23,6 +23,8 @@ get_job, update_job_status, list_user_apps, + upsert_user_app, + get_user_app, ) from fileglancer.model import AppEntryPoint, AppManifest @@ -133,6 +135,27 @@ def _seed_job(db_session, *, status="DONE"): return job +def test_url_normalized_on_write_and_lookup(db_session): + """Stored app URLs are canonicalized on write, and lookups by any cosmetic + variant (.git, trailing slash, /tree/main) resolve to the same row.""" + row = upsert_user_app( + db_session, TEST_USERNAME, + url="https://github.com/owner/repo.git", manifest_path="", + name="Demo", + ) + assert row.url == "https://github.com/owner/repo" + + for variant in ( + "https://github.com/owner/repo", + "https://github.com/owner/repo.git", + "https://github.com/owner/repo/", + "https://github.com/owner/repo/tree/main", + "git@github.com:owner/repo.git", + ): + found = get_user_app(db_session, TEST_USERNAME, variant, "") + assert found is not None and found.id == row.id, variant + + def test_get_apps_empty(test_client): response = test_client.get("/api/apps") assert response.status_code == 200 @@ -161,23 +184,22 @@ def test_get_apps_uses_db_cache(test_client, db_session): def test_get_apps_backfills_null_manifest(test_client, db_session): - _seed_app(db_session, manifest=None, branch=None, name="Stale Name") + _seed_app(db_session, url="https://github.com/owner/repo/tree/dev", + manifest=None, branch="dev", name="Stale Name") manifest = _make_manifest(name="Fresh Name", description="Fresh") - # refresh_cached_manifest calls these directly inside apps/core.py, so - # patches must target the core namespace, not the apps re-export. + # refresh_cached_manifest calls fetch_app_manifest directly inside + # apps/manifest.py, so patch the manifest namespace, not the apps re-export. with patch("fileglancer.apps.manifest.fetch_app_manifest", - new=AsyncMock(return_value=manifest)) as mock_fetch, \ - patch("fileglancer.apps.manifest.get_app_branch", - new=AsyncMock(return_value="dev")) as mock_branch: + new=AsyncMock(return_value=manifest)) as mock_fetch: response = test_client.get("/api/apps") assert response.status_code == 200 assert mock_fetch.await_count == 1 - assert mock_branch.await_count == 1 body = response.json() assert body[0]["name"] == "Fresh Name" + # The backfill only fills the manifest; the requested revision is preserved. assert body[0]["branch"] == "dev" assert body[0]["manifest"]["name"] == "Fresh Name" @@ -228,11 +250,11 @@ def test_get_apps_backfill_handles_fetch_failure(test_client, db_session): def test_add_app_persists_manifest_and_branch(test_client, db_session): + """A bare URL is unpinned: branch is "" and the resolved default (main) folds + to the bare canonical URL.""" manifest = _make_manifest(name="From Add") with patch("fileglancer.apps.discover_app_manifests", - new=AsyncMock(return_value=[("", manifest)])), \ - patch("fileglancer.apps.get_app_branch", - new=AsyncMock(return_value="main")): + new=AsyncMock(return_value=("main", [("", manifest)]))): response = test_client.post( "/api/apps", json={"url": "https://github.com/owner/repo"}, @@ -242,25 +264,103 @@ def test_add_app_persists_manifest_and_branch(test_client, db_session): body = response.json() assert len(body) == 1 assert body[0]["name"] == "From Add" - assert body[0]["branch"] == "main" + assert body[0]["branch"] == "" + assert body[0]["url"] == "https://github.com/owner/repo" assert body[0]["manifest"]["name"] == "From Add" rows = list_user_apps(db_session, TEST_USERNAME) assert len(rows) == 1 assert rows[0].manifest["name"] == "From Add" - assert rows[0].branch == "main" + assert rows[0].branch == "" + + +def test_add_app_bakes_resolved_default_into_url(test_client, db_session): + """A bare URL for a repo whose default is 'master' stores '/tree/master', so + it dedups against an explicit '/tree/master' add. branch stays "" (unpinned).""" + manifest = _make_manifest(name="Master Default") + with patch("fileglancer.apps.discover_app_manifests", + new=AsyncMock(return_value=("master", [("", manifest)]))): + response = test_client.post( + "/api/apps", + json={"url": "https://github.com/owner/repo"}, + ) + + assert response.status_code == 200 + body = response.json() + assert body[0]["url"] == "https://github.com/owner/repo/tree/master" + assert body[0]["branch"] == "" + + rows = list_user_apps(db_session, TEST_USERNAME) + assert len(rows) == 1 + assert rows[0].url == "https://github.com/owner/repo/tree/master" + assert rows[0].branch == "" + + +def test_add_uses_worker_resolved_branch_not_server(test_client, db_session): + """The branch is resolved in the worker (as the user), not the server. add() + must not call the server-side default-branch resolver — otherwise a private + repo's non-main default would be lost to the server's 'main' fallback.""" + manifest = _make_manifest(name="Private") + with patch("fileglancer.apps.discover_app_manifests", + new=AsyncMock(return_value=("develop", [("", manifest)]))), \ + patch("fileglancer.apps.manifest._resolve_default_branch", + new=AsyncMock(side_effect=AssertionError("server must not resolve"))): + response = test_client.post( + "/api/apps", + json={"url": "https://github.com/owner/private-repo"}, + ) + + assert response.status_code == 200 + rows = list_user_apps(db_session, TEST_USERNAME) + assert len(rows) == 1 + # The worker-resolved default (develop) is baked into the stored URL. + assert rows[0].url == "https://github.com/owner/private-repo/tree/develop" + assert rows[0].branch == "" + + +def test_add_app_pinned_revision_kept(test_client, db_session): + """An explicit '/tree/dev' URL is pinned: branch records 'dev'.""" + manifest = _make_manifest(name="Pinned") + with patch("fileglancer.apps.discover_app_manifests", + new=AsyncMock(return_value=("dev", [("", manifest)]))): + response = test_client.post( + "/api/apps", + json={"url": "https://github.com/owner/repo/tree/dev"}, + ) + + assert response.status_code == 200 + rows = list_user_apps(db_session, TEST_USERNAME) + assert len(rows) == 1 + assert rows[0].url == "https://github.com/owner/repo/tree/dev" + assert rows[0].branch == "dev" + + +def test_add_app_dedups_bare_against_resolved_default(test_client, db_session): + """The dedup-hole fix: a bare URL for a master-default repo matches an already + stored '/tree/master' row, so the add is a no-op (409).""" + manifest = _make_manifest() + _seed_app(db_session, url="https://github.com/owner/repo/tree/master", + branch="", manifest=manifest.model_dump(mode="json")) + + with patch("fileglancer.apps.discover_app_manifests", + new=AsyncMock(return_value=("master", [("", manifest)]))): + response = test_client.post( + "/api/apps", + json={"url": "https://github.com/owner/repo"}, + ) + + assert response.status_code == 409 + assert len(list_user_apps(db_session, TEST_USERNAME)) == 1 def test_add_app_dedups(test_client, db_session): """Adding the same repo twice returns 409 and inserts no new rows.""" manifest = _make_manifest() - _seed_app(db_session, url="https://github.com/owner/repo", + _seed_app(db_session, url="https://github.com/owner/repo", branch="", manifest=manifest.model_dump(mode="json")) with patch("fileglancer.apps.discover_app_manifests", - new=AsyncMock(return_value=[("", manifest)])), \ - patch("fileglancer.apps.get_app_branch", - new=AsyncMock(return_value="main")): + new=AsyncMock(return_value=("main", [("", manifest)]))): response = test_client.post( "/api/apps", json={"url": "https://github.com/owner/repo"}, @@ -278,8 +378,8 @@ def test_update_app_persists_manifest(test_client, db_session): with patch("fileglancer.apps.fetch_app_manifest", new=AsyncMock(return_value=fresh)), \ - patch("fileglancer.apps.get_app_branch", - new=AsyncMock(return_value="main")), \ + patch("fileglancer.apps.manifest._resolve_default_branch", + new=AsyncMock(side_effect=AssertionError("must not re-resolve"))), \ patch("fileglancer.apps._ensure_repo_cache", new=AsyncMock(return_value="/tmp/x")) as mock_ensure: response = test_client.post( @@ -293,13 +393,16 @@ def test_update_app_persists_manifest(test_client, db_session): "pull": True, "username": TEST_USERNAME, } - assert mock_ensure.await_args.args == ("https://github.com/owner/repo",) + # A stored bare URL means the fixed "main" revision, not current default. + assert mock_ensure.await_args.args == ("https://github.com/owner/repo/tree/main",) body = response.json() + assert body["url"] == "https://github.com/owner/repo" assert body["name"] == "New" assert body["updated_at"] is not None rows = list_user_apps(db_session, TEST_USERNAME) assert len(rows) == 1 + assert rows[0].url == "https://github.com/owner/repo" assert rows[0].name == "New" assert rows[0].manifest["name"] == "New" assert rows[0].updated_at is not None @@ -320,8 +423,8 @@ def test_update_app_pulls_separate_code_repo(test_client, db_session): with patch("fileglancer.apps.fetch_app_manifest", new=AsyncMock(return_value=fresh)), \ - patch("fileglancer.apps.get_app_branch", - new=AsyncMock(return_value="main")), \ + patch("fileglancer.apps.manifest._resolve_default_branch", + new=AsyncMock(side_effect=AssertionError("must not re-resolve"))), \ patch("fileglancer.apps._ensure_repo_cache", new=AsyncMock(return_value="/tmp/x")) as mock_ensure: response = test_client.post( @@ -332,7 +435,7 @@ def test_update_app_pulls_separate_code_repo(test_client, db_session): assert response.status_code == 200 assert mock_ensure.await_count == 2 first_call, second_call = mock_ensure.await_args_list - assert first_call.args == ("https://github.com/owner/repo",) + assert first_call.args == ("https://github.com/owner/repo/tree/main",) assert first_call.kwargs == {"pull": True, "username": TEST_USERNAME} assert second_call.args == ("https://github.com/tools/code",) assert second_call.kwargs == {"pull": True, "username": TEST_USERNAME} @@ -341,6 +444,83 @@ def test_update_app_pulls_separate_code_repo(test_client, db_session): assert rows[0].manifest["repo_url"] == "https://github.com/tools/code" +def test_update_app_does_not_pull_same_repo_with_cosmetic_url(test_client, db_session): + """A manifest repo_url that canonicalizes to the stored app URL is the same + repo, even when the operational clone URL had to make /tree/main explicit.""" + _seed_app(db_session, url="https://github.com/owner/repo", branch="", + manifest=None, name="Old") + + fresh = AppManifest( + name="New", + description="New", + repo_url="https://github.com/owner/repo/tree/main", + runnables=[AppEntryPoint(id="run", name="Run", command="echo hi")], + ) + + with patch("fileglancer.apps.fetch_app_manifest", + new=AsyncMock(return_value=fresh)), \ + patch("fileglancer.apps.manifest._resolve_default_branch", + new=AsyncMock(side_effect=AssertionError("must not re-resolve"))), \ + patch("fileglancer.apps._ensure_repo_cache", + new=AsyncMock(return_value="/tmp/x")) as mock_ensure: + response = test_client.post( + "/api/apps/update", + json={"url": "https://github.com/owner/repo", "manifest_path": ""}, + ) + + assert response.status_code == 200 + assert mock_ensure.await_count == 1 + assert mock_ensure.await_args.args == ( + "https://github.com/owner/repo/tree/main", + ) + + +@pytest.mark.parametrize( + "stored_url,stored_branch,clone_url", + [ + # Stored bare URL is the fixed main revision; operational git calls make + # that explicit so they don't follow a moved default branch. + ("https://github.com/owner/repo", "", "https://github.com/owner/repo/tree/main"), + # Unpinned app pinned to master at add time. + ("https://github.com/owner/repo/tree/master", "", "https://github.com/owner/repo/tree/master"), + # Explicitly pinned app. + ("https://github.com/owner/repo/tree/dev", "dev", "https://github.com/owner/repo/tree/dev"), + ], +) +def test_update_pulls_stored_revision_and_never_re_resolves( + test_client, db_session, stored_url, stored_branch, clone_url +): + """The revision is fixed at add time: update re-pulls the stored URL as-is, + never re-resolving the default branch or moving the app to a new URL.""" + _seed_app(db_session, url=stored_url, branch=stored_branch, + manifest=None, name="Old") + fresh = _make_manifest(name="New") + + with patch("fileglancer.apps.fetch_app_manifest", + new=AsyncMock(return_value=fresh)), \ + patch("fileglancer.apps.manifest._resolve_default_branch", + new=AsyncMock(side_effect=AssertionError("must not re-resolve"))), \ + patch("fileglancer.apps._ensure_repo_cache", + new=AsyncMock(return_value="/tmp/x")) as mock_ensure: + response = test_client.post( + "/api/apps/update", + json={"url": stored_url, "manifest_path": ""}, + ) + + assert response.status_code == 200 + assert mock_ensure.await_args_list[0].args == (clone_url,) + body = response.json() + assert body["url"] == stored_url + assert body["branch"] == stored_branch + assert body["manifest"]["name"] == "New" + + rows = list_user_apps(db_session, TEST_USERNAME) + assert len(rows) == 1 + assert rows[0].url == stored_url + # The revision fixed at add time is preserved. + assert rows[0].branch == stored_branch + + def test_delete_app_removes_row(test_client, db_session): _seed_app(db_session, manifest=_make_manifest().model_dump(mode="json")) assert len(list_user_apps(db_session, TEST_USERNAME)) == 1 @@ -420,14 +600,13 @@ def test_fetch_manifest_reads_disk_for_uninstalled(test_client, db_session): def test_fetch_manifest_backfills_null_cache(test_client, db_session): - """If row exists with NULL manifest, endpoint reads disk and writes back.""" - _seed_app(db_session, manifest=None, name="Stale", branch=None) + """If a pinned row has a NULL manifest, the endpoint reads disk and writes + back, fetching the pinned (explicit) URL.""" + _seed_app(db_session, manifest=None, name="Stale", branch="") fresh = _make_manifest(name="Backfilled") with patch("fileglancer.apps.manifest.fetch_app_manifest", - new=AsyncMock(return_value=fresh)) as mock_fetch, \ - patch("fileglancer.apps.manifest.get_app_branch", - new=AsyncMock(return_value="main")): + new=AsyncMock(return_value=fresh)) as mock_fetch: response = test_client.post("/api/apps/manifest", json={ "url": "https://github.com/owner/repo", "manifest_path": "", @@ -435,8 +614,30 @@ def test_fetch_manifest_backfills_null_cache(test_client, db_session): assert response.status_code == 200 assert mock_fetch.await_count == 1 + # branch="" is a pinned "main", so the fetch URL is made explicit. + assert mock_fetch.await_args.args == ( + "https://github.com/owner/repo/tree/main", + "", + ) assert response.json()["name"] == "Backfilled" + +def test_fetch_manifest_legacy_null_branch_tracks_default(test_client, db_session): + """A legacy row with NULL branch (unknown default) is fetched with its stored + bare URL — git resolves the default — rather than being pinned to main.""" + _seed_app(db_session, manifest=None, name="Legacy", branch=None) + fresh = _make_manifest(name="Backfilled") + + with patch("fileglancer.apps.manifest.fetch_app_manifest", + new=AsyncMock(return_value=fresh)) as mock_fetch: + response = test_client.post("/api/apps/manifest", json={ + "url": "https://github.com/owner/repo", + "manifest_path": "", + }) + + assert response.status_code == 200 + assert mock_fetch.await_args.args == ("https://github.com/owner/repo", "") + # Row was updated silently (updated_at stays NULL). rows = list_user_apps(db_session, TEST_USERNAME) assert len(rows) == 1 @@ -484,23 +685,28 @@ async def test_refresh_cached_manifest_syncs_existing_row(test_app, db_session): """refresh_cached_manifest updates an existing row from disk.""" from fileglancer.apps import refresh_cached_manifest - _seed_app(db_session, manifest=None, name="Stale") + _seed_app(db_session, url="https://github.com/owner/repo/tree/dev", + manifest=None, name="Stale", branch="dev") fresh = _make_manifest(name="Synced") with patch("fileglancer.apps.manifest.fetch_app_manifest", - new=AsyncMock(return_value=fresh)), \ - patch("fileglancer.apps.manifest.get_app_branch", - new=AsyncMock(return_value="main")): - manifest, branch = await refresh_cached_manifest( - TEST_USERNAME, "https://github.com/owner/repo", "", + new=AsyncMock(return_value=fresh)) as mock_fetch, \ + patch("fileglancer.apps.manifest._resolve_default_branch", + new=AsyncMock(side_effect=AssertionError("must not re-resolve"))): + manifest = await refresh_cached_manifest( + TEST_USERNAME, "https://github.com/owner/repo/tree/dev", "", ) assert manifest.name == "Synced" - assert branch == "main" + assert mock_fetch.await_args.args == ( + "https://github.com/owner/repo/tree/dev", + "", + ) rows = list_user_apps(db_session, TEST_USERNAME) assert rows[0].manifest["name"] == "Synced" - assert rows[0].branch == "main" + # A cache refresh leaves the requested revision (branch) untouched. + assert rows[0].branch == "dev" # Silent refresh by default — updated_at stays NULL. assert rows[0].updated_at is None @@ -512,10 +718,8 @@ async def test_refresh_cached_manifest_no_op_for_uninstalled(test_app, db_sessio fresh = _make_manifest() with patch("fileglancer.apps.manifest.fetch_app_manifest", - new=AsyncMock(return_value=fresh)), \ - patch("fileglancer.apps.manifest.get_app_branch", - new=AsyncMock(return_value="main")): - manifest, branch = await refresh_cached_manifest( + new=AsyncMock(return_value=fresh)): + manifest = await refresh_cached_manifest( TEST_USERNAME, "https://github.com/new/repo", "", ) @@ -532,9 +736,7 @@ async def test_refresh_cached_manifest_bumps_updated_at(test_app, db_session): fresh = _make_manifest(name="Updated") with patch("fileglancer.apps.manifest.fetch_app_manifest", - new=AsyncMock(return_value=fresh)), \ - patch("fileglancer.apps.manifest.get_app_branch", - new=AsyncMock(return_value="main")): + new=AsyncMock(return_value=fresh)): await refresh_cached_manifest( TEST_USERNAME, "https://github.com/owner/repo", "", bump_updated_at=True, diff --git a/tests/test_catalog_endpoints.py b/tests/test_catalog_endpoints.py index cad149c7..ac1d0cc3 100644 --- a/tests/test_catalog_endpoints.py +++ b/tests/test_catalog_endpoints.py @@ -337,31 +337,38 @@ def test_delete_listing_rejects_non_owner(client_factory, db_session): def test_add_from_listing_creates_independent_user_app( client_factory, db_session ): - """Adopter cloning the listing gets their own user_app row with a fresh - manifest fetched from disk — not the listing's snapshot (there is none).""" - listing = _seed_listing(db_session, owner_username=OWNER) + """Adopter cloning the listing gets their own user_app row that preserves + the listing's custom name/description, while the manifest itself is freshly + fetched from disk.""" + listing = _seed_listing( + db_session, owner_username=OWNER, + name="Custom Name", description="Custom description", + ) fresh = _make_manifest(name="Fetched", description="From disk") client = client_factory(ADOPTER) with patch( "fileglancer.apps.fetch_app_manifest", new=AsyncMock(return_value=fresh), - ), patch( - "fileglancer.apps.get_app_branch", - new=AsyncMock(return_value="main"), - ): + ) as mock_fetch: response = client.post(f"/api/catalog/{listing.id}/add") assert response.status_code == 200 + assert mock_fetch.await_args.args == ( + "https://github.com/owner/repo/tree/main", + "", + ) body = response.json() - assert body["name"] == "Fetched" - assert body["description"] == "From disk" + assert body["name"] == "Custom Name" + assert body["description"] == "Custom description" assert body["branch"] == "main" assert body["manifest"]["name"] == "Fetched" rows = list_user_apps(db_session, ADOPTER) assert len(rows) == 1 assert rows[0].url == "https://github.com/owner/repo" + assert rows[0].name == "Custom Name" + assert rows[0].description == "Custom description" def test_add_from_listing_rejects_when_already_added( diff --git a/tests/test_revision_migration.py b/tests/test_revision_migration.py new file mode 100644 index 00000000..069abe14 --- /dev/null +++ b/tests/test_revision_migration.py @@ -0,0 +1,164 @@ +"""Tests for the c1f9a4e7b2d8 "bake revision into app URLs" migration.""" + +import importlib.util +from pathlib import Path + +import pytest +from sqlalchemy import create_engine, text + +from fileglancer.database import Base + + +_MIGRATION = ( + Path(__file__).resolve().parent.parent + / "fileglancer" / "alembic" / "versions" + / "c1f9a4e7b2d8_bake_revision_into_app_urls.py" +) + + +def _load_migration(): + spec = importlib.util.spec_from_file_location("_rev_mig", _MIGRATION) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +mig = _load_migration() + + +class _Row: + def __init__(self, url, branch): + self.url = url + self.branch = branch + + +@pytest.mark.parametrize( + "url,branch,expected_url,expected_requested", + [ + # Bare URL, default resolved to main -> stays bare, unpinned. + ("https://github.com/o/r", "main", "https://github.com/o/r", ""), + # Bare URL, default resolved to master -> revision baked in, unpinned. + ("https://github.com/o/r", "master", "https://github.com/o/r/tree/master", ""), + # Explicitly pinned branch -> URL unchanged, requested recorded. + ("https://github.com/o/r/tree/dev", "dev", "https://github.com/o/r/tree/dev", "dev"), + ], +) +def test_target(url, branch, expected_url, expected_requested): + assert mig._target(_Row(url, branch)) == (expected_url, expected_requested) + + +def test_target_unparseable_left_alone(): + assert mig._target(_Row("not a url", "main")) is None + + +def test_target_null_branch_left_alone(): + # Legacy rows with an unrecorded default are left untouched (not assumed main). + assert mig._target(_Row("https://github.com/o/r", None)) is None + + +@pytest.fixture +def engine(): + eng = create_engine("sqlite://") + Base.metadata.create_all(eng) + yield eng + eng.dispose() + + +def test_bakes_resolved_revision_and_flips_branch(engine): + with engine.begin() as conn: + conn.execute(text( + "INSERT INTO user_apps (username, url, branch, manifest_path, name, added_at) " + "VALUES " + # bare URL whose default was master -> bake /tree/master, unpinned + "('bob', 'https://github.com/o/master_repo', 'master', '', 'm', '2026-01-01')," + # bare URL whose default was main -> stays bare, unpinned + "('bob', 'https://github.com/o/main_repo', 'main', '', 'n', '2026-01-01')," + # explicit pin -> URL kept, branch records the pin + "('bob', 'https://github.com/o/pinned/tree/dev', 'dev', '', 'p', '2026-01-01')" + )) + + with engine.begin() as conn: + mig._migrate_unique_table(conn, "user_apps", "username") + + with engine.begin() as conn: + rows = conn.execute(text( + "SELECT name, url, branch FROM user_apps ORDER BY name" + )).fetchall() + + by_name = {r.name: (r.url, r.branch) for r in rows} + assert by_name["m"] == ("https://github.com/o/master_repo/tree/master", "") + assert by_name["n"] == ("https://github.com/o/main_repo", "") + assert by_name["p"] == ("https://github.com/o/pinned/tree/dev", "dev") + + +def test_null_branch_legacy_row_left_untouched(engine): + """A row with no recorded branch (migrated from user_preferences) keeps its + bare URL and NULL branch — it is not assumed to be 'main'.""" + with engine.begin() as conn: + conn.execute(text( + "INSERT INTO user_apps (username, url, branch, manifest_path, name, added_at) " + "VALUES ('bob', 'https://github.com/o/legacy', NULL, '', 'l', '2026-01-01')" + )) + + with engine.begin() as conn: + mig._migrate_unique_table(conn, "user_apps", "username") + + with engine.begin() as conn: + row = conn.execute(text( + "SELECT url, branch FROM user_apps WHERE name = 'l'" + )).fetchone() + + assert row.url == "https://github.com/o/legacy" + assert row.branch is None + + +def test_rewrite_colliding_with_null_branch_row_is_dropped(engine): + """A known row that bakes to a NULL-branch legacy row's URL is dropped + instead of tripping the table's unique constraint.""" + with engine.begin() as conn: + conn.execute(text( + "INSERT INTO user_apps (username, url, branch, manifest_path, name, added_at) " + "VALUES " + # Left untouched: explicit URL, but the legacy row has no recorded + # requested/resolved branch split. + "('bob', 'https://github.com/o/r/tree/master', NULL, '', 'legacy', '2026-01-01')," + # Would bake to the same URL as the legacy row. + "('bob', 'https://github.com/o/r', 'master', '', 'known', '2026-01-01')" + )) + + with engine.begin() as conn: + mig._migrate_unique_table(conn, "user_apps", "username") + + with engine.begin() as conn: + rows = conn.execute(text( + "SELECT name, url, branch FROM user_apps ORDER BY name" + )).fetchall() + + assert [(r.name, r.url, r.branch) for r in rows] == [ + ("legacy", "https://github.com/o/r/tree/master", None), + ] + + +def test_dedupes_bare_against_explicit_revision(engine): + """A bare master-default row collapses onto an explicit /tree/master row.""" + with engine.begin() as conn: + conn.execute(text( + "INSERT INTO user_apps (username, url, branch, manifest_path, name, added_at) " + "VALUES " + # already-baked explicit row (the winner) + "('bob', 'https://github.com/o/r/tree/master', 'master', '', 'canon', '2026-01-01')," + # bare row that bakes to the same canonical URL -> dropped + "('bob', 'https://github.com/o/r', 'master', '', 'dup', '2026-01-01')" + )) + + with engine.begin() as conn: + mig._migrate_unique_table(conn, "user_apps", "username") + + with engine.begin() as conn: + rows = conn.execute(text( + "SELECT name, url, branch FROM user_apps ORDER BY name" + )).fetchall() + + by_name = {r.name: (r.url, r.branch) for r in rows} + assert "dup" not in by_name + assert by_name["canon"] == ("https://github.com/o/r/tree/master", "master") diff --git a/tests/test_url_migration.py b/tests/test_url_migration.py new file mode 100644 index 00000000..ce972b99 --- /dev/null +++ b/tests/test_url_migration.py @@ -0,0 +1,84 @@ +"""Tests for the b8e4f1a92c37 GitHub-URL canonicalization migration.""" + +import importlib.util +from pathlib import Path + +import pytest +from sqlalchemy import create_engine, text + +from fileglancer.database import Base + + +_MIGRATION = ( + Path(__file__).resolve().parent.parent + / "fileglancer" / "alembic" / "versions" + / "b8e4f1a92c37_canonicalize_github_urls.py" +) + + +def _load_migration(): + spec = importlib.util.spec_from_file_location("_url_mig", _MIGRATION) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +mig = _load_migration() + + +@pytest.mark.parametrize( + "url,expected", + [ + ("https://github.com/Org/Repo", "https://github.com/Org/Repo"), + ("https://github.com/Org/Repo.git", "https://github.com/Org/Repo"), + ("https://github.com/Org/Repo/", "https://github.com/Org/Repo"), + ("https://github.com/Org/Repo/tree/main", "https://github.com/Org/Repo"), + ("git@github.com:Org/Repo.git", "https://github.com/Org/Repo"), + ("https://github.com/Org/Repo/tree/dev", "https://github.com/Org/Repo/tree/dev"), + ("not a url", "not a url"), + (None, None), + ], +) +def test_migration_canonical_matches_runtime(url, expected): + from fileglancer.giturls import canonical_github_url + + assert mig._canonical(url) == expected + if url is not None: + # The migration's inlined copy must agree with the runtime helper. + assert mig._canonical(url) == canonical_github_url(url) + + +@pytest.fixture +def engine(): + eng = create_engine("sqlite://") + Base.metadata.create_all(eng) + yield eng + eng.dispose() + + +def test_unique_table_canonicalizes_and_dedupes(engine): + with engine.begin() as conn: + # Two rows for the same user that collapse to one canonical URL (a + # canonical row already exists), plus one standalone non-canonical row. + conn.execute(text( + "INSERT INTO user_apps (username, url, manifest_path, name, added_at) " + "VALUES " + "('bob', 'https://github.com/o/r', '', 'canon', '2026-01-01')," + "('bob', 'https://github.com/o/r.git', '', 'dup', '2026-01-01')," + "('bob', 'https://github.com/o/other.git', '', 'other', '2026-01-01')" + )) + + with engine.begin() as conn: + mig._canonicalize_unique_table(conn, "user_apps", "username") + + with engine.begin() as conn: + rows = conn.execute(text( + "SELECT name, url FROM user_apps ORDER BY name" + )).fetchall() + + by_name = {r.name: r.url for r in rows} + # The .git duplicate of an existing canonical row is dropped. + assert "dup" not in by_name + assert by_name["canon"] == "https://github.com/o/r" + # The standalone non-canonical row is rewritten in place. + assert by_name["other"] == "https://github.com/o/other"
Branch{app.branch}Revision{revision}
Branch{listing.branch}Revision{revision}