From 84cf22e42b87ecb8973f77a62312e9a18d25f615 Mon Sep 17 00:00:00 2001
From: Abhimanyu Saharan <desk.abhimanyu@gmail.com>
Date: Fri, 13 Feb 2026 22:50:29 +0000
Subject: [PATCH] fix(skills): address Copilot review nits (safety + UI guards)

---
 backend/app/api/gateways.py            |  3 +++
 backend/app/api/skills_marketplace.py  | 17 ++++++++++++++++-
 frontend/src/app/skills/packs/page.tsx |  2 ++
 frontend/src/lib/skills-source.ts      | 10 +++++++++-
 4 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/backend/app/api/gateways.py b/backend/app/api/gateways.py
index 87f48dab..31a102f1 100644
--- a/backend/app/api/gateways.py
+++ b/backend/app/api/gateways.py
@@ -176,6 +176,9 @@ async def delete_gateway(
         await service.clear_agent_foreign_keys(agent_id=agent.id)
         await session.delete(agent)
 
+    # NOTE: The migration declares `ondelete="CASCADE"` for gateway_installed_skills.gateway_id,
+    # but some backends/test environments (e.g. SQLite without FK pragma) may not
+    # enforce cascades. Delete rows explicitly to guarantee cleanup semantics.
     installed_skills = await GatewayInstalledSkill.objects.filter_by(
         gateway_id=gateway.id,
     ).all(session)
diff --git a/backend/app/api/skills_marketplace.py b/backend/app/api/skills_marketplace.py
index c6beec24..46c877b4 100644
--- a/backend/app/api/skills_marketplace.py
+++ b/backend/app/api/skills_marketplace.py
@@ -243,6 +243,15 @@ def _collect_pack_skills_from_index(
     if not index_file.is_file():
         return None
 
+    try:
+        stat = index_file.stat()
+    except OSError as exc:
+        raise RuntimeError("unable to read skills_index.json") from exc
+
+    # Hard cap to avoid loading an arbitrarily large file into memory.
+    if stat.st_size > 256 * 1024:
+        raise RuntimeError("skills_index.json is too large")
+
     try:
         payload = json.loads(index_file.read_text(encoding="utf-8"))
     except OSError as exc:
@@ -382,7 +391,10 @@ def _collect_pack_skills(source_url: str) -> list[PackSkillCandidate]:
             raise RuntimeError("timed out cloning pack repository") from exc
         except subprocess.CalledProcessError as exc:
             stderr = (exc.stderr or "").strip()
-            detail = stderr or "unable to clone pack repository"
+            # Avoid reflecting arbitrary stderr to callers; keep details minimal.
+            detail = "unable to clone pack repository"
+            if stderr:
+                detail = f"{detail}: {stderr.splitlines()[0][:200]}"
             raise RuntimeError(detail) from exc
 
         try:
@@ -397,6 +409,9 @@ def _collect_pack_skills(source_url: str) -> list[PackSkillCandidate]:
             branch = "main"
 
         branch = branch or "main"
+        # Fail closed on weird output to avoid constructing malformed URLs.
+        if any(ch in branch for ch in {"\n", "\r", "\t"}):
+            branch = "main"
 
         return _collect_pack_skills_from_repo(
             repo_dir=repo_dir,
diff --git a/frontend/src/app/skills/packs/page.tsx b/frontend/src/app/skills/packs/page.tsx
index 8b34ce31..b87e4831 100644
--- a/frontend/src/app/skills/packs/page.tsx
+++ b/frontend/src/app/skills/packs/page.tsx
@@ -91,6 +91,8 @@ export default function SkillsPacksPage() {
   };
 
   const handleSyncPack = async (pack: SkillPackRead) => {
+    if (syncingPackIds.has(pack.id)) return;
+
     setSyncingPackIds((previous) => {
       const next = new Set(previous);
       next.add(pack.id);
diff --git a/frontend/src/lib/skills-source.ts b/frontend/src/lib/skills-source.ts
index 982f5ba4..029c8f52 100644
--- a/frontend/src/lib/skills-source.ts
+++ b/frontend/src/lib/skills-source.ts
@@ -9,7 +9,15 @@ export const repoBaseFromSkillSourceUrl = (skillSourceUrl: string): string | nul
     const marker = "/tree/";
     const markerIndex = parsed.pathname.indexOf(marker);
     if (markerIndex <= 0) return null;
-    return normalizeRepoSourceUrl(`${parsed.origin}${parsed.pathname.slice(0, markerIndex)}`);
+
+    // Reject unexpected structures (e.g. multiple /tree/ markers).
+    if (parsed.pathname.indexOf(marker, markerIndex + marker.length) !== -1) return null;
+
+    const repoPath = parsed.pathname.slice(0, markerIndex);
+    if (!repoPath || repoPath === "/") return null;
+    if (repoPath.endsWith("/tree")) return null;
+
+    return normalizeRepoSourceUrl(`${parsed.origin}${repoPath}`);
   } catch {
     return null;
   }