From 2e295a146aa87e21dd3d875e716e6b88d16baa18 Mon Sep 17 00:00:00 2001 From: Miljan Martic Date: Wed, 22 Jul 2026 20:43:01 +0200 Subject: [PATCH 1/6] =?UTF-8?q?feat:=20v2=20=E2=80=94=20install=20from=20p?= =?UTF-8?q?ublic=20skill=20catalogs,=20compat=20verdicts,=20agent-first=20?= =?UTF-8?q?find?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v1 was a read-only browser of the agent's skills. v2 keeps that reading experience and adds the write half of the skills story, built on the platform's new skills API (mobius-os/mobius#146): - list from GET /api/skills: both skill shapes (flat .md and directory /SKILL.md), provenance chips, 30-day usage counts - catalog screen: curated public sources scanned with one git-trees call each (through /api/proxy), background summary prefetch, full-page skill detail with breadcrumbs, install via POST /api/skills/install (manage_skills) - compat verdicts: '✓ Works with Möbius' / '⚠ N things to know' chips with plain-language notes — pre-install on catalog cards and the skill page (predicted from the git tree), post-install on installed skills' rows and detail (judged from what's actually on disk via shared-list) - agent-first Find button (prefilled chat draft; the platform's finding-skills seed skill is the playbook) and two-tap uninstall for install-provenance skills - fires POST /api/skills/catalog-index/refresh when Browse opens, keeping the agent's cached catalog index fresh Version 2.0.0; adds catalog.js (dependency-free catalog core) and its test suite (64 tests total). Co-Authored-By: Claude Fable 5 --- .gitignore | 4 + README.md | 152 ++------ catalog.js | 266 +++++++++++++ domain.js | 53 ++- index.jsx | 891 ++++++++++++++++++++++++++++++++++++++++--- mobius.json | 10 +- test/catalog.test.js | 310 +++++++++++++++ test/domain.test.js | 54 +++ 8 files changed, 1569 insertions(+), 171 deletions(-) create mode 100644 .gitignore create mode 100644 catalog.js create mode 100644 test/catalog.test.js diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..db8043e --- /dev/null +++ b/.gitignore @@ -0,0 +1,4 @@ +.build/ +dist/ +node_modules/ +*.bak diff --git a/README.md b/README.md index 60d0d8d..a320b10 100644 --- a/README.md +++ b/README.md @@ -1,124 +1,56 @@ # app-skills — Skills -A [Möbius](https://github.com/mobius-os) catalog mini-app. Install it from the -in-app App Store. - -Skills is a **read-only catalog** for the agent's skill files — the SKILL-style -markdown guides under `/data/shared/skills/` that shape what the Möbius agent -can do. It enumerates the shared skills folder, parses each file's title and -one-line description, searches locally, and renders the selected skill as -sanitized markdown. Creating and editing a skill is routed to the agent (see -below), never written from the app. -Installed system apps that add always-on instructions are listed below the editable skills so their effect is visible. Those instructions apply for as long as the app stays installed; start a new chat after installing or uninstalling one to be sure the agent is working from the latest set. +A [Möbius](https://github.com/mobius-os) catalog mini-app. v1 was a read-only +skill browser; v2 keeps that reading experience and adds the write half of the +skills story. Requires a platform with the skills API (`mobius-os/mobius` +PR #146 or later). + +**Browse & read** — the list comes from `GET /api/skills`, so it shows every +skill shape (flat `.md` and directory `/SKILL.md`) with provenance +(built-in seed / agent-authored / app-owned / installed-from) and 30-day usage +counts. Tap a skill to read it as sanitized markdown (full markdown fetched +lazily from shared storage). + +**Find (agent-first)** — the ✦ header button opens an agent chat with a +prefilled draft. The agent's playbook is the platform's `finding-skills.md` +seed skill: where to look, how to judge fit, the trust ritual (read a +third-party SKILL.md fully and summarize what it instructs before installing), +and the exact install API call. + +**Catalog screen** — the ▤ header button opens a curated list of public repos +that host SKILL.md skills. One recursive git-trees call per source (through +`/api/proxy`) finds every skill — flat cards, no folder dead ends; summaries +prefetch in the background (raw-file fetches, no API rate cost). Cards install +via `POST /api/skills/install` (gated by this app's `manage_skills` +permission). Sources are app data (`sources.json`) — ask the agent to add a +repo. + +**Uninstall** — install-provenance skills get a two-tap remove button in the +detail view (`DELETE /api/skills/`; the server git-snapshots the bytes +first). Seeds, agent files, and app-owned skills keep their own lifecycles — +editing routes to the agent, as in v1. ## File layout | File | Role | |------|------| -| `index.jsx` | Default-export React component: the list, search, detail view, and all UI/state. | -| `domain.js` | Dependency-free core for parsing, link classification, errors, and supplemental-app load coordination. Network access is injected, so it is unit-testable without React or the DOM. | -| `test/domain.test.js` | Regression tests for `domain.js` (run with `npm test` → `node --test`). | -| `mobius.json` | App manifest (id, permissions, runtime deps, offline contract). | -| `icon.png` | App icon. | - -## Skill markdown shape - -Skill files may be plain markdown or include a small YAML frontmatter block. -`parseSkill()` strips frontmatter from the rendered detail and reads: - -- **Title** — the first `# Heading` outside any fenced code block. If a file has - no `#` heading, the title falls back to `name` from frontmatter, then to a - Title-Cased version of the slug (filename without `.md`). -- **Description** — the first non-empty paragraph after the title, up to ~240 - chars, stopping at the next heading, `---`, or a fenced code block. If the - body has no paragraph, `description` from frontmatter is used as a fallback. - -Fenced code blocks (``` ``` ``` or `~~~`) are tracked so a `# comment` or a -fence marker *inside* a code block is never mistaken for the title or the -description. - -## Data contract (shared storage, read-only) - -The app reads shared storage with its scoped app token — it can **read** shared -files but cannot **write** them, so all mutations route through the agent: - -- `GET /api/storage/shared-list/skills/` — enumerate the skills folder - (immediate children). The app keeps only `type: "file"`, `*.md`, non-dotfile - entries. It never probes guessed paths. -- `GET /api/storage/shared/skills/` — fetch one skill's markdown. - -A per-file fetch that fails (non-OK or thrown) keeps the row but marks it -**Unavailable** rather than rendering a blank skill, and emits an `error` signal -(`source: "skill_load"`) so a corrupt or permission-broken file isn't mistaken -for intentionally empty content. - -A failed **Refresh** keeps the last-known-good list on screen (it does not wipe -it) and surfaces a small "Couldn't refresh" pill; retry from the header refresh -button. The full error screen is reserved for the very first load. - -### Create / edit routes to the agent - -Skills are shared, owner-authored context. Because a mini-app can't write shared -storage, the **Edit** button (detail view) and the empty-state **Ask the agent** -button post a `moebius:new-chat` message to the shell with a pre-filled draft, -opening an agent chat rather than saving in-app. +| `index.jsx` | Default-export React component: list, detail, catalog screen, all UI/state. | +| `domain.js` | Dependency-free core: parsing, link classification, nav state machine, provenance/usage formatting, content-path selection. | +| `catalog.js` | Dependency-free catalog core: source list, tree-scan filtering, summary parsing, prefetch pool. Network is injected. | +| `test/` | Regression tests for both cores (`npm test` → `node --test`). | +| `mobius.json` | App manifest (id, permissions incl. `manage_skills`, runtime deps). | -## Link behavior in a rendered skill +## Dev loop -The whole app lives in one iframe, so an unhandled link click would navigate the -iframe away and brick the view. `classifyLink()` + a delegated click handler on -the rendered markdown route every link safely: - -- **Same-folder `.md` link** (e.g. `[shapes](app-component-shapes.md)` or - `./name.md`) → opened in-app via the detail view, no iframe navigation. If the - target isn't in the loaded set, the tap is swallowed (app stays up) and an - `error` signal fires. -- **`http:` / `https:` link** → opened in a new browser context via - `window.open(url, '_blank', 'noopener,noreferrer')`. -- **In-page `#fragment`** → left to default (harmless, doesn't replace the doc). -- **Any other protocol or sub-path** → navigation is blocked and an `error` - signal fires; the app stays mounted. - -## Signals (for Reflection) - -Emitted via `window.mobius.signal(...)`, all flat primitives: - -- `app_ready { item_count }` — once, after the first successful load (gated so - manual refreshes don't inflate open counts). -- `item_opened { type: "skill", slug }` — a skill detail was opened. -- `edit_requested { type: "skill", slug }` — the owner tapped Edit on a skill. -- `item_created { type: "skill" }` — the empty-state create CTA was launched (a - request to create; it does not claim a file now exists). -- `error { message, source, status? }` — `source` is `skill_load` (per-file - fetch), `markdown_render` (parse/sanitize), `skill_link` (blocked/unknown - link), or `load`/`refresh` (list load). - -## Offline - -This is an online-only viewer (`offline_capable: false`): it reads live shared -storage and has nothing useful to cache for a cold offline open. The manifest's -top-level `offline` object — -`{ "reads": false, "writes": "none", "execution": "none" }` — is the honest -offline contract. The installer reads it and stores it as the app's -`offline_contract` (exposed via the apps API), so it is kept deliberately, not -dead metadata. When offline, the app shows a plain "Offline" pill; the list -stays visible if it was already loaded. - -## Rendering safety - -Markdown is rendered with `DOMPurify.sanitize(marked.parse(...))` — never raw -`marked` output. `marked` and `dompurify` are declared as `esm_deps` in -`mobius.json`. - -## Development +In a dev instance, register once from a chat/shell: ```bash -npm test # node --test over test/*.test.js — no install needed (Node 18+) +cp -r app-skills /data/apps/skills +python "$SCRIPTS_DIR/register_app.py" skills \ + "Browse and install agent skills" /data/apps/skills/index.jsx ``` -The tests cover `domain.js` only (the pure core); the React UI has no runtime -dependencies to mock. To compile-smoke the entry: +Then edit files in place; the watcher recompiles on save. Note that +`register_app.py` does not apply manifest permissions — grant `manage_skills` +through the platform when testing installs. -```bash -esbuild index.jsx --bundle --packages=external --format=esm --loader:.js=jsx --outfile=/dev/null -``` diff --git a/catalog.js b/catalog.js new file mode 100644 index 0000000..eb362e9 --- /dev/null +++ b/catalog.js @@ -0,0 +1,266 @@ +// Dependency-free core for the catalog screen — the curated source list, +// git-trees scan filtering, SKILL.md summary parsing, and the background +// summary prefetch pool. No React and no direct network: fetching is injected +// by index.jsx (which routes it through /api/proxy), so everything here is +// unit-testable (see test/catalog.test.js). + +import { parseSkill, splitFrontmatter } from './domain.js' + +// Verified catalogs that HOST SKILL.md-format skills — link-list "awesome" +// repos don't render here (nothing installable to scan); hand those to the +// agent instead. `path` scopes the tree scan to a subtree; '' scans the whole +// repo. The list is app data too: a sources.json in app storage overrides it, +// so "add this repo as a source" is a chat request, not a code change. +export const DEFAULT_SOURCES = [ + { label: 'Anthropic Skills', repo: 'anthropics/skills', path: 'skills', ref: 'main', + blurb: 'Official Anthropic skills — documents, artifacts, MCP building, testing.' }, + { label: 'Anthropic Knowledge Work', repo: 'anthropics/knowledge-work-plugins', path: '', ref: 'main', + blurb: 'Anthropic’s knowledge-worker plugins — research, bio, finance, legal, and more.' }, + { label: 'Superpowers', repo: 'obra/superpowers', path: 'skills', ref: 'main', + blurb: 'The famous dev-methodology set — brainstorming, planning, TDD, debugging.' }, + { label: 'Trail of Bits Security', repo: 'trailofbits/skills', path: '', ref: 'main', + blurb: 'Security research, vulnerability detection, and audit workflows.' }, + { label: 'Cloudflare', repo: 'cloudflare/skills', path: 'skills', ref: 'main', + blurb: 'Official Cloudflare skills for building on Workers and the CF platform.' }, + { label: 'Hermes bundled', repo: 'NousResearch/hermes-agent', path: 'skills', ref: 'main', + blurb: 'Nous Research’s always-on Hermes agent skills.' }, + { label: 'Hermes optional', repo: 'NousResearch/hermes-agent', path: 'optional-skills', ref: 'main', + blurb: 'The big Hermes catalog — blockchain, research, media, agents, and more.' }, +] + +export function sourceKey(source) { + return `${source?.repo || ''}/${source?.path || ''}` +} + +// One recursive git-trees call finds every SKILL.md in the repo — flat cards, +// no folder drilling, no dead ends. This filters the raw tree down to skill +// directories under the source's path prefix. +export function treeToSkills(tree, pathPrefix) { + const prefix = String(pathPrefix || '').replace(/^\/+|\/+$/g, '') + const entries = Array.isArray(tree) ? tree : [] + return entries + .filter((t) => typeof t?.path === 'string' && t.path.endsWith('/SKILL.md')) + .map((t) => t.path.slice(0, -'/SKILL.md'.length)) + .filter((dir) => !prefix || dir === prefix || dir.startsWith(`${prefix}/`)) + .map((dir) => ({ dir, name: dir.split('/').pop() })) + .sort((a, b) => a.name.localeCompare(b.name)) +} + +// A raw SKILL.md → what the card shows. Frontmatter `description` is the +// ecosystem's "when to use this" line, so it wins over the first body +// paragraph; parseSkill supplies the fence-aware fallback and the +// frontmatter-stripped body for the peek. +export function catalogSummary(text) { + const { meta } = splitFrontmatter(text || '') + const parsed = parseSkill('SKILL.md', text || '') + return { + description: meta.description || parsed.description || 'No description in SKILL.md.', + license: meta.license || null, + peek: (parsed.content || '').trim().slice(0, 700) || null, + } +} + +// Mirror of the backend's install bounds (backend/app/routes/skills.py — +// _RESOURCE_COUNT_MAX / _RESOURCE_TOTAL_MAX / _RESOURCE_MAX_DEPTH / +// _RESOURCE_SUFFIXES). Advisory display only: the backend enforces for real, +// this just predicts what it will do so the badge can warn before install. +export const INSTALL_LIMITS = { + maxFiles: 24, + maxTotalBytes: 2 * 1024 * 1024, + maxDepth: 4, + suffixes: ['.md', '.txt', '.json', '.yaml', '.yml', '.csv', '.py', '.js', '.ts', + '.sh', '.toml', '.html', '.css'], +} + +const SCRIPT_SUFFIXES = ['.py', '.js', '.ts', '.sh'] + +function suffixOf(path) { + const base = path.split('/').pop() || '' + const dot = base.lastIndexOf('.') + return dot > 0 ? base.slice(dot).toLowerCase() : '' +} + +// Relative paths mentioned in SKILL.md — markdown links/images plus bare +// inline-code paths like `scripts/helper.py`. External URLs and anchors are +// not the skill's files, so they're skipped. +export function relativeRefs(raw) { + const refs = new Set() + const consider = (target, { needsSlash } = {}) => { + const t = String(target || '').trim().replace(/^\.\//, '').split(/[#?]/)[0] + if (!t || t.startsWith('/') || t.startsWith('~') || t.includes('..') || /^[a-z][a-z0-9+.-]*:/i.test(t)) return + // A shell snippet (`open /tmp/x.html`) or home path is not a bundled file. + if (/\s/.test(t)) return + if (needsSlash && !t.includes('/')) return + // Files only — a trailing dir ref like `scripts/` isn't checkable. + if (/\.[a-z0-9]{1,6}$/i.test(t)) refs.add(t) + } + for (const m of String(raw || '').matchAll(/!?\[[^\]]*\]\(([^)\s]+)[^)]*\)/g)) consider(m[1]) + // Inline code must be path-shaped (`scripts/helper.py`) — a bare `foo.json` + // is usually a generic mention, not a bundled file. + for (const m of String(raw || '').matchAll(/`([^`\n]+\.[a-z0-9]{1,5})`/gi)) consider(m[1], { needsSlash: true }) + return [...refs] +} + +// Predict how POST /api/skills/install would treat a catalog skill, from data +// the screen already holds: the source's recursive git tree and the raw +// SKILL.md. Returns { ok, caveats: [{ kind, text }] } — ok means "installs +// whole and indexes cleanly", caveats are ordered most→least serious. +export function assessCompat(tree, dir, raw) { + const caveats = [] + const prefix = `${String(dir || '').replace(/\/+$/g, '')}/` + const files = (Array.isArray(tree) ? tree : []) + .filter((t) => t?.type === 'blob' && typeof t?.path === 'string' && t.path.startsWith(prefix)) + .map((t) => ({ rel: t.path.slice(prefix.length), size: Number(t.size) || 0 })) + + const kept = [] + const dropped = [] + for (const f of files) { + if (/^skill\.md$/i.test(f.rel)) continue + const depthOk = f.rel.split('/').length - 1 <= INSTALL_LIMITS.maxDepth + const suffixOk = INSTALL_LIMITS.suffixes.includes(suffixOf(f.rel)) + ;(depthOk && suffixOk ? kept : dropped).push(f) + } + + // Install materializes resources in order and stops adding once over budget; + // predicting the exact survivors would overfit, so over-budget is its own + // "installs partially" caveat instead. + const total = kept.reduce((n, f) => n + f.size, 0) + const overCount = kept.length > INSTALL_LIMITS.maxFiles + const overSize = total > INSTALL_LIMITS.maxTotalBytes + + // A ref is broken when it names a file the install will drop, or a file the + // tree scan proves doesn't exist in the skill dir at all. + const keptSet = new Set(kept.map((f) => f.rel)) + const brokenRefs = relativeRefs(raw).filter( + (r) => !keptSet.has(r) && !/^skill\.md$/i.test(r), + ) + if (brokenRefs.length) { + caveats.push({ + kind: 'broken-refs', + text: `Its instructions mention files that won't be there after install (${nameSome(brokenRefs)}), so the steps that use them may not work.`, + }) + } + if (dropped.length) { + caveats.push({ + kind: 'dropped', + text: `${dropped.length} extra ${dropped.length === 1 ? 'file' : 'files'} won't be copied — Möbius only installs common text files, and ${dropped.length === 1 ? 'this one is' : 'these are'} a different type or buried too deep: ${nameSome(dropped.map((f) => f.rel))}. The main instructions still install fine.`, + }) + } + if (overCount || overSize) { + const parts = [] + if (overCount) parts.push(`${kept.length} files (max ${INSTALL_LIMITS.maxFiles})`) + if (overSize) parts.push(`${(total / (1024 * 1024)).toFixed(1)} MB (max ${INSTALL_LIMITS.maxTotalBytes / (1024 * 1024)} MB)`) + caveats.push({ + kind: 'over-budget', + text: `This skill is bigger than Möbius's install limit — ${parts.join(', ')} — so only part of it will be copied.`, + }) + } + + const scripts = kept.filter((f) => SCRIPT_SUFFIXES.includes(suffixOf(f.rel))) + if (scripts.length) { + caveats.push({ + kind: 'scripts', + text: `Comes with ${scripts.length} helper ${scripts.length === 1 ? 'script' : 'scripts'}. Möbius saves them for the agent to read — nothing runs automatically.`, + }) + } + + const fm = frontmatterCaveat(raw) + if (fm) caveats.push(fm) + + return { ok: caveats.length === 0, caveats } +} + +// Both flat parsers (here and backend) read only `key: value` scalars, so a +// YAML block scalar (`description: >`) leaves just the indicator behind. +function frontmatterCaveat(raw) { + const desc = String(splitFrontmatter(raw || '').meta.description || '').trim() + if (desc && !/^[>|][+-]?$/.test(desc)) return null + return { + kind: 'frontmatter', + text: 'Missing its one-line summary, so skill lists will show its first paragraph instead. Purely cosmetic.', + } +} + +// The same verdict for an already-INSTALLED skill, from what's actually on +// disk: `files` is the installed resource list relative to the skill dir +// (empty for flat skills). The repo-side caveats (dropped, over-budget) are +// install-time facts we can no longer see — their lasting symptom is a +// reference to a file that isn't there, which this does catch. +export function assessInstalled(files, raw) { + const caveats = [] + const rels = (Array.isArray(files) ? files : []) + .map((f) => String(f || '')) + .filter((r) => r && !/^skill\.md$/i.test(r)) + const have = new Set(rels) + + const broken = relativeRefs(raw).filter((r) => !have.has(r) && !/^skill\.md$/i.test(r)) + if (broken.length) { + caveats.push({ + kind: 'broken-refs', + text: `Its instructions mention files that aren't installed (${nameSome(broken)}), so the steps that use them may not work.`, + }) + } + + const scripts = rels.filter((r) => SCRIPT_SUFFIXES.includes(suffixOf(r))) + if (scripts.length) { + caveats.push({ + kind: 'scripts', + text: `Comes with ${scripts.length} helper ${scripts.length === 1 ? 'script' : 'scripts'}. Möbius saves them for the agent to read — nothing runs automatically.`, + }) + } + + const fm = frontmatterCaveat(raw) + if (fm) caveats.push(fm) + + return { ok: caveats.length === 0, caveats } +} + +function nameSome(names, cap = 4) { + const shown = names.slice(0, cap).join(', ') + return names.length > cap ? `${shown}, +${names.length - cap} more` : shown +} + +export function treeScanUrl(source) { + return `https://api.github.com/repos/${source.repo}/git/trees/${source.ref || 'main'}?recursive=1` +} + +export function rawSkillUrl(source, dir) { + return `https://raw.githubusercontent.com/${source.repo}/${source.ref || 'main'}/${dir}/SKILL.md` +} + +export function githubSkillUrl(source, dir) { + return `https://github.com/${source.repo}/blob/${source.ref || 'main'}/${dir}/SKILL.md` +} + +// Background prefetch pool: after a scan, walk every dir through `loadOne` +// with bounded concurrency so all summaries are loaded before the owner +// scrolls to them (raw-file fetches — no GitHub API rate cost). start() +// supersedes any previous pool via a generation counter, so switching sources +// mid-prefetch strands the stale workers instead of racing them; cancel() +// stops without starting a new pool. `loadOne` must dedupe by dir itself — +// viewport-priority loads from the cards may race the pool. +export function createSummaryPrefetcher({ loadOne, concurrency = 5 }) { + let generation = 0 + + function start(dirs) { + const gen = ++generation + const queue = Array.isArray(dirs) ? [...dirs] : [] + let next = 0 + const worker = () => { + if (gen !== generation) return + const dir = queue[next++] + if (dir === undefined) return + Promise.resolve() + .then(() => loadOne(dir)) + .then(worker, worker) + } + const workers = Math.min(concurrency, queue.length) + for (let k = 0; k < workers; k++) worker() + } + + function cancel() { + generation += 1 + } + + return { start, cancel } +} diff --git a/domain.js b/domain.js index 41a722f..727445e 100644 --- a/domain.js +++ b/domain.js @@ -3,7 +3,7 @@ // accepted only as an injected function, so parsing and load coordination can // be unit-tested without bundling react/marked/dompurify. -function splitFrontmatter(content) { +export function splitFrontmatter(content) { const text = content || '' const lines = text.split(/\r?\n/) if (lines[0]?.trim() !== '---') return { body: text, meta: {} } @@ -141,6 +141,57 @@ export function installedAppDisplayName(app) { return name || slug || 'Installed app' } +// ---- v2: installed-list API + catalog screen helpers ---- + +// GET /api/skills row → the shared-storage path serving its markdown. Flat +// skills live at shared/skills/.md, directory skills (the external +// SKILL.md convention) at shared/skills//SKILL.md. +export function skillContentPath(skill) { + const id = encodeURIComponent(String(skill?.id ?? '')) + return skill?.is_dir + ? `/api/storage/shared/skills/${id}/SKILL.md` + : `/api/storage/shared/skills/${id}.md` +} + +// Provenance string from GET /api/skills → a chip the list can render. +// `kind` is a stable CSS modifier; `label` is the visible text; `title` is the +// hover/long-press explanation. +export function provenanceChip(provenance) { + const p = typeof provenance === 'string' ? provenance.trim() : '' + if (p === 'seed') return { kind: 'seed', label: 'built-in', title: 'Shipped with Möbius' } + if (p === 'agent') return { kind: 'agent', label: 'agent-made', title: 'Authored by your agent' } + if (p.startsWith('app:')) { + const slug = p.slice('app:'.length) + return { kind: 'app', label: `app · ${slug}`, title: `Owned by the ${slug} app` } + } + if (p.startsWith('installed:')) { + const src = p.slice('installed:'.length) + return { kind: 'installed', label: src || 'installed', title: `Installed from ${src || 'the ecosystem'}` } + } + return { kind: 'other', label: p || 'unknown', title: p || 'Unknown origin' } +} + +// Only skills recorded by the install API may be removed in-app; seeds, agent +// files, and app-owned skills keep their own lifecycles. +export function isUninstallable(provenance) { + return typeof provenance === 'string' && provenance.startsWith('installed:') +} + +// API skill names are usually slugs; render a readable title without mangling +// a name that already carries real casing or spacing. +export function skillDisplayTitle(name) { + const n = typeof name === 'string' ? name.trim() : '' + if (!n) return 'Untitled skill' + if (/[A-Z ]/.test(n)) return n + return n.replace(/[-_]/g, ' ').replace(/\b\w/g, (c) => c.toUpperCase()) +} + +export function usageLabel(uses) { + const n = Number(uses) + if (!Number.isFinite(n) || n <= 0) return '' + return `${n}× in 30d` +} + // Turn a developer-facing fetch error into copy the owner can act on. export function friendlyLoadError(err) { const raw = String((err && err.message) || err || '') diff --git a/index.jsx b/index.jsx index 48a54de..e0fee16 100644 --- a/index.jsx +++ b/index.jsx @@ -8,16 +8,41 @@ import { createDetailNav, createSystemPromptAppsLoader, installedAppDisplayName, + skillContentPath, + provenanceChip, + isUninstallable, + skillDisplayTitle, + usageLabel, } from './domain.js' +import { + DEFAULT_SOURCES, + sourceKey, + treeToSkills, + catalogSummary, + treeScanUrl, + rawSkillUrl, + githubSkillUrl, + createSummaryPrefetcher, + assessCompat, + assessInstalled, +} from './catalog.js' -// Skills — a read-only browser for the agent's skills (the SKILL-style -// markdown files under /data/shared/skills). Inspired by the Skills screen in -// Hermex. Skills are shared, owner-authored context; a mini-app can READ shared -// storage with its scoped token but not WRITE it, so creating/editing a skill -// is routed to the Möbius agent via a new chat rather than an in-app save. +// Skills — browse, read, and grow the agent's skills (the SKILL-style markdown +// under /data/shared/skills). v1 was a read-only browser; v2 keeps that and +// adds the write half: +// +// - the list comes from GET /api/skills, so directory-shaped skills +// (/SKILL.md) appear too, with provenance + 30-day usage, +// - a Find button opens an agent chat (moebius:new-chat draft) — the +// finding-skills seed skill is the agent's discovery playbook, +// - a catalog screen scans public skill repos (one git-trees call per source +// through /api/proxy) and installs via POST /api/skills/install, +// - install-provenance skills can be removed from the detail view. // -// The pure parsing + link-classification logic lives in ./domain.js so it can -// be unit-tested without bundling react/marked/dompurify (see test/). +// Creating/editing a skill still routes to the agent — a mini-app can read +// shared storage but not write it; install/uninstall go through the skills API +// (gated by permissions.manage_skills). Pure logic lives in ./domain.js and +// ./catalog.js so it stays unit-testable without react/marked/dompurify. const CSS = ` /* mobius-ui:Root v1 — keep in sync; library candidate. */ @@ -63,13 +88,22 @@ const CSS = ` background: color-mix(in srgb, var(--accent) 14%, transparent); color: var(--accent); } .sk-title { margin: 0; font-size: 18px; font-weight: 700; letter-spacing: 0; } .sk-subtitle { display: block; margin-top: 1px; font-size: 12px; color: var(--muted); } -.sk-iconbtn { flex: 0 0 auto; width: 44px; height: 44px; display: inline-flex; align-items: center; +.sk-iconbtn { position: relative; flex: 0 0 auto; width: 44px; height: 44px; display: inline-flex; align-items: center; justify-content: center; border-radius: 10px; border: 1px solid var(--border); background: var(--surface); color: var(--text); cursor: pointer; transition: background .14s ease, transform .1s ease; } +/* custom tooltip — faster than the native title (~0.15s vs ~1s) and stylable */ +.sk-tip { position: absolute; top: calc(100% + 6px); right: 0; z-index: 30; pointer-events: none; + background: var(--surface); color: var(--text); border: 1px solid var(--border); border-radius: 8px; + padding: 5px 10px; font-size: 12px; font-weight: 400; line-height: 1.35; white-space: nowrap; + box-shadow: 0 4px 14px rgba(0, 0, 0, 0.18); opacity: 0; transform: translateY(-2px); + transition: opacity .12s ease .15s, transform .12s ease .15s; } +.sk-iconbtn:hover .sk-tip, +.sk-iconbtn:focus-visible .sk-tip { opacity: 1; transform: none; } .sk-iconbtn:active { transform: scale(0.94); } .sk-iconbtn:disabled { opacity: 0.5; cursor: default; } .sk-iconbtn svg { width: 18px; height: 18px; } .sk-iconbtn.is-spinning svg { animation: sk-spin 0.9s linear infinite; } +.sk-iconbtn.is-armed { border-color: var(--danger); color: var(--danger); } @keyframes sk-spin { to { transform: rotate(360deg); } } /* /mobius-ui:Header */ @@ -104,6 +138,18 @@ const CSS = ` .sk-chev { flex: 0 0 auto; align-self: center; color: var(--muted); opacity: 0.6; } .sk-chev svg { width: 18px; height: 18px; } + +/* provenance + usage chips (rows and detail) */ +.sk-provrow { display: flex; align-items: center; gap: 7px; margin-top: 6px; flex-wrap: wrap; min-width: 0; } +.sk-prov { font-size: 11px; font-weight: 600; padding: 1px 8px; border-radius: 999px; + border: 1px solid var(--border); color: var(--muted); max-width: 100%; + overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } +.sk-prov.seed { color: var(--accent); border-color: color-mix(in srgb, var(--accent) 50%, transparent); } +.sk-prov.installed { color: var(--success, #2e9e5b); border-color: color-mix(in srgb, var(--success, #2e9e5b) 55%, transparent); } +.sk-uses { font-size: 11px; color: var(--muted); white-space: nowrap; } +.sk-detailmeta { display: flex; align-items: center; gap: 7px; flex-wrap: wrap; + max-width: 720px; margin: 0 auto; padding: 14px 18px 0; } + /* installed apps that contribute read-only, always-on prompt context */ .sk-system-apps { margin: 0 20px; padding: 24px 0 max(32px, env(safe-area-inset-bottom)); border-top: 1px solid var(--border); } @@ -175,6 +221,56 @@ const CSS = ` .sk-md hr { border: none; border-top: 1px solid var(--border); margin: 20px 0; } .sk-md img { max-width: 100%; } +/* alerts (catalog + detail actions) */ +.sk-alert { flex: 0 0 auto; margin: 10px 16px 0; padding: 9px 12px; border-radius: 10px; font-size: 13px; + line-height: 1.45; border: 1px solid var(--border); color: var(--muted); background: var(--surface); } +.sk-alert.is-error { border-color: var(--danger); color: var(--danger); white-space: pre-wrap; } + +/* catalog screen (overlay over the list, so list state/scroll survive) */ +.sk-cat { position: absolute; inset: 0; z-index: 10; display: flex; flex-direction: column; background: var(--bg); } +.sk-cat-note { margin: 12px 20px 4px; max-width: 68ch; color: var(--muted); font-size: 13.5px; line-height: 1.5; + text-wrap: pretty; } +/* pre-install compat chip — on every catalog card next to Install, and in the + skill page's header next to Install. Amber chips open the plain-language + notes panel (.sk-caveats) under the skill page's header. */ +.sk-compat { flex: 0 0 auto; display: inline-flex; align-items: center; gap: 5px; padding: 4px 11px; + border-radius: 999px; font-family: var(--font); font-size: 12.5px; font-weight: 600; border: 1px solid; + background: none; white-space: nowrap; } +.sk-compat.is-ok { color: var(--ok, #2e7d32); border-color: color-mix(in srgb, var(--ok, #2e7d32) 45%, transparent); + background: color-mix(in srgb, var(--ok, #2e7d32) 10%, transparent); } +.sk-compat.is-warn { color: var(--warn, #b26a00); border-color: color-mix(in srgb, var(--warn, #b26a00) 45%, transparent); + background: color-mix(in srgb, var(--warn, #b26a00) 10%, transparent); cursor: pointer; } +.sk-compat.sm { font-size: 11px; padding: 1px 8px; cursor: inherit; } +.sk-caveats { flex: 0 0 auto; margin: 10px 16px 0; padding: 9px 12px 9px 28px; border-radius: 10px; + font-size: 13px; line-height: 1.5; + border: 1px solid color-mix(in srgb, var(--warn, #b26a00) 35%, var(--border)); color: var(--muted); + background: color-mix(in srgb, var(--warn, #b26a00) 6%, transparent); } +.sk-caveats li { margin: 3px 0; } +/* variant for panels living inside the reading column (installed-skill detail) */ +.sk-caveats.in-page { width: calc(100% - 36px); max-width: 684px; margin: 12px auto 0; box-sizing: border-box; } + +/* catalog breadcrumb chain — every ancestor segment is clickable */ +.sk-crumbs { flex: 1; min-width: 0; display: flex; align-items: center; gap: 6px; overflow: hidden; } +.sk-crumb { border: none; background: none; padding: 0; font-family: var(--font); font-size: 14.5px; + color: var(--accent); cursor: pointer; min-width: 0; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } +.sk-crumb.cur { color: var(--text); font-weight: 700; cursor: default; } +.sk-crumbsep { flex: 0 0 auto; color: var(--muted); } +.sk-cat-count { padding: 2px 16px 8px; font-size: 12.5px; color: var(--muted); } +.sk-cards { padding: 0 12px 32px; } +.sk-card { border: 1px solid var(--border); border-radius: 12px; background: var(--surface); + padding: 12px 14px; margin-bottom: 10px; cursor: pointer; } +.sk-card h3 { margin: 0 0 4px; font-size: 15px; font-weight: 650; display: flex; align-items: center; + gap: 8px; flex-wrap: wrap; word-break: break-word; } +.sk-carddesc { margin: 0 0 10px; font-size: 13.5px; color: var(--muted); line-height: 1.5; } +.sk-cardbtns { display: flex; gap: 8px; flex-wrap: wrap; align-items: center; } +.sk-btn { min-height: 40px; padding: 8px 16px; border-radius: 10px; border: 1px solid var(--accent); + background: var(--accent); color: var(--accent-fg, #fff); font-family: var(--font); font-size: 13.5px; + font-weight: 600; cursor: pointer; } +.sk-btn:disabled { opacity: 0.5; cursor: default; } +.sk-btn.ghost { background: none; color: var(--accent); text-decoration: none; display: inline-flex; + align-items: center; gap: 6px; } +.sk-btn.ghost svg { width: 15px; height: 15px; } + /* mobius-ui:Focus v1 — keep in sync; library candidate. Required once per app. A visible keyboard-focus ring on every interactive control (WCAG 2.4.7). :focus-visible only shows for keyboard nav, so mouse/touch taps stay clean. */ @@ -203,6 +299,15 @@ const SEARCH = const BACK = const PLUS = +const SPARKLE = +const BOOK = +const TRASH = +const EXTERNAL = + +// Prefilled draft for the Find flow. The agent's playbook is the +// finding-skills seed skill (sources, fit criteria, the trust ritual, and the +// exact install call); the owner just finishes this sentence. +const FIND_DRAFT = "I want to find and install a new skill for my agent. Here's what I'm trying to do: " function initialOnline() { if (typeof window !== 'undefined' && typeof window.mobius?.online === 'boolean') return window.mobius.online @@ -210,14 +315,457 @@ function initialOnline() { return true } +function ProvChips({ provenance, uses, compat }) { + const chip = provenanceChip(provenance) + const usage = usageLabel(uses) + return ( + + {chip.label} + {usage && {usage}} + {compat && (compat.ok ? ( + ✓ Works with Möbius + ) : ( + + ⚠ {compat.caveats.length} {compat.caveats.length === 1 ? 'thing' : 'things'} to know + + ))} + + ) +} + +// One catalog card. Summaries prefetch in the background after the source +// scan; the IntersectionObserver only lets visible cards jump that queue (and +// is the fallback when the pool is cancelled mid-run). Tapping a card opens +// the full SKILL.md as its own page, like an installed skill. +function CatalogCard({ skill, desc, installed, busy, compat, onOpen, onLoad, onInstall, onCaveats }) { + const ref = useRef(null) + + useEffect(() => { + if (desc || !ref.current || typeof IntersectionObserver === 'undefined') return undefined + const obs = new IntersectionObserver((entries) => { + if (entries.some((e) => e.isIntersecting)) { onLoad(); obs.disconnect() } + }, { rootMargin: '250px' }) + obs.observe(ref.current) + return () => obs.disconnect() + }, [desc]) + + const loaded = desc && desc !== 'loading' && desc !== 'failed' + return ( +
+

+ {skill.name} + {installed && installed} +

+

+ {loaded ? desc.description + : desc === 'failed' ? 'Could not load SKILL.md.' + : 'Loading summary…'} +

+
+ + {compat && (compat.ok ? ( + ✓ Works with Möbius + ) : ( + + ))} +
+
+ ) +} + +// The catalog screen: curated sources → one git-trees scan each → flat cards. +// Rendered as a hidden-not-unmounted overlay so scan results and scroll +// survive closing and reopening it. +function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose }) { + const [sources, setSources] = useState(DEFAULT_SOURCES) + const [open, setOpen] = useState(null) // { source } | null = source list + const [skillList, setSkillList] = useState(null) + const [truncated, setTruncated] = useState(false) + const [descs, setDescs] = useState({}) // dir -> { ...summary, raw } | 'loading' | 'failed' + const [filter, setFilter] = useState('') + const [detailDir, setDetailDir] = useState(null) // dir open as a full page + const [showCaveats, setShowCaveats] = useState(false) + const [busyDir, setBusyDir] = useState(null) + const [scanBusy, setScanBusy] = useState(false) + const [error, setError] = useState(null) + const [notice, setNotice] = useState(null) + const descsRef = useRef(descs) + descsRef.current = descs + const inflightRef = useRef(new Set()) // synchronous dedupe (descs lags a render) + const prefetcherRef = useRef(null) + const compatCacheRef = useRef({}) // dir -> assessCompat result, per source scan + const scrollRef = useRef(null) + const listScrollRef = useRef(0) // card-list offset, restored when a page closes + + useEffect(() => { + // Sources are app data: a saved sources.json overrides the defaults, so + // "add this repo as a source" is a chat request, not a code change. + const storage = window.mobius?.storage + if (storage && typeof storage.get === 'function') { + Promise.resolve(storage.get('sources.json')) + .then((saved) => { if (Array.isArray(saved) && saved.length) setSources(saved) }) + .catch(() => {}) + } + return () => prefetcherRef.current?.cancel() + }, []) + + useEffect(() => { + // Owner browsing is the natural freshness signal for the agent's cached + // catalog index — fire-and-forget; the server's 24h gate absorbs repeats. + if (!visible) return + fetch('/api/skills/catalog-index/refresh', { + method: 'POST', + headers: { ...authHeaders, 'Content-Type': 'application/json' }, + body: '{}', + }).catch(() => {}) + }, [visible]) + + // The sources list, the card list, and the skill page all share this one + // scroller, so a scrolled card list would otherwise bleed its offset into + // the page. Open a page at the top; restore the list position on the way back. + useEffect(() => { + const el = scrollRef.current + if (!el) return + if (detailDir) { + listScrollRef.current = el.scrollTop + el.scrollTop = 0 + } else { + el.scrollTop = listScrollRef.current + } + }, [detailDir]) + + const proxied = async (url) => { + const res = await fetch(`/api/proxy?url=${encodeURIComponent(url)}`, { headers: authHeaders }) + if (!res.ok) throw new Error(`fetch failed (${res.status}) for ${url}`) + return res.text() + } + + const loadDescription = async (source, dir) => { + if (descsRef.current[dir] || inflightRef.current.has(dir)) return + inflightRef.current.add(dir) + setDescs((d) => ({ ...d, [dir]: 'loading' })) + try { + // Keep the raw markdown next to the summary: the card only needs the + // description, but the full-page detail view renders the whole file. + const text = await proxied(rawSkillUrl(source, dir)) + setDescs((d) => ({ ...d, [dir]: { ...catalogSummary(text), raw: text } })) + } catch { + setDescs((d) => ({ ...d, [dir]: 'failed' })) + } + } + + const openSource = async (source) => { + prefetcherRef.current?.cancel() + setOpen({ source }) + setSkillList(null); setTruncated(false); setDescs({}); setFilter(''); setDetailDir(null) + setScanBusy(true); setError(null); setNotice(null) + inflightRef.current = new Set() + compatCacheRef.current = {} + listScrollRef.current = 0 + if (scrollRef.current) scrollRef.current.scrollTop = 0 + try { + const data = JSON.parse(await proxied(treeScanUrl(source))) + if (!Array.isArray(data.tree)) throw new Error(data.message || 'unexpected GitHub response (no tree)') + const skills = treeToSkills(data.tree, source.path) + // Keep the raw tree: the skill page's compat badge predicts what the + // installer would drop from it, no extra fetches needed. + setOpen({ source, tree: data.tree }) + setSkillList(skills) + setTruncated(!!data.truncated) + window.mobius?.signal?.('item_opened', { type: 'catalog-source', slug: source.repo }) + const prefetcher = createSummaryPrefetcher({ loadOne: (dir) => loadDescription(source, dir) }) + prefetcherRef.current = prefetcher + prefetcher.start(skills.map((s) => s.dir)) + } catch (e) { + setError(String(e?.message || e)) + window.mobius?.signal?.('error', { message: String(e?.message || e), source: 'catalog_scan' }) + } finally { + setScanBusy(false) + } + } + + const backToSources = () => { + prefetcherRef.current?.cancel() + setOpen(null) + setSkillList(null); setError(null); setNotice(null); setDetailDir(null); setFilter('') + listScrollRef.current = 0 + if (scrollRef.current) scrollRef.current.scrollTop = 0 + } + + // withCaveats: a card's amber chip opens the page with the notes already out. + const openSkillPage = (dir, withCaveats = false) => { + setDetailDir(dir) + setShowCaveats(!!withCaveats) + if (open) loadDescription(open.source, dir) + window.mobius?.signal?.('item_opened', { type: 'catalog-skill', slug: dir }) + } + + const install = async (source, dir) => { + setBusyDir(dir); setError(null); setNotice(null) + try { + const res = await fetch('/api/skills/install', { + method: 'POST', + headers: { ...authHeaders, 'Content-Type': 'application/json' }, + body: JSON.stringify({ repo: source.repo, path: dir, ref: source.ref || 'main' }), + }) + const data = await res.json().catch(() => ({})) + if (!res.ok) throw new Error(data.detail || `install failed (${res.status})`) + setNotice(`Installed "${data.name}" — it's in your agent's skills now.`) + window.mobius?.signal?.('skill_installed', { slug: data.name, source: source.repo }) + onInstalled() + } catch (e) { + setError(String(e?.message || e)) + window.mobius?.signal?.('error', { message: String(e?.message || e), source: 'skill_install' }) + } finally { + setBusyDir(null) + } + } + + const shown = useMemo(() => { + if (!skillList) return null + const q = filter.trim().toLowerCase() + if (!q) return skillList + return skillList.filter((s) => s.dir.toLowerCase().includes(q)) + }, [skillList, filter]) + + const detailName = detailDir ? detailDir.split('/').pop() : null + const detailInstalled = detailName ? existingIds.has(detailName) : false + const detailEntry = detailDir ? descs[detailDir] : null + const detailLoaded = detailEntry && detailEntry !== 'loading' && detailEntry !== 'failed' + const detailHtml = useMemo(() => { + if (!detailLoaded) return '' + try { + return DOMPurify.sanitize(marked.parse(parseSkill('SKILL.md', detailEntry.raw || '').content || '')) + } catch (err) { + window.mobius?.signal?.('error', { message: String(err?.message || err), source: 'markdown_render' }) + return '' + } + }, [detailEntry]) + + // Pre-install compat, per card: predict what the installer would drop, from + // the tree scan we already have. A verdict appears once a card's SKILL.md is + // loaded; cached per dir (cleared each scan) so every new description doesn't + // re-assess the whole list against the tree. + const compatByDir = useMemo(() => { + if (!Array.isArray(open?.tree) || !skillList) return {} + const cache = compatCacheRef.current + const out = {} + for (const s of skillList) { + const d = descs[s.dir] + if (!d || d === 'loading' || d === 'failed') continue + out[s.dir] = cache[s.dir] || (cache[s.dir] = assessCompat(open.tree, s.dir, d.raw || '')) + } + return out + }, [open, skillList, descs]) + const compat = (detailDir && compatByDir[detailDir]) || null + + // Links in a catalog SKILL.md: external → new tab; anything else (relative + // resource paths we haven't fetched) is blocked so the app stays mounted. + function onDetailClick(e) { + const a = e.target.closest && e.target.closest('a') + if (!a) return + const link = classifyLink(a.getAttribute('href')) + if (link.kind === 'anchor') return + e.preventDefault() + if (link.kind === 'external') window.open(link.url, '_blank', 'noopener,noreferrer') + } + + return ( +
+
+ + {/* Full breadcrumb chain — tap any ancestor to jump straight back to it. */} + + {detailDir && open && ( + <> + {compat && (compat.ok ? ( + ✓ Works with Möbius + ) : ( + + ))} + + {EXTERNAL} + + )} +
+ {error &&
{error}
} + {notice && !error &&
{notice}
} + {detailDir && showCaveats && compat && !compat.ok && ( +
    + {compat.caveats.map((c) =>
  • {c.text}
    • )} +
+ )} +
+
+ {!open && ( + <> +

+ Public catalogs that host installable skills. Open one to see every skill it + holds, or use ✦ Find on the main screen to have the agent search them all — + the agent also covers community awesome-lists and the rest of GitHub, which + only index skills and can’t be browsed here. +

+
+ {sources.map((s) => ( + + ))} +
+ + )} + + {detailDir && ( + detailEntry === 'failed' ? ( +
+ +
Couldn’t load this skill
+

SKILL.md for “{detailName}” couldn’t be fetched from GitHub.

+ {open && ( + + Read on GitHub {EXTERNAL} + + )} +
+ ) : detailLoaded ? ( +
+ ) : ( +
+ ) + )} + + {open && !detailDir && ( + <> + {skillList !== null && skillList.length > 8 && ( +
+
+ {SEARCH} + setFilter(e.target.value)} + aria-label="Filter skills" + /> +
+
+ )} + {skillList !== null && ( +
+ {skillList.length} {skillList.length === 1 ? 'skill' : 'skills'} + {truncated ? ' — large repo, GitHub truncated the list; some may be missing' : ''} +
+ )} + {scanBusy && ( +
Scanning {open.source.repo}…
+ )} + {shown && shown.length > 0 && ( +
+ {shown.map((s) => ( + openSkillPage(s.dir)} + onLoad={() => loadDescription(open.source, s.dir)} + onInstall={() => install(open.source, s.dir)} + onCaveats={() => openSkillPage(s.dir, true)} + /> + ))} +
+ )} + {shown && shown.length === 0 && !scanBusy && ( +
+ +
{skillList.length ? 'No matches' : 'No skills here'}
+

+ {skillList.length ? `No skills match “${filter}”.` : 'No SKILL.md files found in this source.'} +

+
+ )} + + )} +
+
+
+ ) +} + export default function SkillsApp({ appId, token }) { const [skills, setSkills] = useState(null) // null = never loaded; [] or [..] = last-known-good const [systemPromptApps, setSystemPromptApps] = useState([]) const [loadError, setLoadError] = useState(null) // user-facing copy for the latest failed load const [refreshing, setRefreshing] = useState(false) const [query, setQuery] = useState('') - const [selected, setSelected] = useState(null) // slug of open skill + const [selected, setSelected] = useState(null) // id of open skill + const [contents, setContents] = useState({}) // id -> { status, text } (lazy detail fetch) + const [removeArmed, setRemoveArmed] = useState(false) + const [removeBusy, setRemoveBusy] = useState(false) + const [removeError, setRemoveError] = useState(null) + const [catalogOpen, setCatalogOpen] = useState(false) + const [catalogMounted, setCatalogMounted] = useState(false) const [online, setOnline] = useState(initialOnline) + const [instCompat, setInstCompat] = useState({}) // id -> assessInstalled verdict (installed:* skills) + const [showInstCaveats, setShowInstCaveats] = useState(false) + const mainScrollRef = useRef(null) + const mainListScrollRef = useRef(0) // list offset, restored when a detail closes // The detail back-sentinel state machine (in domain.js so it is unit-testable // — the double-tap-during-pending-push race can't be exercised through the // React component alone). Created once; onShow/onClose close over the stable @@ -227,12 +775,26 @@ export default function SkillsApp({ appId, token }) { detailNavRef.current = createDetailNav({ label: 'skill-detail', getNavOpen: () => window.mobius?.nav?.open, - onShow: (slug) => { setSelected(slug); window.mobius?.signal?.('item_opened', { type: 'skill', slug }) }, + onShow: (id) => { setSelected(id); window.mobius?.signal?.('item_opened', { type: 'skill', slug: id }) }, onClose: () => setSelected(null), }) } const detailNav = detailNavRef.current + // Second sentinel for the catalog screen — one shell back target per pushed + // screen; navigation INSIDE the catalog (sources ↔ source) is in-screen. + const catalogNavRef = useRef(null) + if (!catalogNavRef.current) { + catalogNavRef.current = createDetailNav({ + label: 'skills-catalog', + getNavOpen: () => window.mobius?.nav?.open, + onShow: () => { setCatalogMounted(true); setCatalogOpen(true); window.mobius?.signal?.('item_opened', { type: 'catalog' }) }, + onClose: () => setCatalogOpen(false), + }) + } + const catalogNav = catalogNavRef.current const readySignalledRef = useRef(false) // gate app_ready to the first successful load + const contentsRef = useRef(contents) + contentsRef.current = contents const systemPromptAppsLoaderRef = useRef(null) if (!systemPromptAppsLoaderRef.current) { systemPromptAppsLoaderRef.current = createSystemPromptAppsLoader({ @@ -252,31 +814,33 @@ export default function SkillsApp({ appId, token }) { // stale generations when refreshes overlap. systemPromptAppsLoaderRef.current.load(authHeaders) try { - const res = await fetch('/api/storage/shared-list/skills/', { headers: authHeaders }) + // One metadata call replaces the old per-file storage crawl — and unlike + // a directory listing it includes directory-shaped skills, provenance, + // and usage. Full markdown is fetched lazily when a detail opens. + const res = await fetch('/api/skills', { headers: authHeaders }) if (!res.ok) throw new Error(`list ${res.status}`) - const { entries } = await res.json() - const files = (entries || []).filter((e) => e.type === 'file' && e.name.endsWith('.md') && !e.name.startsWith('.')) - const parsed = await Promise.all(files.map(async (e) => { - try { - const r = await fetch(`/api/storage/shared/skills/${encodeURIComponent(e.name)}`, { headers: authHeaders }) - if (!r.ok) { - // A per-file failure used to render a blank skill and emit nothing, - // so a corrupt/permission-broken file looked intentionally empty. - window.mobius?.signal?.('error', { message: `skill ${e.name} ${r.status}`, source: 'skill_load', status: r.status }) - return { ...parseSkill(e.name, ''), unavailable: true } + const data = await res.json() + const rows = (Array.isArray(data?.skills) ? data.skills : []) + .map((s) => { + const id = String(s?.id ?? '') + const name = typeof s?.name === 'string' && s.name ? s.name : id + return { + id, + name, + title: skillDisplayTitle(name), + description: typeof s?.description === 'string' ? s.description : '', + provenance: typeof s?.provenance === 'string' ? s.provenance : '', + is_dir: !!s?.is_dir, + uses: Number(s?.uses_30d) || 0, } - return parseSkill(e.name, await r.text()) - } catch (err) { - window.mobius?.signal?.('error', { message: String(err?.message || err), source: 'skill_load', status: 0 }) - return { ...parseSkill(e.name, ''), unavailable: true } - } - })) - parsed.sort((a, b) => a.title.toLowerCase().localeCompare(b.title.toLowerCase())) - setSkills(parsed) + }) + .filter((s) => s.id) + rows.sort((a, b) => a.title.toLowerCase().localeCompare(b.title.toLowerCase())) + setSkills(rows) setLoadError(null) if (!readySignalledRef.current) { readySignalledRef.current = true - window.mobius?.signal?.('app_ready', { item_count: parsed.length }) + window.mobius?.signal?.('app_ready', { item_count: rows.length }) } } catch (err) { setLoadError(friendlyLoadError(err)) @@ -288,14 +852,31 @@ export default function SkillsApp({ appId, token }) { useEffect(() => { load() return () => systemPromptAppsLoaderRef.current.invalidate() - }, []) // shared storage has no subscribe(); refresh is explicit + }, []) // the skills API has no subscribe(); refresh is explicit async function refresh() { setRefreshing(true) + setContents({}) // an explicit refresh also drops cached detail markdown + setInstCompat({}) // compat verdicts derive from that markdown — drop them too await load({ isRefresh: true }) setRefreshing(false) } + // React reuses the scroller DOM node when the list swaps to the detail tree + // (same element type, same position), so a scrolled list would open every + // skill part-way down the page. Open at the top; restore the list offset on + // the way back. Same fix as the catalog screen's shared scroller. + useEffect(() => { + const el = mainScrollRef.current + if (!el) return + if (selected) { + mainListScrollRef.current = el.scrollTop + el.scrollTop = 0 + } else { + el.scrollTop = mainListScrollRef.current + } + }, [selected]) + // Track connectivity for the Offline pill (silent-sync: pill only when offline). useEffect(() => { if (typeof window.mobius?.onOnlineChange === 'function') { @@ -312,19 +893,158 @@ export default function SkillsApp({ appId, token }) { // Open (or cross-link-swap) a skill detail through the await-ready state // machine. All the sentinel lifecycle + race handling lives in detailNav. - const openSkill = (slug) => detailNav.open(slug) + const openSkill = (id) => detailNav.open(id) const closeSkill = () => detailNav.close() + const openCatalog = () => catalogNav.open('catalog') + const closeCatalog = () => catalogNav.close() // If a refresh drops the currently-open skill, close the detail so we don't // leak the nav sentinel (a later device back would otherwise be consumed). useEffect(() => { - if (selected && skills && !skills.some((s) => s.slug === selected)) closeSkill() + if (selected && skills && !skills.some((s) => s.id === selected)) closeSkill() }, [selected, skills]) function askAgent(draft) { window.parent.postMessage({ type: 'moebius:new-chat', draft }, window.location.origin) } + function findSkills() { + window.mobius?.signal?.('find_skills_requested', {}) + askAgent(FIND_DRAFT) + } + + const current = selected && skills ? skills.find((s) => s.id === selected) : null + + // Lazy detail fetch: the list is metadata-only now, so the full markdown is + // pulled from shared storage the first time a skill opens (then cached until + // an explicit refresh). + useEffect(() => { + if (!current || contentsRef.current[current.id]) return + const { id } = current + const path = skillContentPath(current) + setContents((c) => ({ ...c, [id]: { status: 'loading' } })) + fetch(path, { headers: authHeaders }) + .then(async (r) => { + if (!r.ok) throw new Error(`content ${r.status}`) + const text = await r.text() + setContents((c) => ({ ...c, [id]: { status: 'ready', text } })) + }) + .catch((err) => { + window.mobius?.signal?.('error', { message: String(err?.message || err), source: 'skill_load', status: 0 }) + setContents((c) => ({ ...c, [id]: { status: 'failed' } })) + }) + }, [selected, skills]) + + // Every installed file under shared/skills//, relative to the skill dir. + // Bounded BFS over the non-recursive shared-list API — depth and page caps + // match the install bounds, so anything real fits well inside them. + async function listSkillFiles(id) { + const root = `skills/${encodeURIComponent(id)}` + const queue = [''] + const out = [] + let pages = 0 + while (queue.length && pages < 12) { + const sub = queue.shift() + pages += 1 + const res = await fetch(`/api/storage/shared-list/${root}${sub ? `/${sub}` : ''}?limit=200`, { headers: authHeaders }) + if (!res.ok) return null + const data = await res.json().catch(() => null) + if (!Array.isArray(data?.entries)) return null + for (const e of data.entries) { + const rel = sub ? `${sub}/${e.name}` : String(e.name || '') + if (e.type === 'directory') { + if (rel.split('/').length <= 4) queue.push(rel) + } else { + out.push(rel) + } + } + } + return out + } + + // Assess every installed:* skill in the background once the list loads, so + // the chip shows on the top-level rows too — not only after opening one. + // Sequential on purpose: a handful of installed skills at most, and the + // fetched markdown seeds the detail cache so opening the skill is instant. + useEffect(() => { + if (!skills) return undefined + const todo = skills.filter((s) => isUninstallable(s.provenance) && !instCompat[s.id]) + if (!todo.length) return undefined + let stale = false + ;(async () => { + for (const s of todo) { + try { + const res = await fetch(skillContentPath(s), { headers: authHeaders }) + if (!res.ok) continue + const raw = await res.text() + const files = s.is_dir ? await listSkillFiles(s.id) : [] + if (files === null || stale) continue + setContents((c) => (c[s.id] ? c : { ...c, [s.id]: { status: 'ready', text: raw } })) + setInstCompat((m) => ({ ...m, [s.id]: assessInstalled(files, raw) })) + } catch { /* no verdict beats a wrong one */ } + if (stale) return + } + })() + return () => { stale = true } + }, [skills]) + + // Post-install compat for the open skill. installed:* provenance only — + // seed and agent-authored skills are written against this instance and + // routinely mention files elsewhere in /data, which would read as false + // alarms here. + useEffect(() => { + if (!current || !isUninstallable(current.provenance)) return undefined + const { id, is_dir: isDir } = current + const entry = contents[id] + if (entry?.status !== 'ready' || instCompat[id]) return undefined + let stale = false + ;(async () => { + let files = [] + if (isDir) { + files = await listSkillFiles(id) + if (files === null) return // listing failed — no verdict beats a wrong one + } + const verdict = assessInstalled(files, entry.text || '') + if (!stale) setInstCompat((m) => ({ ...m, [id]: verdict })) + })().catch(() => {}) + return () => { stale = true } + }, [current, contents]) + + // Uninstall is a two-tap: first tap arms (danger ring + explainer), a second + // within 4s executes. Modal confirms don't exist inside the sandboxed iframe. + useEffect(() => { setRemoveArmed(false); setRemoveError(null); setShowInstCaveats(false) }, [selected]) + useEffect(() => { + if (!removeArmed) return undefined + const t = setTimeout(() => setRemoveArmed(false), 4000) + return () => clearTimeout(t) + }, [removeArmed]) + + async function deleteSkill(id) { + const res = await fetch(`/api/skills/${encodeURIComponent(id)}`, { + method: 'DELETE', headers: authHeaders, + }) + const data = await res.json().catch(() => ({})) + if (!res.ok) throw new Error(data?.detail || `uninstall ${res.status}`) + window.mobius?.signal?.('skill_uninstalled', { slug: id }) + } + + async function uninstallCurrent() { + if (!current || removeBusy) return + setRemoveBusy(true) + setRemoveError(null) + try { + await deleteSkill(current.id) + closeSkill() + load({ isRefresh: true }) + } catch (err) { + setRemoveError(String(err?.message || err)) + window.mobius?.signal?.('error', { message: String(err?.message || err), source: 'skill_uninstall' }) + } finally { + setRemoveBusy(false) + setRemoveArmed(false) + } + } + // Keep the app mounted when a link is tapped inside a rendered skill. function onDetailClick(e) { const a = e.target.closest && e.target.closest('a') @@ -333,7 +1053,7 @@ export default function SkillsApp({ appId, token }) { if (link.kind === 'anchor') return // in-page fragment — harmless, leave default if (link.kind === 'skill') { e.preventDefault() - if (skills && skills.some((s) => s.slug === link.slug)) { + if (skills && skills.some((s) => s.id === link.slug)) { openSkill(link.slug) } else { window.mobius?.signal?.('error', { message: `unknown skill link ${link.slug}`, source: 'skill_link' }) @@ -355,7 +1075,7 @@ export default function SkillsApp({ appId, token }) { const q = query.trim().toLowerCase() if (!q) return skills return skills.filter((s) => - s.title.toLowerCase().includes(q) || s.slug.toLowerCase().includes(q) || s.description.toLowerCase().includes(q)) + s.title.toLowerCase().includes(q) || s.id.toLowerCase().includes(q) || s.description.toLowerCase().includes(q)) }, [skills, query]) // Search analytics for Reflection: emit once per settled query (debounced so a @@ -373,16 +1093,22 @@ export default function SkillsApp({ appId, token }) { return () => clearTimeout(t) }, [query]) - const current = selected && skills ? skills.find((s) => s.slug === selected) : null + const currentContent = current ? contents[current.id] : null + const detailParsed = useMemo(() => { + if (!current || currentContent?.status !== 'ready') return null + return parseSkill(`${current.id}.md`, currentContent.text || '') + }, [current, currentContent]) const detailHtml = useMemo(() => { - if (!current || current.unavailable) return '' + if (!detailParsed) return '' try { - return DOMPurify.sanitize(marked.parse(current.content || '')) + return DOMPurify.sanitize(marked.parse(detailParsed.content || '')) } catch (err) { window.mobius?.signal?.('error', { message: String(err?.message || err), source: 'markdown_render' }) return '' } - }, [current]) + }, [detailParsed]) + + const existingIds = useMemo(() => new Set((skills || []).map((s) => s.id)), [skills]) const syncPill = !online ?
Offline
@@ -392,34 +1118,69 @@ export default function SkillsApp({ appId, token }) { // ---- Detail view ---- if (current) { + const removable = isUninstallable(current.provenance) + const compatInfo = instCompat[current.id] || null return (
{syncPill}
-
{current.title}
+
{detailParsed?.title || current.title}
+ {removable && ( + + )} + window.mobius?.signal?.('edit_requested', { type: 'skill', slug: current.id }) + askAgent(`Help me edit the "${current.id}" skill. Here's what I want to change: `) + }} aria-label="Edit skill with the agent">{PLUS}
-
- {current.unavailable ? ( + {removeArmed && !removeError && ( +
Tap the bin again to remove “{current.id}”. Its bytes are saved to git history first.
+ )} + {removeError &&
{removeError}
} +
+
+ + {compatInfo && (compatInfo.ok ? ( + ✓ Works with Möbius + ) : ( + + ))} +
+ {compatInfo && !compatInfo.ok && showInstCaveats && ( +
    + {compatInfo.caveats.map((c) =>
  • {c.text}
    • )} +
+ )} + {currentContent?.status === 'failed' ? (
Couldn’t load this skill
-

The file for “{current.slug}” couldn’t be read. Try refreshing, or ask the agent to check it.

+

The file for “{current.id}” couldn’t be read. Try refreshing, or ask the agent to check it.

- ) : ( + ) : currentContent?.status === 'ready' ? (
+ ) : ( +
)}
) } - // ---- List view ---- + // ---- List view (the catalog screen overlays it when open) ---- const loading = skills === null && !loadError const initialError = skills === null && loadError return ( @@ -449,10 +1210,18 @@ export default function SkillsApp({ appId, token }) { {skills ? `${skills.length} agent ${skills.length === 1 ? 'skill' : 'skills'}` : 'Your agent’s abilities'}
- + + + -
+
{skills !== null && skills.length > 0 && (
@@ -481,7 +1250,8 @@ export default function SkillsApp({ appId, token }) {
No skills yet
-

Skills extend what your agent can do. Ask the agent to create one and it’ll appear here.

+

Skills extend what your agent can do. Ask the agent to find or create one and it’ll appear here.

+ @@ -552,6 +1321,16 @@ export default function SkillsApp({ appId, token }) { )}
+ + {catalogMounted && ( + load({ isRefresh: true })} + onClose={closeCatalog} + /> + )}
) } diff --git a/mobius.json b/mobius.json index fa6a30f..062b02b 100644 --- a/mobius.json +++ b/mobius.json @@ -1,20 +1,22 @@ { "id": "skills", "name": "Skills", - "version": "1.1.2", - "description": "Browse and search your agent's skills — the SKILL.md guides that shape what it can do. Tap any skill to read it. Create or edit through the agent.", + "version": "2.0.0", + "description": "Browse and read your agent's skills, and grow them: install from public skill catalogs, or ask the agent to find the right one for you.", "author": "mobius-os", "license": "MIT", "homepage": "https://github.com/mobius-os/app-skills", "entry": "index.jsx", "icon": "icon.png", "source_files": [ - "domain.js" + "domain.js", + "catalog.js" ], "offline_capable": false, "permissions": { "cross_app_access": "none", - "share_with_apps": "none" + "share_with_apps": "none", + "manage_skills": true }, "runtime": { "imports": [ diff --git a/test/catalog.test.js b/test/catalog.test.js new file mode 100644 index 0000000..e058cd1 --- /dev/null +++ b/test/catalog.test.js @@ -0,0 +1,310 @@ +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { + DEFAULT_SOURCES, + sourceKey, + treeToSkills, + catalogSummary, + treeScanUrl, + rawSkillUrl, + githubSkillUrl, + createSummaryPrefetcher, + assessCompat, + assessInstalled, +} from '../catalog.js' + +// Regression tests for the catalog core. Portable: no absolute paths, no +// install, discovered by `node --test` on a fresh clone. + +test('DEFAULT_SOURCES: every entry is scannable (repo shape, label, ref)', () => { + assert.ok(DEFAULT_SOURCES.length >= 5) + for (const s of DEFAULT_SOURCES) { + assert.match(s.repo, /^[\w.-]+\/[\w.-]+$/) + assert.ok(s.label) + assert.ok(s.ref) + assert.equal(typeof s.path, 'string') + } +}) + +test('sourceKey: distinguishes two subtrees of the same repo', () => { + const bundled = { repo: 'NousResearch/hermes-agent', path: 'skills' } + const optional = { repo: 'NousResearch/hermes-agent', path: 'optional-skills' } + assert.notEqual(sourceKey(bundled), sourceKey(optional)) +}) + +test('treeToSkills: keeps only SKILL.md dirs, sorted by name', () => { + const tree = [ + { path: 'skills/pdf/SKILL.md' }, + { path: 'skills/artifacts/SKILL.md' }, + { path: 'skills/pdf/scripts/fill.py' }, + { path: 'README.md' }, + { path: 'skills/notes.md' }, + ] + assert.deepEqual(treeToSkills(tree, ''), [ + { dir: 'skills/artifacts', name: 'artifacts' }, + { dir: 'skills/pdf', name: 'pdf' }, + ]) +}) + +test('treeToSkills: a path prefix scopes to that subtree (boundary-safe)', () => { + const tree = [ + { path: 'skills/a/SKILL.md' }, + { path: 'skills-extra/b/SKILL.md' }, + { path: 'other/c/SKILL.md' }, + ] + assert.deepEqual(treeToSkills(tree, 'skills'), [{ dir: 'skills/a', name: 'a' }]) +}) + +test('treeToSkills: tolerates malformed tree entries and a non-array input', () => { + assert.deepEqual(treeToSkills(null, ''), []) + assert.deepEqual(treeToSkills([{}, { path: 42 }, null, { path: 'x/SKILL.md' }], ''), [ + { dir: 'x', name: 'x' }, + ]) +}) + +test('catalogSummary: frontmatter description wins; license and peek extracted', () => { + const md = [ + '---', + 'name: pdf', + 'description: Work with PDF files.', + 'license: Complete terms in LICENSE.txt', + '---', + '# PDF', + '', + 'Body paragraph here.', + ].join('\n') + const s = catalogSummary(md) + assert.equal(s.description, 'Work with PDF files.') + assert.equal(s.license, 'Complete terms in LICENSE.txt') + assert.ok(s.peek.startsWith('# PDF')) + assert.ok(!s.peek.includes('---\nname'), 'peek is the frontmatter-stripped body') +}) + +test('catalogSummary: falls back to the first body paragraph, then placeholder copy', () => { + assert.equal(catalogSummary('# T\n\nFirst paragraph.').description, 'First paragraph.') + assert.equal(catalogSummary('').description, 'No description in SKILL.md.') + assert.equal(catalogSummary('').peek, null) +}) + +test('catalogSummary: peek is capped at 700 chars', () => { + const s = catalogSummary(`# T\n\n${'x'.repeat(2000)}`) + assert.equal(s.peek.length, 700) +}) + +test('url builders: ref defaults to main and paths compose correctly', () => { + const src = { repo: 'anthropics/skills', path: 'skills' } + assert.equal(treeScanUrl(src), 'https://api.github.com/repos/anthropics/skills/git/trees/main?recursive=1') + assert.equal(rawSkillUrl(src, 'skills/pdf'), 'https://raw.githubusercontent.com/anthropics/skills/main/skills/pdf/SKILL.md') + assert.equal(githubSkillUrl({ ...src, ref: 'v2' }, 'skills/pdf'), 'https://github.com/anthropics/skills/blob/v2/skills/pdf/SKILL.md') +}) + +test('prefetcher: bounds concurrency and still visits every dir', async () => { + let inflight = 0 + let peak = 0 + const seen = [] + const prefetcher = createSummaryPrefetcher({ + concurrency: 2, + loadOne: async (dir) => { + inflight += 1 + peak = Math.max(peak, inflight) + seen.push(dir) + await new Promise((resolve) => setImmediate(resolve)) + inflight -= 1 + }, + }) + prefetcher.start(['a', 'b', 'c', 'd', 'e']) + await new Promise((resolve) => setTimeout(resolve, 50)) + assert.deepEqual([...seen].sort(), ['a', 'b', 'c', 'd', 'e']) + assert.ok(peak <= 2, `peak concurrency ${peak} exceeded the bound`) +}) + +test('prefetcher: a rejecting loadOne does not stall the pool', async () => { + const seen = [] + const prefetcher = createSummaryPrefetcher({ + concurrency: 1, + loadOne: async (dir) => { + seen.push(dir) + if (dir === 'bad') throw new Error('boom') + }, + }) + prefetcher.start(['bad', 'good']) + await new Promise((resolve) => setTimeout(resolve, 20)) + assert.deepEqual(seen, ['bad', 'good']) +}) + +test('prefetcher: starting a new pool strands the previous generation', async () => { + const seen = [] + let releaseFirst + const gate = new Promise((resolve) => { releaseFirst = resolve }) + const prefetcher = createSummaryPrefetcher({ + concurrency: 1, + loadOne: async (dir) => { + seen.push(dir) + if (dir === 'old-1') await gate // old pool blocks until after the switch + }, + }) + prefetcher.start(['old-1', 'old-2']) + await new Promise((resolve) => setImmediate(resolve)) + prefetcher.start(['new-1']) + releaseFirst() + await new Promise((resolve) => setTimeout(resolve, 20)) + assert.ok(seen.includes('new-1')) + assert.ok(!seen.includes('old-2'), 'the superseded pool must not continue its queue') +}) + +test('prefetcher: cancel() stops the pool without starting another', async () => { + const seen = [] + let release + const gate = new Promise((resolve) => { release = resolve }) + const prefetcher = createSummaryPrefetcher({ + concurrency: 1, + loadOne: async (dir) => { + seen.push(dir) + if (dir === 'a') await gate + }, + }) + prefetcher.start(['a', 'b']) + await new Promise((resolve) => setImmediate(resolve)) + prefetcher.cancel() + release() + await new Promise((resolve) => setTimeout(resolve, 20)) + assert.deepEqual(seen, ['a']) +}) + +// --- assessCompat: the pre-install badge's prediction of the installer --- + +const blob = (path, size = 100) => ({ path, type: 'blob', size }) +const DIR = 'skills/pdf' +const OK_MD = '---\nname: pdf\ndescription: Fill and read PDFs.\n---\n\nProse body.\n' + +test('assessCompat: clean prose skill is ok', () => { + const tree = [blob(`${DIR}/SKILL.md`), blob(`${DIR}/references/forms.md`)] + const res = assessCompat(tree, DIR, OK_MD) + assert.equal(res.ok, true) + assert.deepEqual(res.caveats, []) +}) + +test('assessCompat: disallowed extensions and deep nesting are flagged as dropped', () => { + const tree = [ + blob(`${DIR}/SKILL.md`), + blob(`${DIR}/binary.wasm`), + blob(`${DIR}/a/b/c/d/e/deep.md`), + ] + const res = assessCompat(tree, DIR, OK_MD) + const dropped = res.caveats.find((c) => c.kind === 'dropped') + assert.ok(dropped) + assert.match(dropped.text, /2 extra files/) + assert.match(dropped.text, /binary\.wasm/) +}) + +test('assessCompat: over the file-count budget → installs partially', () => { + const tree = [blob(`${DIR}/SKILL.md`)] + for (let i = 0; i < 30; i++) tree.push(blob(`${DIR}/ref-${i}.md`)) + const res = assessCompat(tree, DIR, OK_MD) + const over = res.caveats.find((c) => c.kind === 'over-budget') + assert.ok(over) + assert.match(over.text, /30 files \(max 24\)/) +}) + +test('assessCompat: over the total-size budget → installs partially', () => { + const tree = [blob(`${DIR}/SKILL.md`), blob(`${DIR}/big.csv`, 3 * 1024 * 1024)] + const res = assessCompat(tree, DIR, OK_MD) + const over = res.caveats.find((c) => c.kind === 'over-budget') + assert.ok(over) + assert.match(over.text, /max 2 MB/) +}) + +test('assessCompat: bundled scripts are an informational caveat', () => { + const tree = [blob(`${DIR}/SKILL.md`), blob(`${DIR}/scripts/fill.py`)] + const res = assessCompat(tree, DIR, OK_MD) + assert.equal(res.ok, false) + const scripts = res.caveats.find((c) => c.kind === 'scripts') + assert.match(scripts.text, /nothing runs automatically/) +}) + +test('assessCompat: missing frontmatter description is flagged', () => { + const tree = [blob(`${DIR}/SKILL.md`)] + const res = assessCompat(tree, DIR, '# PDF skill\n\nJust a body.\n') + const fm = res.caveats.find((c) => c.kind === 'frontmatter') + assert.ok(fm) +}) + +test('assessCompat: multi-line YAML description defeats the flat parser → flagged', () => { + const raw = '---\nname: pdf\ndescription: >\n Long folded\n description.\n---\n\nBody.\n' + const res = assessCompat([blob(`${DIR}/SKILL.md`)], DIR, raw) + assert.ok(res.caveats.find((c) => c.kind === 'frontmatter')) +}) + +test('assessCompat: refs to dropped or absent files are the broken-refs caveat', () => { + const tree = [blob(`${DIR}/SKILL.md`), blob(`${DIR}/helper.rb`)] + const raw = `${OK_MD}\nRun [the helper](helper.rb), read \`scripts/gone.py\`, see [docs](https://example.com/x.md).\n` + const res = assessCompat(tree, DIR, raw) + const broken = res.caveats.find((c) => c.kind === 'broken-refs') + assert.ok(broken) + assert.match(broken.text, /helper\.rb/) + assert.match(broken.text, /scripts\/gone\.py/) + assert.ok(!broken.text.includes('example.com')) +}) + +test('assessCompat: bare inline-code filenames and dir refs are not treated as refs', () => { + const tree = [blob(`${DIR}/SKILL.md`)] + const raw = `${OK_MD}\nMention \`package.json\` and [the scripts](scripts/) generically.\n` + const res = assessCompat(tree, DIR, raw) + assert.equal(res.caveats.find((c) => c.kind === 'broken-refs'), undefined) +}) + +test('assessCompat: shell snippets and home/abs paths in inline code are not refs', () => { + const tree = [blob(`${DIR}/SKILL.md`)] + const raw = `${OK_MD}\nRun \`open /tmp/review_.html\`, save to \`~/Downloads/set.json\`, read \`/etc/hosts.conf\`.\n` + const res = assessCompat(tree, DIR, raw) + assert.equal(res.caveats.find((c) => c.kind === 'broken-refs'), undefined) +}) + +// --- assessInstalled: the same verdict for skills already on disk --- + +test('assessInstalled: clean installed skill with its files present is ok', () => { + const raw = `${OK_MD}\nSee [the forms guide](references/forms.md).\n` + const res = assessInstalled(['references/forms.md'], raw) + assert.equal(res.ok, true) + assert.deepEqual(res.caveats, []) +}) + +test('assessInstalled: refs to files not on disk are broken-refs', () => { + const raw = `${OK_MD}\nRun \`scripts/fill.py\` first.\n` + const res = assessInstalled([], raw) + const broken = res.caveats.find((c) => c.kind === 'broken-refs') + assert.ok(broken) + assert.match(broken.text, /scripts\/fill\.py/) +}) + +test('assessInstalled: installed scripts are the informational caveat', () => { + const res = assessInstalled(['scripts/fill.py'], `${OK_MD}\nRun \`scripts/fill.py\`.\n`) + assert.equal(res.ok, false) + const scripts = res.caveats.find((c) => c.kind === 'scripts') + assert.match(scripts.text, /nothing runs automatically/) + assert.equal(res.caveats.find((c) => c.kind === 'broken-refs'), undefined) +}) + +test('assessInstalled: flat skill (no files) with plain prose and a description is ok', () => { + assert.equal(assessInstalled([], OK_MD).ok, true) +}) + +test('assessInstalled: missing frontmatter description is flagged', () => { + const res = assessInstalled([], '# Notes\n\nJust a body.\n') + assert.ok(res.caveats.find((c) => c.kind === 'frontmatter')) +}) + +test('assessInstalled: SKILL.md itself never counts as a resource or a broken ref', () => { + const res = assessInstalled(['SKILL.md'], `${OK_MD}\nSee [itself](SKILL.md).\n`) + assert.equal(res.ok, true) +}) + +test('assessCompat: files outside the skill dir are ignored', () => { + const tree = [ + blob(`${DIR}/SKILL.md`), + blob('skills/other/huge.bin', 10 * 1024 * 1024), + blob('README.rb'), + ] + const res = assessCompat(tree, DIR, OK_MD) + assert.equal(res.ok, true) +}) diff --git a/test/domain.test.js b/test/domain.test.js index acd3ede..aae38f6 100644 --- a/test/domain.test.js +++ b/test/domain.test.js @@ -8,6 +8,11 @@ import { createSystemPromptAppsLoader, installedAppDisplayName, friendlyLoadError, + skillContentPath, + provenanceChip, + isUninstallable, + skillDisplayTitle, + usageLabel, } from '../domain.js' // Regression tests for the dependency-free core. Portable: no absolute paths, @@ -193,3 +198,52 @@ test('installedAppDisplayName: trims name, then falls back to slug and neutral c assert.equal(installedAppDisplayName({ name: '', slug: '' }), 'Installed app') assert.equal(installedAppDisplayName(null), 'Installed app') }) + +test('skillContentPath: flat skills read .md, directory skills read /SKILL.md', () => { + assert.equal(skillContentPath({ id: 'cron', is_dir: false }), '/api/storage/shared/skills/cron.md') + assert.equal(skillContentPath({ id: 'skill-creator', is_dir: true }), '/api/storage/shared/skills/skill-creator/SKILL.md') +}) + +test('skillContentPath: the id is URL-encoded, never path-spliced raw', () => { + assert.equal(skillContentPath({ id: 'a b', is_dir: false }), '/api/storage/shared/skills/a%20b.md') + assert.equal(skillContentPath({ id: 'x/y', is_dir: true }), '/api/storage/shared/skills/x%2Fy/SKILL.md') +}) + +test('provenanceChip: each provenance family maps to a stable kind + readable label', () => { + assert.deepEqual(provenanceChip('seed').kind, 'seed') + assert.equal(provenanceChip('seed').label, 'built-in') + assert.equal(provenanceChip('agent').kind, 'agent') + assert.deepEqual(provenanceChip('app:memory'), { kind: 'app', label: 'app · memory', title: 'Owned by the memory app' }) + const inst = provenanceChip('installed:anthropics/skills') + assert.equal(inst.kind, 'installed') + assert.equal(inst.label, 'anthropics/skills') +}) + +test('provenanceChip: unknown or missing provenance degrades to a neutral chip', () => { + assert.equal(provenanceChip('').kind, 'other') + assert.equal(provenanceChip(undefined).kind, 'other') + assert.equal(provenanceChip('installed:').label, 'installed') +}) + +test('isUninstallable: only installed:* provenance may be removed in-app', () => { + assert.equal(isUninstallable('installed:anthropics/skills'), true) + assert.equal(isUninstallable('seed'), false) + assert.equal(isUninstallable('agent'), false) + assert.equal(isUninstallable('app:memory'), false) + assert.equal(isUninstallable(undefined), false) +}) + +test('skillDisplayTitle: slugs become Title Case; real names pass through', () => { + assert.equal(skillDisplayTitle('finding-skills'), 'Finding Skills') + assert.equal(skillDisplayTitle('skill_creator'), 'Skill Creator') + assert.equal(skillDisplayTitle('PDF Processing'), 'PDF Processing') + assert.equal(skillDisplayTitle(''), 'Untitled skill') + assert.equal(skillDisplayTitle(null), 'Untitled skill') +}) + +test('usageLabel: zero/invalid usage renders nothing, positive counts read naturally', () => { + assert.equal(usageLabel(0), '') + assert.equal(usageLabel(undefined), '') + assert.equal(usageLabel(-2), '') + assert.equal(usageLabel(7), '7× in 30d') +}) From 0c0759c186767666b56645802ac06f589582b84b Mon Sep 17 00:00:00 2001 From: Miljan Martic Date: Wed, 22 Jul 2026 21:19:26 +0200 Subject: [PATCH 2/6] fix: review-gated install, race-proof catalog, one pinned revision per scan (PR review) Addresses the round-1 review: - Install arms only once the exact SKILL.md and its compat verdict are on screen - on cards and on the skill page. A failed fetch keeps Install off and offers Retry (cards) / Try again + GitHub (page). An amber verdict auto-expands its notes on the skill page, so the warning is visible before the first installable tap. - Every scan takes a generation token (createGenerationGuard in catalog.js, unit-tested with A-then-B reversed-response races for both the tree and markdown requests); any response holding a stale token is dropped, so a slow source can never overwrite state after another became current. - One immutable revision per scan: the source ref resolves to a commit OID first; the tree listing, every SKILL.md preview, the compat verdict, the GitHub page link, and the install POST all name that OID. The reviewed bytes are provably the installed bytes (backend records the OID in provenance). - Tree scans are scoped to the source's subtree via the : tree spec (paths re-prefixed to repo-relative at the scan boundary), and a truncated=true response is an error state, never a quietly incomplete catalog. 69 tests. Co-Authored-By: Claude Fable 5 --- catalog.js | 54 ++++++++++++++++--- index.jsx | 126 ++++++++++++++++++++++++++++++++++--------- test/catalog.test.js | 113 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 258 insertions(+), 35 deletions(-) diff --git a/catalog.js b/catalog.js index eb362e9..494d897 100644 --- a/catalog.js +++ b/catalog.js @@ -220,16 +220,58 @@ function nameSome(names, cap = 4) { return names.length > cap ? `${shown}, +${names.length - cap} more` : shown } -export function treeScanUrl(source) { - return `https://api.github.com/repos/${source.repo}/git/trees/${source.ref || 'main'}?recursive=1` +// The mutable ref (main, a tag) resolves to an immutable commit OID once per +// scan; the tree listing, every SKILL.md preview, the GitHub page link, and +// the install POST all name that OID. What the owner reviews is provably what +// installs, even if the source repo moves mid-browse. +export function resolveCommitUrl(source) { + return `https://api.github.com/repos/${source.repo}/commits/${encodeURIComponent(source.ref || 'main')}` } -export function rawSkillUrl(source, dir) { - return `https://raw.githubusercontent.com/${source.repo}/${source.ref || 'main'}/${dir}/SKILL.md` +export function commitOidOf(payload) { + const sha = payload && typeof payload.sha === 'string' ? payload.sha : '' + return /^[0-9a-f]{40}$/.test(sha) ? sha : null } -export function githubSkillUrl(source, dir) { - return `https://github.com/${source.repo}/blob/${source.ref || 'main'}/${dir}/SKILL.md` +// Scoped to the source's subtree via git's `:` rev syntax — one +// request, and a large repo outside the path can't push the listing into +// GitHub's truncation. +export function treeScanUrl(source, oid) { + const at = oid || source.ref || 'main' + const spec = source.path ? `${at}:${source.path.replace(/^\/+|\/+$/g, '')}` : at + return `https://api.github.com/repos/${source.repo}/git/trees/${encodeURIComponent(spec)}?recursive=1` +} + +// A scoped tree's paths are subtree-relative; everything downstream (skill +// dirs, compat assessment, install coordinates) speaks repo-relative paths, +// so re-prefix once at the scan boundary. +export function prefixTree(tree, pathPrefix) { + const prefix = String(pathPrefix || '').replace(/^\/+|\/+$/g, '') + if (!prefix) return Array.isArray(tree) ? tree : [] + return (Array.isArray(tree) ? tree : []).map((e) => + e && typeof e.path === 'string' ? { ...e, path: `${prefix}/${e.path}` } : e, + ) +} + +export function rawSkillUrl(source, dir, oid) { + return `https://raw.githubusercontent.com/${source.repo}/${oid || source.ref || 'main'}/${dir}/SKILL.md` +} + +export function githubSkillUrl(source, dir, oid) { + return `https://github.com/${source.repo}/blob/${oid || source.ref || 'main'}/${dir}/SKILL.md` +} + +// Monotonic generation guard for the catalog's async flows: every new scan +// takes a token; any response holding a stale token must be dropped, never +// written into state that now belongs to a different source. Pure so the +// A→B reversed-response races are unit-testable. +export function createGenerationGuard() { + let generation = 0 + return { + next() { return ++generation }, + isCurrent(token) { return token === generation }, + cancel() { generation += 1 }, + } } // Background prefetch pool: after a scan, walk every dir through `loadOne` diff --git a/index.jsx b/index.jsx index e0fee16..11c7663 100644 --- a/index.jsx +++ b/index.jsx @@ -19,10 +19,14 @@ import { sourceKey, treeToSkills, catalogSummary, + resolveCommitUrl, + commitOidOf, treeScanUrl, + prefixTree, rawSkillUrl, githubSkillUrl, createSummaryPrefetcher, + createGenerationGuard, assessCompat, assessInstalled, } from './catalog.js' @@ -337,7 +341,7 @@ function ProvChips({ provenance, uses, compat }) { // scan; the IntersectionObserver only lets visible cards jump that queue (and // is the fallback when the pool is cancelled mid-run). Tapping a card opens // the full SKILL.md as its own page, like an installed skill. -function CatalogCard({ skill, desc, installed, busy, compat, onOpen, onLoad, onInstall, onCaveats }) { +function CatalogCard({ skill, desc, installed, busy, compat, onOpen, onLoad, onInstall, onCaveats, onRetry }) { const ref = useRef(null) useEffect(() => { @@ -362,14 +366,29 @@ function CatalogCard({ skill, desc, installed, busy, compat, onOpen, onLoad, onI : 'Loading summary…'}

+ {/* Install arms only once the exact SKILL.md AND its compat verdict + are in hand — a quick tap can never install unreviewed bytes. On a + failed fetch it stays disabled and Retry takes its place beside it. */} + {desc === 'failed' && ( + + )} {compat && (compat.ok ? ( ✓ Works with Möbius ) : ( @@ -393,7 +412,6 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose const [sources, setSources] = useState(DEFAULT_SOURCES) const [open, setOpen] = useState(null) // { source } | null = source list const [skillList, setSkillList] = useState(null) - const [truncated, setTruncated] = useState(false) const [descs, setDescs] = useState({}) // dir -> { ...summary, raw } | 'loading' | 'failed' const [filter, setFilter] = useState('') const [detailDir, setDetailDir] = useState(null) // dir open as a full page @@ -409,6 +427,10 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose const compatCacheRef = useRef({}) // dir -> assessCompat result, per source scan const scrollRef = useRef(null) const listScrollRef = useRef(0) // card-list offset, restored when a page closes + // Every scan takes a generation token; a response carrying a stale token is + // dropped, so a slow source A can never overwrite state after B is current. + const guardRef = useRef(null) + if (!guardRef.current) guardRef.current = createGenerationGuard() useEffect(() => { // Sources are app data: a saved sources.json overrides the defaults, so @@ -453,52 +475,82 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose return res.text() } - const loadDescription = async (source, dir) => { - if (descsRef.current[dir] || inflightRef.current.has(dir)) return + const loadDescription = async (source, dir, oid, token, { force = false } = {}) => { + if (!force && (descsRef.current[dir] || inflightRef.current.has(dir))) return inflightRef.current.add(dir) + if (!guardRef.current.isCurrent(token)) return setDescs((d) => ({ ...d, [dir]: 'loading' })) try { // Keep the raw markdown next to the summary: the card only needs the // description, but the full-page detail view renders the whole file. - const text = await proxied(rawSkillUrl(source, dir)) + // Fetched at the scan's pinned OID — the previewed bytes are the exact + // bytes an install of this scan would materialize. + const text = await proxied(rawSkillUrl(source, dir, oid)) + if (!guardRef.current.isCurrent(token)) return setDescs((d) => ({ ...d, [dir]: { ...catalogSummary(text), raw: text } })) } catch { + if (!guardRef.current.isCurrent(token)) return setDescs((d) => ({ ...d, [dir]: 'failed' })) } } + const retryDescription = (dir) => { + if (!open) return + inflightRef.current.delete(dir) + loadDescription(open.source, dir, open.oid, open.token, { force: true }) + } + const openSource = async (source) => { prefetcherRef.current?.cancel() + const token = guardRef.current.next() setOpen({ source }) - setSkillList(null); setTruncated(false); setDescs({}); setFilter(''); setDetailDir(null) + setSkillList(null); setDescs({}); setFilter(''); setDetailDir(null) setScanBusy(true); setError(null); setNotice(null) inflightRef.current = new Set() compatCacheRef.current = {} listScrollRef.current = 0 if (scrollRef.current) scrollRef.current.scrollTop = 0 try { - const data = JSON.parse(await proxied(treeScanUrl(source))) + // Pin this scan to one immutable commit: tree, previews, verdicts, and + // the install POST all name this OID. + const oid = commitOidOf(JSON.parse(await proxied(resolveCommitUrl(source)))) + if (!oid) throw new Error('could not pin this source to a commit') + if (!guardRef.current.isCurrent(token)) return + const data = JSON.parse(await proxied(treeScanUrl(source, oid))) + if (!guardRef.current.isCurrent(token)) return if (!Array.isArray(data.tree)) throw new Error(data.message || 'unexpected GitHub response (no tree)') - const skills = treeToSkills(data.tree, source.path) - // Keep the raw tree: the skill page's compat badge predicts what the - // installer would drop from it, no extra fetches needed. - setOpen({ source, tree: data.tree }) + if (data.truncated) { + // An incomplete listing is an error, not a catalog — arbitrary skills + // would silently be missing from what looks like a complete list. + throw new Error( + 'GitHub truncated the listing for this source, so results would be ' + + 'incomplete. Use a source with a narrower path.', + ) + } + // Re-prefix the scoped tree to repo-relative paths; keep it on `open` — + // the compat badge predicts installer behavior from it, zero refetches. + const fullTree = prefixTree(data.tree, source.path) + const skills = treeToSkills(fullTree, source.path) + setOpen({ source, oid, token, tree: fullTree }) setSkillList(skills) - setTruncated(!!data.truncated) window.mobius?.signal?.('item_opened', { type: 'catalog-source', slug: source.repo }) - const prefetcher = createSummaryPrefetcher({ loadOne: (dir) => loadDescription(source, dir) }) + const prefetcher = createSummaryPrefetcher({ + loadOne: (dir) => loadDescription(source, dir, oid, token), + }) prefetcherRef.current = prefetcher prefetcher.start(skills.map((s) => s.dir)) } catch (e) { + if (!guardRef.current.isCurrent(token)) return setError(String(e?.message || e)) window.mobius?.signal?.('error', { message: String(e?.message || e), source: 'catalog_scan' }) } finally { - setScanBusy(false) + if (guardRef.current.isCurrent(token)) setScanBusy(false) } } const backToSources = () => { prefetcherRef.current?.cancel() + guardRef.current.cancel() // in-flight scan/preview responses become stale setOpen(null) setSkillList(null); setError(null); setNotice(null); setDetailDir(null); setFilter('') listScrollRef.current = 0 @@ -509,17 +561,23 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose const openSkillPage = (dir, withCaveats = false) => { setDetailDir(dir) setShowCaveats(!!withCaveats) - if (open) loadDescription(open.source, dir) + if (open) loadDescription(open.source, dir, open.oid, open.token) window.mobius?.signal?.('item_opened', { type: 'catalog-skill', slug: dir }) } const install = async (source, dir) => { setBusyDir(dir); setError(null); setNotice(null) try { + // Install the exact revision that was previewed: the scan's pinned OID, + // never the mutable branch name. const res = await fetch('/api/skills/install', { method: 'POST', headers: { ...authHeaders, 'Content-Type': 'application/json' }, - body: JSON.stringify({ repo: source.repo, path: dir, ref: source.ref || 'main' }), + body: JSON.stringify({ + repo: source.repo, + path: dir, + ref: open?.oid || source.ref || 'main', + }), }) const data = await res.json().catch(() => ({})) if (!res.ok) throw new Error(data.detail || `install failed (${res.status})`) @@ -572,6 +630,12 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose }, [open, skillList, descs]) const compat = (detailDir && compatByDir[detailDir]) || null + // An amber verdict must be on screen BEFORE the first moment Install can be + // tapped — the notes open themselves rather than waiting behind the chip. + useEffect(() => { + if (detailDir && compat && !compat.ok) setShowCaveats(true) + }, [detailDir, compat]) + // Links in a catalog SKILL.md: external → new tab; anything else (relative // resource paths we haven't fetched) is blocked so the app stays mounted. function onDetailClick(e) { @@ -618,21 +682,27 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose ⚠ {compat.caveats.length} {compat.caveats.length === 1 ? 'thing' : 'things'} to know ))} + {/* Same arming rule as the cards: no install before the exact + SKILL.md and its verdict are on screen. */} {EXTERNAL} )} @@ -675,9 +745,13 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose
Couldn’t load this skill
-

SKILL.md for “{detailName}” couldn’t be fetched from GitHub.

+

+ SKILL.md for “{detailName}” couldn’t be fetched from GitHub, so + it can’t be reviewed — and Install stays off until it can. +

+ {open && ( - + Read on GitHub {EXTERNAL} )} @@ -707,7 +781,6 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose {skillList !== null && (
{skillList.length} {skillList.length === 1 ? 'skill' : 'skills'} - {truncated ? ' — large repo, GitHub truncated the list; some may be missing' : ''}
)} {scanBusy && ( @@ -724,9 +797,10 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose busy={busyDir === s.dir} compat={compatByDir[s.dir] || null} onOpen={() => openSkillPage(s.dir)} - onLoad={() => loadDescription(open.source, s.dir)} + onLoad={() => loadDescription(open.source, s.dir, open.oid, open.token)} onInstall={() => install(open.source, s.dir)} onCaveats={() => openSkillPage(s.dir, true)} + onRetry={() => retryDescription(s.dir)} /> ))}
diff --git a/test/catalog.test.js b/test/catalog.test.js index e058cd1..4c0473a 100644 --- a/test/catalog.test.js +++ b/test/catalog.test.js @@ -5,10 +5,14 @@ import { sourceKey, treeToSkills, catalogSummary, + resolveCommitUrl, + commitOidOf, treeScanUrl, + prefixTree, rawSkillUrl, githubSkillUrl, createSummaryPrefetcher, + createGenerationGuard, assessCompat, assessInstalled, } from '../catalog.js' @@ -91,13 +95,51 @@ test('catalogSummary: peek is capped at 700 chars', () => { assert.equal(s.peek.length, 700) }) -test('url builders: ref defaults to main and paths compose correctly', () => { +const OID = 'a1b2c3d4'.repeat(5) + +test('url builders: scan is scoped to the subtree and everything pins to the OID', () => { const src = { repo: 'anthropics/skills', path: 'skills' } - assert.equal(treeScanUrl(src), 'https://api.github.com/repos/anthropics/skills/git/trees/main?recursive=1') - assert.equal(rawSkillUrl(src, 'skills/pdf'), 'https://raw.githubusercontent.com/anthropics/skills/main/skills/pdf/SKILL.md') + assert.equal( + resolveCommitUrl(src), + 'https://api.github.com/repos/anthropics/skills/commits/main', + ) + assert.equal( + treeScanUrl(src, OID), + `https://api.github.com/repos/anthropics/skills/git/trees/${encodeURIComponent(`${OID}:skills`)}?recursive=1`, + ) + // A whole-repo source scans the OID itself; without an OID the ref shows. + assert.equal( + treeScanUrl({ repo: 'o/r' }, OID), + `https://api.github.com/repos/o/r/git/trees/${OID}?recursive=1`, + ) + assert.equal(treeScanUrl(src), `https://api.github.com/repos/anthropics/skills/git/trees/${encodeURIComponent('main:skills')}?recursive=1`) + assert.equal(rawSkillUrl(src, 'skills/pdf', OID), `https://raw.githubusercontent.com/anthropics/skills/${OID}/skills/pdf/SKILL.md`) + assert.equal(githubSkillUrl(src, 'skills/pdf', OID), `https://github.com/anthropics/skills/blob/${OID}/skills/pdf/SKILL.md`) assert.equal(githubSkillUrl({ ...src, ref: 'v2' }, 'skills/pdf'), 'https://github.com/anthropics/skills/blob/v2/skills/pdf/SKILL.md') }) +test('commitOidOf accepts only a 40-hex sha', () => { + assert.equal(commitOidOf({ sha: OID }), OID) + assert.equal(commitOidOf({ sha: 'main' }), null) + assert.equal(commitOidOf({ sha: OID.slice(1) }), null) + assert.equal(commitOidOf({}), null) + assert.equal(commitOidOf(null), null) +}) + +test('prefixTree re-prefixes scoped tree paths to repo-relative', () => { + const scoped = [ + { type: 'blob', path: 'pdf/SKILL.md', size: 4 }, + { type: 'tree', path: 'pdf' }, + ] + assert.deepEqual( + prefixTree(scoped, 'skills/').map((e) => e.path), + ['skills/pdf/SKILL.md', 'skills/pdf'], + ) + // Whole-repo sources pass through untouched. + assert.equal(prefixTree(scoped, '')[0].path, 'pdf/SKILL.md') + assert.deepEqual(prefixTree(null, 'x'), []) +}) + test('prefetcher: bounds concurrency and still visits every dir', async () => { let inflight = 0 let peak = 0 @@ -171,6 +213,71 @@ test('prefetcher: cancel() stops the pool without starting another', async () => assert.deepEqual(seen, ['a']) }) +// --- generation guard: stale catalog responses must be dropped --- + +test('generationGuard: tokens go stale on next() and cancel()', () => { + const guard = createGenerationGuard() + const a = guard.next() + assert.equal(guard.isCurrent(a), true) + const b = guard.next() + assert.equal(guard.isCurrent(a), false) + assert.equal(guard.isCurrent(b), true) + guard.cancel() + assert.equal(guard.isCurrent(b), false) +}) + +// Models CatalogScreen.openSource exactly: each scan takes a token, awaits +// its tree fetch, and only commits state while its token is current. A opens +// first but responds LAST — its late tree must not overwrite B's. +test('generationGuard: slow tree response for A cannot overwrite current B', async () => { + const guard = createGenerationGuard() + const state = {} + const gate = {} + const trees = { + A: new Promise((res) => { gate.A = () => res(['a-skill']) }), + B: new Promise((res) => { gate.B = () => res(['b-skill']) }), + } + const openSource = async (name) => { + const token = guard.next() + const tree = await trees[name] + if (!guard.isCurrent(token)) return + state.open = name + state.skills = tree + } + const a = openSource('A') + const b = openSource('B') + gate.B(); await b // B (current) resolves first and commits + gate.A(); await a // A resolves after — stale token, dropped + assert.equal(state.open, 'B') + assert.deepEqual(state.skills, ['b-skill']) +}) + +// Models loadDescription: markdown for a dir name that exists in BOTH sources +// arrives after the source switched — the stale bytes must not populate the +// new source's cache for that same dir key. +test('generationGuard: stale markdown for a shared dir name is dropped', async () => { + const guard = createGenerationGuard() + let descs = {} + const gate = {} + const md = { + A: new Promise((res) => { gate.A = () => res('# from A') }), + B: new Promise((res) => { gate.B = () => res('# from B') }), + } + const loadDescription = async (sourceName, dir, token) => { + const text = await md[sourceName] + if (!guard.isCurrent(token)) return + descs = { ...descs, [dir]: text } + } + const tokenA = guard.next() + const inflightA = loadDescription('A', 'skills/pdf', tokenA) + const tokenB = guard.next() // source switched; caches were reset + descs = {} + const inflightB = loadDescription('B', 'skills/pdf', tokenB) + gate.B(); await inflightB + gate.A(); await inflightA // A's bytes arrive last, for the SAME dir key + assert.equal(descs['skills/pdf'], '# from B') +}) + // --- assessCompat: the pre-install badge's prediction of the installer --- const blob = (path, size = 100) => ({ path, type: 'blob', size }) From 5437897c4cf958fca15bcd312370ead7c5208979 Mon Sep 17 00:00:00 2001 From: Miljan Martic Date: Thu, 23 Jul 2026 01:17:53 +0200 Subject: [PATCH 3/6] fix: keyboard-open cards, exact installer parity, resource links, race-safe loads, complete-or-no-verdict (PR review round 2 + re-review) - Catalog cards open through a real
{/* Install arms only once the exact SKILL.md AND its compat verdict are in hand — a quick tap can never install unreviewed bytes. On a @@ -434,11 +446,17 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose useEffect(() => { // Sources are app data: a saved sources.json overrides the defaults, so - // "add this repo as a source" is a chat request, not a code change. + // "add this repo as a source" is a chat request, not a code change. The + // override is normalized before anything renders or builds URLs from it — + // ill-typed or hostile entries are dropped, and an override with nothing + // valid keeps the defaults. const storage = window.mobius?.storage if (storage && typeof storage.get === 'function') { Promise.resolve(storage.get('sources.json')) - .then((saved) => { if (Array.isArray(saved) && saved.length) setSources(saved) }) + .then((saved) => { + const cleaned = normalizeSources(saved) + if (cleaned.length) setSources(cleaned) + }) .catch(() => {}) } return () => prefetcherRef.current?.cancel() @@ -600,7 +618,9 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose }, [skillList, filter]) const detailName = detailDir ? detailDir.split('/').pop() : null - const detailInstalled = detailName ? existingIds.has(detailName) : false + // Installed-ness compares the id an install would actually create + // (lowercased basename), matching the server's derivation. + const detailInstalled = detailName ? existingIds.has(installIdOf(detailName)) : false const detailEntry = detailDir ? descs[detailDir] : null const detailLoaded = detailEntry && detailEntry !== 'loading' && detailEntry !== 'failed' const detailHtml = useMemo(() => { @@ -636,12 +656,13 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose if (detailDir && compat && !compat.ok) setShowCaveats(true) }, [detailDir, compat]) - // Links in a catalog SKILL.md: external → new tab; anything else (relative - // resource paths we haven't fetched) is blocked so the app stays mounted. + // Links in a catalog SKILL.md: external → new tab; relative refs are the + // (not-yet-installed) skill's own bundled resources — blocked so the app + // stays mounted, never misread as other skills. function onDetailClick(e) { const a = e.target.closest && e.target.closest('a') if (!a) return - const link = classifyLink(a.getAttribute('href')) + const link = classifyLink(a.getAttribute('href'), { dirSkill: true }) if (link.kind === 'anchor') return e.preventDefault() if (link.kind === 'external') window.open(link.url, '_blank', 'noopener,noreferrer') @@ -793,7 +814,7 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose key={s.dir} skill={s} desc={descs[s.dir]} - installed={existingIds.has(s.name)} + installed={existingIds.has(s.id)} busy={busyDir === s.dir} compat={compatByDir[s.dir] || null} onOpen={() => openSkillPage(s.dir)} @@ -838,6 +859,7 @@ export default function SkillsApp({ appId, token }) { const [online, setOnline] = useState(initialOnline) const [instCompat, setInstCompat] = useState({}) // id -> assessInstalled verdict (installed:* skills) const [showInstCaveats, setShowInstCaveats] = useState(false) + const [linkNotice, setLinkNotice] = useState(null) // tapped bundled-resource link const mainScrollRef = useRef(null) const mainListScrollRef = useRef(0) // list offset, restored when a detail closes // The detail back-sentinel state machine (in domain.js so it is unit-testable @@ -876,6 +898,12 @@ export default function SkillsApp({ appId, token }) { onApps: setSystemPromptApps, }) } + const skillsLoaderRef = useRef(null) + if (!skillsLoaderRef.current) { + skillsLoaderRef.current = createSkillsLoader({ + fetchImpl: (...args) => fetch(...args), + }) + } const authHeaders = useMemo(() => ({ Authorization: `Bearer ${token}` }), [token]) @@ -884,48 +912,35 @@ export default function SkillsApp({ appId, token }) { // error empty state is reserved for the very first load (skills === null). async function load({ isRefresh = false } = {}) { // Fire-and-forget by construction: Refresh completion depends only on the - // skills request. The loader commits [] on every apps failure and ignores - // stale generations when refreshes overlap. + // skills request. Both loaders ignore stale generations when requests + // overlap — an older /api/skills response can never overwrite a newer + // list, error state, or the app_ready count. systemPromptAppsLoaderRef.current.load(authHeaders) - try { - // One metadata call replaces the old per-file storage crawl — and unlike - // a directory listing it includes directory-shaped skills, provenance, - // and usage. Full markdown is fetched lazily when a detail opens. - const res = await fetch('/api/skills', { headers: authHeaders }) - if (!res.ok) throw new Error(`list ${res.status}`) - const data = await res.json() - const rows = (Array.isArray(data?.skills) ? data.skills : []) - .map((s) => { - const id = String(s?.id ?? '') - const name = typeof s?.name === 'string' && s.name ? s.name : id - return { - id, - name, - title: skillDisplayTitle(name), - description: typeof s?.description === 'string' ? s.description : '', - provenance: typeof s?.provenance === 'string' ? s.provenance : '', - is_dir: !!s?.is_dir, - uses: Number(s?.uses_30d) || 0, - } - }) - .filter((s) => s.id) - rows.sort((a, b) => a.title.toLowerCase().localeCompare(b.title.toLowerCase())) - setSkills(rows) + const result = await skillsLoaderRef.current.load(authHeaders) + if (!result.applied) return // superseded — the newer request's outcome wins + if (result.ok) { + setSkills(result.rows) setLoadError(null) if (!readySignalledRef.current) { readySignalledRef.current = true - window.mobius?.signal?.('app_ready', { item_count: rows.length }) + window.mobius?.signal?.('app_ready', { item_count: result.rows.length }) } - } catch (err) { - setLoadError(friendlyLoadError(err)) - window.mobius?.signal?.('error', { message: String(err?.message || err), source: isRefresh ? 'refresh' : 'load' }) + } else { + setLoadError(friendlyLoadError(result.error)) + window.mobius?.signal?.('error', { + message: String(result.error?.message || result.error), + source: isRefresh ? 'refresh' : 'load', + }) // Keep the last-known-good list intact; on the first load skills stays null. } } useEffect(() => { load() - return () => systemPromptAppsLoaderRef.current.invalidate() + return () => { + systemPromptAppsLoaderRef.current.invalidate() + skillsLoaderRef.current.invalidate() + } }, []) // the skills API has no subscribe(); refresh is explicit async function refresh() { @@ -1009,31 +1024,16 @@ export default function SkillsApp({ appId, token }) { }) }, [selected, skills]) - // Every installed file under shared/skills//, relative to the skill dir. - // Bounded BFS over the non-recursive shared-list API — depth and page caps - // match the install bounds, so anything real fits well inside them. - async function listSkillFiles(id) { - const root = `skills/${encodeURIComponent(id)}` - const queue = [''] - const out = [] - let pages = 0 - while (queue.length && pages < 12) { - const sub = queue.shift() - pages += 1 - const res = await fetch(`/api/storage/shared-list/${root}${sub ? `/${sub}` : ''}?limit=200`, { headers: authHeaders }) + // Authed JSON fetch for the installed-files walk (listInstalledFiles in + // catalog.js owns the traversal, encoding, and completeness rules). + const fetchJson = async (url) => { + try { + const res = await fetch(url, { headers: authHeaders }) if (!res.ok) return null - const data = await res.json().catch(() => null) - if (!Array.isArray(data?.entries)) return null - for (const e of data.entries) { - const rel = sub ? `${sub}/${e.name}` : String(e.name || '') - if (e.type === 'directory') { - if (rel.split('/').length <= 4) queue.push(rel) - } else { - out.push(rel) - } - } + return await res.json() + } catch { + return null } - return out } // Assess every installed:* skill in the background once the list loads, so @@ -1051,7 +1051,7 @@ export default function SkillsApp({ appId, token }) { const res = await fetch(skillContentPath(s), { headers: authHeaders }) if (!res.ok) continue const raw = await res.text() - const files = s.is_dir ? await listSkillFiles(s.id) : [] + const files = s.is_dir ? await listInstalledFiles(fetchJson, s.id) : [] if (files === null || stale) continue setContents((c) => (c[s.id] ? c : { ...c, [s.id]: { status: 'ready', text: raw } })) setInstCompat((m) => ({ ...m, [s.id]: assessInstalled(files, raw) })) @@ -1075,7 +1075,7 @@ export default function SkillsApp({ appId, token }) { ;(async () => { let files = [] if (isDir) { - files = await listSkillFiles(id) + files = await listInstalledFiles(fetchJson, id) if (files === null) return // listing failed — no verdict beats a wrong one } const verdict = assessInstalled(files, entry.text || '') @@ -1086,7 +1086,9 @@ export default function SkillsApp({ appId, token }) { // Uninstall is a two-tap: first tap arms (danger ring + explainer), a second // within 4s executes. Modal confirms don't exist inside the sandboxed iframe. - useEffect(() => { setRemoveArmed(false); setRemoveError(null); setShowInstCaveats(false) }, [selected]) + useEffect(() => { + setRemoveArmed(false); setRemoveError(null); setShowInstCaveats(false); setLinkNotice(null) + }, [selected]) useEffect(() => { if (!removeArmed) return undefined const t = setTimeout(() => setRemoveArmed(false), 4000) @@ -1108,8 +1110,11 @@ export default function SkillsApp({ appId, token }) { setRemoveError(null) try { await deleteSkill(current.id) + // The reload is AWAITED (removeBusy keeps the truthful pending state up) + // so the list behind the closing detail is already current — never a + // fire-and-forget refresh racing the close. + await load({ isRefresh: true }) closeSkill() - load({ isRefresh: true }) } catch (err) { setRemoveError(String(err?.message || err)) window.mobius?.signal?.('error', { message: String(err?.message || err), source: 'skill_uninstall' }) @@ -1123,8 +1128,19 @@ export default function SkillsApp({ appId, token }) { function onDetailClick(e) { const a = e.target.closest && e.target.closest('a') if (!a) return - const link = classifyLink(a.getAttribute('href')) + // Inside a directory skill, relative links name that skill's own bundled + // files — never other top-level skills (that convention is flat-skill + // only, preserved below). + const link = classifyLink(a.getAttribute('href'), { dirSkill: !!current?.is_dir }) if (link.kind === 'anchor') return // in-page fragment — harmless, leave default + if (link.kind === 'resource') { + e.preventDefault() + setLinkNotice( + `“${link.path}” is a file bundled inside this skill. The agent reads ` + + 'it when using the skill — ask in chat if you want to see it.', + ) + return + } if (link.kind === 'skill') { e.preventDefault() if (skills && skills.some((s) => s.id === link.slug)) { @@ -1218,9 +1234,24 @@ export default function SkillsApp({ appId, token }) {
Tap the bin again to remove “{current.id}”. Its bytes are saved to git history first.
)} {removeError &&
{removeError}
} + {linkNotice && !removeError && ( +
{linkNotice}
+ )}
+ {current.commit && current.sourceRepo && current.sourcePath && ( + /* the exact reviewed+installed revision — same OID the catalog + pinned at install time, straight from provenance */ + + source @ {current.commit.slice(0, 7)} + + )} {compatInfo && (compatInfo.ok ? ( ✓ Works with Möbius ) : ( diff --git a/test/a11y.test.js b/test/a11y.test.js new file mode 100644 index 0000000..a06e679 --- /dev/null +++ b/test/a11y.test.js @@ -0,0 +1,98 @@ +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { createRequire } from 'node:module' +import { existsSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { pathToFileURL, fileURLToPath } from 'node:url' + +// Accessibility render test: the catalog card's open action must be a REAL, +// focusable control — not a click-only div (WCAG 2.1.1). Renders the actual +// CatalogCard through esbuild + react-dom/server, so the assertion is on the +// markup keyboard/SR users actually get. +// +// Needs the shell's frontend deps (react, react-dom, esbuild). CI provides +// them via MOBIUS_FRONTEND_NODE_MODULES; locally a sibling mobius checkout +// works too. Skips cleanly when neither is available — the pure suites don't +// pay this cost. + +function frontendNodeModules() { + const fromEnv = process.env.MOBIUS_FRONTEND_NODE_MODULES + if (fromEnv && existsSync(join(fromEnv, 'react'))) return fromEnv + const here = fileURLToPath(new URL('.', import.meta.url)) + for (const rel of ['../.mobius/frontend/node_modules', '../../mobius/frontend/node_modules']) { + const candidate = join(here, '..', rel) + if (existsSync(join(candidate, 'react'))) return candidate + } + return null +} + +const nm = frontendNodeModules() + +test( + 'a11y: the catalog card opens through a real, focusable button', + { skip: nm ? false : 'frontend deps unavailable (set MOBIUS_FRONTEND_NODE_MODULES)' }, + async () => { + const require2 = createRequire(join(nm, 'noop.js')) + const esbuild = require2('esbuild') + + const workDir = mkdtempSync(join(tmpdir(), 'skills-a11y-')) + try { + // marked/dompurify are runtime esm_deps of the shell, not render deps of + // this test — stub them so only react does real work. + const stubs = join(workDir, 'stubs') + writeFileSync(join(workDir, 'marked-stub.mjs'), 'export const marked = { parse: () => "" }\n') + writeFileSync( + join(workDir, 'dompurify-stub.mjs'), + 'export default { sanitize: (x) => x }\n', + ) + + const built = await esbuild.build({ + entryPoints: [fileURLToPath(new URL('../index.jsx', import.meta.url))], + bundle: true, + write: false, + format: 'esm', + loader: { '.jsx': 'jsx' }, + jsx: 'automatic', // index.jsx never imports React itself + external: ['react', 'react-dom', 'react/jsx-runtime'], + alias: { + marked: join(workDir, 'marked-stub.mjs'), + dompurify: join(workDir, 'dompurify-stub.mjs'), + }, + }) + // Import the bundle from a dir whose node_modules is the frontend's, so + // the react externals resolve to the exact packages the shell serves. + symlinkSync(nm, join(workDir, 'node_modules')) + const bundlePath = join(workDir, 'app.mjs') + writeFileSync(bundlePath, built.outputFiles[0].text) + const { CatalogCard } = await import(pathToFileURL(bundlePath)) + assert.ok(CatalogCard, 'CatalogCard must stay exported for this test') + + const React = require2('react') + const { renderToStaticMarkup } = require2('react-dom/server') + const html = renderToStaticMarkup(React.createElement(CatalogCard, { + skill: { name: 'pdf', dir: 'skills/pdf', id: 'pdf' }, + desc: { description: 'Fill PDFs.', raw: '' }, + installed: false, + busy: false, + compat: { ok: true, caveats: [] }, + onOpen: () => {}, onLoad: () => {}, onInstall: () => {}, + onCaveats: () => {}, onRetry: () => {}, + })) + + // The open action is a semantic button whose accessible name is the + // skill's own title + summary (name comes from content). + const open = html.match(/]*class="sk-cardopen"[^>]*>([\s\S]*?)<\/button>/) + assert.ok(open, 'expected a - {desc === 'failed' && ( + {desc === 'failed' && installable && ( )} - {compat && (compat.ok ? ( + {!installable ? ( + + ⚠ {skill.collision ? 'Duplicate id' : 'Unsupported name'} + + ) : compat && (compat.ok ? ( ✓ Works with Möbius ) : ( {EXTERNAL} - +
)}
{error &&
{error}
} @@ -814,8 +876,9 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose key={s.dir} skill={s} desc={descs[s.dir]} - installed={existingIds.has(s.id)} + installed={existingIds.has(s.id) || installedLocally.has(s.id)} busy={busyDir === s.dir} + anyBusy={busyDir !== null} compat={compatByDir[s.dir] || null} onOpen={() => openSkillPage(s.dir)} onLoad={() => loadDescription(open.source, s.dir, open.oid, open.token)} @@ -889,6 +952,8 @@ export default function SkillsApp({ appId, token }) { } const catalogNav = catalogNavRef.current const readySignalledRef = useRef(false) // gate app_ready to the first successful load + const refreshCountRef = useRef(0) // in-flight refresh count, so the spinner ends on the LAST + const contentGenRef = useRef(0) // detail/compat fetch generation — newest wins const contentsRef = useRef(contents) contentsRef.current = contents const systemPromptAppsLoaderRef = useRef(null) @@ -917,7 +982,10 @@ export default function SkillsApp({ appId, token }) { // list, error state, or the app_ready count. systemPromptAppsLoaderRef.current.load(authHeaders) const result = await skillsLoaderRef.current.load(authHeaders) - if (!result.applied) return // superseded — the newer request's outcome wins + // Return the settled outcome so mutation callers (uninstall/install) can + // tell a real reconciliation from a swallowed failure instead of assuming + // the list is "already current." + if (!result.applied) return { applied: false, ok: false } // superseded if (result.ok) { setSkills(result.rows) setLoadError(null) @@ -933,6 +1001,7 @@ export default function SkillsApp({ appId, token }) { }) // Keep the last-known-good list intact; on the first load skills stays null. } + return { applied: true, ok: result.ok } } useEffect(() => { @@ -944,11 +1013,19 @@ export default function SkillsApp({ appId, token }) { }, []) // the skills API has no subscribe(); refresh is explicit async function refresh() { + // Ref-count overlapping refreshes: an older call finishing first must not + // flip the spinner off while a newer one is still in flight. + refreshCountRef.current += 1 setRefreshing(true) + contentGenRef.current += 1 // invalidate in-flight detail/compat fetches setContents({}) // an explicit refresh also drops cached detail markdown setInstCompat({}) // compat verdicts derive from that markdown — drop them too - await load({ isRefresh: true }) - setRefreshing(false) + try { + await load({ isRefresh: true }) + } finally { + refreshCountRef.current -= 1 + if (refreshCountRef.current === 0) setRefreshing(false) + } } // React reuses the scroller DOM node when the list swaps to the detail tree @@ -1011,15 +1088,21 @@ export default function SkillsApp({ appId, token }) { if (!current || contentsRef.current[current.id]) return const { id } = current const path = skillContentPath(current) + // Newest-generation-wins: a refresh (which clears the cache) bumps the + // generation, so a detail fetch that started earlier can't complete late + // and overwrite the freshly-invalidated state with stale bytes. + const gen = contentGenRef.current setContents((c) => ({ ...c, [id]: { status: 'loading' } })) fetch(path, { headers: authHeaders }) .then(async (r) => { if (!r.ok) throw new Error(`content ${r.status}`) const text = await r.text() + if (gen !== contentGenRef.current) return setContents((c) => ({ ...c, [id]: { status: 'ready', text } })) }) .catch((err) => { window.mobius?.signal?.('error', { message: String(err?.message || err), source: 'skill_load', status: 0 }) + if (gen !== contentGenRef.current) return setContents((c) => ({ ...c, [id]: { status: 'failed' } })) }) }, [selected, skills]) @@ -1051,7 +1134,11 @@ export default function SkillsApp({ appId, token }) { const res = await fetch(skillContentPath(s), { headers: authHeaders }) if (!res.ok) continue const raw = await res.text() - const files = s.is_dir ? await listInstalledFiles(fetchJson, s.id) : [] + // Prefer the authoritative installer inventory; fall back to the + // shared-list walk only when the row carries none (older installs). + const files = s.files != null + ? s.files + : s.is_dir ? await listInstalledFiles(fetchJson, s.id) : [] if (files === null || stale) continue setContents((c) => (c[s.id] ? c : { ...c, [s.id]: { status: 'ready', text: raw } })) setInstCompat((m) => ({ ...m, [s.id]: assessInstalled(files, raw) })) @@ -1074,7 +1161,9 @@ export default function SkillsApp({ appId, token }) { let stale = false ;(async () => { let files = [] - if (isDir) { + if (current.files != null) { + files = current.files // authoritative installer inventory + } else if (isDir) { files = await listInstalledFiles(fetchJson, id) if (files === null) return // listing failed — no verdict beats a wrong one } @@ -1109,10 +1198,14 @@ export default function SkillsApp({ appId, token }) { setRemoveBusy(true) setRemoveError(null) try { - await deleteSkill(current.id) - // The reload is AWAITED (removeBusy keeps the truthful pending state up) - // so the list behind the closing detail is already current — never a - // fire-and-forget refresh racing the close. + const removedId = current.id + await deleteSkill(removedId) + // The DELETE is confirmed, so the skill is gone regardless of what the + // reconciling reload does next. Drop it from authoritative local state + // NOW: if load() then fails (it swallows the error into loadError and + // returns ok:false), the list still must not show a skill the server + // already removed. The awaited reload is the full reconciliation on top. + setSkills((rows) => (Array.isArray(rows) ? rows.filter((s) => s.id !== removedId) : rows)) await load({ isRefresh: true }) closeSkill() } catch (err) { diff --git a/test/catalog.test.js b/test/catalog.test.js index 2ebd0fb..85c6f39 100644 --- a/test/catalog.test.js +++ b/test/catalog.test.js @@ -49,8 +49,8 @@ test('treeToSkills: keeps only SKILL.md dirs, sorted by name', () => { { path: 'skills/notes.md' }, ] assert.deepEqual(treeToSkills(tree, ''), [ - { dir: 'skills/artifacts', name: 'artifacts', id: 'artifacts' }, - { dir: 'skills/pdf', name: 'pdf', id: 'pdf' }, + { dir: 'skills/artifacts', name: 'artifacts', id: 'artifacts', installable: true }, + { dir: 'skills/pdf', name: 'pdf', id: 'pdf', installable: true }, ]) }) @@ -60,16 +60,45 @@ test('treeToSkills: a path prefix scopes to that subtree (boundary-safe)', () => { path: 'skills-extra/b/SKILL.md' }, { path: 'other/c/SKILL.md' }, ] - assert.deepEqual(treeToSkills(tree, 'skills'), [{ dir: 'skills/a', name: 'a', id: 'a' }]) + assert.deepEqual(treeToSkills(tree, 'skills'), [ + { dir: 'skills/a', name: 'a', id: 'a', installable: true }, + ]) }) test('treeToSkills: tolerates malformed tree entries and a non-array input', () => { assert.deepEqual(treeToSkills(null, ''), []) assert.deepEqual(treeToSkills([{}, { path: 42 }, null, { path: 'x/SKILL.md' }], ''), [ - { dir: 'x', name: 'x', id: 'x' }, + { dir: 'x', name: 'x', id: 'x', installable: true }, ]) }) +test('treeToSkills: a name the installer would reject is non-installable', () => { + // Spaces, `#`, `?`, `%`, leading punctuation, and non-ASCII all violate the + // backend `^[a-z0-9][a-z0-9._-]*$` contract — the card must not offer an + // install that inevitably 400s. + for (const bad of ['Bad Skill', 'a#frag', 'a?q', 'a%2f', '-lead', 'café']) { + const [s] = treeToSkills([{ path: `skills/${bad}/SKILL.md` }], 'skills') + assert.equal(s.id, null, `${bad} → no install id`) + assert.equal(s.installable, false, `${bad} → not installable`) + } + // A clean lowercase name stays installable. + const [ok] = treeToSkills([{ path: 'skills/pdf/SKILL.md' }], 'skills') + assert.equal(ok.installable, true) +}) + +test('treeToSkills: two dirs normalizing to one id are a non-installable collision', () => { + const skills = treeToSkills( + [{ path: 'skills/PDF/SKILL.md' }, { path: 'skills/pdf/SKILL.md' }], + 'skills', + ) + assert.equal(skills.length, 2) + for (const s of skills) { + assert.equal(s.id, 'pdf') + assert.equal(s.installable, false, `${s.dir} collides → not installable`) + assert.equal(s.collision, true) + } +}) + test('catalogSummary: frontmatter description wins; license and peek extracted', () => { const md = [ '---', @@ -122,6 +151,29 @@ test('url builders: scan is scoped to the subtree and everything pins to the OID assert.equal(githubSkillUrl({ ...src, ref: 'v2' }, 'skills/pdf'), 'https://github.com/anthropics/skills/blob/v2/skills/pdf/SKILL.md') }) +test('url builders percent-encode each path segment (preview == install target)', () => { + const src = { repo: 'o/r', path: 'skills' } + // `#`, `?`, `%`, space, and non-ASCII must be encoded per segment (the `/` + // separators survive) so the previewed/assessed URL is the one the backend — + // which quotes the same path — actually fetches. + assert.equal( + rawSkillUrl(src, 'skills/a#frag', OID), + `https://raw.githubusercontent.com/o/r/${OID}/skills/a%23frag/SKILL.md`, + ) + assert.equal( + rawSkillUrl(src, 'skills/a?d=1', OID), + `https://raw.githubusercontent.com/o/r/${OID}/skills/a%3Fd%3D1/SKILL.md`, + ) + assert.equal( + githubSkillUrl(src, 'skills/a b', OID), + `https://github.com/o/r/blob/${OID}/skills/a%20b/SKILL.md`, + ) + assert.equal( + rawSkillUrl(src, 'skills/café', OID), + `https://raw.githubusercontent.com/o/r/${OID}/skills/caf%C3%A9/SKILL.md`, + ) +}) + test('commitOidOf accepts only a 40-hex sha', () => { assert.equal(commitOidOf({ sha: OID }), OID) assert.equal(commitOidOf({ sha: 'main' }), null) @@ -258,6 +310,13 @@ test('installIdOf lowercases like the server derivation; treeToSkills carries it assert.equal(skill.id, 'pdf') // installed checks compare THIS }) +test('installIdOf returns null for names the backend name contract rejects', () => { + for (const bad of ['Bad Skill', 'a#frag', 'a?q', 'a%2f', '-lead', '.dot', 'café', '']) { + assert.equal(installIdOf(bad), null, bad) + } + assert.equal(installIdOf('pdf-forms.v2'), 'pdf-forms.v2') +}) + test('normalizeSources drops malformed entries, caps the list, keeps defaults-compatible shapes', () => { const good = { label: 'Ok', repo: 'o/r', path: 'skills', ref: 'main' } const out = normalizeSources([ @@ -280,6 +339,13 @@ test('normalizeSources drops malformed entries, caps the list, keeps defaults-co assert.equal(normalizeSources(DEFAULT_SOURCES).length, DEFAULT_SOURCES.length) }) +test('normalizeSources rejects a control character in the path (URL-builder safety)', () => { + assert.equal(normalizeSources([{ repo: 'o/r', path: 'a\u0000b' }]).length, 0) + assert.equal(normalizeSources([{ repo: 'o/r', path: 'a\u001fb' }]).length, 0) + // A legitimate hyphenated path is untouched. + assert.equal(normalizeSources([{ repo: 'o/r', path: 'sub-dir/skills' }]).length, 1) +}) + // --- listInstalledFiles: encoded, complete-or-no-verdict traversal --- function cannedLister(pages) { @@ -413,6 +479,17 @@ test('assessCompat: clean prose skill is ok', () => { assert.deepEqual(res.caveats, []) }) +test('assessCompat: a SKILL.md over the 256 KiB fetch cap is flagged (not false-green)', () => { + // Boundary: exactly at the cap is fine; one byte over is a caveat. + const cap = 256 * 1024 + const atCap = assessCompat([blob(`${DIR}/SKILL.md`, cap)], DIR, OK_MD) + assert.equal(atCap.ok, true) + const over = assessCompat([blob(`${DIR}/SKILL.md`, cap + 1)], DIR, OK_MD) + const c = over.caveats.find((x) => x.kind === 'skill-too-large') + assert.ok(c, 'expected a skill-too-large caveat') + assert.equal(over.caveats[0].kind, 'skill-too-large') // most serious first +}) + test('assessCompat: disallowed extensions and deep nesting are flagged as dropped', () => { const tree = [ blob(`${DIR}/SKILL.md`), diff --git a/test/domain.test.js b/test/domain.test.js index e03fe54..84fc3de 100644 --- a/test/domain.test.js +++ b/test/domain.test.js @@ -286,6 +286,18 @@ test('mapSkillRows sorts by title and retains commit/source identity fields', () assert.equal(rows[1].commit, null) }) +test('mapSkillRows carries the authoritative installer files inventory (or null)', () => { + const [withFiles, withoutFiles] = mapSkillRows({ skills: [ + { id: 'a', name: 'a', files: ['SKILL.md', 'scripts/fill.py'] }, + { id: 'b', name: 'b' }, // no record → null, so the caller falls back to the walk + ] }) + assert.deepEqual(withFiles.files, ['SKILL.md', 'scripts/fill.py']) + assert.equal(withoutFiles.files, null) + // A malformed (non-array) files field degrades to null, never a bad verdict. + const [bad] = mapSkillRows({ skills: [{ id: 'c', name: 'c', files: 'oops' }] }) + assert.equal(bad.files, null) +}) + // --- createSkillsLoader: newest-generation-wins for the primary list --- test('createSkillsLoader: a slow older response reports applied=false', async () => { From d9e6226b7b126a2c3d45394026322f3c6b39ed8b Mon Sep 17 00:00:00 2001 From: Miljan Martic Date: Thu, 23 Jul 2026 15:52:51 +0200 Subject: [PATCH 5/6] fix: closed installability result, single installed-id source, encoded source link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exact-head architecture review (round 4) on 8a6c870 — three state/contract gaps. B1 — a skill that can't install was still offered: the Install button only required a truthy compat object, so a SKILL.md over the hard 256 KiB fetch cap (which the backend rejects) stayed enabled. Adds one closed installability result — installability(skill, compat) → loading | installable | unsupported — derived from the same hard contract (BLOCKING_CAVEATS). A blocking caveat, an invalid id, or a duplicate id is 'unsupported' with Install disabled; soft caveats (dropped/over-budget/scripts/frontmatter) stay installable. Every Install control (cards + detail) derives from this, never from compat presence. B2 — installed state had two owners: CatalogScreen's `installedLocally` set only grew and the screen stayed mounted, so a card could stay labelled Installed after the skill was uninstalled elsewhere. Removed the monotonic overlay; the single installed-id set is the app-root existingIds (from /api/skills), refreshed after every confirmed install and uninstall. Install awaits that refresh with busyDir held, so the card shows "Installing…" through settlement and then reflects the authoritative set both ways. B3 — the installed-detail "source @" link spliced current.sourcePath raw into a GitHub blob URL. It now reuses githubSkillUrl (the same per-segment encoder as the catalog links), so #, ?, spaces etc. can't point at different bytes. Also: v2 and the platform API land together, so both installed-compat effects now require the authoritative `files` inventory and the incomplete listInstalledFiles shared-list fallback (and its fetchJson) is removed. Tests: installability (over-cap unsupported, soft-caveat installable, loading, invalid/duplicate id), BLOCKING_CAVEATS membership, encoded installed-detail link; render-level catalog-card test (over-cap Install disabled+Unsupported, soft-caveat enabled, installed-ness prop-driven both ways). 94 tests pass. Co-Authored-By: Claude Fable 5 --- catalog.js | 30 ++++++++ index.jsx | 142 ++++++++++++++++---------------------- test/catalog-card.test.js | 113 ++++++++++++++++++++++++++++++ test/catalog.test.js | 58 ++++++++++++++++ 4 files changed, 261 insertions(+), 82 deletions(-) create mode 100644 test/catalog-card.test.js diff --git a/catalog.js b/catalog.js index 22f1b5c..32e5cb9 100644 --- a/catalog.js +++ b/catalog.js @@ -281,6 +281,36 @@ export function assessCompat(tree, dir, raw) { return { ok: caveats.length === 0, caveats } } +// Caveats the backend treats as a HARD reject rather than a partial install: +// the install cannot succeed at all, so the skill must present as unsupported +// (Install disabled) — never an amber-but-runnable action. A SKILL.md over the +// fetch cap is rejected outright by the installer; the rest (dropped resources, +// over-budget, scripts, missing summary) still install, just partially. +export const BLOCKING_CAVEATS = new Set(['skill-too-large']) + +// One closed installability result for a catalog entry — the SINGLE source the +// Install control derives from, never the mere presence of a compat object: +// 'loading' compat verdict not computed yet +// 'unsupported' can never install cleanly (invalid/duplicate id, or a +// blocking compat caveat from the same hard contract as the +// backend) — Install disabled, with `reason`/`chip` to show +// 'installable' safe to offer +export function installability(skill, compat) { + if (skill && skill.installable === false) { + return { + status: 'unsupported', + reason: skill.collision + ? `Another directory in this source also installs as "${skill.id}", so neither can be installed cleanly — ask the agent to pick one.` + : 'This name isn’t valid for a Möbius skill (lowercase letters, digits, and . _ - only), so it can’t be installed here.', + chip: skill.collision ? 'Duplicate id' : 'Unsupported name', + } + } + if (!compat) return { status: 'loading', reason: '', chip: '' } + const blocking = compat.caveats.find((c) => BLOCKING_CAVEATS.has(c.kind)) + if (blocking) return { status: 'unsupported', reason: blocking.text, chip: 'Too large' } + return { status: 'installable', reason: '', chip: '' } +} + // Both flat parsers (here and backend) read only `key: value` scalars, so a // YAML block scalar (`description: >`) leaves just the indicator behind. function frontmatterCaveat(raw) { diff --git a/index.jsx b/index.jsx index 548b4da..abb06a1 100644 --- a/index.jsx +++ b/index.jsx @@ -31,7 +31,7 @@ import { createGenerationGuard, assessCompat, assessInstalled, - listInstalledFiles, + installability, } from './catalog.js' // Skills — browse, read, and grow the agent's skills (the SKILL-style markdown @@ -375,14 +375,12 @@ export function CatalogCard({ skill, desc, installed, busy, anyBusy, compat, onO }, [desc]) const loaded = desc && desc !== 'loading' && desc !== 'failed' - // A card whose derived id violates the backend name contract, or collides - // with another directory normalizing to the same id, can never install - // cleanly — render it explicitly unsupported with Install disabled rather - // than offering an install that 400s or races its twin. - const installable = skill.installable !== false - const unsupportedReason = skill.collision - ? `Another directory in this source also installs as "${skill.id}", so neither can be installed cleanly — ask the agent to pick one.` - : 'This name isn’t valid for a Möbius skill (lowercase letters, digits, and . _ - only), so it can’t be installed here.' + // One closed installability result: an id that violates the name contract or + // collides with a twin, OR a blocking compat caveat (a SKILL.md over the hard + // fetch cap the backend rejects), makes this 'unsupported' — Install disabled, + // not an amber-but-runnable action. Every control below derives from `inst`. + const inst = installability(skill, compat) + const supported = inst.status !== 'unsupported' return (
- {desc === 'failed' && installable && ( + {desc === 'failed' && supported && ( )} - {!installable ? ( - - ⚠ {skill.collision ? 'Duplicate id' : 'Unsupported name'} + {inst.status === 'unsupported' ? ( + + ⚠ {inst.chip} ) : compat && (compat.ok ? ( ✓ Works with Möbius @@ -457,7 +456,6 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose const [detailDir, setDetailDir] = useState(null) // dir open as a full page const [showCaveats, setShowCaveats] = useState(false) const [busyDir, setBusyDir] = useState(null) - const [installedLocally, setInstalledLocally] = useState(() => new Set()) // ids just installed this session const [scanBusy, setScanBusy] = useState(false) const [error, setError] = useState(null) const [notice, setNotice] = useState(null) @@ -634,14 +632,14 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose }) const data = await res.json().catch(() => ({})) if (!res.ok) throw new Error(data.detail || `install failed (${res.status})`) - // Mark installed optimistically so the card settles to "Installed" (and - // stays disabled) even if the parent reconciliation reload later fails. - if (data.name) setInstalledLocally((s) => new Set(s).add(data.name)) window.mobius?.signal?.('skill_installed', { slug: data.name, source: source.repo }) - // AWAIT the authoritative reconciliation; busyDir stays set through it, so - // the action can never re-enable mid-settlement (a fire-and-forget reload - // would leave the card tappable-and-saying-Install until the list caught - // up, or forever if it failed). + // AWAIT the authoritative reconciliation before clearing busy: the single + // installed-id set lives at the app root (existingIds, from /api/skills), + // and this refresh is what updates it. busyDir stays set through the await, + // so the card shows "Installing…" — never a tappable "Install" — until the + // authoritative list reflects the new skill. No session-local overlay: a + // set that only grows is exactly what lets a card claim "Installed" after + // the skill was uninstalled elsewhere. await onInstalled() if (guardRef.current.isCurrent(opToken)) { setNotice(`Installed "${data.name}" — it's in your agent's skills now.`) @@ -665,19 +663,14 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose const detailName = detailDir ? detailDir.split('/').pop() : null const detailSkill = detailDir && skillList ? skillList.find((s) => s.dir === detailDir) : null - // A card whose id is invalid or collides can be opened/read but never - // installed — the detail Install button mirrors the card's rule. - const detailInstallable = detailSkill ? detailSkill.installable !== false : true - const detailUnsupportedReason = detailSkill?.collision - ? `Another directory in this source also installs as "${detailSkill.id}", so neither can be installed cleanly — ask the agent to pick one.` - : 'This name isn’t valid for a Möbius skill (lowercase letters, digits, and . _ - only), so it can’t be installed here.' - // Installed-ness compares the id an install would actually create - // (lowercased basename), matching the server's derivation. The session-local - // just-installed set bridges the moment between a successful install and the - // parent list reflecting it (and survives a failed reconciliation reload). + // Installed-ness compares the id an install would actually create (lowercased + // basename), matching the server's derivation, against the single root set — + // existingIds, derived from /api/skills and refreshed after every install and + // uninstall. No session-local overlay, so this never lies "Installed" about a + // skill that was removed elsewhere. const detailIdForInstalled = detailName ? installIdOf(detailName) : null const detailInstalled = detailIdForInstalled != null - && (existingIds.has(detailIdForInstalled) || installedLocally.has(detailIdForInstalled)) + && existingIds.has(detailIdForInstalled) const detailEntry = detailDir ? descs[detailDir] : null const detailLoaded = detailEntry && detailEntry !== 'loading' && detailEntry !== 'failed' const detailHtml = useMemo(() => { @@ -706,6 +699,9 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose return out }, [open, skillList, descs]) const compat = (detailDir && compatByDir[detailDir]) || null + // The detail Install button mirrors the card's rule from the same closed + // result: invalid/duplicate id or a blocking compat caveat -> unsupported. + const detailInst = installability(detailSkill, compat) // An amber verdict must be on screen BEFORE the first moment Install can be // tapped — the notes open themselves rather than waiting behind the chip. @@ -749,9 +745,9 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose {detailDir && open && (
- {!detailInstallable ? ( - - ⚠ {detailSkill?.collision ? 'Duplicate id' : 'Unsupported name'} + {detailInst.status === 'unsupported' ? ( + + ⚠ {detailInst.chip} ) : compat && (compat.ok ? ( ✓ Works with Möbius @@ -765,19 +761,20 @@ function CatalogScreen({ visible, authHeaders, existingIds, onInstalled, onClose ))} {/* Same arming rule as the cards: no install before the exact - SKILL.md and its verdict are on screen. */} + SKILL.md and its verdict are on screen, and never when the closed + result is anything but installable. */} { - try { - const res = await fetch(url, { headers: authHeaders }) - if (!res.ok) return null - return await res.json() - } catch { - return null - } - } - // Assess every installed:* skill in the background once the list loads, so // the chip shows on the top-level rows too — not only after opening one. // Sequential on purpose: a handful of installed skills at most, and the @@ -1130,18 +1115,18 @@ export default function SkillsApp({ appId, token }) { let stale = false ;(async () => { for (const s of todo) { + // v2 and the platform API ship together, so an installed row always + // carries the authoritative `files` inventory. A row without it is a + // contract violation, not a legacy case to walk around with a partial + // shared-list crawl — skip it (no verdict beats a wrong one). + if (s.files == null) continue try { const res = await fetch(skillContentPath(s), { headers: authHeaders }) if (!res.ok) continue const raw = await res.text() - // Prefer the authoritative installer inventory; fall back to the - // shared-list walk only when the row carries none (older installs). - const files = s.files != null - ? s.files - : s.is_dir ? await listInstalledFiles(fetchJson, s.id) : [] - if (files === null || stale) continue + if (stale) return setContents((c) => (c[s.id] ? c : { ...c, [s.id]: { status: 'ready', text: raw } })) - setInstCompat((m) => ({ ...m, [s.id]: assessInstalled(files, raw) })) + setInstCompat((m) => ({ ...m, [s.id]: assessInstalled(s.files, raw) })) } catch { /* no verdict beats a wrong one */ } if (stale) return } @@ -1152,25 +1137,16 @@ export default function SkillsApp({ appId, token }) { // Post-install compat for the open skill. installed:* provenance only — // seed and agent-authored skills are written against this instance and // routinely mention files elsewhere in /data, which would read as false - // alarms here. + // alarms here. Keyed off the authoritative `files` inventory only. useEffect(() => { if (!current || !isUninstallable(current.provenance)) return undefined - const { id, is_dir: isDir } = current + const { id } = current const entry = contents[id] if (entry?.status !== 'ready' || instCompat[id]) return undefined - let stale = false - ;(async () => { - let files = [] - if (current.files != null) { - files = current.files // authoritative installer inventory - } else if (isDir) { - files = await listInstalledFiles(fetchJson, id) - if (files === null) return // listing failed — no verdict beats a wrong one - } - const verdict = assessInstalled(files, entry.text || '') - if (!stale) setInstCompat((m) => ({ ...m, [id]: verdict })) - })().catch(() => {}) - return () => { stale = true } + if (current.files == null) return undefined // require the authoritative inventory + const verdict = assessInstalled(current.files, entry.text || '') + setInstCompat((m) => ({ ...m, [id]: verdict })) + return undefined }, [current, contents]) // Uninstall is a two-tap: first tap arms (danger ring + explainer), a second @@ -1335,10 +1311,12 @@ export default function SkillsApp({ appId, token }) { {current.commit && current.sourceRepo && current.sourcePath && ( /* the exact reviewed+installed revision — same OID the catalog - pinned at install time, straight from provenance */ + pinned at install time, straight from provenance. Built through + the one segment-encoding helper (like the catalog links), so a + path with #, ?, spaces, etc. can't point at different bytes. */ diff --git a/test/catalog-card.test.js b/test/catalog-card.test.js new file mode 100644 index 0000000..77dae36 --- /dev/null +++ b/test/catalog-card.test.js @@ -0,0 +1,113 @@ +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { createRequire } from 'node:module' +import { existsSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { pathToFileURL, fileURLToPath } from 'node:url' + +// Render-level regression tests for the catalog card's Install control, through +// the same esbuild + react-dom/server harness as a11y.test.js. The install +// button's enabled/label state is derived, so assert it on the real markup a +// keyboard/SR user gets. Skips cleanly when the shell's frontend deps are +// unavailable (CI sets MOBIUS_FRONTEND_NODE_MODULES; a sibling mobius checkout +// also works). + +function frontendNodeModules() { + const fromEnv = process.env.MOBIUS_FRONTEND_NODE_MODULES + if (fromEnv && existsSync(join(fromEnv, 'react'))) return fromEnv + const here = fileURLToPath(new URL('.', import.meta.url)) + for (const rel of ['../.mobius/frontend/node_modules', '../../mobius/frontend/node_modules']) { + const candidate = join(here, '..', rel) + if (existsSync(join(candidate, 'react'))) return candidate + } + return null +} + +const nm = frontendNodeModules() + +// The Install control is the first non-ghost `sk-btn` in the card's button row. +function installButton(html) { + const m = html.match(/