From cda960d7ca257c52b352d03eed1698cd6c44802f Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 24 Jun 2026 15:14:18 +0000 Subject: [PATCH 01/18] fix(catalog): preserve custom name/description when adding from catalog Adding a shared catalog listing to "my apps" discarded the publisher's custom name/description and used the freshly-fetched manifest's values instead. The add_from_catalog endpoint already loaded the listing, so capture and persist its name/description while still fetching the manifest for current capabilities/branch. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/server.py | 4 +++- tests/test_catalog_endpoints.py | 16 +++++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/fileglancer/server.py b/fileglancer/server.py index 6327d2cd..80a5243e 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -1890,6 +1890,8 @@ 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 try: manifest = await apps_module.fetch_app_manifest( @@ -1909,7 +1911,7 @@ async def add_from_catalog(listing_id: int, row = db.upsert_user_app( session, username, url=listing_url, manifest_path=listing_manifest_path, - name=manifest.name, description=manifest.description, + name=listing_name, description=listing_description, branch=branch, manifest=manifest.model_dump(mode="json"), ) diff --git a/tests/test_catalog_endpoints.py b/tests/test_catalog_endpoints.py index cad149c7..59900391 100644 --- a/tests/test_catalog_endpoints.py +++ b/tests/test_catalog_endpoints.py @@ -337,9 +337,13 @@ 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) @@ -354,14 +358,16 @@ def test_add_from_listing_creates_independent_user_app( assert response.status_code == 200 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( From 73ef28e067aeda69839089197ad3c989f6973a27 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Fri, 26 Jun 2026 15:13:35 +0000 Subject: [PATCH 02/18] feat(apps): add share/unshare button to My Apps cards Add a share/unshare icon button to each My Apps card, between the info and trash buttons, styled like the catalog's ghost IconButton. When the app is shared it unshares directly; when not shared it opens the app info dialog straight into the share form (via a new startInShareView prop) so the name/description can still be customized. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/components/ui/AppsPage/AppCard.tsx | 40 ++++++++++++++++++- .../components/ui/AppsPage/AppInfoDialog.tsx | 24 +++++++---- 2 files changed, 55 insertions(+), 9 deletions(-) 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..e50f7795 100644 --- a/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx +++ b/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx @@ -31,6 +31,7 @@ interface AppInfoDialogProps { readonly removing: boolean; readonly sharing: boolean; readonly unsharing: boolean; + readonly startInShareView?: boolean; } function AppInfoTable({ app }: { readonly app: UserApp }) { @@ -86,7 +87,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 +99,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 +106,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'); From 135d0c87fcb0376a50b7f15d339491822c7df51f Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Fri, 26 Jun 2026 15:30:09 +0000 Subject: [PATCH 03/18] feat(apps): show user's custom app name on launch and job pages The name a user customizes when adding an app from the catalog (or sharing) now appears instead of the raw manifest name. submit_job labels jobs with the user's saved app name (falling back to the manifest name), so the jobs list and job detail pages reflect it, and the launch page/form prefer the installed app's name. The internal job work_dir still uses the manifest name as a stable path identifier. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/apps/jobs.py | 8 +++++++- frontend/src/components/AppLaunch.tsx | 10 ++++++++-- frontend/src/components/ui/AppsPage/AppLaunchForm.tsx | 9 +++++++-- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/fileglancer/apps/jobs.py b/fileglancer/apps/jobs.py index 7c8c49ec..de9e6c45 100644 --- a/fileglancer/apps/jobs.py +++ b/fileglancer/apps/jobs.py @@ -543,11 +543,17 @@ async def submit_job( 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 + 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, diff --git a/frontend/src/components/AppLaunch.tsx b/frontend/src/components/AppLaunch.tsx index 37626736..9e660097 100644 --- a/frontend/src/components/AppLaunch.tsx +++ b/frontend/src/components/AppLaunch.tsx @@ -80,9 +80,10 @@ export default function AppLaunch() { const relaunchContainerArgs = relaunchState?.container_args; // Check if app is in user's library - const isInstalled = appsQuery.data?.some( + const installedApp = appsQuery.data?.find( a => a.url === appUrl && a.manifest_path === manifestPath ); + const isInstalled = installedApp !== undefined; useEffect(() => { if (appUrl) { @@ -97,6 +98,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 +234,7 @@ export default function AppLaunch() {
) : manifest && selectedEntryPoint ? ( - {manifest.name} + {displayName} {manifest.description ? ( {manifest.description} diff --git a/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx b/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx index abfd921b..3b497895 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} From 7d98437a42b8bbe7f8262d21ba70e3b9391e2a7a Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Fri, 26 Jun 2026 15:36:06 +0000 Subject: [PATCH 04/18] feat(apps): show empty-state message when an app has no parameters The launch form's Parameters tab now displays "There are no parameters to set for this app." when the entry point defines no parameters, instead of rendering an empty panel. The all-hidden case is unchanged (handled by the existing "Show hidden" toggle). Co-Authored-By: Claude Opus 4.8 (1M context) --- frontend/src/components/ui/AppsPage/AppLaunchForm.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx b/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx index 3b497895..5dce41d5 100644 --- a/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx +++ b/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx @@ -1279,7 +1279,11 @@ export default function AppLaunchForm({
- {hasSections ? ( + {allParams.length === 0 ? ( + + There are no parameters to set for this app. + + ) : hasSections ? ( Date: Fri, 26 Jun 2026 15:46:28 +0000 Subject: [PATCH 05/18] feat(apps): show app + entry point and a repo link in job Execution box MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The job detail Execution card now labels "App" as " — " and moves the GitHub repository to its own "Repository" row that links to and displays the repo URL (hidden when the URL isn't parseable). Replaces the previous setup where the app name linked to the repo and the entry point had a separate row. Co-Authored-By: Claude Opus 4.8 (1M context) --- frontend/src/components/JobDetail.tsx | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/frontend/src/components/JobDetail.tsx b/frontend/src/components/JobDetail.tsx index 185b9abf..bddf580a 100644 --- a/frontend/src/components/JobDetail.tsx +++ b/frontend/src/components/JobDetail.tsx @@ -317,15 +317,16 @@ function JobOverview({ + {job.app_name} - ) : ( - job.app_name - ) + {repoUrl} + ) : null } /> - Date: Fri, 26 Jun 2026 16:29:05 +0000 Subject: [PATCH 06/18] feat(apps): refine job Execution box repo link and 40/60 card layout Show the repository link as "owner/repo" with "(branch)" appended only for non-default branches, instead of the full URL. Lay out the Status and Execution cards as 40%/60% (a 5-column grid spanning 2 and 3) on md+ screens; InfoCard now forwards an optional className. Co-Authored-By: Claude Opus 4.8 (1M context) --- frontend/src/components/JobDetail.tsx | 34 ++++++++++++++++++--------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/frontend/src/components/JobDetail.tsx b/frontend/src/components/JobDetail.tsx index bddf580a..ee0d38ff 100644 --- a/frontend/src/components/JobDetail.tsx +++ b/frontend/src/components/JobDetail.tsx @@ -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,7 +324,7 @@ function JobOverview({ /> - + {repoUrl} + repoLink ? ( + + {repoLink.label} + ) : null } /> From 0452cb8a90e09ee99c205ca57ac9e3e6383a59b2 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Fri, 26 Jun 2026 16:35:14 +0000 Subject: [PATCH 07/18] feat(apps): rename user-facing "Branch" label to "Revision" Relabel the branch field/rows as "Revision" in the add app dialog (with helper text "Tag or branch name") and the app/listing info dialogs. Code identifiers and the manifest's branch field are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- frontend/src/components/ui/AppsPage/AddAppDialog.tsx | 7 ++++++- frontend/src/components/ui/AppsPage/AppInfoDialog.tsx | 2 +- frontend/src/components/ui/AppsPage/ListingInfoDialog.tsx | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/frontend/src/components/ui/AppsPage/AddAppDialog.tsx b/frontend/src/components/ui/AppsPage/AddAppDialog.tsx index ad7fce01..3f91e358 100644 --- a/frontend/src/components/ui/AppsPage/AddAppDialog.tsx +++ b/frontend/src/components/ui/AppsPage/AddAppDialog.tsx @@ -108,7 +108,12 @@ export default function AddAppDialog({ /> - + { setBranch(e.target.value); diff --git a/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx b/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx index e50f7795..3bea377f 100644 --- a/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx +++ b/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx @@ -52,7 +52,7 @@ function AppInfoTable({ app }: { readonly app: UserApp }) { {app.branch ? ( - Branch + Revision {app.branch} ) : null} diff --git a/frontend/src/components/ui/AppsPage/ListingInfoDialog.tsx b/frontend/src/components/ui/AppsPage/ListingInfoDialog.tsx index 7d06e1da..5b90f390 100644 --- a/frontend/src/components/ui/AppsPage/ListingInfoDialog.tsx +++ b/frontend/src/components/ui/AppsPage/ListingInfoDialog.tsx @@ -41,7 +41,7 @@ function ListingInfoTable({ listing }: { readonly listing: AppListing }) { {listing.branch ? ( - Branch + Revision {listing.branch} ) : null} From a452ee33917d7bb64182eee1a53ead1f803827a2 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Fri, 26 Jun 2026 17:36:02 +0000 Subject: [PATCH 08/18] feat(apps): support SSH URLs and fall back to SSH for private repos MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding an app from a private GitHub repo failed because the clone ran over HTTPS with prompts disabled. Now the add dialog accepts both HTTPS and SSH (git@github.com / ssh://) URLs, and the backend clone/ls-remote tries HTTPS first, then retries over SSH as the user on an auth/access error — using BatchMode + accept-new so it never hangs. When both transports fail, surface a clear message naming the repo/revision and the SSH-key requirement instead of a nested "500: Git command failed". - manifest.py: _parse_github_url accepts SSH forms; _clone_repo and _resolve_default_branch do HTTPS->SSH fallback; _run_git gains extra_env; auth-error detection + user-facing error helpers. - server.py: add_user_app re-raises the worker's HTTPException so its clean message isn't double-wrapped. - frontend: parseGithubUrl accepts SSH; new isGithubRepoUrl/buildAppUrl; AddAppDialog validation/help text updated for HTTPS or SSH. - tests: backend SSH parsing + clone fallback; frontend parser helpers. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/apps/manifest.py | 166 ++++++++++++++---- fileglancer/server.py | 6 + .../src/__tests__/unitTests/appUrls.test.ts | 43 +++++ .../components/ui/AppsPage/AddAppDialog.tsx | 28 +-- frontend/src/utils/appUrls.ts | 50 +++++- frontend/src/utils/index.ts | 2 + tests/test_apps.py | 99 +++++++++++ 7 files changed, 335 insertions(+), 59 deletions(-) diff --git a/fileglancer/apps/manifest.py b/fileglancer/apps/manifest.py index 3ee6f777..8fc7574f 100644 --- a/fileglancer/apps/manifest.py +++ b/fileglancer/apps/manifest.py @@ -3,6 +3,7 @@ import asyncio import os import re +import shutil from contextlib import suppress from pathlib import Path, PurePosixPath @@ -55,20 +56,33 @@ def _get_repo_lock(owner: str, repo: str, branch: str) -> asyncio.Lock: return _repo_locks[key] +_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). - Branch is None when not specified in the URL. + 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. """ - 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() + 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)]: @@ -141,13 +155,64 @@ 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 _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 +249,68 @@ 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: + if not _is_git_auth_error(str(https_err)): + raise + logger.info( + f"HTTPS clone of {owner}/{repo} failed authentication; retrying over SSH" + ) + # git creates the target directory before failing, so a leftover partial + # clone would make the SSH attempt fail with "already exists". Clear it. + shutil.rmtree(repo_dir, ignore_errors=True) + 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) + 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 +327,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) @@ -245,10 +353,7 @@ async def _ensure_repo_cache(url: str, pull: bool = False, 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 @@ -473,6 +578,5 @@ 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 diff --git a/fileglancer/server.py b/fileglancer/server.py index 80a5243e..c77582ed 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -1688,6 +1688,12 @@ async def add_user_app(body: AppAddRequest, username=username) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) + except HTTPException: + # The worker already produced a meaningful, user-facing message + # (e.g. a private-repo clone failure). Surface it directly instead + # of nesting it inside a generic "Failed to clone or scan repo: ..." + # wrapper, which would prepend the inner status code as noise. + raise except Exception as e: raise HTTPException(status_code=400, detail=f"Failed to clone or scan repo: {str(e)}") diff --git a/frontend/src/__tests__/unitTests/appUrls.test.ts b/frontend/src/__tests__/unitTests/appUrls.test.ts index 327519ea..ae390555 100644 --- a/frontend/src/__tests__/unitTests/appUrls.test.ts +++ b/frontend/src/__tests__/unitTests/appUrls.test.ts @@ -1,8 +1,10 @@ import { describe, expect, test } from 'vitest'; import { + buildAppUrl, buildLaunchPath, buildRelaunchPath, + isGithubRepoUrl, parseGithubUrl } from '@/utils/appUrls'; @@ -13,6 +15,47 @@ 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('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/ui/AppsPage/AddAppDialog.tsx b/frontend/src/components/ui/AppsPage/AddAppDialog.tsx index 3f91e358..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. diff --git a/frontend/src/utils/appUrls.ts b/frontend/src/utils/appUrls.ts index 17aa7abe..8d15ba95 100644 --- a/frontend/src/utils/appUrls.ts +++ b/frontend/src/utils/appUrls.ts @@ -5,21 +5,55 @@ 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}`); +} + +/** 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..0bf1bb5a 100644 --- a/frontend/src/utils/index.ts +++ b/frontend/src/utils/index.ts @@ -393,6 +393,8 @@ export type { // Re-export app URL utilities export { parseGithubUrl, + isGithubRepoUrl, + buildAppUrl, buildGithubUrl, buildLaunchPath, buildLaunchPathFromApp, diff --git a/tests/test_apps.py b/tests/test_apps.py index 4c100314..e0fe24ef 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,103 @@ 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) + + @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 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_non_auth_clone_error_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") + + monkeypatch.setattr(m, "_run_git", fake_run_git) + monkeypatch.setattr(m.shutil, "rmtree", lambda *a, **k: None) + + with pytest.raises(ValueError, match="Remote branch v9 not found"): + await _clone_repo("org", "tool", "v9", tmp_path / "dest") + # Only the HTTPS attempt should have run — no SSH retry for a non-auth error. + assert len(calls) == 1 + @pytest.mark.asyncio async def test_get_app_branch_returns_slash_branch_without_remote_lookup(self): branch = await get_app_branch( From c25c1406ebbe9bc3f8ca2e333dd4080dc4ca5a02 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Fri, 26 Jun 2026 17:46:31 +0000 Subject: [PATCH 09/18] fix(apps): name pixi apps from pixi.toml, not repo/branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pixi adapter named apps "/", and git rev-parse on a detached-HEAD clone returns "HEAD" — producing names like "SmartSPIMGlancer/HEAD". Use the pixi project's declared name first, falling back to the git repo name, then the directory name. The app's branch/revision is still tracked separately on the UserApp record. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/apps/pixi.py | 33 ++++++++++++++------------------- tests/test_apps.py | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 19 deletions(-) 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/tests/test_apps.py b/tests/test_apps.py index e0fe24ef..4686c32b 100644 --- a/tests/test_apps.py +++ b/tests/test_apps.py @@ -1600,3 +1600,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 From 3df8b3780b2ed4af8bb62678824af885fa39a887 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Fri, 26 Jun 2026 18:11:14 +0000 Subject: [PATCH 10/18] fix(apps): accurate clone errors and tag-safe pulls for private repos Two private-repo papercuts: - A mistyped tag produced a misleading "can't access / maybe private" message (HTTPS auth-fails, SSH reaches the repo and reveals the real "ref not found"). _clone_repo now detects ref-not-found at both the HTTPS and SSH stages and reports the revision plainly, and add_user_app surfaces it as 400 instead of a scary 500. - Pulling a cached tag/SHA failed with "ambiguous argument 'origin/'" because origin/ only exists for branches. The update path now resets to FETCH_HEAD (valid for branches, tags and SHAs) and passes the SSH env so SSH-origin private repos don't prompt. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/apps/manifest.py | 46 +++++++++++++++++++--- fileglancer/server.py | 13 ++++--- tests/test_apps.py | 74 ++++++++++++++++++++++++++++++++++-- 3 files changed, 119 insertions(+), 14 deletions(-) diff --git a/fileglancer/apps/manifest.py b/fileglancer/apps/manifest.py index 8fc7574f..7a12905f 100644 --- a/fileglancer/apps/manifest.py +++ b/fileglancer/apps/manifest.py @@ -189,6 +189,25 @@ def _github_remote_urls(owner: str, repo: str) -> tuple[str, str]: ) +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.""" @@ -293,14 +312,17 @@ async def _clone_repo(owner: str, repo: str, branch: str, repo_dir: Path) -> Non ) 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" ) - # git creates the target directory before failing, so a leftover partial - # clone would make the SSH attempt fail with "already exists". Clear it. - shutil.rmtree(repo_dir, ignore_errors=True) try: await _run_git( ["git", "clone", "--branch", branch, ssh_url, str(repo_dir)], @@ -308,6 +330,10 @@ async def _clone_repo(owner: str, repo: str, branch: str, repo_dir: Path) -> Non ) 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)) ) @@ -348,8 +374,18 @@ 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) diff --git a/fileglancer/server.py b/fileglancer/server.py index c77582ed..6246a43d 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -1688,12 +1688,15 @@ async def add_user_app(body: AppAddRequest, username=username) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) - except HTTPException: + except HTTPException as e: # The worker already produced a meaningful, user-facing message - # (e.g. a private-repo clone failure). Surface it directly instead - # of nesting it inside a generic "Failed to clone or scan repo: ..." - # wrapper, which would prepend the inner status code as noise. - raise + # (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)}") diff --git a/tests/test_apps.py b/tests/test_apps.py index 4686c32b..5e73285c 100644 --- a/tests/test_apps.py +++ b/tests/test_apps.py @@ -1156,23 +1156,89 @@ async def fake_run_git(args, timeout=60, extra_env=None): await _clone_repo("org", "tool", "v0.1.0", tmp_path / "dest") @pytest.mark.asyncio - async def test_non_auth_clone_error_is_not_retried(self, tmp_path, monkeypatch): + 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") + 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="Remote branch v9 not found"): + with pytest.raises(ValueError, match="Revision 'v9' was not found"): await _clone_repo("org", "tool", "v9", tmp_path / "dest") - # Only the HTTPS attempt should have run — no SSH retry for a non-auth error. + # 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( From 3d3181b13c1e519d44aae8c498f823763b7f7fa8 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Sat, 27 Jun 2026 00:33:57 +0000 Subject: [PATCH 11/18] fix(apps): canonicalize GitHub URLs to fix false "not in your library" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The launch page decided "installed" by exact string match against a URL rebuilt into canonical form, so any stored app whose URL carried a ".git" suffix, trailing slash, or "/tree/main" wrongly showed as not in the library — common for shared apps, whose URL is whatever the sharer entered. Fix it at the root by giving every GitHub URL one canonical form: - new fileglancer/giturls.py (standalone, no app deps) with canonical_github_url, mirroring the frontend helper - database.py canonicalizes on write (upsert_user_app, create_app_listing, create_job) and on lookup (get_user_app, delete_user_app, get_app_listing_for_app), so any cosmetic variant resolves to one row (also fixes the submit_job custom-name lookup) - migration b8e4f1a92c37 rewrites existing user_apps/app_listings/jobs URLs, deduping rows that collapse under the unique constraint - frontend AppLaunch matches installed apps via canonicalGithubUrl Co-Authored-By: Claude Opus 4.8 (1M context) --- .../b8e4f1a92c37_canonicalize_github_urls.py | 94 +++++++++++++++++++ fileglancer/apps/manifest.py | 53 +---------- fileglancer/database.py | 11 ++- fileglancer/giturls.py | 72 ++++++++++++++ .../src/__tests__/unitTests/appUrls.test.ts | 23 +++++ frontend/src/components/AppLaunch.tsx | 11 ++- frontend/src/utils/appUrls.ts | 15 +++ frontend/src/utils/index.ts | 1 + tests/test_apps.py | 31 ++++++ tests/test_apps_endpoints.py | 23 +++++ tests/test_url_migration.py | 84 +++++++++++++++++ 11 files changed, 362 insertions(+), 56 deletions(-) create mode 100644 fileglancer/alembic/versions/b8e4f1a92c37_canonicalize_github_urls.py create mode 100644 fileglancer/giturls.py create mode 100644 tests/test_url_migration.py 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/apps/manifest.py b/fileglancer/apps/manifest.py index 7a12905f..3af23233 100644 --- a/fileglancer/apps/manifest.py +++ b/fileglancer/apps/manifest.py @@ -2,7 +2,6 @@ import asyncio import os -import re import shutil from contextlib import suppress from pathlib import Path, PurePosixPath @@ -12,6 +11,10 @@ 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 _parse_github_url, canonical_github_url # noqa: F401 from fileglancer.model import AppManifest from fileglancer.settings import get_settings @@ -56,54 +59,6 @@ def _get_repo_lock(owner: str, repo: str, branch: str) -> asyncio.Lock: return _repo_locks[key] -_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 validate_manifest_path(manifest_path: str) -> str: """Validate and normalize a user-supplied manifest path. diff --git a/fileglancer/database.py b/fileglancer/database.py index cf351276..7c7875a5 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() @@ -1096,6 +1097,7 @@ def upsert_user_app(session: Session, username: str, url: str, refreshes like a lazy manifest backfill. """ now = datetime.now(UTC) + url = canonical_github_url(url) row = get_user_app(session, username, url, manifest_path) if row is None: row = UserAppDB( @@ -1125,7 +1127,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 +1160,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 +1170,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..d43ea6b4 --- /dev/null +++ b/fileglancer/giturls.py @@ -0,0 +1,72 @@ +"""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, trailing slash, and a redundant "/tree/main", and + folds SSH forms to https — so the same repo always has one stored + representation. A non-default branch is kept as "/tree/". 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}" diff --git a/frontend/src/__tests__/unitTests/appUrls.test.ts b/frontend/src/__tests__/unitTests/appUrls.test.ts index ae390555..b86950c1 100644 --- a/frontend/src/__tests__/unitTests/appUrls.test.ts +++ b/frontend/src/__tests__/unitTests/appUrls.test.ts @@ -4,6 +4,7 @@ import { buildAppUrl, buildLaunchPath, buildRelaunchPath, + canonicalGithubUrl, isGithubRepoUrl, parseGithubUrl } from '@/utils/appUrls'; @@ -43,6 +44,28 @@ describe('app URL helpers', () => { 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' diff --git a/frontend/src/components/AppLaunch.tsx b/frontend/src/components/AppLaunch.tsx index 9e660097..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,9 +79,14 @@ export default function AppLaunch() { const relaunchContainer = relaunchState?.container; const relaunchContainerArgs = relaunchState?.container_args; - // Check if app is in user's library + // 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 => a.url === appUrl && a.manifest_path === manifestPath + a => + canonicalGithubUrl(a.url) === appUrl && a.manifest_path === manifestPath ); const isInstalled = installedApp !== undefined; diff --git a/frontend/src/utils/appUrls.ts b/frontend/src/utils/appUrls.ts index 8d15ba95..761cbe8e 100644 --- a/frontend/src/utils/appUrls.ts +++ b/frontend/src/utils/appUrls.ts @@ -35,6 +35,21 @@ export function parseGithubUrl(url: string): { 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 { diff --git a/frontend/src/utils/index.ts b/frontend/src/utils/index.ts index 0bf1bb5a..748ea97f 100644 --- a/frontend/src/utils/index.ts +++ b/frontend/src/utils/index.ts @@ -394,6 +394,7 @@ export type { export { parseGithubUrl, isGithubRepoUrl, + canonicalGithubUrl, buildAppUrl, buildGithubUrl, buildLaunchPath, diff --git a/tests/test_apps.py b/tests/test_apps.py index 5e73285c..33b716ea 100644 --- a/tests/test_apps.py +++ b/tests/test_apps.py @@ -1088,6 +1088,37 @@ def test_branch_name_may_contain_slashes(self): 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", [ diff --git a/tests/test_apps_endpoints.py b/tests/test_apps_endpoints.py index d0bd2ba5..5415f410 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 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" From ad0e0e8453861c80a40d12d06a658cdbf2fa80c8 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Sat, 27 Jun 2026 01:01:39 +0000 Subject: [PATCH 12/18] fix(apps): guard os.geteuid() for Windows in ensure_repo debug log os.geteuid() doesn't exist on Windows, breaking test_pull_resets_to_fetch_head_not_origin_branch. Fall back to 'n/a' when unavailable. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/apps/manifest.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fileglancer/apps/manifest.py b/fileglancer/apps/manifest.py index 3af23233..996be5b1 100644 --- a/fileglancer/apps/manifest.py +++ b/fileglancer/apps/manifest.py @@ -318,7 +318,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()) From c448308a27c93765ecbb5f6a33ba2eaec6f66d59 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Mon, 29 Jun 2026 15:52:29 +0000 Subject: [PATCH 13/18] feat(apps): confirm before canceling a job from the jobs list Add a confirmation dialog when canceling/stopping a job from the jobs list page, matching the existing dialog on the job detail page. Extract the shared dialog into a CancelJobDialog component used by both pages. Co-Authored-By: Claude Opus 4.8 (1M context) --- frontend/src/components/AppJobs.tsx | 27 ++++++++-- frontend/src/components/JobDetail.tsx | 42 ++++------------ .../src/components/ui/Dialogs/CancelJob.tsx | 49 +++++++++++++++++++ .../components/ui/Table/appsJobsColumns.tsx | 4 +- 4 files changed, 82 insertions(+), 40 deletions(-) create mode 100644 frontend/src/components/ui/Dialogs/CancelJob.tsx 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/JobDetail.tsx b/frontend/src/components/JobDetail.tsx index ee0d38ff..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, @@ -686,40 +686,16 @@ export default function JobDetail() { ) : null} {/* Cancel/Stop confirmation dialog */} - 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/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 }, { From 92722ad4ddab29fe9ab85cad81b7e15d73b9d735 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Mon, 29 Jun 2026 18:07:58 +0000 Subject: [PATCH 14/18] fix(apps): pin the cloned revision in the app URL at add time Resolve the revision once when an app is added and bake it into the stored canonical URL: a repo whose default is e.g. "master" is stored as ".../tree/master" rather than a bare URL, so it dedups against an explicit "/tree/master" and the URL always names the revision actually cloned. This closes the bare-vs-default-branch dedup hole where the same app could be added twice. The branch column now records the user's requested revision ("" = took the default at add time) instead of the resolved one, and the Revision shown in the app/listing dialogs is parsed from the URL. The revision is fixed at add time: update just re-pulls the stored URL and refreshes the manifest, without re-resolving the default branch or moving the row to a new URL. To follow a changed default branch, delete and re-add the app. Includes a migration that bakes the resolved revision (from the old branch column) into existing URLs and derives the requested revision from the URL shape, de-duplicating any rows that collapse together. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...1f9a4e7b2d8_bake_revision_into_app_urls.py | 119 ++++++++++++++ fileglancer/apps/__init__.py | 1 + fileglancer/apps/manifest.py | 39 +++-- fileglancer/database.py | 8 +- fileglancer/giturls.py | 28 +++- fileglancer/model.py | 4 +- fileglancer/server.py | 31 ++-- .../components/ui/AppsPage/AppInfoDialog.tsx | 18 ++- .../ui/AppsPage/ListingInfoDialog.tsx | 18 ++- tests/test_apps.py | 18 +++ tests/test_apps_endpoints.py | 151 +++++++++++++++--- tests/test_revision_migration.py | 113 +++++++++++++ 12 files changed, 487 insertions(+), 61 deletions(-) create mode 100644 fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py create mode 100644 tests/test_revision_migration.py 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..1926a702 --- /dev/null +++ b/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py @@ -0,0 +1,119 @@ +"""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 and +should track the default branch. + +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 = ""). jobs carry no branch column and are historical, so they are +left untouched. + +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.""" + parsed = _parse(row.url) + if parsed is None: + return None + owner, repo, url_branch = parsed + requested = url_branch or "" + # branch column held the resolved revision; fall back defensively. + 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 already at their target URL are the winners on collision. + seen = {} + for r in rows: + t = targets[r.id] + if t is not None and 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..242fb054 100644 --- a/fileglancer/apps/__init__.py +++ b/fileglancer/apps/__init__.py @@ -8,6 +8,7 @@ get_app_branch, get_or_load_manifest, refresh_cached_manifest, + resolve_app_url, set_worker_exec, ) from fileglancer.apps.command import ( # noqa: F401 diff --git a/fileglancer/apps/manifest.py b/fileglancer/apps/manifest.py index 996be5b1..7cd553c1 100644 --- a/fileglancer/apps/manifest.py +++ b/fileglancer/apps/manifest.py @@ -14,7 +14,11 @@ # 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 _parse_github_url, canonical_github_url # noqa: F401 +from fileglancer.giturls import ( # noqa: F401 + _parse_github_url, + canonical_github_url, + github_url_at_branch, +) from fileglancer.model import AppManifest from fileglancer.settings import get_settings @@ -516,13 +520,13 @@ async def get_or_load_manifest(username: str, url: str, manifest = await fetch_app_manifest(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, name=manifest.name, description=manifest.description, - branch=branch, manifest=manifest.model_dump(mode="json"), bump_updated_at=False, ) @@ -533,7 +537,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 @@ -541,12 +545,11 @@ 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: @@ -555,12 +558,11 @@ async def refresh_cached_manifest(username: str, url: str, session, username, url=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: @@ -572,3 +574,22 @@ async def get_app_branch(url: str) -> str: if not branch: branch = await _resolve_default_branch(owner, repo) return branch + + +async def resolve_app_url(url: str) -> tuple[str, str]: + """Resolve an app URL into its (canonical_url, requested_branch) pair. + + Called once, at add time, to fix the app's revision. The canonical URL + carries the revision that will be cloned: the branch named in the URL, or — + when the URL is bare — the repo's default branch resolved over the network. + So a repo whose default is "master" yields ".../tree/master" even from a bare + URL, while "main" folds back to the bare URL. The revision is fixed from here + on; the app does not re-resolve later (re-add it to pick up a moved default). + + 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. + """ + owner, repo, url_branch = _parse_github_url(url) + requested = url_branch or "" + revision = requested or await _resolve_default_branch(owner, repo) + return github_url_at_branch(owner, repo, revision), requested diff --git a/fileglancer/database.py b/fileglancer/database.py index 7c7875a5..e99b7a38 100644 --- a/fileglancer/database.py +++ b/fileglancer/database.py @@ -1095,6 +1095,11 @@ 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 unpinned / track the + default branch). 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) @@ -1114,7 +1119,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 diff --git a/fileglancer/giturls.py b/fileglancer/giturls.py index d43ea6b4..93d48512 100644 --- a/fileglancer/giturls.py +++ b/fileglancer/giturls.py @@ -58,10 +58,20 @@ def _parse_github_url(url: str) -> tuple[str, str, str | None]: def canonical_github_url(url: str) -> str: """Normalize a GitHub URL to its canonical https form. - Strips a ".git" suffix, trailing slash, and a redundant "/tree/main", and - folds SSH forms to https — so the same repo always has one stored - representation. A non-default branch is kept as "/tree/". Returns the - input unchanged if it isn't a parseable GitHub URL. + 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 resolve_app_url) 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) @@ -70,3 +80,13 @@ def canonical_github_url(url: str) -> str: 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}") diff --git a/fileglancer/model.py b/fileglancer/model.py index 56110b7f..5a4ab0f8 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 track the repo's default branch. The actually-cloned revision is baked into url.", 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 track the repo's default branch. The actually-cloned revision is baked into url.", 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 6246a43d..0cc182ad 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -1664,7 +1664,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,7 +1675,6 @@ 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], @@ -1708,18 +1707,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 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 = await apps_module.resolve_app_url(body.url) 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( @@ -1755,6 +1757,9 @@ 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. try: await apps_module._ensure_repo_cache(body.url, pull=True, username=username) except Exception as e: @@ -1781,14 +1786,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, name=manifest.name, description=manifest.description, - branch=branch, manifest=manifest.model_dump(mode="json"), ) return UserApp( @@ -1901,6 +1904,7 @@ async def add_from_catalog(listing_id: int, listing_manifest_path = listing.manifest_path listing_name = listing.name listing_description = listing.description + listing_branch = listing.branch or "" try: manifest = await apps_module.fetch_app_manifest( @@ -1911,17 +1915,14 @@ async def add_from_catalog(listing_id: int, 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=listing_name, description=listing_description, - branch=branch, + branch=listing_branch, manifest=manifest.model_dump(mode="json"), ) return UserApp( diff --git a/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx b/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx index 3bea377f..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; @@ -38,6 +51,7 @@ 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 ( @@ -50,10 +64,10 @@ function AppInfoTable({ app }: { readonly app: UserApp }) { - {app.branch ? ( + {revision ? ( - + ) : null} {app.description ? ( diff --git a/frontend/src/components/ui/AppsPage/ListingInfoDialog.tsx b/frontend/src/components/ui/AppsPage/ListingInfoDialog.tsx index 5b90f390..bc828081 100644 --- a/frontend/src/components/ui/AppsPage/ListingInfoDialog.tsx +++ b/frontend/src/components/ui/AppsPage/ListingInfoDialog.tsx @@ -8,6 +8,19 @@ import FgButton from '@/components/designSystem/atoms/FgButton'; import FgExternalLink from '@/components/designSystem/atoms/FgExternalLink'; import FgTooltip from '@/components/ui/widgets/FgTooltip'; import { formatDateString } from '@/utils'; +import { parseGithubUrl } from '@/utils/appUrls'; + +/** + * The revision actually cloned, parsed out of the canonical listing URL (which + * always carries it). Falls back to the requested branch, then null. + */ +function listingRevision(listing: AppListing): string | null { + try { + return parseGithubUrl(listing.url).branch; + } catch { + return listing.branch || null; + } +} interface ListingInfoDialogProps { readonly listing: AppListing; @@ -27,6 +40,7 @@ function ListingInfoTable({ listing }: { readonly listing: AppListing }) { const valueClass = 'text-foreground py-1.5'; const publishedAt = formatDateString(listing.published_at); + const revision = listingRevision(listing); return (
Revision{app.branch}{revision}
@@ -39,10 +53,10 @@ function ListingInfoTable({ listing }: { readonly listing: AppListing }) { - {listing.branch ? ( + {revision ? ( - + ) : null} diff --git a/tests/test_apps.py b/tests/test_apps.py index 33b716ea..641458c5 100644 --- a/tests/test_apps.py +++ b/tests/test_apps.py @@ -1132,6 +1132,24 @@ def test_rejects_non_github_urls(self, url): _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 TestCloneFallback: def test_auth_error_detection(self): assert _is_git_auth_error( diff --git a/tests/test_apps_endpoints.py b/tests/test_apps_endpoints.py index 5415f410..b04490c4 100644 --- a/tests/test_apps_endpoints.py +++ b/tests/test_apps_endpoints.py @@ -184,23 +184,21 @@ 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, 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" @@ -251,10 +249,12 @@ 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", + patch("fileglancer.apps.manifest._resolve_default_branch", new=AsyncMock(return_value="main")): response = test_client.post( "/api/apps", @@ -265,24 +265,86 @@ 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=[("", manifest)])), \ + patch("fileglancer.apps.manifest._resolve_default_branch", + new=AsyncMock(return_value="master")): + 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_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=[("", 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=[("", manifest)])), \ + patch("fileglancer.apps.manifest._resolve_default_branch", + new=AsyncMock(return_value="master")): + 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", + patch("fileglancer.apps.manifest._resolve_default_branch", new=AsyncMock(return_value="main")): response = test_client.post( "/api/apps", @@ -364,6 +426,49 @@ def test_update_app_pulls_separate_code_repo(test_client, db_session): assert rows[0].manifest["repo_url"] == "https://github.com/tools/code" +@pytest.mark.parametrize( + "stored_url,stored_branch", + [ + # Unpinned app pinned to master at add time. + ("https://github.com/owner/repo/tree/master", ""), + # Explicitly pinned app. + ("https://github.com/owner/repo/tree/dev", "dev"), + ], +) +def test_update_pulls_stored_revision_and_never_re_resolves( + test_client, db_session, stored_url, stored_branch +): + """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 == (stored_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 @@ -507,23 +612,21 @@ 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, 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( + new=AsyncMock(return_value=fresh)): + manifest = await refresh_cached_manifest( TEST_USERNAME, "https://github.com/owner/repo", "", ) assert manifest.name == "Synced" - assert branch == "main" 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 @@ -535,10 +638,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", "", ) @@ -555,9 +656,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_revision_migration.py b/tests/test_revision_migration.py new file mode 100644 index 00000000..e542f25e --- /dev/null +++ b/tests/test_revision_migration.py @@ -0,0 +1,113 @@ +"""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"), + # Defensive: missing branch column falls back to the URL/main. + ("https://github.com/o/r", None, "https://github.com/o/r", ""), + ], +) +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 + + +@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_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") From 387596d6c962fb63d2176b85ee8470ff07a896fc Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Mon, 29 Jun 2026 18:28:16 +0000 Subject: [PATCH 15/18] fix(apps): resolve the default branch in the worker, not the server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit add_user_app resolved the default branch in the shared server process, which has no access to a user's SSH key — so for a private repo with a non-main default the resolution failed and fell back to "main", storing a bare URL instead of e.g. ".../tree/master" and defeating the dedup and pin-at-add-time guarantees. Resolve the branch in the per-user worker (which holds the user's SSH credentials) and return it from discover_manifests, then build the canonical URL from that worker-resolved branch via the new pure canonical_app_url helper. The server no longer makes a network call to resolve branches. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/apps/__init__.py | 2 +- fileglancer/apps/manifest.py | 46 ++++++++++++++++++++---------------- fileglancer/giturls.py | 5 ++-- fileglancer/server.py | 11 +++++---- fileglancer/user_worker.py | 17 ++++++++++--- tests/test_apps_endpoints.py | 40 +++++++++++++++++++++---------- 6 files changed, 77 insertions(+), 44 deletions(-) diff --git a/fileglancer/apps/__init__.py b/fileglancer/apps/__init__.py index 242fb054..ff57ada1 100644 --- a/fileglancer/apps/__init__.py +++ b/fileglancer/apps/__init__.py @@ -6,9 +6,9 @@ discover_app_manifests, fetch_app_manifest, get_app_branch, + canonical_app_url, get_or_load_manifest, refresh_cached_manifest, - resolve_app_url, set_worker_exec, ) from fileglancer.apps.command import ( # noqa: F401 diff --git a/fileglancer/apps/manifest.py b/fileglancer/apps/manifest.py index 7cd553c1..8834676f 100644 --- a/fileglancer/apps/manifest.py +++ b/fileglancer/apps/manifest.py @@ -445,25 +445,32 @@ 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) + branch = await get_app_branch(url) + return branch, _find_manifests_in_repo(repo_dir) async def fetch_app_manifest(url: str, manifest_path: str = "", @@ -576,20 +583,19 @@ async def get_app_branch(url: str) -> str: return branch -async def resolve_app_url(url: str) -> tuple[str, str]: - """Resolve an app URL into its (canonical_url, requested_branch) pair. +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. The canonical URL - carries the revision that will be cloned: the branch named in the URL, or — - when the URL is bare — the repo's default branch resolved over the network. - So a repo whose default is "master" yields ".../tree/master" even from a bare - URL, while "main" folds back to the bare URL. The revision is fixed from here - on; the app does not re-resolve later (re-add it to pick up a moved default). + 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). - 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 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) - requested = url_branch or "" - revision = requested or await _resolve_default_branch(owner, repo) - return github_url_at_branch(owner, repo, revision), requested + return github_url_at_branch(owner, repo, resolved_branch), (url_branch or "") diff --git a/fileglancer/giturls.py b/fileglancer/giturls.py index 93d48512..2af0d59f 100644 --- a/fileglancer/giturls.py +++ b/fileglancer/giturls.py @@ -67,8 +67,9 @@ def canonical_github_url(url: str) -> str: 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 resolve_app_url) rather than relying on - this. The frontend's buildGithubUrl/canonicalGithubUrl fold "main" the same + 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. diff --git a/fileglancer/server.py b/fileglancer/server.py index 0cc182ad..4912403d 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -1681,10 +1681,11 @@ async def get_user_apps(username: str = Depends(get_current_user)): 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: @@ -1707,10 +1708,10 @@ async def add_user_app(body: AppAddRequest, f"Make sure a manifest exists in the repository.", ) - # Bake the resolved revision into the stored URL (so a repo whose + # 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 = await apps_module.resolve_app_url(body.url) + 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: diff --git a/fileglancer/user_worker.py b/fileglancer/user_worker.py index efd7280f..3b82def2 100644 --- a/fileglancer/user_worker.py +++ b/fileglancer/user_worker.py @@ -1035,18 +1035,29 @@ 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, + get_app_branch, + ) repo_dir = _run_async(_ensure_repo_cache( url=request["url"], pull=True, )) + branch = _run_async(get_app_branch(request["url"])) 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/tests/test_apps_endpoints.py b/tests/test_apps_endpoints.py index b04490c4..e0f6db6f 100644 --- a/tests/test_apps_endpoints.py +++ b/tests/test_apps_endpoints.py @@ -253,9 +253,7 @@ def test_add_app_persists_manifest_and_branch(test_client, db_session): 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.manifest._resolve_default_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"}, @@ -280,9 +278,7 @@ def test_add_app_bakes_resolved_default_into_url(test_client, db_session): 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=[("", manifest)])), \ - patch("fileglancer.apps.manifest._resolve_default_branch", - new=AsyncMock(return_value="master")): + new=AsyncMock(return_value=("master", [("", manifest)]))): response = test_client.post( "/api/apps", json={"url": "https://github.com/owner/repo"}, @@ -299,11 +295,33 @@ def test_add_app_bakes_resolved_default_into_url(test_client, db_session): 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=[("", manifest)])): + new=AsyncMock(return_value=("dev", [("", manifest)]))): response = test_client.post( "/api/apps", json={"url": "https://github.com/owner/repo/tree/dev"}, @@ -324,9 +342,7 @@ def test_add_app_dedups_bare_against_resolved_default(test_client, db_session): branch="", manifest=manifest.model_dump(mode="json")) with patch("fileglancer.apps.discover_app_manifests", - new=AsyncMock(return_value=[("", manifest)])), \ - patch("fileglancer.apps.manifest._resolve_default_branch", - new=AsyncMock(return_value="master")): + new=AsyncMock(return_value=("master", [("", manifest)]))): response = test_client.post( "/api/apps", json={"url": "https://github.com/owner/repo"}, @@ -343,9 +359,7 @@ def test_add_app_dedups(test_client, db_session): manifest=manifest.model_dump(mode="json")) with patch("fileglancer.apps.discover_app_manifests", - new=AsyncMock(return_value=[("", manifest)])), \ - patch("fileglancer.apps.manifest._resolve_default_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"}, From 7ce0a4b3d7b60acc1b29a4a44d9b696d490d1f5b Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Mon, 29 Jun 2026 14:45:00 -0400 Subject: [PATCH 16/18] fix(apps): keep added app revisions fixed Treat stored bare app URLs as the fixed main revision for clone/fetch/update paths so they do not follow later default-branch moves. Preserve the folded storage URL for dedupe/UI compatibility while using explicit operational URLs. Co-authored-by: Codex --- ...1f9a4e7b2d8_bake_revision_into_app_urls.py | 5 +- fileglancer/apps/__init__.py | 1 + fileglancer/apps/jobs.py | 12 ++- fileglancer/apps/manifest.py | 39 +++++++-- fileglancer/database.py | 10 ++- fileglancer/giturls.py | 10 +++ fileglancer/model.py | 4 +- fileglancer/server.py | 19 +++-- fileglancer/user_worker.py | 8 +- tests/test_apps.py | 18 ++++ tests/test_apps_endpoints.py | 83 +++++++++++++++---- tests/test_catalog_endpoints.py | 9 +- 12 files changed, 173 insertions(+), 45 deletions(-) diff --git a/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py b/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py index 1926a702..8dbbef93 100644 --- a/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py +++ b/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py @@ -3,8 +3,9 @@ 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 and -should track the default branch. +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 diff --git a/fileglancer/apps/__init__.py b/fileglancer/apps/__init__.py index ff57ada1..ad92584b 100644 --- a/fileglancer/apps/__init__.py +++ b/fileglancer/apps/__init__.py @@ -3,6 +3,7 @@ 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, diff --git a/fileglancer/apps/jobs.py b/fileglancer/apps/jobs.py index de9e6c45..47daa2b4 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,6 +540,9 @@ async def submit_job( if v is not None } + stored_app_url = app_url + app_clone_url = clone_url_for_stored_app(stored_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") @@ -548,6 +553,9 @@ async def submit_job( # 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) db_job = db.create_job( session=session, @@ -582,7 +590,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) @@ -590,7 +598,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 8834676f..6109b5db 100644 --- a/fileglancer/apps/manifest.py +++ b/fileglancer/apps/manifest.py @@ -18,6 +18,7 @@ _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 @@ -469,7 +470,10 @@ async def discover_app_manifests( return result["branch"], manifests repo_dir = await _ensure_repo_cache(url, pull=True) - branch = await get_app_branch(url) + 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) @@ -517,6 +521,7 @@ 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 if stored is not None: try: @@ -524,7 +529,8 @@ 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) if row_exists else url + manifest = await fetch_app_manifest(fetch_url, manifest_path, username=username) if row_exists: # branch=None: this is a cache refresh, so leave the requested revision @@ -532,7 +538,7 @@ async def get_or_load_manifest(username: str, url: str, 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, manifest=manifest.model_dump(mode="json"), bump_updated_at=False, @@ -556,14 +562,20 @@ async def refresh_cached_manifest(username: str, url: str, Returns the refreshed manifest. """ - manifest = await fetch_app_manifest(url, manifest_path, username=username) - 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 + + fetch_url = clone_url_for_stored_app(row_url) 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, manifest=manifest.model_dump(mode="json"), bump_updated_at=bump_updated_at, @@ -599,3 +611,16 @@ def canonical_app_url(url: str, resolved_branch: str) -> tuple[str, str]: """ 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) -> str: + """Return the explicit GitHub URL to clone/fetch for a stored app URL. + + 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". Treat a stored bare URL as the fixed "main" + revision and make it explicit before cloning, fetching, or reading from disk. + """ + owner, repo, branch = _parse_github_url(url) + return github_url_with_branch(owner, repo, branch or "main") diff --git a/fileglancer/database.py b/fileglancer/database.py index e99b7a38..4d6bb9c3 100644 --- a/fileglancer/database.py +++ b/fileglancer/database.py @@ -1096,10 +1096,12 @@ def upsert_user_app(session: Session, username: str, url: str, 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 unpinned / track the - default branch). 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. + 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) diff --git a/fileglancer/giturls.py b/fileglancer/giturls.py index 2af0d59f..47d8ff65 100644 --- a/fileglancer/giturls.py +++ b/fileglancer/giturls.py @@ -91,3 +91,13 @@ def github_url_at_branch(owner: str, repo: str, branch: str) -> str: 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 5a4ab0f8..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="Revision the user requested; empty means track the repo's default branch. The actually-cloned revision is baked into url.", 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="Revision the user requested; empty means track the repo's default branch. The actually-cloned revision is baked into url.", 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 4912403d..d0defdf4 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 @@ -1761,20 +1762,27 @@ async def update_user_app(body: ManifestFetchRequest, # 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 + + clone_url = apps_module.clone_url_for_stored_app(stored_url) 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, @@ -1791,7 +1799,7 @@ async def update_user_app(body: ManifestFetchRequest, # 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, manifest=manifest.model_dump(mode="json"), ) @@ -1907,9 +1915,10 @@ async def add_from_catalog(listing_id: int, listing_description = listing.description listing_branch = listing.branch or "" + clone_url = apps_module.clone_url_for_stored_app(listing_url) 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)) diff --git a/fileglancer/user_worker.py b/fileglancer/user_worker.py index 3b82def2..34112abe 100644 --- a/fileglancer/user_worker.py +++ b/fileglancer/user_worker.py @@ -1044,13 +1044,17 @@ def _action_discover_manifests(request: dict, ctx: WorkerContext) -> dict: from fileglancer.apps.manifest import ( _ensure_repo_cache, _find_manifests_in_repo, - get_app_branch, + _parse_github_url, + _repo_cache_base, ) repo_dir = _run_async(_ensure_repo_cache( url=request["url"], pull=True, )) - branch = _run_async(get_app_branch(request["url"])) + 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, diff --git a/tests/test_apps.py b/tests/test_apps.py index 641458c5..902cd9fa 100644 --- a/tests/test_apps.py +++ b/tests/test_apps.py @@ -1150,6 +1150,24 @@ def test_non_default_branch_is_explicit(self): ) +class TestCloneUrlForStoredApp: + def test_bare_stored_url_means_fixed_main(self): + from fileglancer.apps import clone_url_for_stored_app + + 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") + == "https://github.com/Org/Repo/tree/master" + ) + + class TestCloneFallback: def test_auth_error_detection(self): assert _is_git_auth_error( diff --git a/tests/test_apps_endpoints.py b/tests/test_apps_endpoints.py index e0f6db6f..e5a29af6 100644 --- a/tests/test_apps_endpoints.py +++ b/tests/test_apps_endpoints.py @@ -184,7 +184,8 @@ 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="dev", 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 fetch_app_manifest directly inside @@ -377,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( @@ -392,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 @@ -419,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( @@ -431,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} @@ -440,17 +444,51 @@ 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", + "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", "", "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", "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 + 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.""" @@ -470,7 +508,7 @@ def test_update_pulls_stored_revision_and_never_re_resolves( ) assert response.status_code == 200 - assert mock_ensure.await_args_list[0].args == (stored_url,) + assert mock_ensure.await_args_list[0].args == (clone_url,) body = response.json() assert body["url"] == stored_url assert body["branch"] == stored_branch @@ -568,8 +606,8 @@ def test_fetch_manifest_backfills_null_cache(test_client, db_session): 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")): + patch("fileglancer.apps.manifest._resolve_default_branch", + new=AsyncMock(side_effect=AssertionError("must not re-resolve"))): response = test_client.post("/api/apps/manifest", json={ "url": "https://github.com/owner/repo", "manifest_path": "", @@ -577,6 +615,10 @@ def test_fetch_manifest_backfills_null_cache(test_client, db_session): assert response.status_code == 200 assert mock_fetch.await_count == 1 + assert mock_fetch.await_args.args == ( + "https://github.com/owner/repo/tree/main", + "", + ) assert response.json()["name"] == "Backfilled" # Row was updated silently (updated_at stays NULL). @@ -626,16 +668,23 @@ 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", branch="dev") + _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)): + 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", "", + TEST_USERNAME, "https://github.com/owner/repo/tree/dev", "", ) assert manifest.name == "Synced" + 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" diff --git a/tests/test_catalog_endpoints.py b/tests/test_catalog_endpoints.py index 59900391..ac1d0cc3 100644 --- a/tests/test_catalog_endpoints.py +++ b/tests/test_catalog_endpoints.py @@ -350,13 +350,14 @@ def test_add_from_listing_creates_independent_user_app( 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"] == "Custom Name" assert body["description"] == "Custom description" From 2afd88e0c79d871d85b1b06c711c383014b8d326 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Mon, 29 Jun 2026 21:04:59 +0000 Subject: [PATCH 17/18] fix(apps): keep legacy null-branch apps tracking the default branch A bare stored URL whose branch column is NULL is a legacy app migrated from user_preferences, whose default branch was never recorded. Treating it as the fixed "main" revision (as the prior pass did) breaks apps whose repo defaults to e.g. "master", since "main" may not exist. clone_url_for_stored_app now takes the row's branch: NULL means "unknown default", so the stored URL is returned unchanged and git resolves the current default (the row's historical behavior); "" or an explicit name means pinned, so the revision is made explicit. The migration leaves NULL-branch rows untouched instead of rewriting them to main. Such rows get pinned the next time the app is re-added. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...1f9a4e7b2d8_bake_revision_into_app_urls.py | 22 ++++++++++++--- fileglancer/apps/jobs.py | 6 ++-- fileglancer/apps/manifest.py | 27 ++++++++++++------ fileglancer/server.py | 8 ++++-- tests/test_apps.py | 16 +++++++++-- tests/test_apps_endpoints.py | 27 ++++++++++++++---- tests/test_revision_migration.py | 28 +++++++++++++++++-- 7 files changed, 108 insertions(+), 26 deletions(-) diff --git a/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py b/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py index 8dbbef93..694c94c7 100644 --- a/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py +++ b/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py @@ -15,8 +15,14 @@ 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 = ""). jobs carry no branch column and are historical, so they are -left untouched. +(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 @@ -62,13 +68,21 @@ def _at_branch(owner, repo, branch): def _target(row): - """Return (new_url, requested_branch) for a row, or None to leave it alone.""" + """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 "" - # branch column held the resolved revision; fall back defensively. resolved = row.branch or url_branch or "main" return _at_branch(owner, repo, resolved), requested diff --git a/fileglancer/apps/jobs.py b/fileglancer/apps/jobs.py index 47daa2b4..066d91ca 100644 --- a/fileglancer/apps/jobs.py +++ b/fileglancer/apps/jobs.py @@ -541,7 +541,9 @@ async def submit_job( } stored_app_url = app_url - app_clone_url = clone_url_for_stored_app(stored_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 @@ -555,7 +557,7 @@ async def submit_job( 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) + app_clone_url = clone_url_for_stored_app(stored_app_url, user_app.branch) db_job = db.create_job( session=session, diff --git a/fileglancer/apps/manifest.py b/fileglancer/apps/manifest.py index 6109b5db..6e4c8307 100644 --- a/fileglancer/apps/manifest.py +++ b/fileglancer/apps/manifest.py @@ -522,6 +522,7 @@ async def get_or_load_manifest(username: str, url: str, 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: @@ -529,7 +530,7 @@ async def get_or_load_manifest(username: str, url: str, except ValidationError as e: logger.warning(f"Stored manifest schema mismatch for {url}: {e}") - fetch_url = clone_url_for_stored_app(row_url) if row_exists else url + 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: @@ -567,8 +568,9 @@ async def refresh_cached_manifest(username: str, url: str, 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) if row_exists else url + 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: @@ -613,14 +615,23 @@ def canonical_app_url(url: str, resolved_branch: str) -> tuple[str, str]: return github_url_at_branch(owner, repo, resolved_branch), (url_branch or "") -def clone_url_for_stored_app(url: str) -> str: - """Return the explicit GitHub URL to clone/fetch for a stored app URL. +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". Treat a stored bare URL as the fixed "main" - revision and make it explicit before cloning, fetching, or reading from disk. + 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. """ - owner, repo, branch = _parse_github_url(url) - return github_url_with_branch(owner, repo, branch or "main") + 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/server.py b/fileglancer/server.py index d0defdf4..c277299b 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -1767,8 +1767,9 @@ async def update_user_app(body: ManifestFetchRequest, 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) + clone_url = apps_module.clone_url_for_stored_app(stored_url, stored_branch) try: await apps_module._ensure_repo_cache(clone_url, pull=True, username=username) except Exception as e: @@ -1913,9 +1914,10 @@ async def add_from_catalog(listing_id: int, listing_manifest_path = listing.manifest_path listing_name = listing.name listing_description = listing.description - listing_branch = listing.branch or "" + # Keep None (legacy "track default") distinct from "" (pinned main). + listing_branch = listing.branch - clone_url = apps_module.clone_url_for_stored_app(listing_url) + clone_url = apps_module.clone_url_for_stored_app(listing_url, listing_branch) try: manifest = await apps_module.fetch_app_manifest( clone_url, listing_manifest_path, username=username, diff --git a/tests/test_apps.py b/tests/test_apps.py index 902cd9fa..4ccc35a2 100644 --- a/tests/test_apps.py +++ b/tests/test_apps.py @@ -1154,8 +1154,9 @@ 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") + clone_url_for_stored_app("https://github.com/Org/Repo", "") == "https://github.com/Org/Repo/tree/main" ) @@ -1163,10 +1164,21 @@ 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") + 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): diff --git a/tests/test_apps_endpoints.py b/tests/test_apps_endpoints.py index e5a29af6..9c62b1a9 100644 --- a/tests/test_apps_endpoints.py +++ b/tests/test_apps_endpoints.py @@ -600,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._resolve_default_branch", - new=AsyncMock(side_effect=AssertionError("must not re-resolve"))): + new=AsyncMock(return_value=fresh)) as mock_fetch: response = test_client.post("/api/apps/manifest", json={ "url": "https://github.com/owner/repo", "manifest_path": "", @@ -615,12 +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 diff --git a/tests/test_revision_migration.py b/tests/test_revision_migration.py index e542f25e..2f116b5f 100644 --- a/tests/test_revision_migration.py +++ b/tests/test_revision_migration.py @@ -41,8 +41,6 @@ def __init__(self, url, branch): ("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"), - # Defensive: missing branch column falls back to the URL/main. - ("https://github.com/o/r", None, "https://github.com/o/r", ""), ], ) def test_target(url, branch, expected_url, expected_requested): @@ -53,6 +51,11 @@ 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://") @@ -88,6 +91,27 @@ def test_bakes_resolved_revision_and_flips_branch(engine): 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_dedupes_bare_against_explicit_revision(engine): """A bare master-default row collapses onto an explicit /tree/master row.""" with engine.begin() as conn: From 4bed3c8ec10cd315de678d5505e5a256255d73eb Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Mon, 29 Jun 2026 17:20:29 -0400 Subject: [PATCH 18/18] fix(apps): handle legacy app migration collisions Register unchanged null-branch legacy rows as collision winners before rewriting known app URLs, so rows that bake to the same URL are dropped instead of violating the unique constraint. Co-authored-by: Codex --- ...1f9a4e7b2d8_bake_revision_into_app_urls.py | 10 +++++-- tests/test_revision_migration.py | 27 +++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py b/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py index 694c94c7..bb8f8a70 100644 --- a/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py +++ b/fileglancer/alembic/versions/c1f9a4e7b2d8_bake_revision_into_app_urls.py @@ -97,11 +97,17 @@ def _migrate_unique_table(conn, table, owner_col): targets = {r.id: _target(r) for r in rows} - # Pass 1: rows already at their target URL are the winners on collision. + # 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 not None and t[0] == r.url: + 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. diff --git a/tests/test_revision_migration.py b/tests/test_revision_migration.py index 2f116b5f..069abe14 100644 --- a/tests/test_revision_migration.py +++ b/tests/test_revision_migration.py @@ -112,6 +112,33 @@ def test_null_branch_legacy_row_left_untouched(engine): 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:
Revision{listing.branch}{revision}