diff --git a/.changeset/cli-and-action-edges.md b/.changeset/cli-and-action-edges.md new file mode 100644 index 00000000..0b9cb1ca --- /dev/null +++ b/.changeset/cli-and-action-edges.md @@ -0,0 +1,19 @@ +--- +'onsystem': minor +--- + +Upgrading: three changes can fail a CI run that passed before. An option of another command, such as `check . --out report.json` or `inspect --max-warnings 0`, now exits 2: drop it, or give it to the command it belongs to. An empty `--max-warnings=`, as an unset variable gives, now exits 2 instead of being read as 0: set the variable, or leave the option out. And `check` run from a folder inside a project that has no config, `components.json`, `package.json` or `project.json` of its own (`app/settings`) now checks against the project above it, so it can report findings where it reported none: fix them, record them with `--update-baseline`, or pass `--root .` to check that folder on its own as before. + +`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, or one that is not a whole number, exits 2 with one line; an empty one was read as 0. + +An option of another command 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. `onsystem` (the MCP server) only says on stderr that it ignores such an option, and starts, so a stale argument in an agent's MCP config does not stop the server. + +`check`, `inspect`, `export` and `init` run from a folder inside a project with none of those files (`app/settings`) work on the nearest folder above it with one, as the Claude Code hook does, and `check` says which: `Checking the project in ../..` (on stderr with `--format json`, `sarif` or `github`). 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`, from a folder with a baseline, or from the home folder, the root is what it was; the home folder is never the project. So is it from a folder with a `components/ui` or `src/components/ui` folder of its own, as a design system without a `package.json`, unless that folder is the project's `src` or `app` folder, or the project has a `components.json` or a config. At a monorepo root, the notices name the projects relative to the working directory, as the files are named. + +`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, on a line of its own, and exits 130, instead of printing an `AbortError` stack trace. `init` run from a folder below the project names the baseline it records as it names the config (`../../onsystem.baseline.json`). + +`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`. The CLI it runs gets the same `safe.directory` entries, so it leaves out what git ignores there too. diff --git a/action/check.mjs b/action/check.mjs index 8c5e1efe..accf8ade 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,57 @@ 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('/')}`]); +} + +/** + * Git `-c key=value` options, such as `trusted()` gives, as the environment variables git reads + * the same settings from (GIT_CONFIG_COUNT, GIT_CONFIG_KEY_, GIT_CONFIG_VALUE_), after the + * entries `env` already has. The CLI the action runs gets them: in a container job its + * `git check-ignore` failed without the safe.directory entries, so git-ignored files were checked. + * + * @param {string[]} options `-c key=value` pairs + * @param {Io['env']} env + * @returns {Record} + */ +export function configEnv(options, env) { + if (!options.length) return {}; + /** @type {Record} */ + const vars = {}; + const given = env.GIT_CONFIG_COUNT ?? ''; + let n = /^\d+$/.test(given) ? Number(given) : 0; + for (let i = 1; i < options.length; i += 2) { + const setting = options[i] ?? ''; + const at = setting.indexOf('='); + vars[`GIT_CONFIG_KEY_${n}`] = setting.slice(0, at); + vars[`GIT_CONFIG_VALUE_${n}`] = setting.slice(at + 1); + n++; + } + vars.GIT_CONFIG_COUNT = String(n); + return vars; +} + /** * 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 +256,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 +303,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 +428,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; } @@ -375,6 +471,7 @@ export function run(io) { fs.writeFileSync(linesFile, JSON.stringify(changedLinesByPath(changed, top))); } const all = [...prefix, ...args]; + const childEnv = { ...process.env, ...env }; let checked; try { checked = spawnSync(command, shell ? all.map(shellQuote) : all, { @@ -382,7 +479,7 @@ export function run(io) { encoding: 'utf8', shell, maxBuffer: 1024 * 1024 * 1024, - env: { ...process.env, ...env, ONSYSTEM_CHANGED_LINES: linesFile }, + env: { ...childEnv, ...configEnv(trust, childEnv), ONSYSTEM_CHANGED_LINES: linesFile }, }); } finally { if (linesDir) fs.rmSync(linesDir, { recursive: true, force: true }); @@ -399,7 +496,15 @@ export function run(io) { } if (!Array.isArray(results)) { if (checked.stdout) io.stdout(checked.stdout.trimEnd()); - const why = checked.error?.message ?? checked.stderr?.trim().split('\n')[0] ?? ''; + // Why it could not check, not which project it checked: run from a folder with no project + // marker, the CLI says that first. + const why = + checked.error?.message ?? + checked.stderr + ?.trim() + .split('\n') + .find((line) => !line.startsWith('Checking the project in ')) ?? + ''; io.stdout( `::error title=onsystem::onsystem check exited with ${checked.status ?? checked.signal}${why ? `: ${data(why)}` : ''}`, ); diff --git a/docs/cli.md b/docs/cli.md index 4f5880a8..65b98ebf 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -20,11 +20,13 @@ It needs Node.js 20.19 or later. The options are the ones `onsystem --help` prin At a monorepo root with no design system of its own, every command works per project: each file is checked against the design system of the nearest folder with a config, a `components.json` or a `package.json`, and `inspect` lists every project ([monorepo roots](configuration.md#monorepo-roots)). +A command given another command's option exits 2: `check . --out report.json` would write no file. `serve` only says on stderr that it ignores the option, and starts, so a stale argument in an agent's MCP config does not leave the agent without the server. + ## Options for every command | Option | What it does | | --------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `--root ` | The project root. By default, the folder of the file `--config` names, or the working directory | +| `--root ` | The project root. By default, the folder of the file `--config` names, or the working directory, or the project around it (below) | | `--config ` | The config file. By default, the first of `onsystem.config.json`, `onsystem.config.ts`, `onsystem.config.mts`, `onsystem.config.js` and `onsystem.config.mjs` in the root; the TypeScript ones need Node.js 22.18 or later ([config file](configuration.md#config-file)) | | `--components ` | Component sources, repeatable. By default, what [zero config](configuration.md#zero-config) finds | | `--tokens ` | A token file, repeatable: a DTCG `.json`, a stylesheet or a `DESIGN.md` ([where tokens come from](tokens.md#where-tokens-come-from)) | @@ -33,6 +35,8 @@ At a monorepo root with no design system of its own, every command works per pro | `-h`, `--help` | Prints the help | | `-v`, `--version` | Prints the version | +Run from a folder with no config, `components.json`, `package.json` or `project.json` (such as `app/settings`), `check`, `inspect`, `export` and `init` take the nearest folder above it with one as the root, as the Claude Code hook does, and `check` says so: `Checking the project in ../..`. They keep the working directory when given `--root`, `--config`, `--components`, `--tokens` or `--docs`, in a folder with a baseline, and in the home folder; the home folder and the filesystem root are never taken. A folder with a `components/ui` or `src/components/ui` folder of its own is kept too, as a design system without a `package.json`, unless it is the project's `src` or `app` folder, or the project has a `components.json` or a config, which say where its components are. `--root .` keeps the working directory. + ## `serve` `onsystem` or `onsystem serve` starts the MCP server on stdio. An agent starts it from its MCP config; typed in a terminal, it says that it is waiting for an agent, and Ctrl+C quits. It answers the handshake right away and reads the design system in the background, then reads it again when the component, token or docs files, the config or the tsconfig change ([how it works](how-it-works.md)). Started at a monorepo root, it serves every project ([how the server finds your project](clients.md#how-the-server-finds-your-project)). @@ -49,12 +53,12 @@ It exits with: - `0` when there are no errors, and no more warnings than `--max-warnings`. - `1` when there are errors, or more warnings than `--max-warnings`. -- `2` when it could not check: an unknown option, no paths given, an unknown `--format`, a `--max-warnings` that is not an integer, a broken config, paths that match no file, a file it could not read, a `--baseline` file that does not exist (unless `--update-baseline` writes it), `--prune-baseline` with no baseline to prune, or, with `--require-design-system`, no design system or nothing to check. +- `2` when it could not check: an unknown option, an option of another command (such as `--out`), no paths given, an unknown `--format`, a `--max-warnings` that is empty (an unset variable, `--max-warnings=`) or not a whole number, a broken config, paths that match no file, a file it could not read, a `--baseline` file that does not exist (unless `--update-baseline` writes it), `--prune-baseline` with no baseline to prune, or, with `--require-design-system`, no design system or nothing to check. | Option | What it does | | ------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `--format ` | `pretty` (the default), `json`, `github` (annotations on the pull request) or `sarif` ([code scanning](ci.md#code-scanning-and-other-sarif-tools)) | -| `--max-warnings ` | Exits 1 when there are more than `n` warnings | +| `--max-warnings ` | Exits 1 when there are more than `n` warnings. `--max-warnings=-1` is no limit, as in ESLint (with `=`: a `-1` on its own reads as an option) | | `--update-baseline` | Records the current findings as accepted in the baseline, and exits 0 ([adopting it in an existing codebase](ci.md#adopting-it-in-an-existing-codebase)) | | `--prune-baseline` | Drops the baseline entries that no longer occur, and accepts nothing new | | `--baseline ` | The baseline file. By default `onsystem.baseline.json` in the root, used whenever it exists | diff --git a/src/cli.ts b/src/cli.ts index a2ae85e1..f472d99f 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'; @@ -28,6 +29,7 @@ import { import { ConfigError, excludedBy, + hasOwnConfig, RULE_IDS, rulesFor, type LoadConfigOptions, @@ -47,7 +49,13 @@ 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, + samePath, + type Project, + type Workspace, +} from './workspace.js'; import { detectProject, type DetectionStep } from './detect.js'; const DOCS = 'https://onsystem.vercel.app/docs'; @@ -75,7 +83,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 @@ -89,6 +100,7 @@ Options 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=-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 +183,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 +215,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 +230,26 @@ 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. + // The server only says so: a stale flag in an MCP client's args (`["onsystem", "-y"]`) must not + // leave the client with a server that does not start. + const ignored = new Set(); + for (const token of tokens) { + if (token.kind !== 'option') continue; + const commands = COMMAND_OPTIONS[token.name]; + if (!commands || !isCommand(command) || commands.includes(command)) continue; + const owners = listOf(commands, commands.length); + if (command !== 'serve') { + io.stderr( + `${command}: ${token.rawName} is an option of ${owners} only. Run ${NAME} --help for usage.`, + ); + return 2; + } + if (!ignored.has(token.rawName)) { + ignored.add(token.rawName); + io.stderr(`serve: ${token.rawName} is an option of ${owners} only, so it is ignored.`); + } + } // `--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 +259,14 @@ 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]; + let walkedUp: string | undefined; + if (root === undefined && command !== 'serve' && given.every((v) => v === undefined)) { + root = walkedUp = enclosingProject(io.cwd); + } const configOptions = { cwd: io.cwd, root, @@ -255,7 +296,7 @@ export async function main(argv: string[], io: Io = defaultIo): Promise }); return 0; case 'check': - return await check(rest, configOptions, values, io); + return await check(rest, configOptions, values, io, walkedUp); case 'inspect': return await inspect(configOptions, values, io); case 'export': @@ -286,6 +327,13 @@ 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. On a line of its own: the cursor is still on the + // question's line. + if (command === 'init' && (error as Error).name === 'AbortError') { + io.stderr('\ninit: 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 +341,67 @@ 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), when `cwd` is the home folder, or when no folder up to the home folder + * or the filesystem root, neither of which counts, has one. Undefined, too, when `cwd` has a + * folder zero config reads components from (a design system with no package.json) and the + * project above would not read it: `cwd` is not the project's `src` or `app` folder, which hold + * its own code, and the project has no components.json or config to say where its components are. + */ +function enclosingProject(cwd: string): string | undefined { + const has = (dir: string, name: string) => fs.existsSync(path.join(dir, name)); + if (hasProjectMarker(cwd) || [BASELINE_FILE, LEGACY_BASELINE_FILE].some((f) => has(cwd, f))) { + return undefined; + } + // A stray ~/package.json is not the project, as for findProjectRoot. + const home = os.homedir(); + const never = (dir: string) => samePath(dir, home) || dir === path.dirname(dir); + if (samePath(cwd, home)) return undefined; + let above = path.dirname(cwd); + while (!never(above) && !hasProjectMarker(above)) above = path.dirname(above); + if (never(above)) return undefined; + const ownComponents = ['components/ui', 'src/components/ui'].some((f) => has(cwd, f)); + const readAbove = + ['src', 'app'].some((name) => samePath(cwd, path.join(above, name))) || + has(above, 'components.json') || + hasOwnConfig(above); + if (ownComponents && !readAbove) return undefined; + return path.relative(cwd, above); +} + interface CheckValues { cache: boolean; format?: string | undefined; @@ -338,11 +447,16 @@ interface NoMatch { jsxGlobs: string[]; } +/** + * `walkedUp`: the root, relative to the working directory, when the run took the project around + * the working directory as its root (`enclosingProject`). + */ async function check( patterns: string[], configOptions: LoadConfigOptions, values: CheckValues, io: Io, + walkedUp?: string, ): Promise { if (values['quiet-without-design-system'] && values['require-design-system']) { io.stderr('check: pass --quiet-without-design-system or --require-design-system, not both'); @@ -363,19 +477,31 @@ 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 (--max-warnings=-1 for no limit), not "${given}"`, + ); return 2; } + const maxWarnings = given === undefined || Number(given) < 0 ? undefined : Number(given); + // Which project a run from inside one checks, once: on stderr when stdout is JSON, SARIF or + // workflow commands. Not for a hook, which prints nothing when there is nothing to say. + const where = walkedUp === undefined ? undefined : toPosix(walkedUp); + if (where !== undefined && !values['quiet-without-design-system']) { + const text = `Checking the project in ${where}`; + if (format === 'pretty') io.stdout(text); + else io.stderr(text); + } const target = await loadTarget(configOptions); warnDeprecated(target.config.deprecations, format, io); warnDeprecated(configWarnings(target), format, io); const selection = target.workspace ? await selectInWorkspace(target.workspace, patterns, values, format, io) - : await selectInProject(target.config, patterns, values, format, io); + : await selectInProject(target.config, patterns, values, format, io, where); if (typeof selection === 'number') return selection; // The hints repeat the options that decide the findings and the baseline, so the command they // suggest rewrites the same file from the same findings. @@ -844,13 +970,17 @@ function jsxNotice( : `${files.length.toLocaleString('en-US')} .js files have JSX (${listOf(files)}), but folders are checked for .tsx and .jsx files, so they were not checked. Name them to check them: \`${check}\`.`; } -/** One project: the root's config and design system for every file. */ +/** + * One project: the root's config and design system for every file. `where`: the root as the + * notices name it, when the run is not from there. + */ async function selectInProject( config: ResolvedConfig, patterns: string[], values: CheckValues, format: OutputFormat, io: Io, + where?: string, ): Promise { const ds = await loadDesignSystem(config, { cache: values.cache, logger: silentLogger }); // Folders and globs leave out what the config excludes (tests and stories by default) and @@ -906,7 +1036,7 @@ async function selectInProject( // A run that found no components or no color tokens passes without having // checked them: say so, and fail in CI when asked to, unless the components come from a // published library (MUI), whose colors may be in Sass: then it only says so. - const notice = ds.notice(); + const notice = ds.notice(where); if (notice && values['require-design-system'] && !ds.hasDesignSystem()) { io.stderr(`check --require-design-system: ${notice}`); for (const text of notReadNotices([{ ds }])) io.stderr(text); @@ -967,6 +1097,9 @@ async function selectInWorkspace( if (!found) throw new Error(`No project for ${file}`); return found; }; + // A project's folder as the notices name it: relative to the working directory, as the files + // are, so a run from a folder below the root does not call the root `.`. + const shown = (p: Project) => relativePath(io.cwd, p.root) || '.'; const designSystems = new Map>(); const load = (p: Project) => { let ds = designSystems.get(p.root); @@ -1058,8 +1191,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) ?? []), shown(p)]); + } + 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; } @@ -1079,13 +1219,13 @@ async function selectInWorkspace( const notice = ds?.notice(); if (!ds || !notice) continue; const gap = gaps.get(notice) ?? { ds, dirs: [] }; - gap.dirs.push(p.dir); + gap.dirs.push(shown(p)); gaps.set(notice, gap); } const notices = [...gaps.values()].map(({ ds, dirs }) => ds.notice(listOf(dirs)) ?? ''); const checkedProjects = unique(files.map(project)).flatMap((p) => { const ds = loaded.get(p.root); - return ds ? [{ ds, dir: p.dir }] : []; + return ds ? [{ ds, dir: shown(p) }] : []; }); notices.push(...notReadNotices(checkedProjects)); const used = unique(files.map((file) => project(file).root)).length; @@ -1155,6 +1295,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 +1311,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 +1474,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 +1489,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/init.ts b/src/init.ts index 695d2061..a3600a2e 100644 --- a/src/init.ts +++ b/src/init.ts @@ -328,7 +328,10 @@ export async function init( ['check', '.', '--update-baseline', ...(options.cache ? [] : ['--no-cache'])], checkIo, ); - if (recorded.stdout) io.stdout(recorded.stdout); + // `check` ran in the root: the file it names is relative to the root, and is named here + // relative to the working directory, as the config file is (`../../onsystem.baseline.json`). + const shown = relativePath(io.cwd, path.join(root, BASELINE_FILE)) || BASELINE_FILE; + if (recorded.stdout) io.stdout(recorded.stdout.replace(` → ${BASELINE_FILE}`, ` → ${shown}`)); baselineWritten = fs.existsSync(path.join(root, BASELINE_FILE)); } else { io.stdout(`No baseline written; \`${NAME} check . --update-baseline\` records one later.`); 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/src/workspace.ts b/src/workspace.ts index e3d144ef..f785edf6 100644 --- a/src/workspace.ts +++ b/src/workspace.ts @@ -270,7 +270,8 @@ export function findProjectRoot(target: string, home = os.homedir()): string | u return dir; } -function samePath(a: string, b: string): boolean { +/** Whether `a` and `b` are the same folder, as the platform compares paths (case-insensitive on Windows). */ +export function samePath(a: string, b: string): boolean { return path.relative(path.resolve(a), path.resolve(b)) === ''; } diff --git a/test/action.test.ts b/test/action.test.ts index 6335e006..18a8dd4c 100644 --- a/test/action.test.ts +++ b/test/action.test.ts @@ -3,27 +3,38 @@ 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 { annotation, cli, configEnv, onChangedLines, parseDiff, run } from '../action/check.mjs'; import { fixture } from './helpers.js'; // The GitHub Action (action.yml, action/check.mjs) against real git repositories, with a stand-in // for the CLI that reports `bg-[#…]` as an error and `p-[13px]` as a warning on whatever line has -// them. CI runs the action itself, with the built CLI, on the pull request (the `action` job). +// them, and leaves out what git ignores, as the CLI does. CI runs the action itself, with the +// built CLI, on the pull request (the `action` job). -const STUB = `import fs from 'node:fs'; +const STUB = `import { spawnSync } from 'node:child_process'; +import fs from 'node:fs'; import path from 'node:path'; const args = process.argv.slice(2); if (process.env.STUB_ARGS) fs.writeFileSync(process.env.STUB_ARGS, JSON.stringify(args)); if (process.env.STUB_FAIL) { + // Run from a folder with no package.json, the CLI first says which project above it it checks. + let up = process.cwd(); + while (!fs.existsSync(path.join(up, 'package.json')) && path.dirname(up) !== up) up = path.dirname(up); + if (up !== process.cwd() && fs.existsSync(path.join(up, 'package.json'))) + console.error('Checking the project in ' + path.relative(process.cwd(), up).split(path.sep).join('/')); console.error('check --require-design-system: no components were found.'); process.exit(2); } const walk = (dir) => fs.readdirSync(dir, { withFileTypes: true }).flatMap((entry) => entry.isDirectory() ? walk(path.join(dir, entry.name)) : [path.join(dir, entry.name)]); +const found = walk(args[1]).filter((f) => f.endsWith('.tsx')).sort(); +const ignored = spawnSync('git', ['check-ignore', '-z', '--stdin'], { + input: found.map((f) => f.split(path.sep).join('/')).join('\\0'), encoding: 'utf8' }); +const skip = new Set(ignored.status === 0 ? ignored.stdout.split('\\0').filter(Boolean).map((f) => path.resolve(f)) : []); const results = []; -for (const file of walk(args[1]).filter((f) => f.endsWith('.tsx')).sort()) { +for (const file of found.filter((f) => !skip.has(path.resolve(f)))) { const diagnostics = []; fs.readFileSync(file, 'utf8').split('\\n').forEach((text, i) => { for (const [needle, ruleId, severity] of [ @@ -202,6 +213,55 @@ 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("passes git's -c options to the CLI as GIT_CONFIG_* variables, after those already set", () => { + const trust = ['-c', 'safe.directory=/w/repo/apps', '-c', 'safe.directory=/w/repo']; + expect(configEnv(trust, { GIT_CONFIG_COUNT: '2' })).toEqual({ + GIT_CONFIG_KEY_2: 'safe.directory', + GIT_CONFIG_VALUE_2: '/w/repo/apps', + GIT_CONFIG_KEY_3: 'safe.directory', + GIT_CONFIG_VALUE_3: '/w/repo', + GIT_CONFIG_COUNT: '4', + }); + expect(configEnv(['-c', 'a.b=c=d'], {})).toEqual({ + GIT_CONFIG_KEY_0: 'a.b', + GIT_CONFIG_VALUE_0: 'c=d', + GIT_CONFIG_COUNT: '1', + }); + // Outside Actions there is nothing to add, and the count is left as it is. + expect(configEnv([], { GIT_CONFIG_COUNT: '2' })).toEqual({}); + }); + it('keeps the findings that overlap a changed range', () => { const finding = (line: number, endLine = line) => ({ file: 'a.tsx', @@ -370,6 +430,43 @@ 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'); + // Files git ignores, each with an error the CLI must not report: one through .gitignore, and + // one through an excludes file set in the environment, which the action adds to. + fs.writeFileSync(path.join(web, '.gitignore'), 'app/generated.tsx\n'); + fs.writeFileSync(path.join(web, 'app/generated.tsx'), PAGE); + fs.writeFileSync(path.join(web, 'app/local.tsx'), PAGE); + const excludes = path.join(fixture({ excludes: 'local.tsx\n' }), 'excludes'); + vi.stubEnv('GIT_TEST_ASSUME_DIFFERENT_OWNER', '1'); + vi.stubEnv('GIT_CONFIG_NOSYSTEM', '1'); + vi.stubEnv('GIT_CONFIG_GLOBAL', path.join(fixture({ gitconfig: '' }), 'gitconfig')); + vi.stubEnv('GIT_CONFIG_COUNT', '1'); + vi.stubEnv('GIT_CONFIG_KEY_0', 'core.excludesFile'); + vi.stubEnv('GIT_CONFIG_VALUE_0', excludes); + 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,'); + // The CLI's own git reads the checkout too: neither ignored file is checked. + expect(result.stdout).not.toContain('generated.tsx'); + expect(result.stdout).not.toContain('local.tsx'); + 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 }); @@ -424,4 +521,17 @@ describe('the GitHub Action', () => { '::error title=onsystem::onsystem check exited with 2: check --require-design-system: no components were found.', ); }); + + it('fails with why check could not run, not which project it checked', () => { + // From a working-directory with no project marker, the CLI checks the project above it and + // says so first: the annotation took that line for the reason. + const { root, base } = repository(); + fs.writeFileSync(path.join(root, 'package.json'), '{ "name": "app" }'); + const result = action(path.join(root, 'apps/web'), { payload: pr(base) }, { STUB_FAIL: '1' }); + expect(result.code).toBe(1); + expect(result.stderr).toContain('Checking the project in ../..'); + expect(result.stdout).toBe( + '::error title=onsystem::onsystem check exited with 2: check --require-design-system: no components were found.', + ); + }); }); diff --git a/test/cli-docs.test.ts b/test/cli-docs.test.ts index f69ee0c8..875893a4 100644 --- a/test/cli-docs.test.ts +++ b/test/cli-docs.test.ts @@ -1,13 +1,14 @@ 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'; import { main, type Io } from '../src/cli.js'; import { CONFIG_FILES } from '../src/config.js'; -import { DEMO_ROOT } from './helpers.js'; +import { DEMO_ROOT, fixture } from './helpers.js'; import { section } from './markdown.js'; // docs/cli.md is the CLI reference, and the options it lists are the ones `--help` prints and the @@ -208,6 +209,45 @@ 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. `serve` says it ignores + // the option and starts, as an MCP client that passes a stale one needs it to. + let tried = 0; + // No project here: the server that starts reads nothing until a client asks. + const empty = fixture({}); + for (const option of options) { + if (!option.commands.length) continue; + const owners = option.commands.join(', ').replace(/, (\w+)$/, ' and $1'); + for (const command of COMMANDS.filter((c) => !option.commands.includes(c))) { + for (const flag of option.flags) { + const err: string[] = []; + const io: Io = { + cwd: command === 'serve' ? empty : DEMO_ROOT, + color: false, + stdout: () => {}, + stderr: (t) => err.push(t), + // `serve` starts here, and not on stdin. + transport: InMemoryTransport.createLinkedPair()[1], + }; + const args = [command, flag, ...(option.arg ? ['x'] : [])]; + if (command === 'serve') { + expect(await main(args, io), args.join(' ')).toBe(0); + expect(err.join('\n'), args.join(' ')).toBe( + `serve: ${flag} is an option of ${owners} only, so it is ignored.`, + ); + } else { + expect(await main(args, io), args.join(' ')).toBe(2); + expect(err.join('\n'), args.join(' ')).toBe( + `${command}: ${flag} is an option of ${owners} 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..1d33e1b8 --- /dev/null +++ b/test/cli-edges.test.ts @@ -0,0 +1,400 @@ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import readline from 'node:readline/promises'; +import { PassThrough } from 'node:stream'; + +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; +import { afterEach, describe, expect, it, vi } 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. + +afterEach(() => { + vi.restoreAllMocks(); +}); + +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') }; +} + +/** + * The demo's files with its design system under `dir` (`src/components/ui`), its stylesheet at + * `css` and its settings pages in `pages`, named through `alias` (`@/components/ui`) by + * components.json and tsconfig.json, as a Next.js project with a src folder, or a React Router + * one with app, has them. + */ +function demoUnder(layout: { + dir: string; + alias: string; + css: string; + pages: string; +}): Record { + const { dir, alias, css, pages } = layout; + const read = (file: string) => + fs.readFileSync(path.join(DEMO_ROOT, file), 'utf8').replaceAll('"@/', `"${alias}/`); + const files: Record = { + 'package.json': read('package.json'), + 'components.json': read('components.json').replace('"app/globals.css"', `"${css}"`), + 'tsconfig.json': JSON.stringify({ + compilerOptions: { + jsx: 'react-jsx', + strict: true, + paths: { [`${alias}/*`]: [`./${dir}/*`] }, + }, + }), + [css]: read('app/globals.css'), + [`${dir}/lib/utils.ts`]: read('lib/utils.ts'), + [`${pages}/danger-zone.tsx`]: read('app/settings/danger-zone.tsx'), + }; + for (const name of fs.readdirSync(path.join(DEMO_ROOT, 'components/ui'))) { + files[`${dir}/components/ui/${name}`] = read(`components/ui/${name}`); + } + return files; +} + +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 (--max-warnings=-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.', + ); + expect((await run(['inspect', '-y'])).stderr).toBe( + 'inspect: -y is an option of init only. Run onsystem --help for usage.', + ); + }); + + it('are named and ignored by the server, which still starts', async () => { + // A stale flag in an MCP client's args (`npx onsystem -y`) left the client with a server + // that exited 2 at once. + const [clientSide, serverSide] = InMemoryTransport.createLinkedPair(); + const err: string[] = []; + const io: Io = { + cwd: DEMO_ROOT, + color: false, + stdout: () => {}, + stderr: (t) => err.push(t), + transport: serverSide, + }; + const args = ['-y', '--format', 'json', '--format=sarif', '--no-watch', '--no-cache']; + expect(await main(args, io)).toBe(0); + expect(err).toEqual([ + 'serve: -y is an option of init only, so it is ignored.', + 'serve: --format is an option of check, inspect and export only, so it is ignored.', + ]); + const client = new Client({ name: 'test', version: '1.0.0' }); + await client.connect(clientSide); + try { + const { tools } = await client.listTools(); + expect(tools.map((t) => t.name)).toContain('check_ui'); + } finally { + await client.close(); + } + }); + + 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. The cursor is still on the + // question's line: the message starts a line of its own. + const root = fixture({ 'package.json': '{ "name": "app" }' }); + const { code, stderr } = await run(['init'], root, ctrlD); + expect(stderr).toBe('\ninit: 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'); + // Which project, on stderr, so the JSON on stdout still parses. + expect(inside.stderr).toBe('Checking the project in ../..'); + expect(inside.code).toBe(1); + const pretty = await run(['check', 'danger-zone.tsx', '--no-cache'], settings); + expect(pretty.stdout.split('\n')[0]).toBe('Checking the project in ../..'); + expect(pretty.stdout.match(/Checking the project/g)).toHaveLength(1); + // Not from the root, nor for a hook, which prints nothing when there is nothing to say. + expect(fromRoot.stderr).toBe(''); + const hook = await run( + ['check', 'danger-zone.tsx', '--no-cache', '--quiet-without-design-system'], + settings, + ); + expect(hook.stdout).not.toContain('Checking the project'); + + 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