fix: end-to-end QA fixes (YAML quoting, push-to-catalog re-render, LF line endings in git, first pull on new machine, rename references, git conflict guard, UI polish)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
166a834 parent: 77c51e8 modified
packages/core/src/catalog/git.ts +24 -3 | @@ -1,9 +1,9 @@ | ||
| 1 | 1 | import { execFile } from 'node:child_process' |
| 2 | -import { mkdir } from 'node:fs/promises' | |
| 2 | +import { mkdir, readFile, rm } from 'node:fs/promises' | |
| 3 | 3 | import path from 'node:path' |
| 4 | 4 | import type { GitStatus } from '../catalog-model.ts' |
| 5 | 5 | import type { CatalogGit, OpenCatalogGit } from '../contracts.ts' |
| 6 | -import { pathExists } from '../util.ts' | |
| 6 | +import { pathExists, stripBom } from '../util.ts' | |
| 7 | 7 | |
| 8 | 8 | interface GitResult { |
| 9 | 9 | code: number |
| @@ -20,11 +20,18 @@ class GitError extends Error { | ||
| 20 | 20 | } |
| 21 | 21 | } |
| 22 | 22 | |
| 23 | +/** | |
| 24 | + * Line endings are never converted (Git for Windows ships `core.autocrlf=true`): the catalog files must keep | |
| 25 | + * the exact bytes of the repository, otherwise item hashes (recorded in the committed `skilly.lock`) and | |
| 26 | + * rendered files would differ from one machine to another. | |
| 27 | + */ | |
| 28 | +const NO_EOL_CONVERSION = ['-c', 'core.autocrlf=false', '-c', 'core.eol=lf'] | |
| 29 | + | |
| 23 | 30 | function runGit(cwd: string, args: string[]): Promise<GitResult> { |
| 24 | 31 | return new Promise((resolve, reject) => { |
| 25 | 32 | execFile( |
| 26 | 33 | 'git', |
| 27 | - args, | |
| 34 | + [...NO_EOL_CONVERSION, ...args], | |
| 28 | 35 | { |
| 29 | 36 | cwd, |
| 30 | 37 | env: { ...process.env, GIT_TERMINAL_PROMPT: '0', GCM_INTERACTIVE: 'never' }, |
| @@ -154,6 +161,8 @@ class GitCatalog implements CatalogGit { | ||
| 154 | 161 | const ref = `${remote}/${branch}` |
| 155 | 162 | if ((await this.raw(['rev-parse', '--verify', '-q', `refs/remotes/${ref}`])).code !== 0) return this.status() // remote branch does not exist yet |
| 156 | 163 | if (!(await this.hasCommits())) { |
| 164 | + // New machine: `catalog init` left an empty, untracked index that would block the first checkout. | |
| 165 | + await this.dropEmptyIndex() | |
| 157 | 166 | await this.git(['checkout', '-B', branch, '--track', ref]) |
| 158 | 167 | return this.status() |
| 159 | 168 | } |
| @@ -169,6 +178,18 @@ class GitCatalog implements CatalogGit { | ||
| 169 | 178 | return this.status() |
| 170 | 179 | } |
| 171 | 180 | |
| 181 | + /** Removes `.skilly/index.json` when it holds no item metadata (the file written by `CatalogStore.init`). */ | |
| 182 | + private async dropEmptyIndex(): Promise<void> { | |
| 183 | + const file = path.join(this.root, '.skilly', 'index.json') | |
| 184 | + try { | |
| 185 | + const index = JSON.parse(stripBom(await readFile(file, 'utf8'))) as { items?: unknown } | null | |
| 186 | + const items = index?.items | |
| 187 | + if (items && typeof items === 'object' && Object.keys(items).length === 0) await rm(file, { force: true }) | |
| 188 | + } catch { | |
| 189 | + // Missing or not ours: left as is (git reports a clear error if it is in the way). | |
| 190 | + } | |
| 191 | + } | |
| 192 | + | |
| 172 | 193 | async push(): Promise<GitStatus> { |
| 173 | 194 | await this.requireRepo() |
| 174 | 195 | const st = await this.status() |
| @@ -1,9 +1,9 @@ | |||
| 1 | import { execFile } from 'node:child_process' | 1 | import { execFile } from 'node:child_process' |
| 2 | -import { mkdir } from 'node:fs/promises' | 2 | +import { mkdir, readFile, rm } from 'node:fs/promises' |
| 3 | import path from 'node:path' | 3 | import path from 'node:path' |
| 4 | import type { GitStatus } from '../catalog-model.ts' | 4 | import type { GitStatus } from '../catalog-model.ts' |
| 5 | import type { CatalogGit, OpenCatalogGit } from '../contracts.ts' | 5 | import type { CatalogGit, OpenCatalogGit } from '../contracts.ts' |
| 6 | -import { pathExists } from '../util.ts' | 6 | +import { pathExists, stripBom } from '../util.ts' |
| 7 | 7 | ||
| 8 | interface GitResult { | 8 | interface GitResult { |
| 9 | code: number | 9 | code: number |
| @@ -20,11 +20,18 @@ class GitError extends Error { | |||
| 20 | } | 20 | } |
| 21 | } | 21 | } |
| 22 | 22 | ||
| 23 | +/** | ||
| 24 | + * Line endings are never converted (Git for Windows ships `core.autocrlf=true`): the catalog files must keep | ||
| 25 | + * the exact bytes of the repository, otherwise item hashes (recorded in the committed `skilly.lock`) and | ||
| 26 | + * rendered files would differ from one machine to another. | ||
| 27 | + */ | ||
| 28 | +const NO_EOL_CONVERSION = ['-c', 'core.autocrlf=false', '-c', 'core.eol=lf'] | ||
| 29 | + | ||
| 23 | function runGit(cwd: string, args: string[]): Promise<GitResult> { | 30 | function runGit(cwd: string, args: string[]): Promise<GitResult> { |
| 24 | return new Promise((resolve, reject) => { | 31 | return new Promise((resolve, reject) => { |
| 25 | execFile( | 32 | execFile( |
| 26 | 'git', | 33 | 'git', |
| 27 | - args, | 34 | + [...NO_EOL_CONVERSION, ...args], |
| 28 | { | 35 | { |
| 29 | cwd, | 36 | cwd, |
| 30 | env: { ...process.env, GIT_TERMINAL_PROMPT: '0', GCM_INTERACTIVE: 'never' }, | 37 | env: { ...process.env, GIT_TERMINAL_PROMPT: '0', GCM_INTERACTIVE: 'never' }, |
| @@ -154,6 +161,8 @@ class GitCatalog implements CatalogGit { | |||
| 154 | const ref = `${remote}/${branch}` | 161 | const ref = `${remote}/${branch}` |
| 155 | if ((await this.raw(['rev-parse', '--verify', '-q', `refs/remotes/${ref}`])).code !== 0) return this.status() // remote branch does not exist yet | 162 | if ((await this.raw(['rev-parse', '--verify', '-q', `refs/remotes/${ref}`])).code !== 0) return this.status() // remote branch does not exist yet |
| 156 | if (!(await this.hasCommits())) { | 163 | if (!(await this.hasCommits())) { |
| 164 | + // New machine: `catalog init` left an empty, untracked index that would block the first checkout. | ||
| 165 | + await this.dropEmptyIndex() | ||
| 157 | await this.git(['checkout', '-B', branch, '--track', ref]) | 166 | await this.git(['checkout', '-B', branch, '--track', ref]) |
| 158 | return this.status() | 167 | return this.status() |
| 159 | } | 168 | } |
| @@ -169,6 +178,18 @@ class GitCatalog implements CatalogGit { | |||
| 169 | return this.status() | 178 | return this.status() |
| 170 | } | 179 | } |
| 171 | 180 | ||
| 181 | + /** Removes `.skilly/index.json` when it holds no item metadata (the file written by `CatalogStore.init`). */ | ||
| 182 | + private async dropEmptyIndex(): Promise<void> { | ||
| 183 | + const file = path.join(this.root, '.skilly', 'index.json') | ||
| 184 | + try { | ||
| 185 | + const index = JSON.parse(stripBom(await readFile(file, 'utf8'))) as { items?: unknown } | null | ||
| 186 | + const items = index?.items | ||
| 187 | + if (items && typeof items === 'object' && Object.keys(items).length === 0) await rm(file, { force: true }) | ||
| 188 | + } catch { | ||
| 189 | + // Missing or not ours: left as is (git reports a clear error if it is in the way). | ||
| 190 | + } | ||
| 191 | + } | ||
| 192 | + | ||
| 172 | async push(): Promise<GitStatus> { | 193 | async push(): Promise<GitStatus> { |
| 173 | await this.requireRepo() | 194 | await this.requireRepo() |
| 174 | const st = await this.status() | 195 | const st = await this.status() |
modified
packages/core/src/catalog/index.ts +19 -1 | @@ -357,6 +357,7 @@ class Catalog implements CatalogStore { | ||
| 357 | 357 | if (o) entry.origin = o |
| 358 | 358 | index.items[key] = entry |
| 359 | 359 | }) |
| 360 | + if (prevName) await this.renameInPresets(input.type, prevName, input.name) | |
| 360 | 361 | return this.buildItem(ref, describeContent(input.type, content, input.name), await this.readIndex()) |
| 361 | 362 | } |
| 362 | 363 | |
| @@ -532,12 +533,29 @@ class Catalog implements CatalogStore { | ||
| 532 | 533 | if (await pathExists(file)) throw new Error(`Cannot rename: preset "${preset.name}" already exists`) |
| 533 | 534 | } |
| 534 | 535 | await atomicWrite(file, stringify(doc)) |
| 535 | - if (prev) await rm(this.presetPath(prev), { force: true }) | |
| 536 | + if (prev) { | |
| 537 | + await rm(this.presetPath(prev), { force: true }) | |
| 538 | + await this.renameInPresets('preset', prev, preset.name) | |
| 539 | + } | |
| 536 | 540 | const out: Preset = { name: preset.name, includes, items } |
| 537 | 541 | if (preset.description) out.description = preset.description |
| 538 | 542 | return out |
| 539 | 543 | } |
| 540 | 544 | |
| 545 | + /** Points the presets that reference a renamed item (or included preset) at its new name. */ | |
| 546 | + private async renameInPresets(kind: CatalogItemType | 'preset', from: string, to: string): Promise<void> { | |
| 547 | + const swap = (l: string[] | undefined): string[] | undefined => (l?.includes(from) ? l.map((n) => (n === from ? to : n)) : undefined) | |
| 548 | + for (const preset of (await this.snapshot()).presets) { | |
| 549 | + if (kind === 'preset') { | |
| 550 | + const includes = swap(preset.includes) | |
| 551 | + if (includes) await this.savePreset({ ...preset, includes }) | |
| 552 | + } else { | |
| 553 | + const list = swap(preset.items[kind]) | |
| 554 | + if (list) await this.savePreset({ ...preset, items: { ...preset.items, [kind]: list } }) | |
| 555 | + } | |
| 556 | + } | |
| 557 | + } | |
| 558 | + | |
| 541 | 559 | async deletePreset(name: string): Promise<void> { |
| 542 | 560 | await rm(this.presetPath(name), { force: true }) |
| 543 | 561 | } |
| @@ -357,6 +357,7 @@ class Catalog implements CatalogStore { | |||
| 357 | if (o) entry.origin = o | 357 | if (o) entry.origin = o |
| 358 | index.items[key] = entry | 358 | index.items[key] = entry |
| 359 | }) | 359 | }) |
| 360 | + if (prevName) await this.renameInPresets(input.type, prevName, input.name) | ||
| 360 | return this.buildItem(ref, describeContent(input.type, content, input.name), await this.readIndex()) | 361 | return this.buildItem(ref, describeContent(input.type, content, input.name), await this.readIndex()) |
| 361 | } | 362 | } |
| 362 | 363 | ||
| @@ -532,12 +533,29 @@ class Catalog implements CatalogStore { | |||
| 532 | if (await pathExists(file)) throw new Error(`Cannot rename: preset "${preset.name}" already exists`) | 533 | if (await pathExists(file)) throw new Error(`Cannot rename: preset "${preset.name}" already exists`) |
| 533 | } | 534 | } |
| 534 | await atomicWrite(file, stringify(doc)) | 535 | await atomicWrite(file, stringify(doc)) |
| 535 | - if (prev) await rm(this.presetPath(prev), { force: true }) | 536 | + if (prev) { |
| 537 | + await rm(this.presetPath(prev), { force: true }) | ||
| 538 | + await this.renameInPresets('preset', prev, preset.name) | ||
| 539 | + } | ||
| 536 | const out: Preset = { name: preset.name, includes, items } | 540 | const out: Preset = { name: preset.name, includes, items } |
| 537 | if (preset.description) out.description = preset.description | 541 | if (preset.description) out.description = preset.description |
| 538 | return out | 542 | return out |
| 539 | } | 543 | } |
| 540 | 544 | ||
| 545 | + /** Points the presets that reference a renamed item (or included preset) at its new name. */ | ||
| 546 | + private async renameInPresets(kind: CatalogItemType | 'preset', from: string, to: string): Promise<void> { | ||
| 547 | + const swap = (l: string[] | undefined): string[] | undefined => (l?.includes(from) ? l.map((n) => (n === from ? to : n)) : undefined) | ||
| 548 | + for (const preset of (await this.snapshot()).presets) { | ||
| 549 | + if (kind === 'preset') { | ||
| 550 | + const includes = swap(preset.includes) | ||
| 551 | + if (includes) await this.savePreset({ ...preset, includes }) | ||
| 552 | + } else { | ||
| 553 | + const list = swap(preset.items[kind]) | ||
| 554 | + if (list) await this.savePreset({ ...preset, items: { ...preset.items, [kind]: list } }) | ||
| 555 | + } | ||
| 556 | + } | ||
| 557 | + } | ||
| 558 | + | ||
| 541 | async deletePreset(name: string): Promise<void> { | 559 | async deletePreset(name: string): Promise<void> { |
| 542 | await rm(this.presetPath(name), { force: true }) | 560 | await rm(this.presetPath(name), { force: true }) |
| 543 | } | 561 | } |
modified
packages/core/src/marketplace/git.ts +5 -2 | @@ -2,12 +2,15 @@ import { execFile } from 'node:child_process' | ||
| 2 | 2 | |
| 3 | 3 | const GIT_TIMEOUT_MS = 120_000 |
| 4 | 4 | |
| 5 | -/** Runs git with prompts disabled and a timeout. Rejects with a readable Error. */ | |
| 5 | +/** | |
| 6 | + * Runs git with prompts disabled and a timeout. Rejects with a readable Error. Line endings are never | |
| 7 | + * converted, so imported skills keep the exact bytes of their source repository. | |
| 8 | + */ | |
| 6 | 9 | export function runGit(args: string[], cwd?: string): Promise<string> { |
| 7 | 10 | return new Promise((resolve, reject) => { |
| 8 | 11 | execFile( |
| 9 | 12 | 'git', |
| 10 | - args, | |
| 13 | + ['-c', 'core.autocrlf=false', '-c', 'core.eol=lf', ...args], | |
| 11 | 14 | { |
| 12 | 15 | cwd, |
| 13 | 16 | timeout: GIT_TIMEOUT_MS, |
| @@ -2,12 +2,15 @@ import { execFile } from 'node:child_process' | |||
| 2 | 2 | ||
| 3 | const GIT_TIMEOUT_MS = 120_000 | 3 | const GIT_TIMEOUT_MS = 120_000 |
| 4 | 4 | ||
| 5 | -/** Runs git with prompts disabled and a timeout. Rejects with a readable Error. */ | 5 | +/** |
| 6 | + * Runs git with prompts disabled and a timeout. Rejects with a readable Error. Line endings are never | ||
| 7 | + * converted, so imported skills keep the exact bytes of their source repository. | ||
| 8 | + */ | ||
| 6 | export function runGit(args: string[], cwd?: string): Promise<string> { | 9 | export function runGit(args: string[], cwd?: string): Promise<string> { |
| 7 | return new Promise((resolve, reject) => { | 10 | return new Promise((resolve, reject) => { |
| 8 | execFile( | 11 | execFile( |
| 9 | 'git', | 12 | 'git', |
| 10 | - args, | 13 | + ['-c', 'core.autocrlf=false', '-c', 'core.eol=lf', ...args], |
| 11 | { | 14 | { |
| 12 | cwd, | 15 | cwd, |
| 13 | timeout: GIT_TIMEOUT_MS, | 16 | timeout: GIT_TIMEOUT_MS, |
modified
packages/core/src/marketplace/index.ts +11 -6 | @@ -1,8 +1,9 @@ | ||
| 1 | +import { readFileSync } from 'node:fs' | |
| 1 | 2 | import { mkdir, realpath, writeFile } from 'node:fs/promises' |
| 2 | 3 | import path from 'node:path' |
| 3 | 4 | import type { MarketplaceSkill, MarketplaceSource } from '../catalog-model.ts' |
| 4 | 5 | import type { CreateMarketplace } from '../contracts.ts' |
| 5 | -import { isDirectory, pathExists, readJson } from '../util.ts' | |
| 6 | +import { isDirectory, pathExists } from '../util.ts' | |
| 6 | 7 | import { BUILTIN_SOURCES } from './builtin.ts' |
| 7 | 8 | import { runGit } from './git.ts' |
| 8 | 9 | import { findMarketplaceSkills, findSkills, inside } from './scan.ts' |
| @@ -22,23 +23,27 @@ export const createMarketplace: CreateMarketplace = ({ cacheDir, userSources }) | ||
| 22 | 23 | return s |
| 23 | 24 | } |
| 24 | 25 | |
| 25 | - async function loadMeta(id: string): Promise<void> { | |
| 26 | + /** Reads the persisted fetch time once per instance (sync: tiny file, needed by `sources()`). */ | |
| 27 | + function loadMeta(id: string): void { | |
| 26 | 28 | if (loaded.has(id)) return |
| 27 | 29 | loaded.add(id) |
| 28 | 30 | try { |
| 29 | - const meta = await readJson<{ lastFetchedAt?: unknown }>(metaFile(id)) | |
| 31 | + const meta = JSON.parse(readFileSync(metaFile(id), 'utf8')) as { lastFetchedAt?: unknown } | null | |
| 30 | 32 | if (typeof meta?.lastFetchedAt === 'string' && !fetchedAt.has(id)) fetchedAt.set(id, meta.lastFetchedAt) |
| 31 | 33 | } catch { |
| 32 | - // Corrupt metadata: treated as never fetched. | |
| 34 | + // Missing or corrupt metadata: treated as never fetched. | |
| 33 | 35 | } |
| 34 | 36 | } |
| 35 | 37 | |
| 36 | 38 | async function isFetched(id: string): Promise<boolean> { |
| 37 | - await loadMeta(id) | |
| 39 | + loadMeta(id) | |
| 38 | 40 | return fetchedAt.has(id) && (await pathExists(path.join(checkoutDir(id), '.git'))) |
| 39 | 41 | } |
| 40 | 42 | |
| 41 | - const view = (s: MarketplaceSource): MarketplaceSource => ({ ...s, lastFetchedAt: fetchedAt.get(s.id) }) | |
| 43 | + const view = (s: MarketplaceSource): MarketplaceSource => { | |
| 44 | + loadMeta(s.id) | |
| 45 | + return { ...s, lastFetchedAt: fetchedAt.get(s.id) } | |
| 46 | + } | |
| 42 | 47 | |
| 43 | 48 | async function refresh(sourceId: string): Promise<MarketplaceSource> { |
| 44 | 49 | const s = getSource(sourceId) |
| @@ -1,8 +1,9 @@ | |||
| 1 | +import { readFileSync } from 'node:fs' | ||
| 1 | import { mkdir, realpath, writeFile } from 'node:fs/promises' | 2 | import { mkdir, realpath, writeFile } from 'node:fs/promises' |
| 2 | import path from 'node:path' | 3 | import path from 'node:path' |
| 3 | import type { MarketplaceSkill, MarketplaceSource } from '../catalog-model.ts' | 4 | import type { MarketplaceSkill, MarketplaceSource } from '../catalog-model.ts' |
| 4 | import type { CreateMarketplace } from '../contracts.ts' | 5 | import type { CreateMarketplace } from '../contracts.ts' |
| 5 | -import { isDirectory, pathExists, readJson } from '../util.ts' | 6 | +import { isDirectory, pathExists } from '../util.ts' |
| 6 | import { BUILTIN_SOURCES } from './builtin.ts' | 7 | import { BUILTIN_SOURCES } from './builtin.ts' |
| 7 | import { runGit } from './git.ts' | 8 | import { runGit } from './git.ts' |
| 8 | import { findMarketplaceSkills, findSkills, inside } from './scan.ts' | 9 | import { findMarketplaceSkills, findSkills, inside } from './scan.ts' |
| @@ -22,23 +23,27 @@ export const createMarketplace: CreateMarketplace = ({ cacheDir, userSources }) | |||
| 22 | return s | 23 | return s |
| 23 | } | 24 | } |
| 24 | 25 | ||
| 25 | - async function loadMeta(id: string): Promise<void> { | 26 | + /** Reads the persisted fetch time once per instance (sync: tiny file, needed by `sources()`). */ |
| 27 | + function loadMeta(id: string): void { | ||
| 26 | if (loaded.has(id)) return | 28 | if (loaded.has(id)) return |
| 27 | loaded.add(id) | 29 | loaded.add(id) |
| 28 | try { | 30 | try { |
| 29 | - const meta = await readJson<{ lastFetchedAt?: unknown }>(metaFile(id)) | 31 | + const meta = JSON.parse(readFileSync(metaFile(id), 'utf8')) as { lastFetchedAt?: unknown } | null |
| 30 | if (typeof meta?.lastFetchedAt === 'string' && !fetchedAt.has(id)) fetchedAt.set(id, meta.lastFetchedAt) | 32 | if (typeof meta?.lastFetchedAt === 'string' && !fetchedAt.has(id)) fetchedAt.set(id, meta.lastFetchedAt) |
| 31 | } catch { | 33 | } catch { |
| 32 | - // Corrupt metadata: treated as never fetched. | 34 | + // Missing or corrupt metadata: treated as never fetched. |
| 33 | } | 35 | } |
| 34 | } | 36 | } |
| 35 | 37 | ||
| 36 | async function isFetched(id: string): Promise<boolean> { | 38 | async function isFetched(id: string): Promise<boolean> { |
| 37 | - await loadMeta(id) | 39 | + loadMeta(id) |
| 38 | return fetchedAt.has(id) && (await pathExists(path.join(checkoutDir(id), '.git'))) | 40 | return fetchedAt.has(id) && (await pathExists(path.join(checkoutDir(id), '.git'))) |
| 39 | } | 41 | } |
| 40 | 42 | ||
| 41 | - const view = (s: MarketplaceSource): MarketplaceSource => ({ ...s, lastFetchedAt: fetchedAt.get(s.id) }) | 43 | + const view = (s: MarketplaceSource): MarketplaceSource => { |
| 44 | + loadMeta(s.id) | ||
| 45 | + return { ...s, lastFetchedAt: fetchedAt.get(s.id) } | ||
| 46 | + } | ||
| 42 | 47 | ||
| 43 | async function refresh(sourceId: string): Promise<MarketplaceSource> { | 48 | async function refresh(sourceId: string): Promise<MarketplaceSource> { |
| 44 | const s = getSource(sourceId) | 49 | const s = getSource(sourceId) |
modified
packages/core/src/sync/apply.ts +25 -5 | @@ -9,7 +9,9 @@ | ||
| 9 | 9 | * - `take-catalog`: the catalog version overwrites the scope (or the item is removed when no longer enabled / |
| 10 | 10 | * missing from the catalog). An existing unmanaged skill folder is replaced as a whole. |
| 11 | 11 | * - `push-to-catalog`: the scope version is saved into the catalog, then rendered again from the catalog |
| 12 | - * (or removed when no longer enabled), so the scope and the lock are in sync. | |
| 12 | + * (or removed when no longer enabled), so the scope and the lock are in sync. Pushes run first, and the | |
| 13 | + * other renders of a pushed item (the other agent) that would be written from the catalog anyway | |
| 14 | + * (unchanged, add, update, take-catalog) are rendered from the pushed version, so one apply is enough. | |
| 13 | 15 | */ |
| 14 | 16 | |
| 15 | 17 | import { rm } from 'node:fs/promises' |
| @@ -41,6 +43,8 @@ interface ApplyContext { | ||
| 41 | 43 | lock: Map<string, LockEntry> |
| 42 | 44 | /** Paths removed, for empty folder cleanup. */ |
| 43 | 45 | removed: string[] |
| 46 | + /** `type/name` of the items pushed to the catalog during this apply. */ | |
| 47 | + pushed: Set<string> | |
| 44 | 48 | } |
| 45 | 49 | |
| 46 | 50 | export async function applyPlan( |
| @@ -59,10 +63,13 @@ export async function applyPlan( | ||
| 59 | 63 | ws: new Workspace(target.root), |
| 60 | 64 | lock: new Map((internal.lock?.entries ?? []).map((e) => [lockEntryKey(e), e])), |
| 61 | 65 | removed: [], |
| 66 | + pushed: new Set(), | |
| 62 | 67 | } |
| 63 | 68 | const result: ApplyResult = { applied: [], skipped: [], errors: [], lock: emptyLock() } |
| 64 | 69 | |
| 65 | - for (const given of plan.actions) { | |
| 70 | + const isPush = (id: string) => resolutions[id] === 'push-to-catalog' | |
| 71 | + const ordered = [...plan.actions.filter((a) => isPush(a.id)), ...plan.actions.filter((a) => !isPush(a.id))] | |
| 72 | + for (const given of ordered) { | |
| 66 | 73 | const ia = internal.actions.get(given.id) |
| 67 | 74 | if (!ia) { |
| 68 | 75 | result.errors.push({ actionId: given.id, message: 'The scope or the catalog changed since this plan was computed; plan again' }) |
| @@ -97,12 +104,18 @@ function emptyLock(): LockFile { | ||
| 97 | 104 | async function executeAction(ia: InternalAction, resolution: ConflictResolution | undefined, ctx: ApplyContext): Promise<Outcome | undefined> { |
| 98 | 105 | const { action } = ia |
| 99 | 106 | switch (action.kind) { |
| 100 | - case 'unchanged': | |
| 107 | + case 'unchanged': { | |
| 108 | + const fresh = await freshRender(ia, ctx) | |
| 109 | + if (fresh && fresh.sourceHash !== ia.lockEntry?.sourceHash) { | |
| 110 | + await install(fresh, ia.lockEntry, ctx) | |
| 111 | + return 'applied' | |
| 112 | + } | |
| 101 | 113 | if (ia.lockEntry && ia.desired) ctx.lock.set(lockEntryKey(ia.lockEntry), { ...ia.lockEntry, via: ia.desired.via }) |
| 102 | 114 | return undefined |
| 115 | + } | |
| 103 | 116 | case 'add': |
| 104 | 117 | case 'update': |
| 105 | - await install(ia.desired!, ia.lockEntry, ctx) | |
| 118 | + await install((await freshRender(ia, ctx)) ?? ia.desired!, ia.lockEntry, ctx) | |
| 106 | 119 | return 'applied' |
| 107 | 120 | case 'remove': |
| 108 | 121 | await uninstall(ia.lockEntry!, ctx) |
| @@ -121,7 +134,7 @@ async function resolveConflict(ia: InternalAction, resolution: ConflictResolutio | ||
| 121 | 134 | if (entry && (!ia.desired || ia.action.conflict === 'missing-in-catalog')) ctx.lock.delete(lockEntryKey(entry)) |
| 122 | 135 | return |
| 123 | 136 | case 'take-catalog': |
| 124 | - if (ia.desired) await install(ia.desired, entry, ctx) | |
| 137 | + if (ia.desired) await install((await freshRender(ia, ctx)) ?? ia.desired, entry, ctx) | |
| 125 | 138 | else if (entry) await uninstall(entry, ctx) |
| 126 | 139 | return |
| 127 | 140 | case 'push-to-catalog': { |
| @@ -131,6 +144,7 @@ async function resolveConflict(ia: InternalAction, resolution: ConflictResolutio | ||
| 131 | 144 | catalog: ctx.catalog, |
| 132 | 145 | disk: new DiskReader(ctx.target.root), |
| 133 | 146 | }) |
| 147 | + ctx.pushed.add(`${ia.action.type}/${ia.action.name}`) | |
| 134 | 148 | if (!ia.wanted) { |
| 135 | 149 | if (entry) await uninstall(entry, ctx) |
| 136 | 150 | return |
| @@ -143,6 +157,12 @@ async function resolveConflict(ia: InternalAction, resolution: ConflictResolutio | ||
| 143 | 157 | } |
| 144 | 158 | } |
| 145 | 159 | |
| 160 | +/** Render from the current catalog when the item was pushed earlier in this apply (undefined otherwise). */ | |
| 161 | +async function freshRender(ia: InternalAction, ctx: ApplyContext): Promise<DesiredItem | undefined> { | |
| 162 | + if (!ia.desired || !ctx.pushed.has(`${ia.desired.type}/${ia.desired.name}`)) return undefined | |
| 163 | + return ctx.internal.render({ type: ia.desired.type, name: ia.desired.name }, ia.desired.agent, ia.desired.via) | |
| 164 | +} | |
| 165 | + | |
| 146 | 166 | /** |
| 147 | 167 | * Writes a render over what the lock recorded. Shared-file edits are computed first (they may fail on |
| 148 | 168 | * a broken file), then files are written, then the edits are committed to the workspace. |
| @@ -9,7 +9,9 @@ | |||
| 9 | * - `take-catalog`: the catalog version overwrites the scope (or the item is removed when no longer enabled / | 9 | * - `take-catalog`: the catalog version overwrites the scope (or the item is removed when no longer enabled / |
| 10 | * missing from the catalog). An existing unmanaged skill folder is replaced as a whole. | 10 | * missing from the catalog). An existing unmanaged skill folder is replaced as a whole. |
| 11 | * - `push-to-catalog`: the scope version is saved into the catalog, then rendered again from the catalog | 11 | * - `push-to-catalog`: the scope version is saved into the catalog, then rendered again from the catalog |
| 12 | - * (or removed when no longer enabled), so the scope and the lock are in sync. | 12 | + * (or removed when no longer enabled), so the scope and the lock are in sync. Pushes run first, and the |
| 13 | + * other renders of a pushed item (the other agent) that would be written from the catalog anyway | ||
| 14 | + * (unchanged, add, update, take-catalog) are rendered from the pushed version, so one apply is enough. | ||
| 13 | */ | 15 | */ |
| 14 | 16 | ||
| 15 | import { rm } from 'node:fs/promises' | 17 | import { rm } from 'node:fs/promises' |
| @@ -41,6 +43,8 @@ interface ApplyContext { | |||
| 41 | lock: Map<string, LockEntry> | 43 | lock: Map<string, LockEntry> |
| 42 | /** Paths removed, for empty folder cleanup. */ | 44 | /** Paths removed, for empty folder cleanup. */ |
| 43 | removed: string[] | 45 | removed: string[] |
| 46 | + /** `type/name` of the items pushed to the catalog during this apply. */ | ||
| 47 | + pushed: Set<string> | ||
| 44 | } | 48 | } |
| 45 | 49 | ||
| 46 | export async function applyPlan( | 50 | export async function applyPlan( |
| @@ -59,10 +63,13 @@ export async function applyPlan( | |||
| 59 | ws: new Workspace(target.root), | 63 | ws: new Workspace(target.root), |
| 60 | lock: new Map((internal.lock?.entries ?? []).map((e) => [lockEntryKey(e), e])), | 64 | lock: new Map((internal.lock?.entries ?? []).map((e) => [lockEntryKey(e), e])), |
| 61 | removed: [], | 65 | removed: [], |
| 66 | + pushed: new Set(), | ||
| 62 | } | 67 | } |
| 63 | const result: ApplyResult = { applied: [], skipped: [], errors: [], lock: emptyLock() } | 68 | const result: ApplyResult = { applied: [], skipped: [], errors: [], lock: emptyLock() } |
| 64 | 69 | ||
| 65 | - for (const given of plan.actions) { | 70 | + const isPush = (id: string) => resolutions[id] === 'push-to-catalog' |
| 71 | + const ordered = [...plan.actions.filter((a) => isPush(a.id)), ...plan.actions.filter((a) => !isPush(a.id))] | ||
| 72 | + for (const given of ordered) { | ||
| 66 | const ia = internal.actions.get(given.id) | 73 | const ia = internal.actions.get(given.id) |
| 67 | if (!ia) { | 74 | if (!ia) { |
| 68 | result.errors.push({ actionId: given.id, message: 'The scope or the catalog changed since this plan was computed; plan again' }) | 75 | result.errors.push({ actionId: given.id, message: 'The scope or the catalog changed since this plan was computed; plan again' }) |
| @@ -97,12 +104,18 @@ function emptyLock(): LockFile { | |||
| 97 | async function executeAction(ia: InternalAction, resolution: ConflictResolution | undefined, ctx: ApplyContext): Promise<Outcome | undefined> { | 104 | async function executeAction(ia: InternalAction, resolution: ConflictResolution | undefined, ctx: ApplyContext): Promise<Outcome | undefined> { |
| 98 | const { action } = ia | 105 | const { action } = ia |
| 99 | switch (action.kind) { | 106 | switch (action.kind) { |
| 100 | - case 'unchanged': | 107 | + case 'unchanged': { |
| 108 | + const fresh = await freshRender(ia, ctx) | ||
| 109 | + if (fresh && fresh.sourceHash !== ia.lockEntry?.sourceHash) { | ||
| 110 | + await install(fresh, ia.lockEntry, ctx) | ||
| 111 | + return 'applied' | ||
| 112 | + } | ||
| 101 | if (ia.lockEntry && ia.desired) ctx.lock.set(lockEntryKey(ia.lockEntry), { ...ia.lockEntry, via: ia.desired.via }) | 113 | if (ia.lockEntry && ia.desired) ctx.lock.set(lockEntryKey(ia.lockEntry), { ...ia.lockEntry, via: ia.desired.via }) |
| 102 | return undefined | 114 | return undefined |
| 115 | + } | ||
| 103 | case 'add': | 116 | case 'add': |
| 104 | case 'update': | 117 | case 'update': |
| 105 | - await install(ia.desired!, ia.lockEntry, ctx) | 118 | + await install((await freshRender(ia, ctx)) ?? ia.desired!, ia.lockEntry, ctx) |
| 106 | return 'applied' | 119 | return 'applied' |
| 107 | case 'remove': | 120 | case 'remove': |
| 108 | await uninstall(ia.lockEntry!, ctx) | 121 | await uninstall(ia.lockEntry!, ctx) |
| @@ -121,7 +134,7 @@ async function resolveConflict(ia: InternalAction, resolution: ConflictResolutio | |||
| 121 | if (entry && (!ia.desired || ia.action.conflict === 'missing-in-catalog')) ctx.lock.delete(lockEntryKey(entry)) | 134 | if (entry && (!ia.desired || ia.action.conflict === 'missing-in-catalog')) ctx.lock.delete(lockEntryKey(entry)) |
| 122 | return | 135 | return |
| 123 | case 'take-catalog': | 136 | case 'take-catalog': |
| 124 | - if (ia.desired) await install(ia.desired, entry, ctx) | 137 | + if (ia.desired) await install((await freshRender(ia, ctx)) ?? ia.desired, entry, ctx) |
| 125 | else if (entry) await uninstall(entry, ctx) | 138 | else if (entry) await uninstall(entry, ctx) |
| 126 | return | 139 | return |
| 127 | case 'push-to-catalog': { | 140 | case 'push-to-catalog': { |
| @@ -131,6 +144,7 @@ async function resolveConflict(ia: InternalAction, resolution: ConflictResolutio | |||
| 131 | catalog: ctx.catalog, | 144 | catalog: ctx.catalog, |
| 132 | disk: new DiskReader(ctx.target.root), | 145 | disk: new DiskReader(ctx.target.root), |
| 133 | }) | 146 | }) |
| 147 | + ctx.pushed.add(`${ia.action.type}/${ia.action.name}`) | ||
| 134 | if (!ia.wanted) { | 148 | if (!ia.wanted) { |
| 135 | if (entry) await uninstall(entry, ctx) | 149 | if (entry) await uninstall(entry, ctx) |
| 136 | return | 150 | return |
| @@ -143,6 +157,12 @@ async function resolveConflict(ia: InternalAction, resolution: ConflictResolutio | |||
| 143 | } | 157 | } |
| 144 | } | 158 | } |
| 145 | 159 | ||
| 160 | +/** Render from the current catalog when the item was pushed earlier in this apply (undefined otherwise). */ | ||
| 161 | +async function freshRender(ia: InternalAction, ctx: ApplyContext): Promise<DesiredItem | undefined> { | ||
| 162 | + if (!ia.desired || !ctx.pushed.has(`${ia.desired.type}/${ia.desired.name}`)) return undefined | ||
| 163 | + return ctx.internal.render({ type: ia.desired.type, name: ia.desired.name }, ia.desired.agent, ia.desired.via) | ||
| 164 | +} | ||
| 165 | + | ||
| 146 | /** | 166 | /** |
| 147 | * Writes a render over what the lock recorded. Shared-file edits are computed first (they may fail on | 167 | * Writes a render over what the lock recorded. Shared-file edits are computed first (they may fail on |
| 148 | * a broken file), then files are written, then the edits are committed to the workspace. | 168 | * a broken file), then files are written, then the edits are committed to the workspace. |
modified
packages/core/test/catalog-git.test.ts +25 -0 | @@ -3,6 +3,7 @@ import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' | ||
| 3 | 3 | import os from 'node:os' |
| 4 | 4 | import path from 'node:path' |
| 5 | 5 | import { afterAll, beforeAll, describe, expect, it } from 'vitest' |
| 6 | +import { openCatalog } from '../src/catalog/index.ts' | |
| 6 | 7 | import { openCatalogGit } from '../src/catalog/git.ts' |
| 7 | 8 | |
| 8 | 9 | let tmp: string |
| @@ -112,6 +113,30 @@ describe('catalog git', { timeout: 60_000 }, () => { | ||
| 112 | 113 | expect(await readFile(path.join(b, 'note.txt'), 'utf8')).toBe('b again\n') |
| 113 | 114 | }) |
| 114 | 115 | |
| 116 | + it('restores the catalog on a new machine: init the catalog, connect the remote, pull', async () => { | |
| 117 | + const d = path.join(tmp, 'd') | |
| 118 | + await openCatalog(d).init() | |
| 119 | + await setup(d) | |
| 120 | + const st = await openCatalogGit(d).pull() | |
| 121 | + expect(st.conflicts).toEqual([]) | |
| 122 | + expect(st.lastCommit).toBeDefined() | |
| 123 | + expect(await readFile(path.join(d, 'skills', 'x', 'SKILL.md'), 'utf8')).toBe('one\n') | |
| 124 | + }) | |
| 125 | + | |
| 126 | + it('keeps the exact bytes of the repository even when core.autocrlf is on (Git for Windows default)', async () => { | |
| 127 | + const c = path.join(tmp, 'c') | |
| 128 | + await setup(c) | |
| 129 | + git(c, 'config', 'core.autocrlf', 'true') | |
| 130 | + const gc = openCatalogGit(c) | |
| 131 | + await gc.pull() | |
| 132 | + expect(await readFile(path.join(c, 'skills', 'x', 'SKILL.md'), 'utf8')).toBe('one\n') | |
| 133 | + await writeFile(path.join(c, 'rule.md'), 'a\nb\n') | |
| 134 | + await gc.sync('lf file') | |
| 135 | + expect((await gc.status()).dirty).toEqual([]) | |
| 136 | + await openCatalogGit(a).pull() | |
| 137 | + expect(await readFile(path.join(a, 'rule.md'), 'utf8')).toBe('a\nb\n') | |
| 138 | + }) | |
| 139 | + | |
| 115 | 140 | it('surfaces git errors as Errors', async () => { |
| 116 | 141 | const dir = path.join(tmp, 'norepo') |
| 117 | 142 | await mkdir(dir) |
| @@ -3,6 +3,7 @@ import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' | |||
| 3 | import os from 'node:os' | 3 | import os from 'node:os' |
| 4 | import path from 'node:path' | 4 | import path from 'node:path' |
| 5 | import { afterAll, beforeAll, describe, expect, it } from 'vitest' | 5 | import { afterAll, beforeAll, describe, expect, it } from 'vitest' |
| 6 | +import { openCatalog } from '../src/catalog/index.ts' | ||
| 6 | import { openCatalogGit } from '../src/catalog/git.ts' | 7 | import { openCatalogGit } from '../src/catalog/git.ts' |
| 7 | 8 | ||
| 8 | let tmp: string | 9 | let tmp: string |
| @@ -112,6 +113,30 @@ describe('catalog git', { timeout: 60_000 }, () => { | |||
| 112 | expect(await readFile(path.join(b, 'note.txt'), 'utf8')).toBe('b again\n') | 113 | expect(await readFile(path.join(b, 'note.txt'), 'utf8')).toBe('b again\n') |
| 113 | }) | 114 | }) |
| 114 | 115 | ||
| 116 | + it('restores the catalog on a new machine: init the catalog, connect the remote, pull', async () => { | ||
| 117 | + const d = path.join(tmp, 'd') | ||
| 118 | + await openCatalog(d).init() | ||
| 119 | + await setup(d) | ||
| 120 | + const st = await openCatalogGit(d).pull() | ||
| 121 | + expect(st.conflicts).toEqual([]) | ||
| 122 | + expect(st.lastCommit).toBeDefined() | ||
| 123 | + expect(await readFile(path.join(d, 'skills', 'x', 'SKILL.md'), 'utf8')).toBe('one\n') | ||
| 124 | + }) | ||
| 125 | + | ||
| 126 | + it('keeps the exact bytes of the repository even when core.autocrlf is on (Git for Windows default)', async () => { | ||
| 127 | + const c = path.join(tmp, 'c') | ||
| 128 | + await setup(c) | ||
| 129 | + git(c, 'config', 'core.autocrlf', 'true') | ||
| 130 | + const gc = openCatalogGit(c) | ||
| 131 | + await gc.pull() | ||
| 132 | + expect(await readFile(path.join(c, 'skills', 'x', 'SKILL.md'), 'utf8')).toBe('one\n') | ||
| 133 | + await writeFile(path.join(c, 'rule.md'), 'a\nb\n') | ||
| 134 | + await gc.sync('lf file') | ||
| 135 | + expect((await gc.status()).dirty).toEqual([]) | ||
| 136 | + await openCatalogGit(a).pull() | ||
| 137 | + expect(await readFile(path.join(a, 'rule.md'), 'utf8')).toBe('a\nb\n') | ||
| 138 | + }) | ||
| 139 | + | ||
| 115 | it('surfaces git errors as Errors', async () => { | 140 | it('surfaces git errors as Errors', async () => { |
| 116 | const dir = path.join(tmp, 'norepo') | 141 | const dir = path.join(tmp, 'norepo') |
| 117 | await mkdir(dir) | 142 | await mkdir(dir) |
modified
packages/core/test/marketplace.test.ts +2 -1 | @@ -120,8 +120,9 @@ describe('marketplace', () => { | ||
| 120 | 120 | await m.refresh('plain') |
| 121 | 121 | expect((await m.listSkills('plain')).map((s) => s.name)).toContain('late') |
| 122 | 122 | |
| 123 | - // A new instance sees the persisted timestamp and does not re-clone. | |
| 123 | + // A new instance sees the persisted timestamp (even before any other call) and does not re-clone. | |
| 124 | 124 | const m2 = createMarketplace({ cacheDir, userSources: sources }) |
| 125 | + expect(m2.sources().find((s) => s.id === 'plain')?.lastFetchedAt).toBeTruthy() | |
| 125 | 126 | await m2.listSkills('plain') |
| 126 | 127 | expect(m2.sources().find((s) => s.id === 'plain')?.lastFetchedAt).toBeTruthy() |
| 127 | 128 | expect(JSON.parse(await readFile(path.join(cacheDir, 'plain.json'), 'utf8')).lastFetchedAt).toBeTruthy() |
| @@ -120,8 +120,9 @@ describe('marketplace', () => { | |||
| 120 | await m.refresh('plain') | 120 | await m.refresh('plain') |
| 121 | expect((await m.listSkills('plain')).map((s) => s.name)).toContain('late') | 121 | expect((await m.listSkills('plain')).map((s) => s.name)).toContain('late') |
| 122 | 122 | ||
| 123 | - // A new instance sees the persisted timestamp and does not re-clone. | 123 | + // A new instance sees the persisted timestamp (even before any other call) and does not re-clone. |
| 124 | const m2 = createMarketplace({ cacheDir, userSources: sources }) | 124 | const m2 = createMarketplace({ cacheDir, userSources: sources }) |
| 125 | + expect(m2.sources().find((s) => s.id === 'plain')?.lastFetchedAt).toBeTruthy() | ||
| 125 | await m2.listSkills('plain') | 126 | await m2.listSkills('plain') |
| 126 | expect(m2.sources().find((s) => s.id === 'plain')?.lastFetchedAt).toBeTruthy() | 127 | expect(m2.sources().find((s) => s.id === 'plain')?.lastFetchedAt).toBeTruthy() |
| 127 | expect(JSON.parse(await readFile(path.join(cacheDir, 'plain.json'), 'utf8')).lastFetchedAt).toBeTruthy() | 128 | expect(JSON.parse(await readFile(path.join(cacheDir, 'plain.json'), 'utf8')).lastFetchedAt).toBeTruthy() |
modified
packages/core/test/sync.test.ts +23 -4 | @@ -339,17 +339,36 @@ describe('project sync', () => { | ||
| 339 | 339 | expect(action(await plan(), 'claude', 'rule/typescript-strict').kind).toBe('conflict') |
| 340 | 340 | }) |
| 341 | 341 | |
| 342 | - it('push-to-catalog saves the local version, then the other agent gets the update', async () => { | |
| 342 | + it('push-to-catalog saves the local version, and the other agent gets it in the same apply', async () => { | |
| 343 | 343 | const pl = await plan() |
| 344 | 344 | const a = action(pl, 'claude', 'rule/typescript-strict') |
| 345 | + expect(action(pl, 'codex', 'rule/typescript-strict').kind).toBe('unchanged') | |
| 345 | 346 | const { result } = await sync({ [a.id]: 'push-to-catalog' }) |
| 346 | 347 | expect(result.errors).toEqual([]) |
| 347 | 348 | expect(await readFile(path.join(catalog.root, 'rules/typescript-strict.md'), 'utf8')).toContain('Local rule.') |
| 349 | + expect(result.applied).toContain(action(pl, 'codex', 'rule/typescript-strict').id) | |
| 350 | + expect(await read('AGENTS.md')).toContain('## typescript-strict (applies to **/*.ts)\n\n- Local rule.') | |
| 348 | 351 | const next = await plan() |
| 349 | 352 | expect(action(next, 'claude', 'rule/typescript-strict').kind).toBe('unchanged') |
| 350 | - expect(action(next, 'codex', 'rule/typescript-strict')).toMatchObject({ kind: 'update', catalogUpdated: true }) | |
| 351 | - await sync() | |
| 352 | - expect(await read('AGENTS.md')).toContain('## typescript-strict (applies to **/*.ts)\n\n- Local rule.') | |
| 353 | + expect(action(next, 'codex', 'rule/typescript-strict').kind).toBe('unchanged') | |
| 354 | + }) | |
| 355 | + | |
| 356 | + it('push-to-catalog on one agent and take-catalog on the other write the pushed version everywhere', async () => { | |
| 357 | + await put(p('.claude/rules/typescript-strict.md'), (await read('.claude/rules/typescript-strict.md')).replace('- Local rule.', '- Pushed rule.')) | |
| 358 | + await put(p('AGENTS.md'), (await read('AGENTS.md')).replace('- Local rule.', '- Stale codex edit.')) | |
| 359 | + const pl = await plan() | |
| 360 | + const cc = action(pl, 'claude', 'rule/typescript-strict') | |
| 361 | + const cx = action(pl, 'codex', 'rule/typescript-strict') | |
| 362 | + expect(cc.conflict).toBe('modified-locally') | |
| 363 | + expect(cx.conflict).toBe('modified-locally') | |
| 364 | + const { result } = await sync({ [cx.id]: 'take-catalog', [cc.id]: 'push-to-catalog' }) | |
| 365 | + expect(result.errors).toEqual([]) | |
| 366 | + const agentsMd = await read('AGENTS.md') | |
| 367 | + expect(agentsMd).toContain('- Pushed rule.') | |
| 368 | + expect(agentsMd).not.toContain('Stale codex edit') | |
| 369 | + const next = await plan() | |
| 370 | + expect(action(next, 'claude', 'rule/typescript-strict').kind).toBe('unchanged') | |
| 371 | + expect(action(next, 'codex', 'rule/typescript-strict').kind).toBe('unchanged') | |
| 353 | 372 | }) |
| 354 | 373 | |
| 355 | 374 | it('take-catalog overwrites a local edit in a shared file', async () => { |
| @@ -339,17 +339,36 @@ describe('project sync', () => { | |||
| 339 | expect(action(await plan(), 'claude', 'rule/typescript-strict').kind).toBe('conflict') | 339 | expect(action(await plan(), 'claude', 'rule/typescript-strict').kind).toBe('conflict') |
| 340 | }) | 340 | }) |
| 341 | 341 | ||
| 342 | - it('push-to-catalog saves the local version, then the other agent gets the update', async () => { | 342 | + it('push-to-catalog saves the local version, and the other agent gets it in the same apply', async () => { |
| 343 | const pl = await plan() | 343 | const pl = await plan() |
| 344 | const a = action(pl, 'claude', 'rule/typescript-strict') | 344 | const a = action(pl, 'claude', 'rule/typescript-strict') |
| 345 | + expect(action(pl, 'codex', 'rule/typescript-strict').kind).toBe('unchanged') | ||
| 345 | const { result } = await sync({ [a.id]: 'push-to-catalog' }) | 346 | const { result } = await sync({ [a.id]: 'push-to-catalog' }) |
| 346 | expect(result.errors).toEqual([]) | 347 | expect(result.errors).toEqual([]) |
| 347 | expect(await readFile(path.join(catalog.root, 'rules/typescript-strict.md'), 'utf8')).toContain('Local rule.') | 348 | expect(await readFile(path.join(catalog.root, 'rules/typescript-strict.md'), 'utf8')).toContain('Local rule.') |
| 349 | + expect(result.applied).toContain(action(pl, 'codex', 'rule/typescript-strict').id) | ||
| 350 | + expect(await read('AGENTS.md')).toContain('## typescript-strict (applies to **/*.ts)\n\n- Local rule.') | ||
| 348 | const next = await plan() | 351 | const next = await plan() |
| 349 | expect(action(next, 'claude', 'rule/typescript-strict').kind).toBe('unchanged') | 352 | expect(action(next, 'claude', 'rule/typescript-strict').kind).toBe('unchanged') |
| 350 | - expect(action(next, 'codex', 'rule/typescript-strict')).toMatchObject({ kind: 'update', catalogUpdated: true }) | 353 | + expect(action(next, 'codex', 'rule/typescript-strict').kind).toBe('unchanged') |
| 351 | - await sync() | 354 | + }) |
| 352 | - expect(await read('AGENTS.md')).toContain('## typescript-strict (applies to **/*.ts)\n\n- Local rule.') | 355 | + |
| 356 | + it('push-to-catalog on one agent and take-catalog on the other write the pushed version everywhere', async () => { | ||
| 357 | + await put(p('.claude/rules/typescript-strict.md'), (await read('.claude/rules/typescript-strict.md')).replace('- Local rule.', '- Pushed rule.')) | ||
| 358 | + await put(p('AGENTS.md'), (await read('AGENTS.md')).replace('- Local rule.', '- Stale codex edit.')) | ||
| 359 | + const pl = await plan() | ||
| 360 | + const cc = action(pl, 'claude', 'rule/typescript-strict') | ||
| 361 | + const cx = action(pl, 'codex', 'rule/typescript-strict') | ||
| 362 | + expect(cc.conflict).toBe('modified-locally') | ||
| 363 | + expect(cx.conflict).toBe('modified-locally') | ||
| 364 | + const { result } = await sync({ [cx.id]: 'take-catalog', [cc.id]: 'push-to-catalog' }) | ||
| 365 | + expect(result.errors).toEqual([]) | ||
| 366 | + const agentsMd = await read('AGENTS.md') | ||
| 367 | + expect(agentsMd).toContain('- Pushed rule.') | ||
| 368 | + expect(agentsMd).not.toContain('Stale codex edit') | ||
| 369 | + const next = await plan() | ||
| 370 | + expect(action(next, 'claude', 'rule/typescript-strict').kind).toBe('unchanged') | ||
| 371 | + expect(action(next, 'codex', 'rule/typescript-strict').kind).toBe('unchanged') | ||
| 353 | }) | 372 | }) |
| 354 | 373 | ||
| 355 | it('take-catalog overwrites a local edit in a shared file', async () => { | 374 | it('take-catalog overwrites a local edit in a shared file', async () => { |
modified
packages/server/src/catalog-routes.ts +40 -1 | @@ -79,6 +79,42 @@ async function computeUsage(ctx: AppContext): Promise<Record<string, string[]>> | ||
| 79 | 79 | return usage |
| 80 | 80 | } |
| 81 | 81 | |
| 82 | +/** | |
| 83 | + * Points the scope configs (`skilly.json` / `home.json`) at the new name of a renamed item or preset, so that a | |
| 84 | + * rename does not silently disable it. Exact names only: glob patterns are left as is. Nothing is applied. | |
| 85 | + */ | |
| 86 | +async function renameInScopeConfigs(ctx: AppContext, kind: CatalogItemType | 'preset', from: string, to: string): Promise<void> { | |
| 87 | + const swap = (l: string[] | undefined): string[] | undefined => | |
| 88 | + l?.includes(from) ? [...new Set(l.map((n) => (n === from ? to : n)))] : undefined | |
| 89 | + const scopeIds = ['home', ...(await ctx.store.list()).map((p) => p.id)] | |
| 90 | + for (const id of scopeIds) { | |
| 91 | + try { | |
| 92 | + const scope = await ctx.scope(id) | |
| 93 | + const config = await ctx.syncEngine.readConfig(scope.target) | |
| 94 | + if (!config) continue | |
| 95 | + const next: ScopeConfig = { ...config, add: { ...config.add }, exclude: { ...config.exclude }, onDemand: { ...config.onDemand } } | |
| 96 | + let changed = false | |
| 97 | + const set = <T>(value: T | undefined, assign: (v: T) => void): void => { | |
| 98 | + if (value === undefined) return | |
| 99 | + assign(value) | |
| 100 | + changed = true | |
| 101 | + } | |
| 102 | + if (kind === 'preset') { | |
| 103 | + set(swap(config.presets), (v) => (next.presets = v)) | |
| 104 | + } else { | |
| 105 | + set(swap(config.add[kind]), (v) => (next.add[kind] = v)) | |
| 106 | + set(swap(config.exclude[kind]), (v) => (next.exclude[kind] = v)) | |
| 107 | + if (kind === 'skill') set(swap(config.onDemand.skills), (v) => (next.onDemand.skills = v)) | |
| 108 | + } | |
| 109 | + if (!changed) continue | |
| 110 | + await ctx.syncEngine.writeConfig(scope.target, next) | |
| 111 | + ctx.invalidateScan(id) | |
| 112 | + } catch (e) { | |
| 113 | + console.error(`skilly: could not update the config of scope ${id} after a rename:`, e instanceof Error ? e.message : e) | |
| 114 | + } | |
| 115 | + } | |
| 116 | +} | |
| 117 | + | |
| 82 | 118 | function usedMessage(key: string, scopes: string[]): string { |
| 83 | 119 | return `${key} is used by ${scopes.length} scope${scopes.length === 1 ? '' : 's'} (${scopes.join(', ')}); use ?force=1 to delete anyway` |
| 84 | 120 | } |
| @@ -161,6 +197,7 @@ export function registerCatalogRoutes(app: Hono, ctx: AppContext): void { | ||
| 161 | 197 | const catalog = await ctx.catalog() |
| 162 | 198 | if (!(await catalog.exists())) await catalog.init() |
| 163 | 199 | const item = await catalog.saveItem(input) |
| 200 | + if (input.previousName && input.previousName !== name) await renameInScopeConfigs(ctx, type, input.previousName, name) | |
| 164 | 201 | ctx.catalogChanged() |
| 165 | 202 | return c.json(item) |
| 166 | 203 | }) |
| @@ -224,7 +261,9 @@ export function registerCatalogRoutes(app: Hono, ctx: AppContext): void { | ||
| 224 | 261 | if (body.previousName !== undefined && typeof body.previousName !== 'string') throw new HttpError(400, '"previousName" must be a string') |
| 225 | 262 | const catalog = await ctx.catalog() |
| 226 | 263 | if (!(await catalog.exists())) await catalog.init() |
| 227 | - const saved = await catalog.savePreset(normalizePreset(body, name), body.previousName as string | undefined) | |
| 264 | + const previousName = body.previousName as string | undefined | |
| 265 | + const saved = await catalog.savePreset(normalizePreset(body, name), previousName) | |
| 266 | + if (previousName && previousName !== name) await renameInScopeConfigs(ctx, 'preset', previousName, name) | |
| 228 | 267 | ctx.catalogChanged() |
| 229 | 268 | return c.json(saved) |
| 230 | 269 | }) |
| @@ -79,6 +79,42 @@ async function computeUsage(ctx: AppContext): Promise<Record<string, string[]>> | |||
| 79 | return usage | 79 | return usage |
| 80 | } | 80 | } |
| 81 | 81 | ||
| 82 | +/** | ||
| 83 | + * Points the scope configs (`skilly.json` / `home.json`) at the new name of a renamed item or preset, so that a | ||
| 84 | + * rename does not silently disable it. Exact names only: glob patterns are left as is. Nothing is applied. | ||
| 85 | + */ | ||
| 86 | +async function renameInScopeConfigs(ctx: AppContext, kind: CatalogItemType | 'preset', from: string, to: string): Promise<void> { | ||
| 87 | + const swap = (l: string[] | undefined): string[] | undefined => | ||
| 88 | + l?.includes(from) ? [...new Set(l.map((n) => (n === from ? to : n)))] : undefined | ||
| 89 | + const scopeIds = ['home', ...(await ctx.store.list()).map((p) => p.id)] | ||
| 90 | + for (const id of scopeIds) { | ||
| 91 | + try { | ||
| 92 | + const scope = await ctx.scope(id) | ||
| 93 | + const config = await ctx.syncEngine.readConfig(scope.target) | ||
| 94 | + if (!config) continue | ||
| 95 | + const next: ScopeConfig = { ...config, add: { ...config.add }, exclude: { ...config.exclude }, onDemand: { ...config.onDemand } } | ||
| 96 | + let changed = false | ||
| 97 | + const set = <T>(value: T | undefined, assign: (v: T) => void): void => { | ||
| 98 | + if (value === undefined) return | ||
| 99 | + assign(value) | ||
| 100 | + changed = true | ||
| 101 | + } | ||
| 102 | + if (kind === 'preset') { | ||
| 103 | + set(swap(config.presets), (v) => (next.presets = v)) | ||
| 104 | + } else { | ||
| 105 | + set(swap(config.add[kind]), (v) => (next.add[kind] = v)) | ||
| 106 | + set(swap(config.exclude[kind]), (v) => (next.exclude[kind] = v)) | ||
| 107 | + if (kind === 'skill') set(swap(config.onDemand.skills), (v) => (next.onDemand.skills = v)) | ||
| 108 | + } | ||
| 109 | + if (!changed) continue | ||
| 110 | + await ctx.syncEngine.writeConfig(scope.target, next) | ||
| 111 | + ctx.invalidateScan(id) | ||
| 112 | + } catch (e) { | ||
| 113 | + console.error(`skilly: could not update the config of scope ${id} after a rename:`, e instanceof Error ? e.message : e) | ||
| 114 | + } | ||
| 115 | + } | ||
| 116 | +} | ||
| 117 | + | ||
| 82 | function usedMessage(key: string, scopes: string[]): string { | 118 | function usedMessage(key: string, scopes: string[]): string { |
| 83 | return `${key} is used by ${scopes.length} scope${scopes.length === 1 ? '' : 's'} (${scopes.join(', ')}); use ?force=1 to delete anyway` | 119 | return `${key} is used by ${scopes.length} scope${scopes.length === 1 ? '' : 's'} (${scopes.join(', ')}); use ?force=1 to delete anyway` |
| 84 | } | 120 | } |
| @@ -161,6 +197,7 @@ export function registerCatalogRoutes(app: Hono, ctx: AppContext): void { | |||
| 161 | const catalog = await ctx.catalog() | 197 | const catalog = await ctx.catalog() |
| 162 | if (!(await catalog.exists())) await catalog.init() | 198 | if (!(await catalog.exists())) await catalog.init() |
| 163 | const item = await catalog.saveItem(input) | 199 | const item = await catalog.saveItem(input) |
| 200 | + if (input.previousName && input.previousName !== name) await renameInScopeConfigs(ctx, type, input.previousName, name) | ||
| 164 | ctx.catalogChanged() | 201 | ctx.catalogChanged() |
| 165 | return c.json(item) | 202 | return c.json(item) |
| 166 | }) | 203 | }) |
| @@ -224,7 +261,9 @@ export function registerCatalogRoutes(app: Hono, ctx: AppContext): void { | |||
| 224 | if (body.previousName !== undefined && typeof body.previousName !== 'string') throw new HttpError(400, '"previousName" must be a string') | 261 | if (body.previousName !== undefined && typeof body.previousName !== 'string') throw new HttpError(400, '"previousName" must be a string') |
| 225 | const catalog = await ctx.catalog() | 262 | const catalog = await ctx.catalog() |
| 226 | if (!(await catalog.exists())) await catalog.init() | 263 | if (!(await catalog.exists())) await catalog.init() |
| 227 | - const saved = await catalog.savePreset(normalizePreset(body, name), body.previousName as string | undefined) | 264 | + const previousName = body.previousName as string | undefined |
| 265 | + const saved = await catalog.savePreset(normalizePreset(body, name), previousName) | ||
| 266 | + if (previousName && previousName !== name) await renameInScopeConfigs(ctx, 'preset', previousName, name) | ||
| 228 | ctx.catalogChanged() | 267 | ctx.catalogChanged() |
| 229 | return c.json(saved) | 268 | return c.json(saved) |
| 230 | }) | 269 | }) |
modified
packages/server/src/context.ts +28 -0 | @@ -54,6 +54,34 @@ export interface AppContext { | ||
| 54 | 54 | catalogChanged(): void |
| 55 | 55 | } |
| 56 | 56 | |
| 57 | +/** | |
| 58 | + * While a pull left unresolved conflicts in the catalog, the files of conflicted items hold git conflict markers: | |
| 59 | + * reading them for rendering fails (the plan shows a warning and leaves the scope untouched) instead of writing | |
| 60 | + * the markers into projects. Reading items for the editor still works, so the user can see the conflict. | |
| 61 | + */ | |
| 62 | +export async function guardGitConflicts(catalog: CatalogStore, git: CatalogGit): Promise<CatalogStore> { | |
| 63 | + try { | |
| 64 | + await stat(path.join(catalog.root, '.git', 'MERGE_HEAD')) | |
| 65 | + } catch { | |
| 66 | + return catalog | |
| 67 | + } | |
| 68 | + const conflicts = new Set((await git.status()).conflicts) | |
| 69 | + if (conflicts.size === 0) return catalog | |
| 70 | + const itemFiles: CatalogStore['itemFiles'] = async (ref) => { | |
| 71 | + const files = await catalog.itemFiles(ref) | |
| 72 | + const hit = files.map((f) => path.relative(catalog.root, f.abs).replace(/\\/g, '/')).find((rel) => conflicts.has(rel)) | |
| 73 | + if (hit) throw new Error(`unresolved git conflict in the catalog (${hit}); resolve it first`) | |
| 74 | + return files | |
| 75 | + } | |
| 76 | + return new Proxy(catalog, { | |
| 77 | + get(target, prop) { | |
| 78 | + if (prop === 'itemFiles') return itemFiles | |
| 79 | + const v: unknown = Reflect.get(target, prop) | |
| 80 | + return typeof v === 'function' ? v.bind(target) : v | |
| 81 | + }, | |
| 82 | + }) | |
| 83 | +} | |
| 84 | + | |
| 57 | 85 | export async function isDirectory(p: string): Promise<boolean> { |
| 58 | 86 | try { |
| 59 | 87 | return (await stat(p)).isDirectory() |
| @@ -54,6 +54,34 @@ export interface AppContext { | |||
| 54 | catalogChanged(): void | 54 | catalogChanged(): void |
| 55 | } | 55 | } |
| 56 | 56 | ||
| 57 | +/** | ||
| 58 | + * While a pull left unresolved conflicts in the catalog, the files of conflicted items hold git conflict markers: | ||
| 59 | + * reading them for rendering fails (the plan shows a warning and leaves the scope untouched) instead of writing | ||
| 60 | + * the markers into projects. Reading items for the editor still works, so the user can see the conflict. | ||
| 61 | + */ | ||
| 62 | +export async function guardGitConflicts(catalog: CatalogStore, git: CatalogGit): Promise<CatalogStore> { | ||
| 63 | + try { | ||
| 64 | + await stat(path.join(catalog.root, '.git', 'MERGE_HEAD')) | ||
| 65 | + } catch { | ||
| 66 | + return catalog | ||
| 67 | + } | ||
| 68 | + const conflicts = new Set((await git.status()).conflicts) | ||
| 69 | + if (conflicts.size === 0) return catalog | ||
| 70 | + const itemFiles: CatalogStore['itemFiles'] = async (ref) => { | ||
| 71 | + const files = await catalog.itemFiles(ref) | ||
| 72 | + const hit = files.map((f) => path.relative(catalog.root, f.abs).replace(/\\/g, '/')).find((rel) => conflicts.has(rel)) | ||
| 73 | + if (hit) throw new Error(`unresolved git conflict in the catalog (${hit}); resolve it first`) | ||
| 74 | + return files | ||
| 75 | + } | ||
| 76 | + return new Proxy(catalog, { | ||
| 77 | + get(target, prop) { | ||
| 78 | + if (prop === 'itemFiles') return itemFiles | ||
| 79 | + const v: unknown = Reflect.get(target, prop) | ||
| 80 | + return typeof v === 'function' ? v.bind(target) : v | ||
| 81 | + }, | ||
| 82 | + }) | ||
| 83 | +} | ||
| 84 | + | ||
| 57 | export async function isDirectory(p: string): Promise<boolean> { | 85 | export async function isDirectory(p: string): Promise<boolean> { |
| 58 | try { | 86 | try { |
| 59 | return (await stat(p)).isDirectory() | 87 | return (await stat(p)).isDirectory() |
modified
packages/server/src/index.ts +5 -2 | @@ -27,7 +27,7 @@ import { | ||
| 27 | 27 | } from '@skilly/core' |
| 28 | 28 | import { Hono } from 'hono' |
| 29 | 29 | import { registerCatalogRoutes } from './catalog-routes.ts' |
| 30 | -import { HttpError, isDirectory, makeScopeRef, type AppContext } from './context.ts' | |
| 30 | +import { HttpError, guardGitConflicts, isDirectory, makeScopeRef, type AppContext } from './context.ts' | |
| 31 | 31 | import { registerMarketplaceRoutes } from './marketplace-routes.ts' |
| 32 | 32 | import { registerScopeRoutes } from './scope-routes.ts' |
| 33 | 33 | import { FileAccessError, isInside, readAllowedFile } from './files.ts' |
| @@ -188,7 +188,10 @@ export function createAppHandle(options: AppOptions): AppHandle { | ||
| 188 | 188 | createMarketplace, |
| 189 | 189 | marketplaceCacheDir, |
| 190 | 190 | settings: () => store.getSettings(), |
| 191 | - catalog: async () => openCatalog((await store.getSettings()).catalogPath), | |
| 191 | + catalog: async () => { | |
| 192 | + const root = (await store.getSettings()).catalogPath | |
| 193 | + return guardGitConflicts(openCatalog(root), openCatalogGit(root)) | |
| 194 | + }, | |
| 192 | 195 | git: async () => openCatalogGit((await store.getSettings()).catalogPath), |
| 193 | 196 | async scope(id) { |
| 194 | 197 | if (id === 'home') return makeScopeRef('home', 'home', ctx.homeDir, ctx, configDir) |
| @@ -27,7 +27,7 @@ import { | |||
| 27 | } from '@skilly/core' | 27 | } from '@skilly/core' |
| 28 | import { Hono } from 'hono' | 28 | import { Hono } from 'hono' |
| 29 | import { registerCatalogRoutes } from './catalog-routes.ts' | 29 | import { registerCatalogRoutes } from './catalog-routes.ts' |
| 30 | -import { HttpError, isDirectory, makeScopeRef, type AppContext } from './context.ts' | 30 | +import { HttpError, guardGitConflicts, isDirectory, makeScopeRef, type AppContext } from './context.ts' |
| 31 | import { registerMarketplaceRoutes } from './marketplace-routes.ts' | 31 | import { registerMarketplaceRoutes } from './marketplace-routes.ts' |
| 32 | import { registerScopeRoutes } from './scope-routes.ts' | 32 | import { registerScopeRoutes } from './scope-routes.ts' |
| 33 | import { FileAccessError, isInside, readAllowedFile } from './files.ts' | 33 | import { FileAccessError, isInside, readAllowedFile } from './files.ts' |
| @@ -188,7 +188,10 @@ export function createAppHandle(options: AppOptions): AppHandle { | |||
| 188 | createMarketplace, | 188 | createMarketplace, |
| 189 | marketplaceCacheDir, | 189 | marketplaceCacheDir, |
| 190 | settings: () => store.getSettings(), | 190 | settings: () => store.getSettings(), |
| 191 | - catalog: async () => openCatalog((await store.getSettings()).catalogPath), | 191 | + catalog: async () => { |
| 192 | + const root = (await store.getSettings()).catalogPath | ||
| 193 | + return guardGitConflicts(openCatalog(root), openCatalogGit(root)) | ||
| 194 | + }, | ||
| 192 | git: async () => openCatalogGit((await store.getSettings()).catalogPath), | 195 | git: async () => openCatalogGit((await store.getSettings()).catalogPath), |
| 193 | async scope(id) { | 196 | async scope(id) { |
| 194 | if (id === 'home') return makeScopeRef('home', 'home', ctx.homeDir, ctx, configDir) | 197 | if (id === 'home') return makeScopeRef('home', 'home', ctx.homeDir, ctx, configDir) |
modified
packages/server/test/integration.test.ts +48 -0 | @@ -1,3 +1,6 @@ | ||
| 1 | +import { execFileSync } from 'node:child_process' | |
| 2 | +import { readFile, writeFile } from 'node:fs/promises' | |
| 3 | +import path from 'node:path' | |
| 1 | 4 | import { syncEngine, type ApplyResponse, type ProjectSummary, type ScopeConfig, type ScopeState } from '@skilly/core' |
| 2 | 5 | import { afterEach, beforeEach, describe, expect, it } from 'vitest' |
| 3 | 6 | import { makeEnv, skillMd, type Env } from './helpers.ts' |
| @@ -42,4 +45,49 @@ describe('real sync engine', () => { | ||
| 42 | 45 | expect(applied.state.statuses[skill!.id]?.managed).toBe(true) |
| 43 | 46 | expect(applied.state.lock?.entries.length).toBeGreaterThan(0) |
| 44 | 47 | }) |
| 48 | + | |
| 49 | + it.skipIf(placeholder)('never renders catalog files left with git conflict markers by a pull', { timeout: 60_000 }, async () => { | |
| 50 | + const git = (cwd: string, ...args: string[]): string => | |
| 51 | + execFileSync('git', ['-c', 'user.name=t', '-c', 'user.email=t@t', '-c', 'core.autocrlf=false', ...args], { cwd, encoding: 'utf8' }) | |
| 52 | + const remote = path.join(env.tmp, 'remote.git') | |
| 53 | + git(env.tmp, 'init', '--bare', '-q', '-b', 'main', remote) | |
| 54 | + await env.send('POST', '/api/catalog/init') | |
| 55 | + git(env.catalogDir, 'checkout', '-q', '-B', 'main') | |
| 56 | + await env.send('POST', '/api/catalog/git/remote', { url: remote }) | |
| 57 | + await env.send('PUT', '/api/catalog/items/skill/hello', { content: skillMd('hello', 'Base.') }) | |
| 58 | + await env.send('POST', '/api/catalog/git/sync') | |
| 59 | + const projectId = ((await (await env.send('POST', '/api/projects', { path: env.project })).json()) as ProjectSummary).id | |
| 60 | + const config: ScopeConfig = { | |
| 61 | + version: 1, | |
| 62 | + targets: ['claude'], | |
| 63 | + presets: [], | |
| 64 | + add: { skill: ['hello'] }, | |
| 65 | + exclude: {}, | |
| 66 | + onDemand: { skills: [] }, | |
| 67 | + hideGlobal: { skills: [], mcp: [] }, | |
| 68 | + } | |
| 69 | + await env.send('PUT', `/api/scopes/${projectId}/config`, config) | |
| 70 | + await env.send('POST', `/api/scopes/${projectId}/apply`, { resolutions: {} }) | |
| 71 | + | |
| 72 | + // Concurrent edits: another machine pushes, this one commits, then pulls. | |
| 73 | + const other = path.join(env.tmp, 'other') | |
| 74 | + git(env.tmp, 'clone', '-q', remote, other) | |
| 75 | + await writeFile(path.join(other, 'skills/hello/SKILL.md'), skillMd('hello', 'Remote.')) | |
| 76 | + git(other, 'commit', '-qam', 'remote') | |
| 77 | + git(other, 'push', '-q') | |
| 78 | + await env.send('PUT', '/api/catalog/items/skill/hello', { content: skillMd('hello', 'Local.') }) | |
| 79 | + const pulled = (await (await env.send('POST', '/api/catalog/git/sync')).json()) as { conflicts: string[] } | |
| 80 | + expect(pulled.conflicts).toEqual(['skills/hello/SKILL.md']) | |
| 81 | + | |
| 82 | + const state = (await (await env.req(`/api/scopes/${projectId}/state`)).json()) as ScopeState | |
| 83 | + expect(state.plan?.actions.filter((a) => a.kind !== 'unchanged')).toEqual([]) | |
| 84 | + expect(state.plan?.warnings.join(' ')).toMatch(/unresolved git conflict/) | |
| 85 | + const applied = (await (await env.send('POST', `/api/scopes/${projectId}/apply`, { resolutions: {} })).json()) as ApplyResponse | |
| 86 | + expect(applied.result.errors).toEqual([]) | |
| 87 | + expect(await readFile(path.join(env.project, '.claude/skills/hello/SKILL.md'), 'utf8')).not.toContain('<<<<<<<') | |
| 88 | + | |
| 89 | + await env.send('POST', '/api/catalog/git/resolve', { path: 'skills/hello/SKILL.md', side: 'theirs' }) | |
| 90 | + const after = (await (await env.req(`/api/scopes/${projectId}/state`)).json()) as ScopeState | |
| 91 | + expect(after.plan?.actions.some((a) => a.kind === 'update' && a.name === 'hello')).toBe(true) | |
| 92 | + }) | |
| 45 | 93 | }) |
| @@ -1,3 +1,6 @@ | |||
| 1 | +import { execFileSync } from 'node:child_process' | ||
| 2 | +import { readFile, writeFile } from 'node:fs/promises' | ||
| 3 | +import path from 'node:path' | ||
| 1 | import { syncEngine, type ApplyResponse, type ProjectSummary, type ScopeConfig, type ScopeState } from '@skilly/core' | 4 | import { syncEngine, type ApplyResponse, type ProjectSummary, type ScopeConfig, type ScopeState } from '@skilly/core' |
| 2 | import { afterEach, beforeEach, describe, expect, it } from 'vitest' | 5 | import { afterEach, beforeEach, describe, expect, it } from 'vitest' |
| 3 | import { makeEnv, skillMd, type Env } from './helpers.ts' | 6 | import { makeEnv, skillMd, type Env } from './helpers.ts' |
| @@ -42,4 +45,49 @@ describe('real sync engine', () => { | |||
| 42 | expect(applied.state.statuses[skill!.id]?.managed).toBe(true) | 45 | expect(applied.state.statuses[skill!.id]?.managed).toBe(true) |
| 43 | expect(applied.state.lock?.entries.length).toBeGreaterThan(0) | 46 | expect(applied.state.lock?.entries.length).toBeGreaterThan(0) |
| 44 | }) | 47 | }) |
| 48 | + | ||
| 49 | + it.skipIf(placeholder)('never renders catalog files left with git conflict markers by a pull', { timeout: 60_000 }, async () => { | ||
| 50 | + const git = (cwd: string, ...args: string[]): string => | ||
| 51 | + execFileSync('git', ['-c', 'user.name=t', '-c', 'user.email=t@t', '-c', 'core.autocrlf=false', ...args], { cwd, encoding: 'utf8' }) | ||
| 52 | + const remote = path.join(env.tmp, 'remote.git') | ||
| 53 | + git(env.tmp, 'init', '--bare', '-q', '-b', 'main', remote) | ||
| 54 | + await env.send('POST', '/api/catalog/init') | ||
| 55 | + git(env.catalogDir, 'checkout', '-q', '-B', 'main') | ||
| 56 | + await env.send('POST', '/api/catalog/git/remote', { url: remote }) | ||
| 57 | + await env.send('PUT', '/api/catalog/items/skill/hello', { content: skillMd('hello', 'Base.') }) | ||
| 58 | + await env.send('POST', '/api/catalog/git/sync') | ||
| 59 | + const projectId = ((await (await env.send('POST', '/api/projects', { path: env.project })).json()) as ProjectSummary).id | ||
| 60 | + const config: ScopeConfig = { | ||
| 61 | + version: 1, | ||
| 62 | + targets: ['claude'], | ||
| 63 | + presets: [], | ||
| 64 | + add: { skill: ['hello'] }, | ||
| 65 | + exclude: {}, | ||
| 66 | + onDemand: { skills: [] }, | ||
| 67 | + hideGlobal: { skills: [], mcp: [] }, | ||
| 68 | + } | ||
| 69 | + await env.send('PUT', `/api/scopes/${projectId}/config`, config) | ||
| 70 | + await env.send('POST', `/api/scopes/${projectId}/apply`, { resolutions: {} }) | ||
| 71 | + | ||
| 72 | + // Concurrent edits: another machine pushes, this one commits, then pulls. | ||
| 73 | + const other = path.join(env.tmp, 'other') | ||
| 74 | + git(env.tmp, 'clone', '-q', remote, other) | ||
| 75 | + await writeFile(path.join(other, 'skills/hello/SKILL.md'), skillMd('hello', 'Remote.')) | ||
| 76 | + git(other, 'commit', '-qam', 'remote') | ||
| 77 | + git(other, 'push', '-q') | ||
| 78 | + await env.send('PUT', '/api/catalog/items/skill/hello', { content: skillMd('hello', 'Local.') }) | ||
| 79 | + const pulled = (await (await env.send('POST', '/api/catalog/git/sync')).json()) as { conflicts: string[] } | ||
| 80 | + expect(pulled.conflicts).toEqual(['skills/hello/SKILL.md']) | ||
| 81 | + | ||
| 82 | + const state = (await (await env.req(`/api/scopes/${projectId}/state`)).json()) as ScopeState | ||
| 83 | + expect(state.plan?.actions.filter((a) => a.kind !== 'unchanged')).toEqual([]) | ||
| 84 | + expect(state.plan?.warnings.join(' ')).toMatch(/unresolved git conflict/) | ||
| 85 | + const applied = (await (await env.send('POST', `/api/scopes/${projectId}/apply`, { resolutions: {} })).json()) as ApplyResponse | ||
| 86 | + expect(applied.result.errors).toEqual([]) | ||
| 87 | + expect(await readFile(path.join(env.project, '.claude/skills/hello/SKILL.md'), 'utf8')).not.toContain('<<<<<<<') | ||
| 88 | + | ||
| 89 | + await env.send('POST', '/api/catalog/git/resolve', { path: 'skills/hello/SKILL.md', side: 'theirs' }) | ||
| 90 | + const after = (await (await env.req(`/api/scopes/${projectId}/state`)).json()) as ScopeState | ||
| 91 | + expect(after.plan?.actions.some((a) => a.kind === 'update' && a.name === 'hello')).toBe(true) | ||
| 92 | + }) | ||
| 45 | }) | 93 | }) |
modified
packages/server/test/scopes.test.ts +27 -0 | @@ -174,4 +174,31 @@ describe('catalog usage and deletion guards', () => { | ||
| 174 | 174 | expect((await env.send('DELETE', '/api/catalog/items/skill/foo?force=1')).status).toBe(204) |
| 175 | 175 | expect((await env.send('DELETE', '/api/catalog/presets/base?force=1')).status).toBe(204) |
| 176 | 176 | }) |
| 177 | + | |
| 178 | + it('a rename points presets and scope configs at the new name (exact names only)', async () => { | |
| 179 | + await env.send('POST', '/api/catalog/init') | |
| 180 | + await env.send('PUT', '/api/catalog/items/skill/foo', { content: skillMd('foo') }) | |
| 181 | + await env.send('PUT', '/api/catalog/presets/base', { includes: [], items: { skill: ['foo'] } }) | |
| 182 | + await env.send('PUT', '/api/catalog/presets/full', { includes: ['base'], items: {} }) | |
| 183 | + await env.send( | |
| 184 | + 'PUT', | |
| 185 | + `/api/scopes/${projectId}/config`, | |
| 186 | + config({ presets: ['base'], add: { skill: ['foo', 'f*'] }, exclude: { skill: ['foo'] }, onDemand: { skills: ['foo', 'fo*'] } }), | |
| 187 | + ) | |
| 188 | + | |
| 189 | + expect((await env.send('PUT', '/api/catalog/items/skill/foo2', { content: skillMd('foo2'), previousName: 'foo' })).status).toBe(200) | |
| 190 | + expect((await env.send('PUT', '/api/catalog/presets/base2', { includes: [], items: { skill: ['foo2'] }, previousName: 'base' })).status).toBe(200) | |
| 191 | + | |
| 192 | + const cat = (await (await env.req('/api/catalog')).json()) as { snapshot: { presets: { name: string; includes: string[]; items: Record<string, string[]> }[] } } | |
| 193 | + const presets = Object.fromEntries(cat.snapshot.presets.map((p) => [p.name, p])) | |
| 194 | + expect(presets.base2?.items.skill).toEqual(['foo2']) | |
| 195 | + expect(presets.full?.includes).toEqual(['base2']) | |
| 196 | + const state = (await (await env.req(`/api/scopes/${projectId}/state`)).json()) as ScopeState | |
| 197 | + expect(state.config).toMatchObject({ | |
| 198 | + presets: ['base2'], | |
| 199 | + add: { skill: ['foo2', 'f*'] }, | |
| 200 | + exclude: { skill: ['foo2'] }, | |
| 201 | + onDemand: { skills: ['foo2', 'fo*'] }, | |
| 202 | + }) | |
| 203 | + }) | |
| 177 | 204 | }) |
| @@ -174,4 +174,31 @@ describe('catalog usage and deletion guards', () => { | |||
| 174 | expect((await env.send('DELETE', '/api/catalog/items/skill/foo?force=1')).status).toBe(204) | 174 | expect((await env.send('DELETE', '/api/catalog/items/skill/foo?force=1')).status).toBe(204) |
| 175 | expect((await env.send('DELETE', '/api/catalog/presets/base?force=1')).status).toBe(204) | 175 | expect((await env.send('DELETE', '/api/catalog/presets/base?force=1')).status).toBe(204) |
| 176 | }) | 176 | }) |
| 177 | + | ||
| 178 | + it('a rename points presets and scope configs at the new name (exact names only)', async () => { | ||
| 179 | + await env.send('POST', '/api/catalog/init') | ||
| 180 | + await env.send('PUT', '/api/catalog/items/skill/foo', { content: skillMd('foo') }) | ||
| 181 | + await env.send('PUT', '/api/catalog/presets/base', { includes: [], items: { skill: ['foo'] } }) | ||
| 182 | + await env.send('PUT', '/api/catalog/presets/full', { includes: ['base'], items: {} }) | ||
| 183 | + await env.send( | ||
| 184 | + 'PUT', | ||
| 185 | + `/api/scopes/${projectId}/config`, | ||
| 186 | + config({ presets: ['base'], add: { skill: ['foo', 'f*'] }, exclude: { skill: ['foo'] }, onDemand: { skills: ['foo', 'fo*'] } }), | ||
| 187 | + ) | ||
| 188 | + | ||
| 189 | + expect((await env.send('PUT', '/api/catalog/items/skill/foo2', { content: skillMd('foo2'), previousName: 'foo' })).status).toBe(200) | ||
| 190 | + expect((await env.send('PUT', '/api/catalog/presets/base2', { includes: [], items: { skill: ['foo2'] }, previousName: 'base' })).status).toBe(200) | ||
| 191 | + | ||
| 192 | + const cat = (await (await env.req('/api/catalog')).json()) as { snapshot: { presets: { name: string; includes: string[]; items: Record<string, string[]> }[] } } | ||
| 193 | + const presets = Object.fromEntries(cat.snapshot.presets.map((p) => [p.name, p])) | ||
| 194 | + expect(presets.base2?.items.skill).toEqual(['foo2']) | ||
| 195 | + expect(presets.full?.includes).toEqual(['base2']) | ||
| 196 | + const state = (await (await env.req(`/api/scopes/${projectId}/state`)).json()) as ScopeState | ||
| 197 | + expect(state.config).toMatchObject({ | ||
| 198 | + presets: ['base2'], | ||
| 199 | + add: { skill: ['foo2', 'f*'] }, | ||
| 200 | + exclude: { skill: ['foo2'] }, | ||
| 201 | + onDemand: { skills: ['foo2', 'fo*'] }, | ||
| 202 | + }) | ||
| 203 | + }) | ||
| 177 | }) | 204 | }) |
modified
packages/ui/app/components/AddToCatalogModal.vue +1 -1 | @@ -76,7 +76,7 @@ async function submit() { | ||
| 76 | 76 | </template> |
| 77 | 77 | </UAlert> |
| 78 | 78 | <UFormField label="Tags" description="Optional, applied to the imported items."> |
| 79 | - <UInputTags v-model="tags" class="w-full" placeholder="Add a tag, press Enter" /> | |
| 79 | + <UInputTags v-model="tags" add-on-blur class="w-full" placeholder="Add a tag, press Enter" /> | |
| 80 | 80 | </UFormField> |
| 81 | 81 | </div> |
| 82 | 82 | </template> |
| @@ -76,7 +76,7 @@ async function submit() { | |||
| 76 | </template> | 76 | </template> |
| 77 | </UAlert> | 77 | </UAlert> |
| 78 | <UFormField label="Tags" description="Optional, applied to the imported items."> | 78 | <UFormField label="Tags" description="Optional, applied to the imported items."> |
| 79 | - <UInputTags v-model="tags" class="w-full" placeholder="Add a tag, press Enter" /> | 79 | + <UInputTags v-model="tags" add-on-blur class="w-full" placeholder="Add a tag, press Enter" /> |
| 80 | </UFormField> | 80 | </UFormField> |
| 81 | </div> | 81 | </div> |
| 82 | </template> | 82 | </template> |
modified
packages/ui/app/components/ApplyModal.vue +1 -1 | @@ -189,7 +189,7 @@ const label = (id: string) => { | ||
| 189 | 189 | <UBadge v-if="a.catalogUpdated" label="catalog updated" size="sm" color="info" variant="subtle" /> |
| 190 | 190 | <UBadge v-if="a.conflict" :label="a.conflict" size="sm" color="warning" variant="subtle" /> |
| 191 | 191 | </div> |
| 192 | - <p class="text-xs text-muted">{{ a.conflict ? CONFLICT_LABEL[a.conflict] : a.reason }}<span v-if="a.conflict && a.reason"> - {{ a.reason }}</span></p> | |
| 192 | + <p class="text-xs text-muted">{{ a.reason || (a.conflict ? CONFLICT_LABEL[a.conflict] : '') }}</p> | |
| 193 | 193 | </div> |
| 194 | 194 | <UButton |
| 195 | 195 | v-if="a.files.length || a.configKeys.length" |
| @@ -189,7 +189,7 @@ const label = (id: string) => { | |||
| 189 | <UBadge v-if="a.catalogUpdated" label="catalog updated" size="sm" color="info" variant="subtle" /> | 189 | <UBadge v-if="a.catalogUpdated" label="catalog updated" size="sm" color="info" variant="subtle" /> |
| 190 | <UBadge v-if="a.conflict" :label="a.conflict" size="sm" color="warning" variant="subtle" /> | 190 | <UBadge v-if="a.conflict" :label="a.conflict" size="sm" color="warning" variant="subtle" /> |
| 191 | </div> | 191 | </div> |
| 192 | - <p class="text-xs text-muted">{{ a.conflict ? CONFLICT_LABEL[a.conflict] : a.reason }}<span v-if="a.conflict && a.reason"> - {{ a.reason }}</span></p> | 192 | + <p class="text-xs text-muted">{{ a.reason || (a.conflict ? CONFLICT_LABEL[a.conflict] : '') }}</p> |
| 193 | </div> | 193 | </div> |
| 194 | <UButton | 194 | <UButton |
| 195 | v-if="a.files.length || a.configKeys.length" | 195 | v-if="a.files.length || a.configKeys.length" |
modified
packages/ui/app/components/CatalogItemEditor.vue +2 -2 | @@ -309,7 +309,7 @@ const TA = 'block w-full resize-y rounded-lg border border-default bg-default p- | ||
| 309 | 309 | <aside class="space-y-5"> |
| 310 | 310 | <section class="space-y-1.5"> |
| 311 | 311 | <h3 class="text-sm font-semibold">Tags</h3> |
| 312 | - <UInputTags v-model="form.tags" placeholder="Add a tag, press Enter" class="w-full" /> | |
| 312 | + <UInputTags v-model="form.tags" add-on-blur placeholder="Add a tag, press Enter" class="w-full" /> | |
| 313 | 313 | </section> |
| 314 | 314 | |
| 315 | 315 | <section v-if="!isNew" class="space-y-1.5"> |
| @@ -385,7 +385,7 @@ const TA = 'block w-full resize-y rounded-lg border border-default bg-default p- | ||
| 385 | 385 | :title="delConflict ? 'This item is used' : 'Used by'" |
| 386 | 386 | > |
| 387 | 387 | <template #description> |
| 388 | - <p v-if="delConflict" class="mb-1">{{ delConflict }}</p> | |
| 388 | + <p v-if="delConflict" class="mb-1">Still referenced by the configuration of these scopes. If you delete it anyway, they will report it as missing from the catalog.</p> | |
| 389 | 389 | <ul class="list-disc pl-4"> |
| 390 | 390 | <li v-for="s in usedBy" :key="s">{{ scopeLabel(s, scopeNames) }}</li> |
| 391 | 391 | </ul> |
| @@ -309,7 +309,7 @@ const TA = 'block w-full resize-y rounded-lg border border-default bg-default p- | |||
| 309 | <aside class="space-y-5"> | 309 | <aside class="space-y-5"> |
| 310 | <section class="space-y-1.5"> | 310 | <section class="space-y-1.5"> |
| 311 | <h3 class="text-sm font-semibold">Tags</h3> | 311 | <h3 class="text-sm font-semibold">Tags</h3> |
| 312 | - <UInputTags v-model="form.tags" placeholder="Add a tag, press Enter" class="w-full" /> | 312 | + <UInputTags v-model="form.tags" add-on-blur placeholder="Add a tag, press Enter" class="w-full" /> |
| 313 | </section> | 313 | </section> |
| 314 | 314 | ||
| 315 | <section v-if="!isNew" class="space-y-1.5"> | 315 | <section v-if="!isNew" class="space-y-1.5"> |
| @@ -385,7 +385,7 @@ const TA = 'block w-full resize-y rounded-lg border border-default bg-default p- | |||
| 385 | :title="delConflict ? 'This item is used' : 'Used by'" | 385 | :title="delConflict ? 'This item is used' : 'Used by'" |
| 386 | > | 386 | > |
| 387 | <template #description> | 387 | <template #description> |
| 388 | - <p v-if="delConflict" class="mb-1">{{ delConflict }}</p> | 388 | + <p v-if="delConflict" class="mb-1">Still referenced by the configuration of these scopes. If you delete it anyway, they will report it as missing from the catalog.</p> |
| 389 | <ul class="list-disc pl-4"> | 389 | <ul class="list-disc pl-4"> |
| 390 | <li v-for="s in usedBy" :key="s">{{ scopeLabel(s, scopeNames) }}</li> | 390 | <li v-for="s in usedBy" :key="s">{{ scopeLabel(s, scopeNames) }}</li> |
| 391 | </ul> | 391 | </ul> |
modified
packages/ui/app/components/ItemTable.vue +5 -1 | @@ -130,7 +130,11 @@ const MAX_ENV = 4 | ||
| 130 | 130 | </div> |
| 131 | 131 | </td> |
| 132 | 132 | <td :class="TD"><AgentBadge :agent="item.agent" compact /></td> |
| 133 | - <td :class="TD"><StatusBadge :status="statuses?.[item.id]" /></td> | |
| 133 | + <td :class="TD"> | |
| 134 | + <!-- Project instructions are never shared, so a catalog status would be meaningless. --> | |
| 135 | + <span v-if="item.type === 'instructions'" class="text-xs text-dimmed">-</span> | |
| 136 | + <StatusBadge v-else :status="statuses?.[item.id]" /> | |
| 137 | + </td> | |
| 134 | 138 | |
| 135 | 139 | <!-- skills --> |
| 136 | 140 | <template v-if="type === 'skill'"> |
| @@ -130,7 +130,11 @@ const MAX_ENV = 4 | |||
| 130 | </div> | 130 | </div> |
| 131 | </td> | 131 | </td> |
| 132 | <td :class="TD"><AgentBadge :agent="item.agent" compact /></td> | 132 | <td :class="TD"><AgentBadge :agent="item.agent" compact /></td> |
| 133 | - <td :class="TD"><StatusBadge :status="statuses?.[item.id]" /></td> | 133 | + <td :class="TD"> |
| 134 | + <!-- Project instructions are never shared, so a catalog status would be meaningless. --> | ||
| 135 | + <span v-if="item.type === 'instructions'" class="text-xs text-dimmed">-</span> | ||
| 136 | + <StatusBadge v-else :status="statuses?.[item.id]" /> | ||
| 137 | + </td> | ||
| 134 | 138 | ||
| 135 | <!-- skills --> | 139 | <!-- skills --> |
| 136 | <template v-if="type === 'skill'"> | 140 | <template v-if="type === 'skill'"> |
modified
packages/ui/app/components/McpForm.vue +1 -1 | @@ -38,7 +38,7 @@ function patch(p: Partial<McpTemplate>) { | ||
| 38 | 38 | <UInput :model-value="t.command ?? ''" class="w-full" placeholder="npx" :ui="{ base: 'font-mono' }" @update:model-value="(v: string) => patch({ command: v })" /> |
| 39 | 39 | </UFormField> |
| 40 | 40 | <UFormField label="Arguments" description="Press Enter to add each argument."> |
| 41 | - <UInputTags v-model="args" class="w-full" placeholder="-y, @upstash/context7-mcp" /> | |
| 41 | + <UInputTags v-model="args" add-on-blur add-on-paste class="w-full" placeholder="-y, @upstash/context7-mcp" /> | |
| 42 | 42 | </UFormField> |
| 43 | 43 | <UFormField label="Environment variables" description="Variables passed to the server process."> |
| 44 | 44 | <EnvRows :model-value="t.env" key-label="Variable name" key-placeholder="API_KEY" @update:model-value="(v) => patch({ env: v })" /> |
| @@ -38,7 +38,7 @@ function patch(p: Partial<McpTemplate>) { | |||
| 38 | <UInput :model-value="t.command ?? ''" class="w-full" placeholder="npx" :ui="{ base: 'font-mono' }" @update:model-value="(v: string) => patch({ command: v })" /> | 38 | <UInput :model-value="t.command ?? ''" class="w-full" placeholder="npx" :ui="{ base: 'font-mono' }" @update:model-value="(v: string) => patch({ command: v })" /> |
| 39 | </UFormField> | 39 | </UFormField> |
| 40 | <UFormField label="Arguments" description="Press Enter to add each argument."> | 40 | <UFormField label="Arguments" description="Press Enter to add each argument."> |
| 41 | - <UInputTags v-model="args" class="w-full" placeholder="-y, @upstash/context7-mcp" /> | 41 | + <UInputTags v-model="args" add-on-blur add-on-paste class="w-full" placeholder="-y, @upstash/context7-mcp" /> |
| 42 | </UFormField> | 42 | </UFormField> |
| 43 | <UFormField label="Environment variables" description="Variables passed to the server process."> | 43 | <UFormField label="Environment variables" description="Variables passed to the server process."> |
| 44 | <EnvRows :model-value="t.env" key-label="Variable name" key-placeholder="API_KEY" @update:model-value="(v) => patch({ env: v })" /> | 44 | <EnvRows :model-value="t.env" key-label="Variable name" key-placeholder="API_KEY" @update:model-value="(v) => patch({ env: v })" /> |
modified
packages/ui/app/components/PresetEditor.vue +1 -1 | @@ -207,7 +207,7 @@ function askDelete() { | ||
| 207 | 207 | <p>Items of the preset are kept in the catalog. Scopes that enabled it will report the preset as missing.</p> |
| 208 | 208 | <UAlert v-if="delConflict || usedBy.length" color="warning" variant="subtle" icon="i-lucide-triangle-alert" :title="delConflict ? 'This preset is used' : 'Used by'"> |
| 209 | 209 | <template #description> |
| 210 | - <p v-if="delConflict" class="mb-1">{{ delConflict }}</p> | |
| 210 | + <p v-if="delConflict" class="mb-1">Still enabled in these scopes. If you delete it anyway, they will report it as missing from the catalog.</p> | |
| 211 | 211 | <ul class="list-disc pl-4"><li v-for="s in usedBy" :key="s">{{ scopeLabel(s, scopeNames) }}</li></ul> |
| 212 | 212 | </template> |
| 213 | 213 | </UAlert> |
| @@ -207,7 +207,7 @@ function askDelete() { | |||
| 207 | <p>Items of the preset are kept in the catalog. Scopes that enabled it will report the preset as missing.</p> | 207 | <p>Items of the preset are kept in the catalog. Scopes that enabled it will report the preset as missing.</p> |
| 208 | <UAlert v-if="delConflict || usedBy.length" color="warning" variant="subtle" icon="i-lucide-triangle-alert" :title="delConflict ? 'This preset is used' : 'Used by'"> | 208 | <UAlert v-if="delConflict || usedBy.length" color="warning" variant="subtle" icon="i-lucide-triangle-alert" :title="delConflict ? 'This preset is used' : 'Used by'"> |
| 209 | <template #description> | 209 | <template #description> |
| 210 | - <p v-if="delConflict" class="mb-1">{{ delConflict }}</p> | 210 | + <p v-if="delConflict" class="mb-1">Still enabled in these scopes. If you delete it anyway, they will report it as missing from the catalog.</p> |
| 211 | <ul class="list-disc pl-4"><li v-for="s in usedBy" :key="s">{{ scopeLabel(s, scopeNames) }}</li></ul> | 211 | <ul class="list-disc pl-4"><li v-for="s in usedBy" :key="s">{{ scopeLabel(s, scopeNames) }}</li></ul> |
| 212 | </template> | 212 | </template> |
| 213 | </UAlert> | 213 | </UAlert> |
modified
packages/ui/app/components/ScopeView.vue +1 -1 | @@ -371,7 +371,7 @@ const AGENT_LIST: AgentId[] = ['claude', 'codex'] | ||
| 371 | 371 | /> |
| 372 | 372 | |
| 373 | 373 | <!-- Type tabs --> |
| 374 | - <UTabs v-model="tab" :items="tabs" :content="false" variant="link" class="w-full" /> | |
| 374 | + <UTabs v-model="tab" :items="tabs" :content="false" variant="link" class="w-full" :ui="{ list: 'overflow-x-auto overflow-y-hidden [scrollbar-width:thin]', trigger: 'shrink-0 max-xl:px-1.5 max-xl:gap-1 max-xl:text-xs' }" /> | |
| 375 | 375 | |
| 376 | 376 | <!-- Configuration tab --> |
| 377 | 377 | <ScopeConfigPanel |
| @@ -371,7 +371,7 @@ const AGENT_LIST: AgentId[] = ['claude', 'codex'] | |||
| 371 | /> | 371 | /> |
| 372 | 372 | ||
| 373 | <!-- Type tabs --> | 373 | <!-- Type tabs --> |
| 374 | - <UTabs v-model="tab" :items="tabs" :content="false" variant="link" class="w-full" /> | 374 | + <UTabs v-model="tab" :items="tabs" :content="false" variant="link" class="w-full" :ui="{ list: 'overflow-x-auto overflow-y-hidden [scrollbar-width:thin]', trigger: 'shrink-0 max-xl:px-1.5 max-xl:gap-1 max-xl:text-xs' }" /> |
| 375 | 375 | ||
| 376 | <!-- Configuration tab --> | 376 | <!-- Configuration tab --> |
| 377 | <ScopeConfigPanel | 377 | <ScopeConfigPanel |
modified
packages/ui/app/composables/useCatalogGit.ts +10 -1 | @@ -17,7 +17,16 @@ export function useCatalogGit() { | ||
| 17 | 17 | try { |
| 18 | 18 | const status = await request<GitStatus>(`/api/catalog/git/${action}`, { method: 'POST', body, silent: true }) |
| 19 | 19 | setGit(status) |
| 20 | - if (successTitle) toast.add({ title: successTitle, icon: 'i-lucide-check', color: 'success' }) | |
| 20 | + if (status.conflicts.length) { | |
| 21 | + // A pull that stops on conflicts is not a success: show them where they can be resolved. | |
| 22 | + open.value = true | |
| 23 | + toast.add({ | |
| 24 | + title: 'Catalog conflicts to resolve', | |
| 25 | + description: `${status.conflicts.length} file(s) changed on both sides. Choose a version for each one.`, | |
| 26 | + icon: 'i-lucide-triangle-alert', | |
| 27 | + color: 'warning' | |
| 28 | + }) | |
| 29 | + } else if (successTitle) toast.add({ title: successTitle, icon: 'i-lucide-check', color: 'success' }) | |
| 21 | 30 | // Pull / sync / resolve can change the catalog content. |
| 22 | 31 | if (action !== 'remote' && action !== 'commit' && action !== 'push') await refresh() |
| 23 | 32 | return true |
| @@ -17,7 +17,16 @@ export function useCatalogGit() { | |||
| 17 | try { | 17 | try { |
| 18 | const status = await request<GitStatus>(`/api/catalog/git/${action}`, { method: 'POST', body, silent: true }) | 18 | const status = await request<GitStatus>(`/api/catalog/git/${action}`, { method: 'POST', body, silent: true }) |
| 19 | setGit(status) | 19 | setGit(status) |
| 20 | - if (successTitle) toast.add({ title: successTitle, icon: 'i-lucide-check', color: 'success' }) | 20 | + if (status.conflicts.length) { |
| 21 | + // A pull that stops on conflicts is not a success: show them where they can be resolved. | ||
| 22 | + open.value = true | ||
| 23 | + toast.add({ | ||
| 24 | + title: 'Catalog conflicts to resolve', | ||
| 25 | + description: `${status.conflicts.length} file(s) changed on both sides. Choose a version for each one.`, | ||
| 26 | + icon: 'i-lucide-triangle-alert', | ||
| 27 | + color: 'warning' | ||
| 28 | + }) | ||
| 29 | + } else if (successTitle) toast.add({ title: successTitle, icon: 'i-lucide-check', color: 'success' }) | ||
| 21 | // Pull / sync / resolve can change the catalog content. | 30 | // Pull / sync / resolve can change the catalog content. |
| 22 | if (action !== 'remote' && action !== 'commit' && action !== 'push') await refresh() | 31 | if (action !== 'remote' && action !== 'commit' && action !== 'push') await refresh() |
| 23 | return true | 32 | return true |
modified
packages/ui/app/utils/catalog.ts +1 -1 | @@ -39,7 +39,7 @@ const RESERVED = /^(true|false|null|yes|no|on|off|~|[-+]?[0-9][0-9._]*(e[-+]?[0- | ||
| 39 | 39 | |
| 40 | 40 | function scalar(v: string | number | boolean): string { |
| 41 | 41 | if (typeof v !== 'string') return String(v) |
| 42 | - if (v !== '' && !RESERVED.test(v) && /^[A-Za-z0-9_./$@%][A-Za-z0-9_./:@%$=+-]*$/.test(v)) return v | |
| 42 | + if (v !== '' && !RESERVED.test(v) && /^[A-Za-z0-9_./$][A-Za-z0-9_./:@%$=+-]*$/.test(v)) return v | |
| 43 | 43 | return JSON.stringify(v) |
| 44 | 44 | } |
| 45 | 45 | |
| @@ -39,7 +39,7 @@ const RESERVED = /^(true|false|null|yes|no|on|off|~|[-+]?[0-9][0-9._]*(e[-+]?[0- | |||
| 39 | 39 | ||
| 40 | function scalar(v: string | number | boolean): string { | 40 | function scalar(v: string | number | boolean): string { |
| 41 | if (typeof v !== 'string') return String(v) | 41 | if (typeof v !== 'string') return String(v) |
| 42 | - if (v !== '' && !RESERVED.test(v) && /^[A-Za-z0-9_./$@%][A-Za-z0-9_./:@%$=+-]*$/.test(v)) return v | 42 | + if (v !== '' && !RESERVED.test(v) && /^[A-Za-z0-9_./$][A-Za-z0-9_./:@%$=+-]*$/.test(v)) return v |
| 43 | return JSON.stringify(v) | 43 | return JSON.stringify(v) |
| 44 | } | 44 | } |
| 45 | 45 | ||
added
packages/ui/test/catalog-yaml.test.ts +22 -0 | new file mode 100644 | ||
| @@ -0,0 +1,22 @@ | ||
| 1 | +import { describe, expect, it } from 'vitest' | |
| 2 | +import { parseHookYaml, parseMcpYaml } from '../../core/src/catalog/templates.ts' | |
| 3 | +import { hookToYaml, mcpToYaml } from '../app/utils/catalog.ts' | |
| 4 | + | |
| 5 | +describe('mcpToYaml / hookToYaml', () => { | |
| 6 | + it('quotes scalars starting with YAML indicators so the catalog accepts them', () => { | |
| 7 | + const args = ['-y', '@upstash/context7-mcp', '%foo', 'a:b', '--port=3000', 'true', '42', 'with space', '$HOME/x'] | |
| 8 | + const t = parseMcpYaml( | |
| 9 | + mcpToYaml({ name: 'ctx', transport: 'stdio', command: '@scope/bin', args, env: { '@K': { value: '@v' } } }), | |
| 10 | + 'ctx', | |
| 11 | + ) | |
| 12 | + expect(t.command).toBe('@scope/bin') | |
| 13 | + expect(t.args).toEqual(args) | |
| 14 | + expect(t.env).toEqual({ '@K': { value: '@v' } }) | |
| 15 | + }) | |
| 16 | + | |
| 17 | + it('round-trips a hook command starting with @', () => { | |
| 18 | + const h = parseHookYaml(hookToYaml({ name: 'h', event: 'PostToolUse', matcher: 'Edit|Write', handler: { type: 'command', command: '@echo off' } }), 'h') | |
| 19 | + expect(h.handler.command).toBe('@echo off') | |
| 20 | + expect(h.matcher).toBe('Edit|Write') | |
| 21 | + }) | |
| 22 | +}) | |
| new file mode 100644 | |||
| @@ -0,0 +1,22 @@ | |||
| 1 | +import { describe, expect, it } from 'vitest' | ||
| 2 | +import { parseHookYaml, parseMcpYaml } from '../../core/src/catalog/templates.ts' | ||
| 3 | +import { hookToYaml, mcpToYaml } from '../app/utils/catalog.ts' | ||
| 4 | + | ||
| 5 | +describe('mcpToYaml / hookToYaml', () => { | ||
| 6 | + it('quotes scalars starting with YAML indicators so the catalog accepts them', () => { | ||
| 7 | + const args = ['-y', '@upstash/context7-mcp', '%foo', 'a:b', '--port=3000', 'true', '42', 'with space', '$HOME/x'] | ||
| 8 | + const t = parseMcpYaml( | ||
| 9 | + mcpToYaml({ name: 'ctx', transport: 'stdio', command: '@scope/bin', args, env: { '@K': { value: '@v' } } }), | ||
| 10 | + 'ctx', | ||
| 11 | + ) | ||
| 12 | + expect(t.command).toBe('@scope/bin') | ||
| 13 | + expect(t.args).toEqual(args) | ||
| 14 | + expect(t.env).toEqual({ '@K': { value: '@v' } }) | ||
| 15 | + }) | ||
| 16 | + | ||
| 17 | + it('round-trips a hook command starting with @', () => { | ||
| 18 | + const h = parseHookYaml(hookToYaml({ name: 'h', event: 'PostToolUse', matcher: 'Edit|Write', handler: { type: 'command', command: '@echo off' } }), 'h') | ||
| 19 | + expect(h.handler.command).toBe('@echo off') | ||
| 20 | + expect(h.matcher).toBe('Edit|Write') | ||
| 21 | + }) | ||
| 22 | +}) | ||