diff --git a/CHANGELOG.md b/CHANGELOG.md index ff5cb27a..e3445498 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Skip a planning brief for an unkept spike or POC and for routine CI-only work, including a workflow edit that changes how jobs or E2E launch. A CI change that is a new design, a contract or API change, or multi-surface work still stops for a brief and a spec. For a revert of a merged pull request or commit, name the target and files, then ask one yes/no, and name the risk when security or privacy is involved. Recommend a spec for a new design, a contract or API change, or multi-surface work even when the ask is fully specified or single-file; any other fully specified ask gets one line of scope and one yes/no instead of a brief, unless it touches auth, secrets, logs or exports of state, permissions, keyring or vault, migrations, or persisted state, which still gets a brief. Always read the installed skill catalog, including for reverts and fully specified asks. - Document confirmed Appium interaction-stability patterns in `testing/mobile-testing` (`references/appium-e2e.md`): child readiness, fresh interactive queries, shared open retries, virtualized-list proof, native control state, action postconditions, and single assertions for short-lived alerts. +### Fixed + +- Serialize updates of each shared skill source so concurrent syncs and farm prepares cannot race on Git's fetch state. +- Keep the `typescript/tsc-blindspots` description within the skill catalog's size limit. + ## [0.3.1] ### Fixed diff --git a/domains/typescript/skills/tsc-blindspots/skill.md b/domains/typescript/skills/tsc-blindspots/skill.md index 75395b8a..cdadc32e 100644 --- a/domains/typescript/skills/tsc-blindspots/skill.md +++ b/domains/typescript/skills/tsc-blindspots/skill.md @@ -1,25 +1,17 @@ --- name: tsc-blindspots description: >- - Find the type defects `tsc` is structurally unable to report — a green build is - not evidence the types are correct. Covers the two classes: (1) hand-written - types that restate an authoritative source and disagree with it, caught by - substituting the derived type at a fixed commit and diffing `tsc` output; and - (2) the standing blind spots in the language and config — unchecked array/record - indexing, bivariant method parameters, covariant arrays, `any` absorption at - untyped boundaries, precise signatures fed `any` at every call site, ambient - `declare module` assertions that launder an `any` into a confident type, - excess-property checks that only fire on fresh literals, and external data - asserted rather than validated. Also audits typing edits that quietly change - runtime behavior: - stripped `| undefined`, deleted default parameters, literals swapped for runtime - enum lookups, calls made optional so a throw becomes a silent no-op. Use when - reviewing a JS→TS migration, a PR that hand-writes types for values that already - have them, a "rename-only" refactor, or any PR claiming a change is mechanical. - Triggers on /mms-tsc-blindspots, or on phrases like "validate this TypeScript - migration", "is this type right", "does this type match the real shape", - "why didn't CI catch this type", "derive vs define", and "what can tsc not - check". + Find type defects a green `tsc` build cannot rule out: handwritten types that + disagree with an authoritative source, unchecked indexing, variance, `any` at + untyped boundaries or call sites, ambient module assertions, excess-property + checks, and external data asserted without validation. Substitute derived types + at a fixed commit and compare `tsc` output. Audit typing edits for runtime changes: + removed undefined/defaults, enum lookups, and newly optional calls. Use for + JS→TS migrations, types restating existing values, rename-only refactors, or + PRs claiming mechanical changes. Triggers on /mms-tsc-blindspots, "validate this + TypeScript migration", "is this type right", "does this type match the real + shape", "why didn't CI catch this type", "derive vs define", and "what can tsc + not check". Detailed checks and examples are in the skill body and references. maturity: experimental --- diff --git a/test/source-sync-concurrency.test.mjs b/test/source-sync-concurrency.test.mjs new file mode 100644 index 00000000..313fbd48 --- /dev/null +++ b/test/source-sync-concurrency.test.mjs @@ -0,0 +1,218 @@ +import assert from 'node:assert/strict'; +import { execFileSync, spawn } from 'node:child_process'; +import { once } from 'node:events'; +import { + existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync, +} from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { setTimeout as delay } from 'node:timers/promises'; +import { fileURLToPath } from 'node:url'; +import { test } from 'node:test'; + +const root = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); +const bin = path.join(root, 'bin', 'metamask-skills.mjs'); +const realGit = execFileSync('which', ['git'], { encoding: 'utf8' }).trim(); + +async function waitFor(check) { + const deadline = Date.now() + 2500; + while (!check()) { + assert.ok(Date.now() < deadline, 'source update did not reach its barrier'); + await delay(10); + } +} + +function fixture(t) { + const directory = mkdtempSync(path.join(os.tmpdir(), 'mms-source-sync-')); + const children = []; + t.after(async () => { + const results = await Promise.allSettled(children.map(async ({ child, done }) => { + if (child.pid && child.exitCode === null && child.signalCode === null) { + try { + process.kill(-child.pid, 'SIGKILL'); + } catch (error) { + if (error.code !== 'ESRCH') throw error; + } + } + await done; + })); + rmSync(directory, { recursive: true, force: true }); + const failures = results.filter((result) => result.status === 'rejected'); + if (failures.length) throw new AggregateError(failures.map((result) => result.reason), 'child cleanup failed'); + }); + const env = { + ...process.env, + GIT_CONFIG_GLOBAL: os.devNull, + GIT_CONFIG_NOSYSTEM: '1', + GIT_TERMINAL_PROMPT: '0', + METAMASK_SKILLS_DIR: '', + CONSENSYS_SKILLS_DIR: '', + SKILLS_DOMAINS: 'testing', + SKILLS_INCLUDE: '', + SKILLS_EXCLUDE: '', + SKILLS_PRUNE_STALE: '', + SKILLS_MATURITY: 'stable', + SKILLS_AUTO_UPDATE: '0', + }; + for (const name of [ + 'BASH_ENV', 'NODE_OPTIONS', 'NODE_TEST_CONTEXT', 'TMUX', 'TMUX_PANE', + 'GIT_DIR', 'GIT_WORK_TREE', 'GIT_COMMON_DIR', 'GIT_INDEX_FILE', 'GIT_OBJECT_DIRECTORY', + ]) { + delete env[name]; + } + const git = (cwd, ...args) => execFileSync(realGit, ['-C', cwd, ...args], { + env, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], + }).trim(); + const upstream = path.join(directory, 'upstream'); + mkdirSync(upstream); + git(upstream, 'init', '-b', 'main'); + git(upstream, 'config', 'user.name', 'Fixture'); + git(upstream, 'config', 'user.email', 'fixture@example.invalid'); + git(upstream, 'config', 'commit.gpgsign', 'false'); + mkdirSync(path.join(upstream, 'tools')); + writeFileSync(path.join(upstream, 'tools', '.gitkeep'), ''); + const skill = path.join('domains', 'testing', 'skills', 'fresh', 'skill.md'); + mkdirSync(path.dirname(path.join(upstream, skill)), { recursive: true }); + const content = (revision) => `---\nname: fresh\ndescription: Verify source synchronization\nmaturity: stable\n---\nRevision ${revision}.\n`; + writeFileSync(path.join(upstream, skill), content(1)); + git(upstream, 'add', '.'); + git(upstream, 'commit', '-m', 'initial fixture'); + const sources = [0, 1].map((number) => { + const source = path.join(directory, `source-${number}`); + git(directory, 'clone', upstream, source); + git(source, 'config', 'pull.rebase', 'true'); + return source; + }); + writeFileSync(path.join(upstream, skill), content(2)); + git(upstream, 'add', '.'); + git(upstream, 'commit', '-m', 'updated fixture'); + const expectedHead = git(upstream, 'rev-parse', 'HEAD'); + const monitor = path.join(directory, 'monitor'); + const wrapper = path.join(directory, 'bin'); + mkdirSync(monitor); + mkdirSync(wrapper); + symlinkSync('/bin/bash', path.join(wrapper, 'bash')); + // Instrument the Git process boundary. The actual pulls still use real Git. + // Hold the first pull until the other caller has reached Git or its lock lookup. + writeFileSync(path.join(wrapper, 'git'), `#!/usr/bin/env node +const fs = require('node:fs'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); +const args = process.argv.slice(2); +const monitor = process.env.MMS_TEST_MONITOR; +const log = (event) => fs.appendFileSync(path.join(monitor, 'events'), JSON.stringify({ event, pid: process.pid }) + '\\n'); +const pull = args.includes('pull'); +if (args.includes('--absolute-git-dir')) log('lookup'); +if (pull) { + log('start'); + const limit = Date.now() + 2500; + const clock = new Int32Array(new SharedArrayBuffer(4)); + while (!fs.existsSync(path.join(monitor, 'release'))) { + if (Date.now() >= limit) process.exit(90); + Atomics.wait(clock, 0, 0, 10); + } +} +const result = spawnSync(process.env.MMS_TEST_REAL_GIT, args, { stdio: 'inherit' }); +if (pull) log('end'); +if (result.error) throw result.error; +process.exit(result.status ?? 1); +`, { mode: 0o755 }); + const events = () => { + const file = path.join(monitor, 'events'); + return existsSync(file) ? readFileSync(file, 'utf8').trim().split('\n').filter(Boolean).map(JSON.parse) : []; + }; + function sync(source, name) { + const target = path.join(directory, name); + mkdirSync(target); + const child = spawn(process.execPath, [bin, 'sync', '--target', target, '--repo', 'core', '--no-save'], { + detached: true, + env: { + ...env, + BASH: path.join(wrapper, 'bash'), + PATH: `${wrapper}${path.delimiter}${env.PATH}`, + METAMASK_SKILLS_DIR: source, + XDG_CACHE_HOME: path.join(directory, `cache-${name}`), + MMS_TEST_MONITOR: monitor, + MMS_TEST_REAL_GIT: realGit, + }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + let output = ''; + child.stdout.on('data', (data) => { output += data; }); + child.stderr.on('data', (data) => { output += data; }); + const done = once(child, 'close').then(([code, signal]) => ({ code, signal, output })); + children.push({ child, done }); + return { done, target }; + } + return { + sources, git, upstream, expectedHead, sync, events, + release: () => writeFileSync(path.join(monitor, 'release'), ''), + checkInstalled: (target) => assert.match( + readFileSync(path.join(target, '.claude', 'skills', 'mms-fresh', 'SKILL.md'), 'utf8'), + /Revision 2\./u, + ), + alias: () => { + const alias = path.join(directory, 'source-alias'); + symlinkSync(sources[0], alias); + return alias; + }, + }; +} + +function maximumPulls(events) { + let active = 0; + let maximum = 0; + for (const { event } of events) { + if (event === 'start') maximum = Math.max(maximum, ++active); + if (event === 'end') active--; + } + assert.equal(active, 0); + return maximum; +} + +test('concurrent syncs serialize one checkout across aliases and cache directories', { timeout: 10000 }, async (t) => { + const f = fixture(t); + const first = f.sync(f.sources[0], 'slot-a'); + await waitFor(() => f.events().some(({ event }) => event === 'start')); + const second = f.sync(f.alias(), 'slot-b'); + await waitFor(() => f.events().filter(({ event }) => event === 'lookup').length >= 2 + || f.events().filter(({ event }) => event === 'start').length >= 2); + f.release(); + for (const result of await Promise.all([first.done, second.done])) { + assert.equal(result.code, 0, result.output); + } + assert.equal(maximumPulls(f.events()), 1, 'Git pulls of the same checkout overlapped'); + assert.equal(f.git(f.sources[0], 'rev-parse', 'HEAD'), f.expectedHead); + f.checkInstalled(first.target); + f.checkInstalled(second.target); +}); + +test('different source checkouts can update concurrently', { timeout: 10000 }, async (t) => { + const f = fixture(t); + const first = f.sync(f.sources[0], 'slot-a'); + const second = f.sync(f.sources[1], 'slot-b'); + await waitFor(() => f.events().filter(({ event }) => event === 'start').length === 2); + f.release(); + for (const result of await Promise.all([first.done, second.done])) { + assert.equal(result.code, 0, result.output); + } + assert.equal(maximumPulls(f.events()), 2); + f.checkInstalled(first.target); + f.checkInstalled(second.target); +}); + +test('a failed pull refuses stale installation and releases the source lock', { timeout: 10000 }, async (t) => { + const f = fixture(t); + f.git(f.sources[0], 'remote', 'set-url', 'origin', path.join(f.upstream, 'missing')); + f.release(); + const failed = f.sync(f.sources[0], 'failed'); + const result = await failed.done; + assert.equal(result.code, 1); + assert.match(result.output, /Refusing to install potentially stale skills/u); + assert.equal(existsSync(path.join(failed.target, '.claude', 'skills', 'mms-fresh')), false); + f.git(f.sources[0], 'remote', 'set-url', 'origin', f.upstream); + const recovered = f.sync(f.sources[0], 'recovered'); + const success = await recovered.done; + assert.equal(success.code, 0, success.output); + f.checkInstalled(recovered.target); +}); diff --git a/tools/sync b/tools/sync index 87cd0e49..952d83ce 100755 --- a/tools/sync +++ b/tools/sync @@ -191,6 +191,35 @@ EOF exit 1 fi +pull_source() ( + local src="$1" git_dir lock_dir deadline status + git_dir="$(git -C "$src" rev-parse --absolute-git-dir)" || exit 1 + lock_dir="$git_dir/skills-sync.lock.d" + deadline=$((SECONDS + 300)) + # Every caller of this checkout must share the lock, including automatic + # postinstall syncs that do not run through a farm's prepare wrapper. + until mkdir -m 700 "$lock_dir" 2>/dev/null; do + if [[ "$SECONDS" -ge "$deadline" ]]; then + echo "Error: timed out waiting for skill source update lock: $lock_dir" >&2 + exit 1 + fi + sleep 0.1 + done + release_source_lock() { + status=$? + trap - EXIT + if ! rmdir "$lock_dir"; then + [[ "$status" -ne 0 ]] || status=1 + fi + exit "$status" + } + trap release_source_lock EXIT + trap 'exit 130' INT + trap 'exit 143' TERM + trap 'exit 129' HUP + git -C "$src" pull --ff-only +) + for src in "${SOURCES[@]}"; do if [[ ! -d "$src/.git" ]]; then echo "Using packaged skills source: $src" @@ -198,7 +227,7 @@ for src in "${SOURCES[@]}"; do fi echo "Updating $src" - if ! pull_output=$(git -C "$src" pull --ff-only 2>&1); then + if ! pull_output=$(pull_source "$src" 2>&1); then printf '%s\n' "$pull_output" | sed 's/^/ /' >&2 cat >&2 <