From 9a19ed76d08e05a330a4458583c8bd00dfc4b18f Mon Sep 17 00:00:00 2001 From: Diogo Esteves Date: Fri, 30 Oct 2026 17:40:35 +0000 Subject: [PATCH 1/3] fix(cli): CLI flag, run-folder and inspect edges, and two GitHub Action edges - `--max-warnings=-1` is no limit, as in ESLint; an empty or non-integer value exits 2 instead of being read as 0. - An option `--help` gives to other commands exits 2 instead of being ignored (`check . --out report.json` wrote no file). The drift test runs every such pair against `--help`. - From a folder with no config, components.json, package.json or project.json (`app/settings`), check, inspect, export and init work on the nearest folder above with one, as the Claude Code hook does. - `inspect` says when color tokens or components are missing, and the root `--require-design-system` error names what each project lacks. - Ctrl+D or Ctrl+C at an `init` question says it cancelled (exit 130) instead of printing an AbortError stack. - `--format github` titles annotations `onsystem: `, as the Action does. - The Action counts each hunk's lines, so an added `++ ` line is not read as a file header, and passes the workspace folders as git safe.directory, so a container job's checkout is read. Roadmap item OS2-59. --- .changeset/cli-and-action-edges.md | 17 +++ action/check.mjs | 107 ++++++++++++--- src/cli.ts | 141 ++++++++++++++++++-- src/lint/format.ts | 3 +- test/action.test.ts | 58 ++++++++- test/cli-docs.test.ts | 29 +++++ test/cli-edges.test.ts | 203 +++++++++++++++++++++++++++++ test/cli.test.ts | 2 +- test/workspace.test.ts | 4 +- 9 files changed, 531 insertions(+), 33 deletions(-) create mode 100644 .changeset/cli-and-action-edges.md create mode 100644 test/cli-edges.test.ts diff --git a/.changeset/cli-and-action-edges.md b/.changeset/cli-and-action-edges.md new file mode 100644 index 00000000..36d88a44 --- /dev/null +++ b/.changeset/cli-and-action-edges.md @@ -0,0 +1,17 @@ +--- +'onsystem': patch +--- + +`check --max-warnings=-1` is no limit, as in ESLint: it failed a clean file with "Too many warnings (0, max -1)". An empty value, as an unset variable in a CI script gives, or one that is not a whole number, now exits 2 with one line; an empty one was read as 0. + +An option of another command now exits 2 with one line instead of being ignored: `check . --out report.json` wrote no file and exited as if it had, and `check . --explain` explained nothing. + +`check`, `inspect`, `export` and `init` run from a folder inside a project that has no config, `components.json`, `package.json` or `project.json` of its own (`app/settings`) now work on the nearest folder above it with one, as the Claude Code hook does. From there, zero config found no design system, so a file with errors gave "No problems in 1 file." and exit 0. With `--root`, `--config`, `--components`, `--tokens` or `--docs`, or with a baseline in the folder, the root is what it was. + +`inspect` says when the design system has no color tokens, or no components, which `check` said only after it ran: `No color tokens found: check will not run no-hardcoded-color.` At a monorepo root, each project's entry says what it is missing, and `check --require-design-system` names the projects and what each one lacks. + +Ctrl+D or Ctrl+C at one of `init`'s questions now says it cancelled and exits 130, instead of printing an `AbortError` stack trace. + +`check --format github` titles its annotations `onsystem: `, as the GitHub Action does. + +The GitHub Action no longer drops a file's later changes when an added line starts with `++ ` (which the diff shows as `+++ `, like a file header), so their findings are reported. It also reads the checkout in a `container:` job, where git refused the workspace as another user's ("dubious ownership") and every pull request failed; when git still refuses it, the error says to add it to `safe.directory` rather than to check out with `fetch-depth: 0`. diff --git a/action/check.mjs b/action/check.mjs index 8c5e1efe..6e810364 100644 --- a/action/check.mjs +++ b/action/check.mjs @@ -35,7 +35,29 @@ export function parseDiff(diff) { const changed = new Map(); /** @type {[number, number][] | undefined} */ let ranges; + // The lines of the hunk still to come, on the old side and the new: an added line that starts + // with `++ ` reads `+++ ` in the diff, and is not the header of another file. + let removed = 0; + let added = 0; for (const line of diff.split('\n')) { + if (removed || added) { + if (line.startsWith('-') && removed) { + removed--; + continue; + } + if (line.startsWith('+') && added) { + added--; + continue; + } + if (line.startsWith(' ') && removed && added) { + removed--; + added--; + continue; + } + // `\ No newline at end of file`. + if (line.startsWith('\\')) continue; + removed = added = 0; + } if (line.startsWith('+++ ')) { const name = unquote(line.slice(4).trimEnd()); ranges = undefined; @@ -44,10 +66,13 @@ export function parseDiff(diff) { changed.set(name.slice(2), ranges); continue; } - const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/.exec(line); - if (!hunk || !ranges) continue; - const start = Number(hunk[1]); - const count = hunk[2] === undefined ? 1 : Number(hunk[2]); + const hunk = /^@@ -\d+(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/.exec(line); + if (!hunk) continue; + const start = Number(hunk[2]); + const count = hunk[3] === undefined ? 1 : Number(hunk[3]); + removed = hunk[1] === undefined ? 1 : Number(hunk[1]); + added = count; + if (!ranges) continue; ranges.push(count === 0 ? [Math.max(1, start), start + 1] : [start, start + count - 1]); } return changed; @@ -141,9 +166,14 @@ function count(n, word) { /** * @param {string[]} args * @param {string} cwd + * @param {string[]} [trust] `-c safe.directory=…` options, from `trusted()` */ -function git(args, cwd) { - const result = spawnSync('git', args, { cwd, encoding: 'utf8', maxBuffer: 512 * 1024 * 1024 }); +function git(args, cwd, trust = []) { + const result = spawnSync('git', [...trust, ...args], { + cwd, + encoding: 'utf8', + maxBuffer: 512 * 1024 * 1024, + }); return { ok: result.status === 0, out: (result.stdout ?? '').trim(), @@ -168,6 +198,30 @@ function baseOf(env) { return undefined; } +/** + * Git options that let git read the checkout in a container job (`container:`), where the + * workspace belongs to another user and git refuses it ("dubious ownership") unless it is a + * safe.directory: each folder from `cwd` up to GITHUB_WORKSPACE, one of which is the top of the + * repository the workflow checked out. None outside the workspace, or outside Actions. + * + * @param {Io['env']} env + * @param {string} cwd + * @returns {string[]} + */ +export function trusted(env, cwd) { + if (!env.GITHUB_WORKSPACE) return []; + const workspace = realPath(env.GITHUB_WORKSPACE); + /** @type {string[]} */ + const dirs = []; + for (let dir = realPath(cwd); ; dir = path.dirname(dir)) { + const rel = path.relative(workspace, dir); + if (rel.startsWith('..') || path.isAbsolute(rel)) return []; + dirs.push(dir); + if (!rel) break; + } + return dirs.flatMap((dir) => ['-c', `safe.directory=${dir.split(path.sep).join('/')}`]); +} + /** * The lines changed between the base and the files on disk, from where the base and the checked-out * commit last met. actions/checkout fetches one commit by default: what the comparison needs is @@ -175,28 +229,34 @@ function baseOf(env) { * * @param {string} base * @param {string} cwd + * @param {string[]} trust * @returns {{ top: string, changed: ChangedLines }} */ -function changedSince(base, cwd) { - const top = git(['rev-parse', '--show-toplevel'], cwd); +function changedSince(base, cwd, trust) { + const top = git(['rev-parse', '--show-toplevel'], cwd, trust); if (!top.ok) throw new Error(`not in a git repository (${top.err})`); - const head = git(['rev-parse', 'HEAD'], cwd).out; + const head = git(['rev-parse', 'HEAD'], cwd, trust).out; /** @param {string[]} args */ const fetch = (...args) => { - const fetched = git(['fetch', '--quiet', '--no-tags', '--no-recurse-submodules', ...args], cwd); + const fetched = git( + ['fetch', '--quiet', '--no-tags', '--no-recurse-submodules', ...args], + cwd, + trust, + ); if (!fetched.ok) throw new Error(`git fetch ${args.join(' ')} failed: ${fetched.err}`); }; - if (!git(['cat-file', '-e', `${base}^{commit}`], cwd).ok) fetch('--depth=1', 'origin', base); - let mergeBase = git(['merge-base', base, head], cwd); + if (!git(['cat-file', '-e', `${base}^{commit}`], cwd, trust).ok) + fetch('--depth=1', 'origin', base); + let mergeBase = git(['merge-base', base, head], cwd, trust); for (const deepen of [1, 10, 100, 1000]) { if (mergeBase.ok) break; - if (git(['rev-parse', '--is-shallow-repository'], cwd).out !== 'true') break; + if (git(['rev-parse', '--is-shallow-repository'], cwd, trust).out !== 'true') break; fetch(`--deepen=${deepen}`, 'origin', base, head); - mergeBase = git(['merge-base', base, head], cwd); + mergeBase = git(['merge-base', base, head], cwd, trust); } - if (!mergeBase.ok && git(['rev-parse', '--is-shallow-repository'], cwd).out === 'true') { + if (!mergeBase.ok && git(['rev-parse', '--is-shallow-repository'], cwd, trust).out === 'true') { fetch('--unshallow', 'origin', base, head); - mergeBase = git(['merge-base', base, head], cwd); + mergeBase = git(['merge-base', base, head], cwd, trust); } if (!mergeBase.ok) throw new Error(`no common history between ${base} and ${head}`); // Explicit options, so a user's git config (noprefix, relative, external diff) changes nothing. @@ -216,6 +276,7 @@ function changedSince(base, cwd) { '--', ], cwd, + trust, ); if (!diff.ok) throw new Error(`git diff failed: ${diff.err}`); return { top: realPath(top.out), changed: parseDiff(diff.out) }; @@ -340,17 +401,25 @@ export function run(io) { // What changed, before the check: a failed fetch is said plainly rather than after the output. const base = onlyChanged ? baseOf(env) : undefined; + const trust = trusted(env, cwd); // The repository root, which annotations are relative to: the git top-level, which the diff is // relative to too, or the workspace where git is missing or refuses the checkout. - let top = realPath(git(['rev-parse', '--show-toplevel'], cwd).out || env.GITHUB_WORKSPACE || cwd); + let top = realPath( + git(['rev-parse', '--show-toplevel'], cwd, trust).out || env.GITHUB_WORKSPACE || cwd, + ); /** @type {ChangedLines | undefined} */ let changed; if (base) { try { - ({ top, changed } = changedSince(base, cwd)); + ({ top, changed } = changedSince(base, cwd, trust)); } catch (error) { + const message = /** @type {Error} */ (error).message; + // A checkout git refuses to read is not one that lacks history. + const fix = /dubious ownership|safe\.directory/.test(message) + ? 'Add the checkout to git\'s safe.directory (git config --global --add safe.directory "$GITHUB_WORKSPACE")' + : 'Check out with fetch-depth: 0'; io.stdout( - `::error title=onsystem::Could not work out which lines this pull request changed: ${data(/** @type {Error} */ (error).message)}. Check out with fetch-depth: 0, or set only-changed-lines: false to report every finding.`, + `::error title=onsystem::Could not work out which lines this pull request changed: ${data(message)}. ${fix}, or set only-changed-lines: false to report every finding.`, ); return 1; } diff --git a/src/cli.ts b/src/cli.ts index a2ae85e1..1e89aefa 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1,6 +1,7 @@ import { spawnSync } from 'node:child_process'; import fs from 'node:fs'; import fsp from 'node:fs/promises'; +import os from 'node:os'; import path from 'node:path'; import readline from 'node:readline/promises'; import { parseArgs } from 'node:util'; @@ -47,7 +48,7 @@ import { stderrLogger, silentLogger } from './util/log.js'; import { isInside, relativePath, SCRIPT_FILE, toPosix } from './util/paths.js'; import { plural, unique } from './util/strings.js'; import { NAME, VERSION } from './version.js'; -import { loadTarget, type Project, type Workspace } from './workspace.js'; +import { hasProjectMarker, loadTarget, type Project, type Workspace } from './workspace.js'; import { detectProject, type DetectionStep } from './detect.js'; const DOCS = 'https://onsystem.vercel.app/docs'; @@ -75,7 +76,10 @@ project: each file is checked against the design system of the nearest folder with a config, components.json or package.json, and inspect lists every project. Options - --root Project root (default: the config file's directory, or cwd) + --root Project root (default: the config file's directory, or + cwd; from a folder with no config, components.json, + package.json or project.json, the nearest folder above + it with one) --config Config file (default: onsystem.config.{json,ts,mts,js,mjs}) --components Component sources, repeatable (default: found through components.json or a workspace package, else @@ -88,7 +92,8 @@ Options inspect: pretty | json (default: pretty); export: dtcg | design-md --out export: write to this file instead of stdout - --max-warnings check: exit 1 when there are more than n warnings + --max-warnings check: exit 1 when there are more than n warnings (-1: + no limit) --update-baseline check: record the current findings as accepted, and exit 0 --prune-baseline check: drop baseline entries that no longer occur, and accept nothing new @@ -171,6 +176,7 @@ export async function main(argv: string[], io: Io = defaultIo): Promise args: argv, allowPositionals: true, allowNegative: true, + tokens: true, options: { root: { type: 'string' }, config: { type: 'string' }, @@ -202,7 +208,7 @@ export async function main(argv: string[], io: Io = defaultIo): Promise io.stderr(`${(error as Error).message}\n\nRun ${NAME} --help for usage.`); return 2; } - const { values, positionals } = parsed; + const { values, positionals, tokens } = parsed; if (values.help) { io.stdout(HELP); return 0; @@ -217,6 +223,17 @@ export async function main(argv: string[], io: Io = defaultIo): Promise io.stdout(HELP); return 0; } + // An option of another command would be ignored: `check . --out report.json` wrote no file. + for (const token of tokens) { + if (token.kind !== 'option') continue; + const commands = COMMAND_OPTIONS[token.name]; + if (commands && isCommand(command) && !commands.includes(command)) { + io.stderr( + `${command}: ${token.rawName} is an option of ${listOf(commands, commands.length)} only. Run ${NAME} --help for usage.`, + ); + return 2; + } + } // `--root ${workspaceFolder}` from a client that does not expand variables // would make a folder of that name the project. let root = values.root; @@ -226,6 +243,13 @@ export async function main(argv: string[], io: Io = defaultIo): Promise ); root = undefined; } + // Run from a folder of a project (`app/settings`), zero config found no design system there + // and passed every file: the commands work on the project around it, as the Claude Code hook + // does. The server finds its project from the client's roots. + const given = [values.config, values.components, values.tokens, values.docs]; + if (root === undefined && command !== 'serve' && given.every((v) => v === undefined)) { + root = enclosingProject(io.cwd); + } const configOptions = { cwd: io.cwd, root, @@ -286,6 +310,12 @@ export async function main(argv: string[], io: Io = defaultIo): Promise io.stderr(error.message); return 2; } + // Ctrl+D or Ctrl+C at one of init's questions, rather than a stack trace. 130, as a shell + // reports a command the user interrupted. + if ((error as Error).name === 'AbortError') { + io.stderr(`${command}: cancelled; nothing after that question was done.`); + return 130; + } // Exit 1 means findings. A run that failed (a file it cannot read, a bug) exits 2, as a // broken config does, so CI fails the job rather than taking an empty report for findings. io.stderr((error as Error).stack ?? String(error)); @@ -293,6 +323,56 @@ export async function main(argv: string[], io: Io = defaultIo): Promise } } +const COMMANDS = ['serve', 'check', 'inspect', 'export', 'init'] as const; +type Command = (typeof COMMANDS)[number]; + +function isCommand(name: string): name is Command { + return (COMMANDS as readonly string[]).includes(name); +} + +/** + * The options that `--help` gives to some commands only (`check:` in its text), by parser name. + * The others apply to every command. test/cli-docs.test.ts holds the two together. + */ +const COMMAND_OPTIONS: Partial> = { + watch: ['serve'], + format: ['check', 'inspect', 'export'], + out: ['export'], + 'max-warnings': ['check'], + 'update-baseline': ['check'], + 'prune-baseline': ['check'], + baseline: ['check'], + 'ignore-baseline': ['check'], + 'include-design-system': ['check'], + 'include-tests': ['check'], + 'skip-excluded': ['check'], + 'quiet-without-design-system': ['check'], + 'require-design-system': ['check'], + explain: ['inspect'], + yes: ['init'], + force: ['init'], +}; + +/** + * The project around `cwd` when `cwd` is none: the nearest folder above it with a config, a + * components.json, a package.json or a project.json, relative to `cwd` (`../..`), as a command + * that names it with `--root` would. Undefined when `cwd` has one of those, or a baseline (a run + * from there recorded it), or when no folder below the home folder has one. + */ +function enclosingProject(cwd: string): string | undefined { + const baselines = [BASELINE_FILE, LEGACY_BASELINE_FILE]; + if (hasProjectMarker(cwd) || baselines.some((name) => fs.existsSync(path.join(cwd, name)))) { + return undefined; + } + const home = path.resolve(os.homedir()); + let dir = path.dirname(cwd); + while (dir !== path.dirname(dir) && dir !== home) { + if (hasProjectMarker(dir)) return path.relative(cwd, dir); + dir = path.dirname(dir); + } + return undefined; +} + interface CheckValues { cache: boolean; format?: string | undefined; @@ -363,12 +443,14 @@ async function check( io.stderr(`check: unknown --format "${format}" (pretty, json, github, sarif)`); return 2; } - const maxWarnings = - values['max-warnings'] === undefined ? undefined : Number(values['max-warnings']); - if (maxWarnings !== undefined && !Number.isInteger(maxWarnings)) { - io.stderr('check: --max-warnings expects an integer'); + // A whole number: Number() reads an empty value (an unset variable) as 0. A negative one is + // no limit, as in ESLint (`--max-warnings=-1`). + const given = values['max-warnings']; + if (given !== undefined && !/^-?\d+$/.test(given)) { + io.stderr(`check: --max-warnings expects a whole number (-1 for no limit), not "${given}"`); return 2; } + const maxWarnings = given === undefined || Number(given) < 0 ? undefined : Number(given); const target = await loadTarget(configOptions); warnDeprecated(target.config.deprecations, format, io); @@ -1058,8 +1140,15 @@ async function selectInWorkspace( } if (!complete) { const packages = workspace.projects.length + workspace.others.length; + // What each project with part of one lacks: `no color tokens in apps/web and apps/admin`. + const lacking = new Map(); + for (const p of workspace.projects) { + const gap = missingFrom(await load(p)); + if (gap) lacking.set(gap.what, [...(lacking.get(gap.what) ?? []), p.dir]); + } + const why = [...lacking].map(([what, dirs]) => `; ${what} in ${listOf(dirs)}`).join(''); io.stderr( - `check --require-design-system: no workspace package has a design system with components and color tokens (${plural(packages, 'package')} looked at). See ${DOCS}/troubleshooting`, + `check --require-design-system: no workspace package has a design system with components and color tokens (${plural(packages, 'package')} looked at${why}). See ${DOCS}/troubleshooting`, ); return 2; } @@ -1155,6 +1244,11 @@ async function inspect( } lines.push('', `Tokens (${model.tokens.length})`); for (const [category, n] of tokenCounts(model)) lines.push(` ${category.padEnd(18)} ${n}`); + // What check says after its findings, said before it runs. + const gap = missingFrom(ds); + if (gap) { + lines.push('', `${gap.found}: check will not run ${gap.rules}. See ${DOCS}/configuration`); + } if (model.warnings.length) { lines.push('', 'Warnings'); for (const warning of model.warnings) lines.push(` ${warning}`); @@ -1166,6 +1260,31 @@ async function inspect( return 0; } +/** + * What a design system lacks for `check` to run all its rules, as `DesignSystem.notice()` says + * after a check: components, color tokens or both. Undefined when it has both. + */ +function missingFrom(ds: DesignSystem): { what: string; found: string; rules: string } | undefined { + const components = ds.model.components.length > 0; + const colors = ds.tokenIndex.has('color'); + if (components && colors) return undefined; + if (components) { + return { what: 'no color tokens', found: 'No color tokens found', rules: 'no-hardcoded-color' }; + } + if (colors) { + return { + what: 'no components', + found: 'No design-system components found', + rules: 'the component rules', + }; + } + return { + what: 'no components or color tokens', + found: 'No design system found (no components or color tokens)', + rules: 'the component rules or no-hardcoded-color', + }; +} + /** * `MUI (@mui/material 9.4.0): 149 components from its type declarations`, per library the * project depends on, including those the config's `libraries` leaves out. @@ -1304,6 +1423,7 @@ async function inspectWorkspace( const { model } = ds; const roots = ds.roots(); const tokens = [...tokenCounts(model)].map(([category, n]) => `${category} ${n}`); + const gap = missingFrom(ds); const own = project.config.configFile && (project.config.configFile !== workspace.configFile || project.config.designSystemPackage) @@ -1318,6 +1438,9 @@ async function inspectWorkspace( ` files ${plural(model.stats.files.components, 'component file')}, ${plural(model.stats.files.tokens, 'token file')}, ${plural(model.stats.files.docs, 'doc')}`, ` components ${roots.length} + ${ds.components.length - roots.length} parts${roots.length ? `: ${listNames(roots.map((c) => c.name))}` : ''}`, ` tokens ${tokens.length ? tokens.join(', ') : 'none'}`, + ...(gap + ? [` missing ${gap.what.replace(/^no /, '')}: check will not run ${gap.rules}`] + : []), ...(model.warnings.length ? [` warnings ${model.warnings.length}: ${model.warnings[0] ?? ''}`] : []), diff --git a/src/lint/format.ts b/src/lint/format.ts index 4fccbe2f..a5c5e09b 100644 --- a/src/lint/format.ts +++ b/src/lint/format.ts @@ -50,7 +50,8 @@ export function formatDiagnostics( const file = options.uri ? options.uri(result.file) : result.file; return result.diagnostics.map((d) => { const level = d.severity === 'error' ? 'error' : 'warning'; - const props = `file=${escapeProperty(file)},line=${d.line},col=${d.column},endLine=${d.endLine},endColumn=${d.endColumn},title=${escapeProperty(d.ruleId)}`; + // Titled as the GitHub Action titles them (action/check.mjs), so both read the same. + const props = `file=${escapeProperty(file)},line=${d.line},col=${d.column},endLine=${d.endLine},endColumn=${d.endColumn},title=${escapeProperty(`onsystem: ${d.ruleId}`)}`; return `::${level} ${props}::${escapeData(d.message)}`; }); }); diff --git a/test/action.test.ts b/test/action.test.ts index 6335e006..7f611a1f 100644 --- a/test/action.test.ts +++ b/test/action.test.ts @@ -3,7 +3,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { pathToFileURL } from 'node:url'; -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import { annotation, cli, onChangedLines, parseDiff, run } from '../action/check.mjs'; import { fixture } from './helpers.js'; @@ -202,6 +202,37 @@ describe('the GitHub Action, in parts', () => { }); }); + it('reads an added line that starts with ++ as part of its hunk, not as a file header', () => { + // `++ counter;` added reads `+++ counter;` in the diff: taken for a header, it dropped the + // file's later hunks, and the findings on them were not reported. + const diff = [ + 'diff --git a/app/f.tsx b/app/f.tsx', + '--- a/app/f.tsx', + '+++ b/app/f.tsx', + '@@ -3,0 +4 @@', + '+++ counter;', + '@@ -10 +11,2 @@', + '--- removed', + '+-- kept', + '+++ added', + '\\ No newline at end of file', + '@@ -20,0 +22 @@', + '+x', + '+++ b/app/g.tsx', + '@@ -1 +1 @@', + '-a', + '+b', + ].join('\n'); + expect(Object.fromEntries(parseDiff(diff))).toEqual({ + 'app/f.tsx': [ + [4, 4], + [11, 12], + [22, 22], + ], + 'app/g.tsx': [[1, 1]], + }); + }); + it('keeps the findings that overlap a changed range', () => { const finding = (line: number, endLine = line) => ({ file: 'a.tsx', @@ -370,6 +401,31 @@ describe('the GitHub Action', () => { expect(queue.outputs).toEqual({ errors: '1', warnings: '0', 'outside-diff': '1' }); }); + it("reads a checkout that git takes for another user's, as in a container job", () => { + // In a `container:` job the workspace belongs to another user, and git refuses to read it + // ("dubious ownership") unless it is a safe.directory. Git's own test switch stands in, with + // no system or global config, where a runner may list every folder as safe. + const { root, base } = repository(); + const web = path.join(root, 'apps/web'); + vi.stubEnv('GIT_TEST_ASSUME_DIFFERENT_OWNER', '1'); + vi.stubEnv('GIT_CONFIG_NOSYSTEM', '1'); + vi.stubEnv('GIT_CONFIG_GLOBAL', path.join(fixture({ gitconfig: '' }), 'gitconfig')); + try { + expect(spawnSync('git', ['rev-parse', 'HEAD'], { cwd: web }).status).not.toBe(0); + const result = action(web, { payload: pr(base) }, { GITHUB_WORKSPACE: root }); + expect(result.stdout).toContain('::error file=apps/web/app/page.tsx,line=4,'); + expect(result.outputs).toEqual({ errors: '1', warnings: '0', 'outside-diff': '1' }); + // Outside the workspace it is not the checkout: git's advice, not fetch-depth. + const elsewhere = action(web, { payload: pr(base) }, { GITHUB_WORKSPACE: fixture({}) }); + expect(elsewhere.code).toBe(1); + expect(elsewhere.stdout).toMatch( + /^::error title=onsystem::Could not work out which lines this pull request changed: not in a git repository \(.*dubious ownership.*\. Add the checkout to git's safe\.directory \(git config --global --add safe\.directory "\$GITHUB_WORKSPACE"\), or set only-changed-lines: false to report every finding\.$/, + ); + } finally { + vi.unstubAllEnvs(); + } + }); + it('names files from GITHUB_WORKSPACE when there is no git repository', () => { // A checkout without git (actions/checkout falls back to the REST API), on a push. const root = fixture({ 'apps/web/app/page.tsx': PAGE }); diff --git a/test/cli-docs.test.ts b/test/cli-docs.test.ts index f69ee0c8..b03ea657 100644 --- a/test/cli-docs.test.ts +++ b/test/cli-docs.test.ts @@ -1,6 +1,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; import ts from 'typescript'; import { beforeAll, describe, expect, it } from 'vitest'; import { parse } from 'yaml'; @@ -208,6 +209,34 @@ describe('the CLI reference (docs/cli.md)', () => { } }); + it('rejects each option under the commands --help does not give it to', async () => { + // `check . --out report.json` wrote no file and exited as if it had. + let tried = 0; + for (const option of options) { + if (!option.commands.length) continue; + for (const command of COMMANDS.filter((c) => !option.commands.includes(c))) { + for (const flag of option.flags) { + const err: string[] = []; + const io: Io = { + cwd: DEMO_ROOT, + color: false, + stdout: () => {}, + stderr: (t) => err.push(t), + // `serve` that took the option would start here, and not on stdin. + transport: InMemoryTransport.createLinkedPair()[1], + }; + const args = [command, flag, ...(option.arg ? ['x'] : [])]; + expect(await main(args, io), args.join(' ')).toBe(2); + expect(err.join('\n'), args.join(' ')).toBe( + `${command}: ${flag} is an option of ${option.commands.join(', ').replace(/, (\w+)$/, ' and $1')} only. Run onsystem --help for usage.`, + ); + tried++; + } + } + } + expect(tried).toBeGreaterThan(50); + }); + it('runs inspect --format json, the value its row names', async () => { const out: string[] = []; const io: Io = { cwd: DEMO_ROOT, color: false, stdout: (t) => out.push(t), stderr: () => {} }; diff --git a/test/cli-edges.test.ts b/test/cli-edges.test.ts new file mode 100644 index 00000000..eb839cb4 --- /dev/null +++ b/test/cli-edges.test.ts @@ -0,0 +1,203 @@ +import fs from 'node:fs'; +import path from 'node:path'; +import readline from 'node:readline/promises'; +import { PassThrough } from 'node:stream'; + +import { describe, expect, it } from 'vitest'; + +import { main, type Io } from '../src/cli.js'; +import { DEMO_ROOT, fixture } from './helpers.js'; + +// Edges of the CLI's options and of where it runs from: a warning cap of -1 or of an unset +// variable, an option of another command, Ctrl+D at one of init's questions, a run from a +// folder inside a project, and a design system that lacks color tokens. + +async function run(args: string[], cwd = DEMO_ROOT, prompt?: Io['prompt']) { + const out: string[] = []; + const err: string[] = []; + const io: Io = { + cwd, + color: false, + stdout: (t) => out.push(t), + stderr: (t) => err.push(t), + ...(prompt ? { prompt } : {}), + }; + const code = await main(args, io); + return { code, stdout: out.join('\n'), stderr: err.join('\n') }; +} + +const WARN_ONLY = [ + '--config', + path.join(import.meta.dirname, 'fixtures/warn-only.config.json'), + '--root', + DEMO_ROOT, +]; + +describe('--max-warnings', () => { + it('takes -1 as no limit, as ESLint does', async () => { + const clean = await run([ + 'check', + 'app/settings/members.tsx', + '--no-cache', + '--max-warnings=-1', + ]); + expect(clean.stderr).not.toContain('Too many warnings'); + expect(clean.code).toBe(0); + const warned = await run([ + 'check', + 'app/settings/danger-zone.tsx', + '--no-cache', + ...WARN_ONLY, + '--max-warnings=-1', + ]); + expect(warned.stdout).toContain('0 errors, 11 warnings'); + expect(warned.code).toBe(0); + }); + + it('rejects an empty value, from an unset variable, rather than read it as 0', async () => { + for (const value of ['', ' ', 'many', '1.5', '0x10']) { + const { code, stderr } = await run([ + 'check', + 'app/settings/danger-zone.tsx', + '--no-cache', + ...WARN_ONLY, + `--max-warnings=${value}`, + ]); + expect(code, value).toBe(2); + expect(stderr).toBe( + `check: --max-warnings expects a whole number (-1 for no limit), not "${value}"`, + ); + } + }); +}); + +describe('options of another command', () => { + it('are rejected rather than ignored', async () => { + const dir = fixture({}); + const out = await run(['check', 'app', '--out', path.join(dir, 'report.json')]); + expect(out.code).toBe(2); + expect(out.stderr).toBe( + 'check: --out is an option of export only. Run onsystem --help for usage.', + ); + expect(fs.existsSync(path.join(dir, 'report.json'))).toBe(false); + expect(out.stdout).toBe(''); + + expect((await run(['check', 'app', '--explain'])).stderr).toBe( + 'check: --explain is an option of inspect only. Run onsystem --help for usage.', + ); + expect((await run(['inspect', '--max-warnings=0'])).stderr).toBe( + 'inspect: --max-warnings is an option of check only. Run onsystem --help for usage.', + ); + expect((await run(['init', '--no-watch'])).stderr).toBe( + 'init: --no-watch is an option of serve only. Run onsystem --help for usage.', + ); + // `onsystem` alone is `serve`, which answers an MCP client: it does not start. + expect(await run(['--format', 'json'])).toEqual({ + code: 2, + stdout: '', + stderr: + 'serve: --format is an option of check, inspect and export only. Run onsystem --help for usage.', + }); + expect((await run(['inspect', '-y'])).stderr).toBe( + 'inspect: -y is an option of init only. Run onsystem --help for usage.', + ); + }); + + it('still leaves an unknown command to say so', async () => { + expect((await run(['lint', '--out', 'x'])).stderr).toBe( + 'Unknown command "lint". Run onsystem --help for usage.', + ); + }); +}); + +describe('init in a terminal', () => { + /** A terminal question answered with Ctrl+D, as `askInTerminal` asks it. */ + const ctrlD = async (question: string) => { + const input = new PassThrough(); + const rl = readline.createInterface({ input, output: new PassThrough(), terminal: true }); + try { + const answer = rl.question(question); + input.write('\u0004'); + return await answer; + } finally { + rl.close(); + } + }; + + it('says that Ctrl+D cancelled it, without a stack trace', async () => { + // Nothing to find, so init asks where the components are. + const root = fixture({ 'package.json': '{ "name": "app" }' }); + const { code, stderr } = await run(['init'], root, ctrlD); + expect(stderr).toBe('init: cancelled; nothing after that question was done.'); + expect(code).toBe(130); + expect(fs.existsSync(path.join(root, 'onsystem.config.json'))).toBe(false); + }); +}); + +describe('a run from a folder inside a project', () => { + it('checks against the project around it, as from its root', async () => { + const settings = path.join(DEMO_ROOT, 'app/settings'); + const inside = await run( + ['check', 'danger-zone.tsx', '--format', 'json', '--no-cache'], + settings, + ); + const fromRoot = await run([ + 'check', + 'app/settings/danger-zone.tsx', + '--format', + 'json', + '--no-cache', + ]); + type Results = { file: string; diagnostics: unknown[] }[]; + const results = JSON.parse(inside.stdout) as Results; + expect(results[0]?.diagnostics).toHaveLength(11); + expect(results.map((r) => r.diagnostics)).toEqual( + (JSON.parse(fromRoot.stdout) as Results).map((r) => r.diagnostics), + ); + expect(results[0]?.file).toBe('danger-zone.tsx'); + expect(inside.stderr).toBe(''); + expect(inside.code).toBe(1); + + const inspect = await run(['inspect', '--no-cache'], settings); + expect(inspect.stdout.split('\n')[0]).toBe(`root ${DEMO_ROOT}`); + }); + + it('keeps the folder as the root when --root says so, or when a run from there recorded a baseline', async () => { + const files = { + 'package.json': '{ "name": "app" }', + 'components/ui/button.tsx': + 'export function Button(props: { children?: unknown }) { return