From 9f51dab249f516106b2e90a268b2341dbc106c25 Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Wed, 22 Jul 2026 17:34:02 +0000 Subject: [PATCH 1/5] =?UTF-8?q?feat(ui):=20browser=20skill=20management=20?= =?UTF-8?q?console=20=E2=80=94=20install/update/remove/read=20(spec=20003?= =?UTF-8?q?=20phase=203)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clean rewrite of the embedded static UI (index.html/app.js/styles.css): status-first load with capability gating (writable/embeddingProvider), table rebound from dead source_type/source_url to metadata.origin, inline install bar (raw-string origin, no client-side detection), per-row Update gated on a check:true preflight badge (Up to date / Updatable / Pinned) plus Update-all, Remove with confirm(), a right-side detail drawer reading GET /skills/{id}/content into an escaped
,
non-blocking toasts replacing alert(), and a provider-gated Reindex
button. Dead CSS from earlier card-based UI iterations removed.

Co-Authored-By: Claude Opus 4.8 (1M context) 
Claude-Session: https://claude.ai/code/session_0153XpJc4DrMRAJJqVDchxRm
---
 crates/fastskill-core/src/http/static/app.js  | 486 +++++++++++---
 .../fastskill-core/src/http/static/index.html |  44 +-
 .../fastskill-core/src/http/static/styles.css | 623 ++++++++----------
 3 files changed, 731 insertions(+), 422 deletions(-)

diff --git a/crates/fastskill-core/src/http/static/app.js b/crates/fastskill-core/src/http/static/app.js
index 8ed70a1c..4b106726 100644
--- a/crates/fastskill-core/src/http/static/app.js
+++ b/crates/fastskill-core/src/http/static/app.js
@@ -1,68 +1,132 @@
-// FastSkill UI - skill-project.toml view; table lists all installed skills
-class ProjectApp {
+// FastSkill browser console (spec 003 phase 3).
+//
+// Status-first load: /status is fetched before any write control is rendered,
+// so the UI never flashes an enabled control that capability gating then
+// disables. Read set (project/skills/status) is always available; write set
+// (install/update/remove/reindex) is gated on `writable` and, for reindex,
+// on `embeddingProvider` too (ADR-0002/0003, spec 003 §6).
+class FastSkillApp {
     constructor() {
         this.apiBase = '/api/v1';
+        this.status = null;
         this.project = null;
         this.skills = [];
+        // id -> { id, outcome, reason, resolvedVersion } from the last
+        // `POST /skills/update { check: true }` preflight pass.
+        this.preflight = {};
         this.init();
     }
 
     async init() {
+        await this.loadStatus();
+        this.applyCapabilities();
         this.setupEventListeners();
         await this.loadAll();
     }
 
+    // ---- status / capability gating -----------------------------------
+
+    async loadStatus() {
+        try {
+            const res = await fetch(`${this.apiBase}/status`);
+            const data = await res.json().catch(() => null);
+            if (res.ok && data && data.success && data.data) {
+                this.status = data.data;
+            } else {
+                throw new Error((data && data.error && data.error.message) || `HTTP ${res.status}`);
+            }
+        } catch (err) {
+            console.error('Failed to load /status:', err);
+            // Fail closed: treat capabilities as unavailable rather than
+            // risk flashing write controls the server would 403.
+            this.status = { writable: false, embeddingProvider: false };
+        }
+    }
+
+    applyCapabilities() {
+        const writable = !!(this.status && this.status.writable);
+        const hasProvider = !!(this.status && this.status.embeddingProvider);
+
+        this.toggle('readonly-banner', !writable);
+        this.toggle('provider-banner', !hasProvider);
+        this.toggle('install-form', writable);
+        this.toggle('update-all-btn', writable);
+        this.toggle('reindex-btn', writable && hasProvider);
+    }
+
+    toggle(id, visible) {
+        const el = document.getElementById(id);
+        if (!el) return;
+        el.hidden = !visible;
+    }
+
+    // ---- wiring ---------------------------------------------------------
+
     setupEventListeners() {
         const searchInput = document.getElementById('search-input');
-        if (searchInput) {
-            searchInput.addEventListener('input', () => this.render());
-        }
-        const upgradeAllBtn = document.getElementById('upgrade-all-btn');
-        if (upgradeAllBtn) {
-            upgradeAllBtn.addEventListener('click', () => this.upgradeAll());
-        }
+        if (searchInput) searchInput.addEventListener('input', () => this.renderSkillsTable());
+
+        const installForm = document.getElementById('install-form');
+        if (installForm) installForm.addEventListener('submit', (e) => this.handleInstallSubmit(e));
+
+        const updateAllBtn = document.getElementById('update-all-btn');
+        if (updateAllBtn) updateAllBtn.addEventListener('click', () => this.updateAll());
+
+        const reindexBtn = document.getElementById('reindex-btn');
+        if (reindexBtn) reindexBtn.addEventListener('click', () => this.reindex());
+
+        const drawerClose = document.getElementById('drawer-close');
+        if (drawerClose) drawerClose.addEventListener('click', () => this.closeDrawer());
+
+        const drawerBackdrop = document.getElementById('drawer-backdrop');
+        if (drawerBackdrop) drawerBackdrop.addEventListener('click', () => this.closeDrawer());
+
+        document.addEventListener('keydown', (e) => {
+            if (e.key === 'Escape') this.closeDrawer();
+        });
     }
 
+    // ---- data loading -----------------------------------------------------
+
     async loadAll() {
         const loading = document.getElementById('loading');
         const error = document.getElementById('error');
         const content = document.getElementById('content');
 
-        if (loading) loading.style.display = 'block';
-        if (error) { error.style.display = 'none'; error.textContent = ''; }
-        if (content) content.style.display = 'none';
+        if (loading) loading.hidden = false;
+        if (error) { error.hidden = true; error.textContent = ''; }
+        if (content) content.hidden = true;
 
         try {
-            const [projectRes, skillsRes] = await Promise.all([
+            const tasks = [
                 fetch(`${this.apiBase}/project`),
                 fetch(`${this.apiBase}/skills`),
-            ]);
+            ];
+            const [projectRes, skillsRes] = await Promise.all(tasks);
 
             if (!projectRes.ok) throw new Error(`Project: HTTP ${projectRes.status}`);
             const projectData = await projectRes.json();
             if (!projectData.success || !projectData.data) {
-                throw new Error(projectData.error?.message || 'Failed to load project');
+                throw new Error((projectData.error && projectData.error.message) || 'Failed to load project');
             }
             this.project = projectData.data;
 
             if (!skillsRes.ok) throw new Error(`Skills: HTTP ${skillsRes.status}`);
             const skillsData = await skillsRes.json();
-            if (skillsData.success && skillsData.data && skillsData.data.skills) {
-                this.skills = skillsData.data.skills;
-            } else {
-                this.skills = [];
-            }
+            this.skills = (skillsData.success && skillsData.data && skillsData.data.skills) || [];
+
+            await this.loadPreflight();
 
-            if (loading) loading.style.display = 'none';
-            if (content) content.style.display = 'block';
+            if (loading) loading.hidden = true;
+            if (content) content.hidden = false;
             this.render();
         } catch (err) {
-            if (loading) loading.style.display = 'none';
+            if (loading) loading.hidden = true;
             if (error) {
-                error.style.display = 'block';
+                error.hidden = false;
                 error.textContent = `Error: ${err.message}`;
             }
-            if (content) content.style.display = 'block';
+            if (content) content.hidden = false;
             this.project = this.project || { metadata: null, skills_directory: '—', skills: [] };
             this.skills = this.skills || [];
             this.render();
@@ -70,9 +134,34 @@ class ProjectApp {
         }
     }
 
+    // Preflight-classify every installed skill via `check: true` so rows can
+    // show an Up to date / Updatable / Pinned badge and only enable a
+    // per-row Update button on Updatable skills (spec 003 §4/Q5). Skipped
+    // entirely when read-only, since the endpoint is write-gated.
+    async loadPreflight() {
+        this.preflight = {};
+        if (!this.status || !this.status.writable) return;
+        try {
+            const res = await fetch(`${this.apiBase}/skills/update`, {
+                method: 'POST',
+                headers: { 'Content-Type': 'application/json' },
+                body: JSON.stringify({ check: true }),
+            });
+            const data = await res.json().catch(() => null);
+            if (res.ok && data && data.success && Array.isArray(data.data)) {
+                for (const result of data.data) {
+                    this.preflight[result.id] = result;
+                }
+            }
+        } catch (err) {
+            console.error('Preflight check failed:', err);
+        }
+    }
+
+    // ---- rendering -------------------------------------------------------
+
     render() {
         if (!this.project) return;
-
         this.renderMetadata();
         this.renderSkillsDirectory();
         this.renderSkillsTable();
@@ -83,7 +172,7 @@ class ProjectApp {
         if (!el) return;
 
         const m = this.project.metadata;
-        if (!m || (m && !m.id && !m.name && !m.version)) {
+        if (!m || (!m.id && !m.name && !m.version)) {
             el.innerHTML = '

[metadata]

No metadata

'; return; } @@ -114,117 +203,360 @@ class ProjectApp { `; } + // Maps an `Origin` (internally-tagged: git | local | zip-url | repository, + // see crate::core::origin::Origin) to its salient "Location" field. + originLocation(origin) { + if (!origin) return ''; + switch (origin.type) { + case 'git': + case 'zip-url': + return origin.url || ''; + case 'local': + return origin.path || ''; + case 'repository': + return [origin.repo, origin.skill].filter(Boolean).join('/'); + default: + return ''; + } + } + + badgeFor(id) { + const p = this.preflight[id]; + if (!p) return ''; + switch (p.outcome) { + case 'up_to_date': + return 'Up to date'; + case 'would_update': + return 'Updatable'; + case 'immutable': + return `Pinned`; + case 'error': + return `Check failed`; + default: + return ''; + } + } + renderSkillsTable() { const tbody = document.getElementById('skills-tbody'); if (!tbody) return; - const skills = this.skills; const query = (document.getElementById('search-input') || {}).value || ''; const q = query.toLowerCase().trim(); const filtered = q - ? skills.filter(s => { - const name = (s.name || '').toLowerCase(); - const id = (s.id || '').toLowerCase(); - const desc = (s.description || '').toLowerCase(); + ? this.skills.filter((s) => { const meta = s.metadata || {}; - const ver = (meta.version || '').toLowerCase(); - const loc = (meta.source_url || '').toLowerCase(); - const typ = (meta.source_type || '').toLowerCase(); - return name.includes(q) || id.includes(q) || desc.includes(q) || ver.includes(q) || loc.includes(q) || typ.includes(q); + const origin = meta.origin || null; + const haystack = [ + s.name, s.id, s.description, meta.version, + origin && origin.type, origin && this.originLocation(origin), + ].filter(Boolean).join(' ').toLowerCase(); + return haystack.includes(q); }) - : skills; + : this.skills; if (filtered.length === 0) { - tbody.innerHTML = 'No skills'; + tbody.innerHTML = 'No skills'; return; } - tbody.innerHTML = filtered.map(s => { + const writable = !!(this.status && this.status.writable); + + tbody.innerHTML = filtered.map((s) => { const meta = s.metadata || {}; + const origin = meta.origin || null; const name = this.escapeHtml(s.name || '—'); const id = this.escapeHtml(s.id || '—'); const version = this.escapeHtml(meta.version || '—'); - const typ = this.escapeHtml(String(meta.source_type || '—')); - const locRaw = meta.source_url || ''; + const typ = this.escapeHtml((origin && origin.type) || '—'); + const locRaw = origin ? this.originLocation(origin) : ''; const loc = locRaw - ? (locRaw.startsWith('http') ? `${this.escapeHtml(locRaw)}` : this.escapeHtml(locRaw)) + ? (locRaw.startsWith('http') + ? `${this.escapeHtml(locRaw)}` + : this.escapeHtml(locRaw)) : '—'; - const desc = this.escapeHtml((s.description || '').slice(0, 80)); - const descCell = (s.description || '').length > 80 ? desc + '…' : desc || '—'; + const descFull = s.description || ''; + const descTrunc = this.escapeHtml(descFull.slice(0, 80)); + const descCell = descFull.length > 80 ? `${descTrunc}…` : (descTrunc || '—'); + const badge = this.badgeFor(s.id) || ''; + + const pf = this.preflight[s.id]; + const canUpdate = writable && pf && pf.outcome === 'would_update'; + const updateBtn = canUpdate + ? `` + : ''; + const removeBtn = writable + ? `` + : ''; + return ` - + ${name} ${id} ${version} ${typ} ${loc} ${descCell} - - - - + ${badge} + ${updateBtn}${removeBtn} `; }).join(''); - tbody.querySelectorAll('.btn-upgrade').forEach(btn => { - btn.addEventListener('click', () => this.upgradeSkill(btn.dataset.id)); + tbody.querySelectorAll('tr[data-row-id]').forEach((row) => { + row.addEventListener('click', () => this.openDrawerById(row.dataset.rowId)); + }); + tbody.querySelectorAll('.btn-update').forEach((btn) => { + btn.addEventListener('click', (e) => { + e.stopPropagation(); + this.updateSkill(btn.dataset.id); + }); }); - tbody.querySelectorAll('.btn-remove').forEach(btn => { - btn.addEventListener('click', () => this.removeSkill(btn.dataset.id)); + tbody.querySelectorAll('.btn-remove').forEach((btn) => { + btn.addEventListener('click', (e) => { + e.stopPropagation(); + this.removeSkill(btn.dataset.id); + }); }); } - async upgradeAll() { - const btn = document.getElementById('upgrade-all-btn'); - if (btn) { btn.disabled = true; btn.textContent = 'Upgrading...'; } + // ---- install / update / remove / reindex ------------------------------ + + async handleInstallSubmit(e) { + e.preventDefault(); + const input = document.getElementById('install-input'); + const btn = document.getElementById('install-btn'); + const value = (input && input.value || '').trim(); + if (!value) return; + + if (input) input.disabled = true; + if (btn) { btn.disabled = true; btn.textContent = 'Installing…'; } + + try { + const res = await fetch(`${this.apiBase}/skills/install`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ origin: value, groups: [] }), + }); + const data = await res.json().catch(() => null); + + if (res.status === 201 && data && data.success) { + const id = (data.data && data.data.id) || value; + this.toast(`Installed ${id}`, 'success'); + if (input) input.value = ''; + await this.loadAll(); + } else if (res.status === 409) { + this.toast((data && data.error && data.error.message) || 'Skill already installed', 'error'); + } else if (res.status === 403) { + this.toast('Read-only — start with --enable-write to install skills', 'error'); + } else { + this.toast((data && data.error && data.error.message) || `Install failed (HTTP ${res.status})`, 'error'); + } + } catch (err) { + this.toast(`Error: ${err.message}`, 'error'); + } finally { + if (input) input.disabled = false; + if (btn) { btn.disabled = false; btn.textContent = 'Install'; } + } + } + + summarizeUpdateResults(results) { + const counts = {}; + for (const r of results) counts[r.outcome] = (counts[r.outcome] || 0) + 1; + const parts = []; + if (counts.updated) parts.push(`${counts.updated} updated`); + if (counts.would_update) parts.push(`${counts.would_update} updatable`); + if (counts.up_to_date) parts.push(`${counts.up_to_date} up to date`); + if (counts.immutable) parts.push(`${counts.immutable} pinned`); + if (counts.error) parts.push(`${counts.error} failed`); + return parts.length ? parts.join(', ') : 'No skills to update'; + } + + async updateAll() { + const btn = document.getElementById('update-all-btn'); + if (btn) { btn.disabled = true; btn.textContent = 'Updating…'; } try { - const response = await fetch(`${this.apiBase}/skills/upgrade`, { + const res = await fetch(`${this.apiBase}/skills/update`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({}), }); - const data = await response.json(); - if (data.success) { + const data = await res.json().catch(() => null); + if (res.ok && data && data.success && Array.isArray(data.data)) { + this.toast(this.summarizeUpdateResults(data.data), 'success'); await this.loadAll(); - alert(data.data?.message || 'Upgrade completed'); + } else if (res.status === 403) { + this.toast('Read-only — start with --enable-write to update skills', 'error'); } else { - alert(data.error?.message || 'Upgrade failed'); + this.toast((data && data.error && data.error.message) || 'Update failed', 'error'); } } catch (err) { - alert(`Error: ${err.message}`); + this.toast(`Error: ${err.message}`, 'error'); } finally { - if (btn) { btn.disabled = false; btn.textContent = 'Upgrade all'; } + if (btn) { btn.disabled = false; btn.textContent = 'Update all'; } } } - async upgradeSkill(skillId) { + async updateSkill(skillId) { try { - const response = await fetch(`${this.apiBase}/skills/upgrade`, { + const res = await fetch(`${this.apiBase}/skills/update`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ skillId: skillId }), + body: JSON.stringify({ skillId }), }); - const data = await response.json(); - if (data.success) await this.loadAll(); - else alert(data.error?.message || 'Upgrade failed'); + const data = await res.json().catch(() => null); + if (res.ok && data && data.success && Array.isArray(data.data)) { + this.toast(this.summarizeUpdateResults(data.data), 'success'); + await this.loadAll(); + } else if (res.status === 403) { + this.toast('Read-only — start with --enable-write to update skills', 'error'); + } else { + this.toast((data && data.error && data.error.message) || 'Update failed', 'error'); + } } catch (err) { - alert(`Error: ${err.message}`); + this.toast(`Error: ${err.message}`, 'error'); } } async removeSkill(skillId) { - if (!confirm(`Remove skill ${skillId}?`)) return; + if (!confirm(`Remove skill "${skillId}"? This deletes it from disk.`)) return; try { - const response = await fetch(`${this.apiBase}/skills/${encodeURIComponent(skillId)}`, { method: 'DELETE' }); - const data = await response.json(); - if (data.success) await this.loadAll(); - else alert(data.error?.message || 'Remove failed'); + const res = await fetch(`${this.apiBase}/skills/${encodeURIComponent(skillId)}`, { method: 'DELETE' }); + const data = await res.json().catch(() => null); + if (res.ok && data && data.success) { + this.toast(`Removed ${skillId}`, 'success'); + await this.loadAll(); + } else if (res.status === 403) { + this.toast('Read-only — start with --enable-write to remove skills', 'error'); + } else { + this.toast((data && data.error && data.error.message) || 'Remove failed', 'error'); + } } catch (err) { - alert(`Error: ${err.message}`); + this.toast(`Error: ${err.message}`, 'error'); } } + async reindex() { + const btn = document.getElementById('reindex-btn'); + if (btn) { btn.disabled = true; btn.textContent = 'Reindexing…'; } + try { + const res = await fetch(`${this.apiBase}/reindex`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({}), + }); + const data = await res.json().catch(() => null); + if (res.ok && data && data.success && data.data) { + const { reindexed, count, reason } = data.data; + if (reindexed) { + this.toast(`Reindexed ${count} skill${count === 1 ? '' : 's'}`, 'success'); + } else { + this.toast(reason || 'Reindex skipped', 'info'); + } + } else if (res.status === 403) { + this.toast('Read-only — start with --enable-write to reindex', 'error'); + } else { + this.toast((data && data.error && data.error.message) || 'Reindex failed', 'error'); + } + } catch (err) { + this.toast(`Error: ${err.message}`, 'error'); + } finally { + if (btn) { btn.disabled = false; btn.textContent = 'Reindex'; } + } + } + + // ---- detail drawer ----------------------------------------------------- + + openDrawerById(skillId) { + const skill = this.skills.find((s) => s.id === skillId); + if (skill) this.openDrawer(skill); + } + + async openDrawer(skill) { + const drawer = document.getElementById('drawer'); + const title = document.getElementById('drawer-title'); + const body = document.getElementById('drawer-body'); + if (!drawer || !title || !body) return; + + title.textContent = skill.name || skill.id; + + const meta = skill.metadata || {}; + const origin = meta.origin || null; + const originRows = origin + ? ` + origin type${this.escapeHtml(origin.type || '—')} + origin location${this.escapeHtml(this.originLocation(origin) || '—')} + ` + : `origin—`; + + body.innerHTML = ` + + + + + + ${originRows} +
id${this.escapeHtml(skill.id)}
name${this.escapeHtml(skill.name || '—')}
version${this.escapeHtml(meta.version || '—')}
author${this.escapeHtml(meta.author || '—')}
+

SKILL.md

+
Loading…
+ `; + + drawer.classList.add('open'); + drawer.setAttribute('aria-hidden', 'false'); + this._openSkillId = skill.id; + + try { + const res = await fetch(`${this.apiBase}/skills/${encodeURIComponent(skill.id)}/content`); + const data = await res.json().catch(() => null); + // The drawer may have been closed/reopened on a different skill + // while this request was in flight. + if (this._openSkillId !== skill.id) return; + const contentEl = document.getElementById('drawer-content'); + if (!contentEl) return; + if (res.ok && data && data.success && data.data) { + // textContent, not innerHTML: the SKILL.md body is untrusted + // content (SEC-7) and must never be interpreted as markup. + contentEl.textContent = data.data.content != null ? data.data.content : ''; + } else { + contentEl.textContent = `Error loading content: ${(data && data.error && data.error.message) || res.status}`; + } + } catch (err) { + if (this._openSkillId !== skill.id) return; + const contentEl = document.getElementById('drawer-content'); + if (contentEl) contentEl.textContent = `Error: ${err.message}`; + } + } + + closeDrawer() { + const drawer = document.getElementById('drawer'); + if (!drawer) return; + drawer.classList.remove('open'); + drawer.setAttribute('aria-hidden', 'true'); + this._openSkillId = null; + } + + // ---- toasts ----------------------------------------------------------- + + toast(message, kind) { + const container = document.getElementById('toast-container'); + if (!container) return; + + const el = document.createElement('div'); + el.className = `toast toast-${kind || 'info'}`; + el.textContent = message; + container.appendChild(el); + + // Force layout before adding .show so the transition runs. + requestAnimationFrame(() => el.classList.add('show')); + + setTimeout(() => { + el.classList.remove('show'); + setTimeout(() => el.remove(), 250); + }, 3200); + } + + // ---- utilities ---------------------------------------------------------- + escapeHtml(text) { if (text == null) return ''; const div = document.createElement('div'); @@ -233,4 +565,4 @@ class ProjectApp { } } -const app = new ProjectApp(); +const app = new FastSkillApp(); diff --git a/crates/fastskill-core/src/http/static/index.html b/crates/fastskill-core/src/http/static/index.html index c5c33662..5a03fd1a 100644 --- a/crates/fastskill-core/src/http/static/index.html +++ b/crates/fastskill-core/src/http/static/index.html @@ -10,15 +10,34 @@
- - -
+
Loading…
+ +
+ + + +
+ diff --git a/crates/fastskill-core/src/http/static/styles.css b/crates/fastskill-core/src/http/static/styles.css index 3e20cbfd..448b9a8e 100644 --- a/crates/fastskill-core/src/http/static/styles.css +++ b/crates/fastskill-core/src/http/static/styles.css @@ -12,11 +12,13 @@ body { } #app { - max-width: 960px; + max-width: 1080px; margin: 0 auto; padding: 24px; } +/* ---- Header / toolbar ---- */ + .page-header { margin-bottom: 24px; } @@ -28,6 +30,32 @@ body { margin-bottom: 12px; } +.banner { + padding: 10px 14px; + border-radius: 6px; + font-size: 0.875rem; + margin-bottom: 12px; +} + +.banner code { + font-size: 0.8125rem; + background: rgba(0, 0, 0, 0.06); + padding: 1px 5px; + border-radius: 3px; +} + +.banner-warning { + color: #7a5b00; + background: #fff3cd; + border: 1px solid #ffe69c; +} + +.banner-info { + color: #495057; + background: #eef1f4; + border: 1px solid #dee2e6; +} + .toolbar { display: flex; gap: 12px; @@ -35,7 +63,14 @@ body { align-items: center; } -.search-input { +.install-bar { + display: flex; + gap: 8px; + flex: 2; + min-width: 280px; +} + +.install-input { flex: 1; min-width: 200px; padding: 8px 12px; @@ -44,352 +79,61 @@ body { font-size: 14px; } -.search-input:focus { +.install-input:focus { outline: none; border-color: #5c6bc0; } -.search-box { - flex: 1; - display: flex; - gap: 10px; - min-width: 300px; -} - -.search-box input { +.search-input { flex: 1; - padding: 10px 15px; - border: 2px solid #ddd; - border-radius: 6px; - font-size: 16px; + min-width: 180px; + padding: 8px 12px; + border: 1px solid #dee2e6; + border-radius: 4px; + font-size: 14px; } -.search-box input:focus { +.search-input:focus { outline: none; - border-color: #667eea; -} - -.search-box button { - padding: 10px 20px; - background: #667eea; - color: white; - border: none; - border-radius: 6px; - cursor: pointer; - font-weight: 500; -} - -.search-box button:hover { - background: #5568d3; -} - -.filters { - display: flex; - gap: 10px; -} - -.filters select { - padding: 10px 15px; - border: 2px solid #ddd; - border-radius: 6px; - font-size: 14px; - cursor: pointer; + border-color: #5c6bc0; } -.btn-refresh { - padding: 10px 20px; - background: #ff9800; - color: white; +.btn { + padding: 8px 16px; border: none; - border-radius: 6px; + border-radius: 4px; cursor: pointer; font-weight: 500; font-size: 14px; - transition: all 0.2s; -} - -.btn-refresh:hover { - background: #f57c00; -} - -.btn-refresh:disabled { - opacity: 0.6; - cursor: not-allowed; -} - -.content { - display: grid; - grid-template-columns: 250px 1fr; - gap: 20px; -} - -.sidebar { - background: white; - padding: 20px; - border-radius: 8px; - box-shadow: 0 2px 4px rgba(0, 0, 0, 0.1); - height: fit-content; - position: sticky; - top: 20px; -} - -.sidebar h2 { - margin-bottom: 15px; - color: #333; -} - -.sidebar ul { - list-style: none; -} - -.sidebar li { - padding: 10px; - margin: 5px 0; - border-radius: 6px; - cursor: pointer; - transition: background 0.2s; -} - -.sidebar li:hover { - background: #f0f0f0; -} - -.sidebar li.active { - background: #667eea; - color: white; -} - -.main-content { - background: white; - padding: 20px; - border-radius: 8px; - box-shadow: 0 2px 4px rgba(0, 0, 0, 0.1); - min-height: 400px; -} - -.loading, -.error { - text-align: center; - padding: 40px; - color: #666; -} - -.no-skills, .no-results { - text-align: center; - padding: 40px; - color: #666; - background: white; - border-radius: 8px; - margin: 20px 0; - font-size: 1.1em; - line-height: 1.8; -} - -.error { - color: #d32f2f; - background: #ffebee; - border-radius: 6px; -} - -.skills-container { - display: none; -} - -.skills-list { - display: grid; - gap: 15px; -} - -.skill-card { - border: 1px solid #e0e0e0; - border-radius: 8px; - padding: 18px; - transition: all 0.2s; - background: #fff; - box-shadow: 0 1px 2px rgba(0, 0, 0, 0.04); -} - -.skill-card:hover { - box-shadow: 0 2px 8px rgba(0, 0, 0, 0.08); - border-color: #c5cae9; -} - -.skill-card.installed { - border-left: 4px solid #28a745; -} - -.skill-card.available { - border-left: 4px solid #667eea; -} - -.skill-header { - display: flex; - justify-content: space-between; - align-items: start; - margin-bottom: 10px; -} - -.skill-name { - font-size: 1.3em; - font-weight: 600; - color: #333; -} - -.skill-badge { - padding: 4px 12px; - border-radius: 12px; - font-size: 0.85em; - font-weight: 500; -} - -.skill-badge.installed { - background: #28a745; - color: white; -} - -.skill-badge.available { - background: #667eea; - color: white; -} - -.skill-status-badges { - display: flex; - gap: 8px; - flex-wrap: wrap; - align-items: center; -} - -.status-badge { - padding: 4px 10px; - border-radius: 12px; - font-size: 0.8em; - font-weight: 500; white-space: nowrap; + transition: background 0.15s; } -.status-badge.in-manifest { - background: #2196f3; - color: white; -} - -.status-badge.installed { - background: #28a745; - color: white; -} - -.status-badge.available { - background: #9e9e9e; - color: white; -} - -.skill-description { - color: #666; - margin-bottom: 15px; - line-height: 1.5; -} - -.skill-meta { - display: flex; - gap: 15px; - flex-wrap: wrap; - margin-bottom: 15px; - font-size: 0.9em; - color: #888; -} - -.skill-tags { - display: flex; - gap: 8px; - flex-wrap: wrap; - margin-bottom: 15px; -} - -.tag { - padding: 4px 10px; - background: #f0f0f0; - border-radius: 12px; - font-size: 0.85em; - color: #666; -} - -.skill-actions { - display: flex; - gap: 10px; -} - -.skill-actions button { - padding: 8px 16px; - border: none; - border-radius: 6px; - cursor: pointer; - font-weight: 500; - transition: all 0.2s; -} - -.btn-install { - background: #667eea; - color: white; -} - -.btn-install:hover { - background: #5568d3; -} - -.btn-uninstall { - background: #dc3545; - color: white; -} - -.btn-uninstall:hover { - background: #c82333; -} - -.btn-install:disabled, -.btn-uninstall:disabled { - opacity: 0.5; - cursor: not-allowed; -} - -.btn-upgrade-all { - padding: 10px 20px; - background: #5c6bc0; - color: white; - border: none; - border-radius: 6px; - cursor: pointer; - font-weight: 500; -} - -.btn-upgrade-all:hover:not(:disabled) { - background: #3f51b5; -} - -.btn-upgrade-all:disabled { +.btn:disabled { opacity: 0.6; cursor: not-allowed; } -.btn-upgrade { +.btn-primary { background: #5c6bc0; color: white; } -.btn-upgrade:hover { +.btn-primary:hover:not(:disabled) { background: #3f51b5; } -.source-link { - color: #5c6bc0; - text-decoration: none; +.btn-secondary { + background: #ffffff; + color: #495057; + border: 1px solid #dee2e6; } -.source-link:hover { - text-decoration: underline; +.btn-secondary:hover:not(:disabled) { + background: #f1f3f5; } -.source-none { - color: #9e9e9e; -} +/* ---- Content panel / sections (metadata, skills-dir) ---- */ .content-panel { background: #fff; @@ -438,6 +182,32 @@ body { border-radius: 3px; } +.break-all { + word-break: break-all; +} + +.muted { + color: #6c757d; + font-size: 0.875rem; +} + +.loading, +.error { + text-align: center; + padding: 24px; + font-size: 0.875rem; +} + +.error { + color: #dc3545; + background: #f8d7da; + border: 1px solid #f5c2c7; + border-radius: 6px; + margin-bottom: 16px; +} + +/* ---- Skills table ---- */ + .skills-table { width: 100%; border-collapse: collapse; @@ -458,10 +228,14 @@ body { } .skills-table .th-actions { - width: 160px; + width: 170px; text-align: right; } +.skills-table tbody tr { + cursor: pointer; +} + .skills-table tbody tr:hover { background: #f8f9fa; } @@ -487,13 +261,14 @@ body { } .skills-table .description { - max-width: 240px; + max-width: 220px; font-size: 0.8125rem; color: #6c757d; } .skills-table .location { word-break: break-all; + max-width: 220px; } .skills-table .location a { @@ -507,6 +282,7 @@ body { .skills-table .actions { text-align: right; + white-space: nowrap; } .skills-table .actions button { @@ -518,51 +294,218 @@ body { cursor: pointer; } +.btn-row { + background: #eef0fb; + color: #3f51b5; +} + +.btn-row:hover { + background: #dde1f5; +} + +.btn-row.btn-remove { + background: #fdecea; + color: #b3261e; +} + +.btn-row.btn-remove:hover { + background: #fadbd8; +} + .skills-table .empty { color: #6c757d; text-align: center; padding: 24px; } -.muted { +/* ---- Status badges ---- */ + +.badge { + display: inline-block; + padding: 3px 10px; + border-radius: 12px; + font-size: 0.75rem; + font-weight: 500; + white-space: nowrap; +} + +.badge-updatable { + background: #e3f2fd; + color: #1565c0; +} + +.badge-muted { + background: #f1f3f5; color: #6c757d; - font-size: 0.875rem; } -.loading, -.error { - text-align: center; - padding: 24px; +.badge-pinned { + background: #f3e8ff; + color: #7c3aed; +} + +.badge-error { + background: #fdecea; + color: #b3261e; +} + +/* ---- Detail drawer ---- */ + +.drawer { + position: fixed; + inset: 0; + visibility: hidden; + pointer-events: none; + z-index: 100; +} + +.drawer.open { + visibility: visible; + pointer-events: auto; +} + +.drawer-backdrop { + position: absolute; + inset: 0; + background: rgba(0, 0, 0, 0.25); + opacity: 0; + transition: opacity 0.2s; +} + +.drawer.open .drawer-backdrop { + opacity: 1; +} + +.drawer-panel { + position: absolute; + top: 0; + right: 0; + bottom: 0; + width: min(560px, 100%); + background: #fff; + box-shadow: -4px 0 16px rgba(0, 0, 0, 0.12); + display: flex; + flex-direction: column; + transform: translateX(100%); + transition: transform 0.2s ease-out; +} + +.drawer.open .drawer-panel { + transform: translateX(0); +} + +.drawer-header { + display: flex; + align-items: center; + justify-content: space-between; + padding: 16px 20px; + border-bottom: 1px solid #e9ecef; +} + +.drawer-header h2 { + font-size: 1.125rem; + font-weight: 600; + word-break: break-all; +} + +.btn-close { + background: none; + border: none; + font-size: 1.5rem; + line-height: 1; + color: #6c757d; + cursor: pointer; + padding: 4px 8px; +} + +.btn-close:hover { + color: #212529; +} + +.drawer-body { + padding: 16px 20px; + overflow-y: auto; + flex: 1; +} + +.drawer-subtitle { font-size: 0.875rem; + font-weight: 600; + color: #495057; + margin: 20px 0 8px; } -.error { - color: #dc3545; - background: #f8d7da; - border: 1px solid #f5c2c7; +.skill-content { + white-space: pre-wrap; + word-break: break-word; + font-family: ui-monospace, SFMono-Regular, Menlo, Consolas, monospace; + font-size: 0.8125rem; + background: #f8f9fa; + border: 1px solid #e9ecef; border-radius: 6px; - margin-bottom: 16px; + padding: 12px 14px; + max-height: 60vh; + overflow-y: auto; +} + +/* ---- Toasts ---- */ + +.toast-container { + position: fixed; + bottom: 20px; + right: 20px; + display: flex; + flex-direction: column; + gap: 8px; + z-index: 200; } -.skills-container { - display: block; +.toast { + min-width: 220px; + max-width: 360px; + padding: 10px 14px; + border-radius: 6px; + font-size: 0.875rem; + color: white; + background: #343a40; + box-shadow: 0 2px 8px rgba(0, 0, 0, 0.15); + opacity: 0; + transform: translateY(8px); + transition: opacity 0.2s, transform 0.2s; } -@media (max-width: 768px) { - .content { - grid-template-columns: 1fr; - } +.toast.show { + opacity: 1; + transform: translateY(0); +} - .sidebar { - position: static; - } +.toast-success { + background: #2f9e44; +} +.toast-error { + background: #c92a2a; +} + +.toast-info { + background: #343a40; +} + +@media (max-width: 768px) { .toolbar { flex-direction: column; align-items: stretch; } - .search-box { + .install-bar { min-width: auto; } -} \ No newline at end of file + + .skills-table .description { + display: none; + } + + .drawer-panel { + width: 100%; + } +} From cf2bf6a745e95fe26d7940f73e41f7ae00a07805 Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Wed, 22 Jul 2026 17:40:12 +0000 Subject: [PATCH 2/5] fix(ui): escape quotes in escapeHtml to close attribute-breakout XSS The textContent/innerHTML escaper left " and ' intact, allowing untrusted skill content (id/name/origin/reason) to break out of quoted attributes (title=, href=, data-id=) and inject event handlers. Escape all five HTML metacharacters explicitly (SEC-7/SEC-8). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_0153XpJc4DrMRAJJqVDchxRm --- crates/fastskill-core/src/http/static/app.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/crates/fastskill-core/src/http/static/app.js b/crates/fastskill-core/src/http/static/app.js index 4b106726..f34e5920 100644 --- a/crates/fastskill-core/src/http/static/app.js +++ b/crates/fastskill-core/src/http/static/app.js @@ -558,10 +558,18 @@ class FastSkillApp { // ---- utilities ---------------------------------------------------------- escapeHtml(text) { + // Escapes for BOTH element-text and quoted-attribute contexts. The + // textContent/innerHTML trick only escapes & < >, leaving " and ' + // intact — which is an attribute-breakout XSS when interpolated into + // title="…" / href="…" / data-id="…" with untrusted skill content + // (SEC-7/SEC-8). Escape all five explicitly; & must go first. if (text == null) return ''; - const div = document.createElement('div'); - div.textContent = String(text); - return div.innerHTML; + return String(text) + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/"/g, '"') + .replace(/'/g, '''); } } From 8e9fc075a67b7d097638ae58f193b46d9e3f8f1b Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Wed, 22 Jul 2026 17:44:35 +0000 Subject: [PATCH 3/5] feat(core,http): Origin-ref inference seam + skill content read endpoint (spec 003 phase 3) Add FastSkillService::infer_origin(&str) -> Result in a new core::origin_infer module: the single canonical string->Origin classification path (git/http(s) URL, .zip URL, local path, or scope/skill[@version] id), porting the CLI's detect_skill_source/ parse_git_url/is_skill_id logic into core so the HTTP install endpoint and the CLI's `add` command share one implementation instead of two drifting copies. - POST /api/v1/skills/install now takes { origin: string, groups?: [] } and resolves it via infer_origin before add_from_origin(Fresh); a bad ref or missing default repository is a 400, not a 500. - New GET /api/v1/skills/{id}/content (always-on read route, not write-gated) returns { path, content } for a skill's SKILL.md, path-confined to the configured skills directory via security::path::validate_path_within_root. - fastskill-cli's add command now delegates its common (no --source-type) Origin construction to service.infer_origin, applying --branch/--tag/ --editable on top of the inferred Origin. The --source-type override path keeps its own narrow per-variant construction. utils::is_skill_id/ parse_git_url/parse_skill_id/GitUrlInfo are now thin wrappers over the core seam instead of duplicating it. BREAKING CHANGE: InstallSkillRequest.origin changes from a typed Origin object to a plain ref string; there is no back-compat shim (greenfield API break per spec 003). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_0153XpJc4DrMRAJJqVDchxRm --- crates/fastskill-cli/src/commands/add/mod.rs | 63 ++- crates/fastskill-cli/src/utils.rs | 108 +--- crates/fastskill-core/src/core/mod.rs | 4 + .../fastskill-core/src/core/origin_infer.rs | 473 ++++++++++++++++++ .../src/http/handlers/skills.rs | 74 ++- crates/fastskill-core/src/http/models.rs | 21 +- crates/fastskill-core/src/http/server.rs | 1 + .../tests/http_handler_route_tests.rs | 106 +++- .../fastskill-core/tests/write_gate_tests.rs | 2 +- 9 files changed, 747 insertions(+), 105 deletions(-) create mode 100644 crates/fastskill-core/src/core/origin_infer.rs diff --git a/crates/fastskill-cli/src/commands/add/mod.rs b/crates/fastskill-cli/src/commands/add/mod.rs index 9bb6fa50..6d527565 100644 --- a/crates/fastskill-cli/src/commands/add/mod.rs +++ b/crates/fastskill-cli/src/commands/add/mod.rs @@ -357,9 +357,64 @@ fn validate_folder_has_skill(path: &Path) -> CliResult<()> { } /// Build the install-intent [`Origin`] for the common (single-skill, -/// project-level) add path from the classified `source` and parsed args -/// (ADR-0005: `detect_skill_source` → `Origin`, feeding `add_from_origin`). -fn build_origin(source: &SkillSource, args: &AddArgs) -> CliResult { +/// project-level) add path (ADR-0005 / spec 003 Phase 3: `infer_origin` is the +/// one canonical string → `Origin` inference path — the CLI no longer keeps +/// its own private copy of that classification logic). +/// +/// `--source-type` is an explicit CLI override of source classification (not +/// part of the Origin-ref inference contract itself, and not exercised by the +/// browser UI); it keeps its own narrow per-variant construction +/// (`build_origin_explicit_source_type`) rather than being routed through the +/// core seam. The natural (no `--source-type`) path below — the common case — +/// delegates the raw `args.source` string straight to +/// `FastSkillService::infer_origin`, then applies the CLI's explicit +/// `--branch`/`--tag`/`--editable` overrides on top of the inferred `Origin`. +async fn build_origin( + service: &FastSkillService, + source: &SkillSource, + args: &AddArgs, +) -> CliResult { + if args.source_type.is_some() { + return build_origin_explicit_source_type(source, args); + } + + let mut origin = service.infer_origin(&args.source).await?; + + match &mut origin { + Origin::Local { path, editable } => { + // `infer_origin` deliberately does not canonicalize (the fetch + // step resolves relative paths against cwd) — the CLI still wants + // an upfront, actionable "path does not exist" error and an + // absolute path recorded in the Manifest, mirroring pre-seam + // behavior. + let canonical = path.canonicalize().map_err(|e| { + CliError::InvalidSource(format!( + "Failed to resolve absolute path for '{}': {}", + path.display(), + e + )) + })?; + *path = canonical; + *editable = args.editable; + } + Origin::Git { r#ref, .. } => { + if let Some(b) = &args.branch { + *r#ref = GitRef::Branch(b.clone()); + } else if matches!(r#ref, GitRef::Default) { + if let Some(t) = &args.tag { + *r#ref = GitRef::Tag(t.clone()); + } + } + } + Origin::ZipUrl { .. } | Origin::Repository { .. } => {} + } + + Ok(origin) +} + +/// Pre-seam per-`SkillSource`-variant `Origin` construction, kept only for the +/// explicit `--source-type` override (see `build_origin`). +fn build_origin_explicit_source_type(source: &SkillSource, args: &AddArgs) -> CliResult { match source { SkillSource::ZipFile(path) => { let canonical = path.canonicalize().map_err(|e| { @@ -528,7 +583,7 @@ pub async fn execute_add(service: &FastSkillService, args: AddArgs, global: bool validate_skill_structure(path)?; } - let origin = build_origin(&source, &args)?; + let origin = build_origin(service, &source, &args).await?; let mode = if args.force { AddMode::Update } else { diff --git a/crates/fastskill-cli/src/utils.rs b/crates/fastskill-cli/src/utils.rs index a2875462..1b040e38 100644 --- a/crates/fastskill-cli/src/utils.rs +++ b/crates/fastskill-cli/src/utils.rs @@ -28,44 +28,27 @@ pub fn service_error_to_cli( CliError::Service(e) } -/// Git repository information parsed from URL -#[derive(Debug, Clone)] -pub struct GitUrlInfo { - pub repo_url: String, - pub branch: Option, - pub subdir: Option, -} - -/// Detect if input is a skill ID (format: skillid@version, skillid, or scope/skillid@version) +/// Git repository information parsed from URL. +/// +/// Re-exported from the core Origin-ref inference seam (ADR-0005 / spec 003 +/// Phase 3) rather than duplicated here — see `fastskill_core::core::origin_infer`. +pub use fastskill_core::core::origin_infer::GitUrlInfo; + +/// Detect if input is a skill ID (format: skillid@version, skillid, or scope/skillid@version). +/// +/// Thin wrapper over the core seam's classification rule (ADR-0005 / spec 003 +/// Phase 3) — kept here only so the rest of the CLI's `SkillSource` +/// classification (which core has no notion of: Folder vs ZipFile, the +/// `--recursive`/`--global` paths, `--source-type` overrides) has a single +/// place to call it from. pub fn is_skill_id(input: &str) -> bool { - // Skill ID format: alphanumeric with dashes/underscores, optionally followed by @version or @tag - // Can also be scoped: scope/skillid or scope/skillid@version - // Examples: pptx@1.2.3, web-scraper, my_skill@2.0.0, aroff@teste, dev-user/test-skill, org/my-skill@1.0.0 - // Must not contain backslashes or colons (URL schemes) - if input.contains('\\') || input.contains(':') { - return false; - } - - // Check if it looks like a skill ID (not a file path) - // Supports both unscoped (skillid) and scoped (scope/skillid) formats - // The @suffix can be a version (1.2.3) or any identifier (teste, label, etc.) - // This regex is a compile-time constant pattern, so it should never fail - #[allow(clippy::expect_used)] - let skill_id_pattern = - regex::Regex::new(r"^[a-zA-Z0-9_-]+(/[a-zA-Z0-9_-]+)?(@[a-zA-Z0-9_.-]+)?$") - .expect("Invalid regex pattern"); - skill_id_pattern.is_match(input) + fastskill_core::core::origin_infer::is_skill_id(input) } -/// Parse skill ID and version from input (format: skillid@version) +/// Parse skill ID and version from input (format: skillid@version). Thin +/// wrapper over the core seam's ref splitting (see `is_skill_id`). pub fn parse_skill_id(input: &str) -> (String, Option) { - if let Some(at_pos) = input.find('@') { - let skill_id = input[..at_pos].to_string(); - let version = input[at_pos + 1..].to_string(); - (skill_id, Some(version)) - } else { - (input.to_string(), None) - } + fastskill_core::core::origin_infer::parse_skill_id_ref(input) } /// Detect the type of skill source (zip, folder, git URL, remote zip URL, or skill ID) @@ -116,59 +99,12 @@ pub enum SkillSource { SkillId(String), } -/// Parse git URL to extract repository information +/// Parse git URL to extract repository information. Thin wrapper over the +/// core seam's `parse_git_url` (ADR-0005 / spec 003 Phase 3) — the CLI keeps +/// only the `ServiceError` → `CliError` mapping. pub fn parse_git_url(git_url: &str) -> CliResult { - let url = Url::parse(git_url) - .map_err(|e| CliError::InvalidSource(format!("Invalid git URL: {}", e)))?; - - // Extract query parameters for branch - let query_branch = url - .query_pairs() - .find(|(key, _)| key == "branch") - .map(|(_, value)| value.to_string()); - - // For GitHub tree URLs, extract branch and subdir - let path_segments: Vec<&str> = url.path().split('/').filter(|s| !s.is_empty()).collect(); - let mut branch = None; - let mut subdir = None; - - if url.host_str() == Some("github.com") - && path_segments.len() >= 3 - && path_segments.get(2) == Some(&"tree") - && path_segments.len() >= 4 - { - // GitHub tree URL: /org/repo/tree/branch[/subdir...] - branch = Some(path_segments[3].to_string()); - if path_segments.len() > 4 { - subdir = Some(std::path::PathBuf::from(path_segments[4..].join("/"))); - } - } - - // Construct clean repo URL - let mut clean_url = url.clone(); - clean_url.set_query(None); - let mut path = clean_url.path().to_string(); - - // Remove tree/branch/subdir from path for GitHub tree URLs - if url.host_str() == Some("github.com") && path.contains("/tree/") { - if let Some(tree_pos) = path.find("/tree/") { - path = path[..tree_pos].to_string(); - } - } - - // Ensure .git extension - if !path.ends_with(".git") { - path.push_str(".git"); - } - - clean_url.set_path(&path); - let repo_url = clean_url.to_string(); - - Ok(GitUrlInfo { - repo_url, - branch: branch.or(query_branch), - subdir, - }) + fastskill_core::core::origin_infer::parse_git_url(git_url) + .map_err(|e| CliError::InvalidSource(e.to_string())) } /// Validate skill structure follows Claude Code standard. diff --git a/crates/fastskill-core/src/core/mod.rs b/crates/fastskill-core/src/core/mod.rs index edd6e5b4..bc96f6e9 100644 --- a/crates/fastskill-core/src/core/mod.rs +++ b/crates/fastskill-core/src/core/mod.rs @@ -13,6 +13,7 @@ pub mod lock; pub mod manifest; pub mod metadata; pub mod origin; +pub mod origin_infer; pub mod project; pub mod project_config; pub mod reconciliation; @@ -67,6 +68,9 @@ pub use metadata::{ // origin pub use origin::{GitRef, Origin, Resolved}; +// origin_infer (the Origin-ref inference seam, spec 003 Phase 3) +pub use origin_infer::{is_skill_id, parse_git_url, parse_skill_id_ref, GitUrlInfo}; + // install seam pub use install::{AddMode, AddOutcome, Fetched, UpdatePreflight}; diff --git a/crates/fastskill-core/src/core/origin_infer.rs b/crates/fastskill-core/src/core/origin_infer.rs new file mode 100644 index 00000000..6d0c2818 --- /dev/null +++ b/crates/fastskill-core/src/core/origin_infer.rs @@ -0,0 +1,473 @@ +//! The core `Origin`-ref inference seam (spec 003 Phase 3, "smart input"). +//! +//! `FastSkillService::infer_origin(&str) -> Result` resolves +//! a single raw string — the **Origin ref** a user types or pastes (a git URL, a +//! `.zip` URL, a local path, or a `scope/skill[@version]` id) — into a typed +//! [`Origin`]. This is the *one* inference path: both `fastskill serve`'s HTTP +//! install endpoint and `fastskill-cli`'s `add` command call it, so there is a +//! single place the git-URL/skill-id/path classification rules live rather than +//! two copies drifting apart (the CLI previously had its own private +//! `detect_skill_source` + `parse_git_url`; see ADR-0005 and the "Origin ref" +//! entry in `CONTEXT.md`). +//! +//! Classification rules (identical to the pre-seam CLI behavior): +//! - `scope/skill`, `skill`, or `skill[@version]` (an id, not a URL/path) → +//! [`Origin::Repository`], resolved against the injected +//! [`RepositoryManager`](crate::core::repository::RepositoryManager)'s default +//! repository (recorded by its concrete name — ADR-0005 §Q4). +//! - A `git`/`http`/`https` URL whose path does **not** end in `.zip` → +//! [`Origin::Git`] (branch/tag/subdir parsed the same way `parse_git_url` did: +//! a `?branch=` query param, or a GitHub `/tree/[/]` path). +//! - A URL whose path ends in `.zip` → [`Origin::ZipUrl`]. +//! - Anything else (a local path, `.zip` or directory) → [`Origin::Local`] +//! (`editable: false`; the CLI's `-e`/`--editable` flag is applied on top by +//! the caller, since it is not part of the ref string itself). + +use crate::core::origin::{GitRef, Origin}; +use crate::core::service::{FastSkillService, ServiceError}; +use crate::core::version::VersionConstraint; +use std::path::PathBuf; +use url::Url; + +/// Git repository info parsed from a URL: the clean clone URL plus any +/// branch/subdir extracted from query params or a GitHub tree-URL path. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct GitUrlInfo { + /// The repository URL, normalized to end in `.git`. + pub repo_url: String, + pub branch: Option, + pub subdir: Option, +} + +/// True for skill-id refs: `skill`, `skill@version`, `scope/skill`, or +/// `scope/skill@version`. Not a URL scheme (contains `:`) or a Windows-style +/// path (contains `\`). +pub fn is_skill_id(input: &str) -> bool { + if input.contains('\\') || input.contains(':') { + return false; + } + #[allow(clippy::expect_used)] + let skill_id_pattern = + regex::Regex::new(r"^[a-zA-Z0-9_-]+(/[a-zA-Z0-9_-]+)?(@[a-zA-Z0-9_.-]+)?$") + .expect("skill-id pattern is a compile-time constant and always valid"); + skill_id_pattern.is_match(input) +} + +/// Split `skill@version` into `(skill, Some(version))`, or `(skill, None)` when +/// there is no `@`. +pub fn parse_skill_id_ref(input: &str) -> (String, Option) { + match input.find('@') { + Some(at_pos) => ( + input[..at_pos].to_string(), + Some(input[at_pos + 1..].to_string()), + ), + None => (input.to_string(), None), + } +} + +/// Parse a git URL into a clean clone URL plus any branch/subdir it encodes: +/// a `?branch=` query param, or a GitHub `/org/repo/tree/[/]` +/// path. Mirrors the pre-seam CLI's `parse_git_url`. +pub fn parse_git_url(git_url: &str) -> Result { + let url = Url::parse(git_url) + .map_err(|e| ServiceError::InvalidOperation(format!("Invalid git URL: {e}")))?; + + let query_branch = url + .query_pairs() + .find(|(key, _)| key == "branch") + .map(|(_, value)| value.to_string()); + + let path_segments: Vec<&str> = url.path().split('/').filter(|s| !s.is_empty()).collect(); + let mut branch = None; + let mut subdir = None; + + if url.host_str() == Some("github.com") + && path_segments.len() >= 3 + && path_segments.get(2) == Some(&"tree") + && path_segments.len() >= 4 + { + // GitHub tree URL: /org/repo/tree/branch[/subdir...] + branch = Some(path_segments[3].to_string()); + if path_segments.len() > 4 { + subdir = Some(PathBuf::from(path_segments[4..].join("/"))); + } + } + + let mut clean_url = url.clone(); + clean_url.set_query(None); + let mut path = clean_url.path().to_string(); + + if url.host_str() == Some("github.com") && path.contains("/tree/") { + if let Some(tree_pos) = path.find("/tree/") { + path = path[..tree_pos].to_string(); + } + } + + if !path.ends_with(".git") { + path.push_str(".git"); + } + + clean_url.set_path(&path); + let repo_url = clean_url.to_string(); + + Ok(GitUrlInfo { + repo_url, + branch: branch.or(query_branch), + subdir, + }) +} + +impl FastSkillService { + /// Resolve a raw **Origin ref** string into a typed [`Origin`] (ADR-0005 / + /// spec 003 Phase 3). See the module docs for the full classification + /// table. This does not fetch or validate anything — it only classifies; + /// `add_from_origin` performs the actual fetch. + pub async fn infer_origin(&self, origin_ref: &str) -> Result { + let trimmed = origin_ref.trim(); + if trimmed.is_empty() { + return Err(ServiceError::InvalidOperation( + "Origin ref cannot be empty".to_string(), + )); + } + + if is_skill_id(trimmed) { + return self.infer_repository_origin(trimmed).await; + } + + if let Ok(url) = Url::parse(trimmed) { + if matches!(url.scheme(), "git" | "http" | "https") { + if url.path().to_ascii_lowercase().ends_with(".zip") { + return Ok(Origin::ZipUrl { + url: trimmed.to_string(), + }); + } + let git_info = parse_git_url(trimmed)?; + let r#ref = match &git_info.branch { + Some(b) => GitRef::Branch(b.clone()), + None => GitRef::Default, + }; + // The recorded URL is the ref exactly as given (not the + // `.git`-normalized `repo_url`) — `parse_git_url` is consulted + // only for the branch/subdir it can extract, mirroring the + // pre-seam CLI's `build_origin`. + return Ok(Origin::Git { + url: trimmed.to_string(), + r#ref, + subdir: git_info.subdir, + }); + } + } + + // Anything else is a local filesystem path — a `.zip` archive or a + // directory; the installer detects which at fetch time + // (`add_from_origin`'s `fetch_local`), so both map to `Origin::Local`. + Ok(Origin::Local { + path: PathBuf::from(trimmed), + editable: false, + }) + } + + /// Resolve a classified skill-id ref (`scope/skill`, `skill`, or + /// `skill@version`) into `Origin::Repository`, against the injected + /// [`RepositoryManager`](crate::core::repository::RepositoryManager)'s + /// default repository (recorded by its concrete name, ADR-0005 §Q4). + async fn infer_repository_origin(&self, skill_id_ref: &str) -> Result { + let (skill, version_str) = parse_skill_id_ref(skill_id_ref); + let version = version_str + .as_deref() + .map(VersionConstraint::parse) + .transpose() + .map_err(|e| { + ServiceError::InvalidOperation(format!("Invalid version constraint: {e}")) + })?; + + let repo_manager = self.repository_manager().ok_or_else(|| { + ServiceError::Config( + "No default repository configured. Use 'fastskill repos add' to add a \ + repository before installing by skill id." + .to_string(), + ) + })?; + let default_repo = repo_manager.get_default_repository().ok_or_else(|| { + ServiceError::Config( + "No default repository configured. Use 'fastskill repos add' to add a \ + repository before installing by skill id." + .to_string(), + ) + })?; + + Ok(Origin::Repository { + repo: default_repo.name.clone(), + skill, + version, + }) + } +} + +#[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] +mod tests { + use super::*; + use crate::core::repository::RepositoryManager; + use crate::{FastSkillService, ServiceConfig}; + use std::sync::Arc; + use tempfile::TempDir; + + async fn make_service(storage: &std::path::Path) -> FastSkillService { + let config = ServiceConfig { + skill_storage_path: storage.to_path_buf(), + ..Default::default() + }; + let mut service = FastSkillService::new(config).await.unwrap(); + service.initialize().await.unwrap(); + service + } + + fn with_default_repo(service: FastSkillService, name: &str) -> FastSkillService { + use crate::core::repository::{RepositoryConfig, RepositoryDefinition, RepositoryType}; + let manager = RepositoryManager::from_definitions(vec![RepositoryDefinition { + name: name.to_string(), + repo_type: RepositoryType::HttpRegistry, + priority: 0, + config: RepositoryConfig::HttpRegistry { + index_url: "https://example.com/index".to_string(), + }, + auth: None, + storage: None, + }]); + service.with_repository_manager(Arc::new(manager)) + } + + // ── git ──────────────────────────────────────────────────────────────── + + #[tokio::test] + async fn infer_git_default_branch() { + let tmp = TempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = service + .infer_origin("https://github.com/org/repo.git") + .await + .unwrap(); + assert_eq!( + origin, + Origin::Git { + url: "https://github.com/org/repo.git".to_string(), + r#ref: GitRef::Default, + subdir: None, + } + ); + } + + #[tokio::test] + async fn infer_git_branch_from_query_param() { + let tmp = TempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = service + .infer_origin("https://github.com/org/repo?branch=dev") + .await + .unwrap(); + assert_eq!( + origin, + Origin::Git { + url: "https://github.com/org/repo?branch=dev".to_string(), + r#ref: GitRef::Branch("dev".to_string()), + subdir: None, + } + ); + } + + #[tokio::test] + async fn infer_git_tree_url_branch_and_subdir() { + let tmp = TempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = service + .infer_origin("https://github.com/org/repo/tree/main/skills/inner") + .await + .unwrap(); + assert_eq!( + origin, + Origin::Git { + url: "https://github.com/org/repo/tree/main/skills/inner".to_string(), + r#ref: GitRef::Branch("main".to_string()), + subdir: Some(PathBuf::from("skills/inner")), + } + ); + } + + #[tokio::test] + async fn infer_git_tag_is_not_inferred_from_string() { + // There is no ref-string encoding for "tag" (only a GitHub tree URL's + // branch, or a `?branch=` query param) — tag selection is a CLI/UI + // override applied on top of the inferred `Default` ref. + let tmp = TempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = service + .infer_origin("https://gitlab.com/org/repo.git") + .await + .unwrap(); + assert_eq!( + origin, + Origin::Git { + url: "https://gitlab.com/org/repo.git".to_string(), + r#ref: GitRef::Default, + subdir: None, + } + ); + } + + // ── zip-url ──────────────────────────────────────────────────────────── + + #[tokio::test] + async fn infer_zip_url() { + let tmp = TempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = service + .infer_origin("https://example.com/skills/bundle.zip") + .await + .unwrap(); + assert_eq!( + origin, + Origin::ZipUrl { + url: "https://example.com/skills/bundle.zip".to_string(), + } + ); + } + + #[tokio::test] + async fn infer_zip_url_with_query_string() { + let tmp = TempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = service + .infer_origin("https://example.com/download.zip?token=abc") + .await + .unwrap(); + assert!(matches!(origin, Origin::ZipUrl { .. })); + } + + // ── local ────────────────────────────────────────────────────────────── + + #[tokio::test] + async fn infer_local_dir() { + let tmp = TempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = service.infer_origin("./some/folder").await.unwrap(); + assert_eq!( + origin, + Origin::Local { + path: PathBuf::from("./some/folder"), + editable: false, + } + ); + } + + #[tokio::test] + async fn infer_local_zip() { + let tmp = TempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let origin = service.infer_origin("./bundle.zip").await.unwrap(); + assert_eq!( + origin, + Origin::Local { + path: PathBuf::from("./bundle.zip"), + editable: false, + } + ); + } + + // ── skill id / repository ────────────────────────────────────────────── + + #[tokio::test] + async fn infer_scoped_skill_id() { + let tmp = TempDir::new().unwrap(); + let service = with_default_repo(make_service(&tmp.path().join("storage")).await, "main"); + let origin = service.infer_origin("acme/widget").await.unwrap(); + assert_eq!( + origin, + Origin::Repository { + repo: "main".to_string(), + skill: "acme/widget".to_string(), + version: None, + } + ); + } + + #[tokio::test] + async fn infer_bare_skill_id() { + let tmp = TempDir::new().unwrap(); + let service = with_default_repo(make_service(&tmp.path().join("storage")).await, "main"); + let origin = service.infer_origin("web-scraper").await.unwrap(); + assert_eq!( + origin, + Origin::Repository { + repo: "main".to_string(), + skill: "web-scraper".to_string(), + version: None, + } + ); + } + + #[tokio::test] + async fn infer_skill_id_with_version() { + let tmp = TempDir::new().unwrap(); + let service = with_default_repo(make_service(&tmp.path().join("storage")).await, "main"); + let origin = service.infer_origin("pptx@1.2.3").await.unwrap(); + match origin { + Origin::Repository { + repo, + skill, + version, + } => { + assert_eq!(repo, "main"); + assert_eq!(skill, "pptx"); + assert_eq!(version.expect("version constraint").to_string(), "=1.2.3"); + } + other => panic!("expected Repository, got {:?}", other), + } + } + + #[tokio::test] + async fn infer_skill_id_no_default_repo_errors() { + let tmp = TempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let result = service.infer_origin("acme/widget").await; + assert!(matches!(result, Err(ServiceError::Config(_)))); + } + + #[tokio::test] + async fn infer_empty_ref_errors() { + let tmp = TempDir::new().unwrap(); + let service = make_service(&tmp.path().join("storage")).await; + let result = service.infer_origin(" ").await; + assert!(matches!(result, Err(ServiceError::InvalidOperation(_)))); + } + + // ── parse_git_url / is_skill_id (unit-level) ──────────────────────────── + + #[test] + fn parse_git_url_plain_adds_git_suffix() { + let info = parse_git_url("https://github.com/org/repo").unwrap(); + assert_eq!(info.repo_url, "https://github.com/org/repo.git"); + assert!(info.branch.is_none()); + assert!(info.subdir.is_none()); + } + + #[test] + fn parse_git_url_invalid_errors() { + let result = parse_git_url("not a url"); + assert!(matches!(result, Err(ServiceError::InvalidOperation(_)))); + } + + #[test] + fn is_skill_id_accepts_scoped_and_bare() { + assert!(is_skill_id("web-scraper")); + assert!(is_skill_id("scope/id")); + assert!(is_skill_id("scope/id@1.0.0")); + } + + #[test] + fn is_skill_id_rejects_urls_and_paths() { + assert!(!is_skill_id("https://github.com/org/repo")); + assert!(!is_skill_id("git@github.com:org/repo")); + assert!(!is_skill_id("/absolute/path")); + } +} diff --git a/crates/fastskill-core/src/http/handlers/skills.rs b/crates/fastskill-core/src/http/handlers/skills.rs index 3a37cf48..10b757c6 100644 --- a/crates/fastskill-core/src/http/handlers/skills.rs +++ b/crates/fastskill-core/src/http/handlers/skills.rs @@ -85,6 +85,58 @@ pub async fn get_skill( Ok(axum::Json(ApiResponse::success(response))) } +/// GET /api/v1/skills/{id}/content - Read-only view of the installed skill's +/// `SKILL.md` (spec 003 §5 / Phase 3 §Q4). Always mounted on the read router +/// (not write-gated). PATH-CONFINEMENT is a hard requirement here: the +/// resolved file must canonicalize to somewhere inside the canonicalized +/// skills directory, or the request is rejected — this endpoint must never +/// become a directory-traversal primitive, even though `serve` itself is not a +/// security boundary (ADR-0003). +pub async fn get_skill_content( + State(state): State, + Path(skill_id): Path, +) -> HttpResult>> { + let skill_id_parsed = crate::core::service::SkillId::new(skill_id.clone()) + .map_err(|_| HttpError::BadRequest("Invalid skill ID format".to_string()))?; + + let skills = state.service.skill_manager().list_skills().await?; + let skill = skills + .into_iter() + .find(|s| s.id == skill_id_parsed) + .ok_or_else(|| HttpError::NotFound(format!("Skill not found: {}", skill_id)))?; + + // `skill_file` may be stored relative (e.g. `./skills/{id}/SKILL.md`) or + // absolute; resolve a relative one against the skills directory before + // touching the filesystem. + let candidate = if skill.skill_file.is_absolute() { + skill.skill_file.clone() + } else { + state.skills_directory.join(&skill.skill_file) + }; + + let confined = + crate::security::path::validate_path_within_root(&candidate, &state.skills_directory) + .map_err(|e| match e { + crate::security::path::PathSecurityError::EscapesRoot(msg) + | crate::security::path::PathSecurityError::TraversalAttempt(msg) + | crate::security::path::PathSecurityError::InvalidComponent(msg) => { + HttpError::BadRequest(msg) + } + crate::security::path::PathSecurityError::CanonicalizationFailed(_) => { + HttpError::NotFound(format!("Skill file not found on disk: {}", skill_id)) + } + })?; + + let content = tokio::fs::read_to_string(&confined) + .await + .map_err(|_| HttpError::NotFound(format!("Skill file not found on disk: {}", skill_id)))?; + + Ok(axum::Json(ApiResponse::success(SkillContentResponse { + path: confined.to_string_lossy().to_string(), + content, + }))) +} + /// DELETE /api/skills/{id} - Delete skill (remove from manifest and storage, unregister) pub async fn delete_skill( State(state): State, @@ -157,17 +209,29 @@ pub async fn delete_skill( })))) } -/// POST /api/v1/skills/install - Fresh-install a skill from an [`Origin`] -/// (core install seam, ADR-0005 / spec 003 §2). `AddMode::Fresh` fails with a -/// 409 if the resolved id is already installed; other seam errors map to -/// 400/500 via the blanket `ServiceError` → `HttpError` conversion. +/// POST /api/v1/skills/install - Fresh-install a skill from an **Origin ref** +/// string (core install seam, ADR-0005 / spec 003 §2 + Phase 3). The server +/// classifies `request.origin` via `infer_origin` — the UI performs no +/// detection of its own — then `AddMode::Fresh` fails with a 409 if the +/// resolved id is already installed; other seam errors map to 400/500 via the +/// blanket `ServiceError` → `HttpError` conversion. pub async fn install_skill( State(state): State, Json(request): Json, ) -> HttpResult<(StatusCode, axum::Json>)> { + // A bad ref / no-default-repo is a client error (400), not a 500 — surface + // it distinctly from the blanket `ServiceError` → `HttpError` conversion + // below (which maps `Config` to 500, appropriate for the *fetch* path but + // not for classifying the ref itself). + let origin = state + .service + .infer_origin(&request.origin) + .await + .map_err(|e| HttpError::BadRequest(e.to_string()))?; + match state .service - .add_from_origin(request.origin, AddMode::Fresh, request.groups) + .add_from_origin(origin, AddMode::Fresh, request.groups) .await { Ok(outcome) => { diff --git a/crates/fastskill-core/src/http/models.rs b/crates/fastskill-core/src/http/models.rs index 66434566..e21761b1 100644 --- a/crates/fastskill-core/src/http/models.rs +++ b/crates/fastskill-core/src/http/models.rs @@ -249,13 +249,15 @@ pub struct UpdateSkillRequest { pub version: Option, } -/// POST /api/v1/skills/install request body: a fresh install from an [`Origin`] -/// (core install seam, ADR-0005). `Origin` deserializes directly (internally -/// tagged by `type`). +/// POST /api/v1/skills/install request body: a fresh install from an **Origin +/// ref** — a raw string (git URL, `.zip` URL, local path, or +/// `scope/skill[@version]` id) that the server classifies via the core +/// `infer_origin` seam (ADR-0005 / spec 003 Phase 3) — the UI performs no +/// detection of its own; it just sends what the user typed. #[derive(Debug, Deserialize, Clone)] #[serde(rename_all = "camelCase")] pub struct InstallSkillRequest { - pub origin: crate::core::origin::Origin, + pub origin: String, #[serde(default)] pub groups: Vec, } @@ -280,6 +282,17 @@ pub struct UpdateSkillsRequest { pub check: bool, } +/// GET /api/v1/skills/{id}/content response: the installed skill's `SKILL.md` +/// as raw text (path-confined to the skills directory; see +/// `handlers::skills::get_skill_content`). Rendered HTML-escaped by the UI — +/// no Markdown rendering (spec 003 §5 / SEC-7). +#[derive(Debug, Serialize, Clone)] +#[serde(rename_all = "camelCase")] +pub struct SkillContentResponse { + pub path: String, + pub content: String, +} + /// Per-skill outcome of a `POST /api/v1/skills/update` call. #[derive(Debug, Serialize, Clone)] #[serde(rename_all = "camelCase")] diff --git a/crates/fastskill-core/src/http/server.rs b/crates/fastskill-core/src/http/server.rs index e40a3119..1a61fc1f 100644 --- a/crates/fastskill-core/src/http/server.rs +++ b/crates/fastskill-core/src/http/server.rs @@ -287,6 +287,7 @@ impl FastSkillServer { Router::new() .route("/skills", get(skills::list_skills)) .route("/skills/{id}", get(skills::get_skill)) + .route("/skills/{id}/content", get(skills::get_skill_content)) .route("/project", get(manifest::get_project)) .route("/search", post(search::search_skills)) .route("/resolve", post(resolve::resolve_context)) diff --git a/crates/fastskill-core/tests/http_handler_route_tests.rs b/crates/fastskill-core/tests/http_handler_route_tests.rs index b0a0e690..7429a174 100644 --- a/crates/fastskill-core/tests/http_handler_route_tests.rs +++ b/crates/fastskill-core/tests/http_handler_route_tests.rs @@ -99,6 +99,34 @@ async fn fixture_with_skills(enable_write: bool) -> Fixture { } } +/// A fixture where `state.skills_directory` actually points at the directory +/// the fixture's skills are written under (unlike `fixture_with_skills`, whose +/// `skills_directory` points at an unrelated, nonexistent path only used for +/// the manifest-view display string) — required for `get_skill_content`'s +/// path-confinement check to have anything meaningful to confine against. +async fn fixture_for_content() -> Fixture { + let storage = TempDir::new().unwrap(); + let store = skills_root(&storage); + write_skill(&store, "alpha-skill", "Alpha Skill", "First test skill"); + + let project = TempDir::new().unwrap(); + let project_file_path = project.path().join("skill-project.toml"); + + let service = make_service(store.clone(), None).await; + let mut state = AppState::new(service).unwrap(); + state.project_file_path = project_file_path.clone(); + state.project_root = project.path().to_path_buf(); + state.skills_directory = store; + state.enable_write = false; + + Fixture { + _storage: storage, + _project: project, + state, + project_file_path, + } +} + /// A fixture for the install/update seam (spec 003 Phase 2): a temp project /// with a valid `skill-project.toml` (so `resolve_project_file` succeeds), and /// the *service's* `project_root` injected — mirroring what `serve`'s edge @@ -160,6 +188,7 @@ fn router(state: AppState) -> Router { Router::new() .route("/skills", get(skills::list_skills)) .route("/skills/{id}", get(skills::get_skill)) + .route("/skills/{id}/content", get(skills::get_skill_content)) .route("/skills/{id}", delete(skills::delete_skill)) .route("/skills/install", post(skills::install_skill)) .route("/skills/update", post(skills::update_skills)) @@ -265,6 +294,73 @@ async fn get_skill_unknown_is_404() { assert_eq!(status, StatusCode::NOT_FOUND); } +#[tokio::test] +async fn get_skill_content_happy_path() { + let f = fixture_for_content().await; + let (status, body) = do_get(f.state, "/skills/alpha-skill/content").await; + assert_eq!(status, StatusCode::OK, "body: {body}"); + assert!(body.contains("Alpha Skill"), "body: {body}"); + assert!(body.contains("\"path\""), "body: {body}"); + assert!(body.contains("\"content\""), "body: {body}"); +} + +#[tokio::test] +async fn get_skill_content_unknown_id_is_404() { + let f = fixture_for_content().await; + let (status, _b) = do_get(f.state, "/skills/does-not-exist/content").await; + assert_eq!(status, StatusCode::NOT_FOUND); +} + +#[tokio::test] +async fn get_skill_content_missing_file_on_disk_is_404() { + // Skill is registered (indexed) but its SKILL.md has since been deleted + // from disk -- the confinement check passes (the parent dir still + // exists), but the read itself must 404, not 500. + let f = fixture_for_content().await; + let skill_file = f.state.skills_directory.join("alpha-skill/SKILL.md"); + fs::remove_file(&skill_file).unwrap(); + let (status, _b) = do_get(f.state, "/skills/alpha-skill/content").await; + assert_eq!(status, StatusCode::NOT_FOUND); +} + +#[tokio::test] +async fn get_skill_content_traversal_attempt_is_rejected() { + // Directly register a skill whose `skill_file` points outside the + // configured skills directory (simulating a corrupted/crafted record) -- + // the handler must refuse to read it rather than following the path. + let f = fixture_for_content().await; + + let outside_dir = TempDir::new().unwrap(); + let secret = outside_dir.path().join("secret.md"); + fs::write(&secret, "TOP SECRET").unwrap(); + + let id = fastskill_core::SkillId::new("escapee".to_string()).unwrap(); + let mut def = fastskill_core::SkillDefinition::new( + id, + "Escapee".to_string(), + "desc".to_string(), + "1.0.0".to_string(), + fastskill_core::core::origin::Origin::Local { + path: outside_dir.path().to_path_buf(), + editable: false, + }, + ); + def.skill_file = secret.clone(); + f.state + .service + .skill_manager() + .force_register_skill(def) + .await + .unwrap(); + + let (status, body) = do_get(f.state, "/skills/escapee/content").await; + assert_ne!(status, StatusCode::OK, "body: {body}"); + assert!( + !body.contains("TOP SECRET"), + "traversal must not leak file contents: {body}" + ); +} + #[tokio::test] async fn delete_skill_invalid_id_is_400() { let f = fixture_with_skills(true).await; @@ -317,7 +413,7 @@ async fn install_fresh_returns_201() { let (status, body) = post_json( f.state, "/skills/install", - serde_json::json!({"origin": {"type": "local", "path": src.to_string_lossy()}}), + serde_json::json!({"origin": src.to_string_lossy()}), ) .await; assert_eq!(status, StatusCode::CREATED, "body: {body}"); @@ -329,7 +425,7 @@ async fn install_fresh_returns_201() { async fn install_fresh_conflict_is_409() { let f = fixture_for_install(true).await; let src = write_source_skill(f._project.path(), "dup-skill"); - let payload = serde_json::json!({"origin": {"type": "local", "path": src.to_string_lossy()}}); + let payload = serde_json::json!({"origin": src.to_string_lossy()}); let (status1, body1) = post_json(f.state.clone(), "/skills/install", payload.clone()).await; assert_eq!(status1, StatusCode::CREATED, "body: {body1}"); @@ -347,7 +443,7 @@ async fn install_invalid_operation_is_400() { let (status, _b) = post_json( f.state, "/skills/install", - serde_json::json!({"origin": {"type": "local", "path": missing.to_string_lossy()}}), + serde_json::json!({"origin": missing.to_string_lossy()}), ) .await; assert_eq!(status, StatusCode::BAD_REQUEST); @@ -388,7 +484,7 @@ async fn update_check_mode_reports_would_update_without_applying() { let (install_status, install_body) = post_json( f.state.clone(), "/skills/install", - serde_json::json!({"origin": {"type": "local", "path": src.to_string_lossy()}}), + serde_json::json!({"origin": src.to_string_lossy()}), ) .await; assert_eq!(install_status, StatusCode::CREATED, "body: {install_body}"); @@ -410,7 +506,7 @@ async fn update_applies_updatable_origin() { let (install_status, install_body) = post_json( f.state.clone(), "/skills/install", - serde_json::json!({"origin": {"type": "local", "path": src.to_string_lossy()}}), + serde_json::json!({"origin": src.to_string_lossy()}), ) .await; assert_eq!(install_status, StatusCode::CREATED, "body: {install_body}"); diff --git a/crates/fastskill-core/tests/write_gate_tests.rs b/crates/fastskill-core/tests/write_gate_tests.rs index 655fa000..18757daa 100644 --- a/crates/fastskill-core/tests/write_gate_tests.rs +++ b/crates/fastskill-core/tests/write_gate_tests.rs @@ -79,7 +79,7 @@ async fn write_route_returns_403_when_write_disabled() { // Also cover the other new write routes (spec 003 Phase 2): install/update. let install_resp = client .post(format!("http://127.0.0.1:{port}/api/v1/skills/install")) - .json(&serde_json::json!({"origin": {"type": "local", "path": "/tmp/does-not-matter"}})) + .json(&serde_json::json!({"origin": "/tmp/does-not-matter"})) .send() .await .expect("POST /api/v1/skills/install"); From 53e7a6be8cfd5148821c8961dd847500519982d2 Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Wed, 22 Jul 2026 17:46:02 +0000 Subject: [PATCH 4/5] docs(context): add Origin ref glossary term + infer seam (spec 003 phase 3) Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_0153XpJc4DrMRAJJqVDchxRm --- CONTEXT.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CONTEXT.md b/CONTEXT.md index 1b7d118e..b7909089 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -73,6 +73,9 @@ _Avoid_: **source**, **registry** — both are deprecated command aliases now fo Where a single installed skill came from — the install **intent** (what the user asked for), recorded as provenance on the installed skill. Variants: `git` (url + ref + subdir), `local` (a filesystem path — directory *or* `.zip` — plus `editable`, dir-only), `zip-url` (a remote zip), and `repository` (a *reference into* a configured **Repository**: `{repo, skill, version?}`). The `repository` variant is the only one **Version constraint** / ADR-0004 governs; `git`/`local`/`zip-url` are ref-based and versionless. `Origin` is intent only: the **resolved** facts (exact commit, resolved version, checksum, timestamps) live in the **Lock**, not in `Origin`. It is the single canonical model — replacing the former `SkillSource` (two colliding types), `SourceType`, `SourceSpecificFields`, and the flat `source_*` fields on the manifest/lock/skill records. _Avoid_: **source** (banned, see above); do not blur `Origin::repository` (a reference; always names a concrete Repository) with **Repository** (the configured place itself). +**Origin ref**: +The *textual* form of an **Origin** — the single string a user types to name where a skill comes from (a git URL, a `.zip` URL, a local path, or `scope/skill`). It is resolved into a typed `Origin` by one seam, `Origin::infer(&str)`, which is the **only** place ref→`Origin` inference lives: both the CLI (`add`) and the HTTP install route call it, so the browser never re-implements detection. An Origin ref is *unresolved intent as text*; the `Origin` is *typed intent*; the **Lock** holds *resolved facts*. (Do not call it a "source" — banned.) + ### Serving surfaces Two orthogonal, first-class ways to expose skills to a client — distinguished by *protocol/consumer*, not redundant: From f52241e8ffeccd78cfbebf06b6745a9650e7be4a Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Wed, 22 Jul 2026 17:52:39 +0000 Subject: [PATCH 5/5] fix(http): return skills-dir-relative path from skill content endpoint Avoid disclosing the server's absolute filesystem layout in the GET /skills/{id}/content response; the UI only needs the logical location. Falls back to the bare file name. (opus review, Low) Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_0153XpJc4DrMRAJJqVDchxRm --- .../src/http/handlers/skills.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/crates/fastskill-core/src/http/handlers/skills.rs b/crates/fastskill-core/src/http/handlers/skills.rs index 10b757c6..24991242 100644 --- a/crates/fastskill-core/src/http/handlers/skills.rs +++ b/crates/fastskill-core/src/http/handlers/skills.rs @@ -131,8 +131,25 @@ pub async fn get_skill_content( .await .map_err(|_| HttpError::NotFound(format!("Skill file not found on disk: {}", skill_id)))?; + // Report the skills-dir-relative path, not the absolute server path — the UI + // only needs the logical location, and leaking the server's directory layout + // is needless disclosure if `serve` is ever exposed. Fall back to the bare + // file name if the relative strip fails. + let display_path = state + .skills_directory + .canonicalize() + .ok() + .and_then(|root| { + confined + .strip_prefix(&root) + .ok() + .map(std::path::Path::to_path_buf) + }) + .or_else(|| confined.file_name().map(std::path::PathBuf::from)) + .unwrap_or_else(|| confined.clone()); + Ok(axum::Json(ApiResponse::success(SkillContentResponse { - path: confined.to_string_lossy().to_string(), + path: display_path.to_string_lossy().to_string(), content, }))) }