From 64a92cb2669864c9c74e68d03b4528371ad1caca Mon Sep 17 00:00:00 2001 From: Diogo Esteves Date: Fri, 16 Oct 2026 09:21:31 +0100 Subject: [PATCH 1/2] fix(hook): check the files a Codex apply_patch changes, and offer an AGENTS.md section in init Codex installs the Claude Code plugin and runs its hook on apply_patch, which matches Write|Edit, but the patch is in tool_input.command and the hook read only tool_input.file_path, so it exited 0 on every Codex edit. The hook now reads the files a patch adds, updates or moves, checks each .tsx or .jsx file in one run per project, and holds an updated file to the lines its hunks change. It also reads path or filePath when the tool's input carries the text it wrote. init, in a terminal, offers to add a marked onsystem section to AGENTS.md or CLAUDE.md, with the MCP and CLI forms, and updates it in place on later runs. OS2-38 --- .changeset/codex-apply-patch-hook.md | 7 + README.md | 2 +- docs/clients.md | 8 +- docs/plugin.md | 1 + plugins/onsystem/hooks/check-ui.mjs | 419 ++++++++++++++++++--------- src/init.ts | 82 +++++- test/hook-apply-patch.test.ts | 279 ++++++++++++++++++ test/init-agents-md.test.ts | 130 +++++++++ 8 files changed, 786 insertions(+), 142 deletions(-) create mode 100644 .changeset/codex-apply-patch-hook.md create mode 100644 test/hook-apply-patch.test.ts create mode 100644 test/init-agents-md.test.ts diff --git a/.changeset/codex-apply-patch-hook.md b/.changeset/codex-apply-patch-hook.md new file mode 100644 index 00000000..eef71647 --- /dev/null +++ b/.changeset/codex-apply-patch-hook.md @@ -0,0 +1,7 @@ +--- +'onsystem': patch +--- + +The plugin's hook now checks edits in Codex. Codex installs the Claude Code plugin from the same marketplace and runs its hook on `apply_patch`, but the hook read only `tool_input.file_path`, and Codex puts the patch in `tool_input.command`, so every edit passed unchecked. The hook now reads the files a patch adds or updates from its `*** Add File:`, `*** Update File:` and `*** Move to:` lines, checks each `.tsx` or `.jsx` file in one run per project, and holds an updated file to the lines its hunks change, as it holds an Edit. It also reads the file from `path` or `filePath`, as other clients' tools name it, when the tool's input carries the text it wrote. Codex runs a plugin's hooks once you trust them in `/hooks`. + +`init`, run in a terminal, offers to add a short section to the project's `AGENTS.md` (create-next-app writes one) or `CLAUDE.md`: use the onsystem tools before writing UI, and run `check_ui`, or `npx -y onsystem check ` without the tools, on every UI file changed. It sits between `` and ``, which a later run updates in place. A `CLAUDE.md` that imports `AGENTS.md` is left alone, and `--yes` never writes it. diff --git a/README.md b/README.md index 36a83fb0..12ac9c52 100644 --- a/README.md +++ b/README.md @@ -85,7 +85,7 @@ In a new create-next-app, the template page's hardcoded colors already make the /plugin install onsystem@dgesteves ``` -After each Write or Edit of a `.tsx` or `.jsx` file, the hook runs `onsystem check` on it. Errors go back to Claude as a blocking error with the fix, so Claude corrects the file before it moves on; warnings go as a note. It stays quiet in projects without a design system. [What it runs and touches](https://onsystem.vercel.app/docs/plugin). +After each Write or Edit of a `.tsx` or `.jsx` file, the hook runs `onsystem check` on it. Errors go back to Claude as a blocking error with the fix, so Claude corrects the file before it moves on; warnings go as a note. It stays quiet in projects without a design system. Codex installs the same plugin, and the hook checks the files its `apply_patch` changes. [What it runs and touches](https://onsystem.vercel.app/docs/plugin). **Cursor or VS Code.** [![Install in Cursor](https://img.shields.io/badge/Cursor-Install_server-0d0f12?style=flat-square&logo=cursor&logoColor=white)](https://cursor.com/en/install-mcp?name=onsystem&config=eyJjb21tYW5kIjoibnB4IiwiYXJncyI6WyIteSIsIm9uc3lzdGVtIiwiLS1yb290IiwiJHt3b3Jrc3BhY2VGb2xkZXJ9Il19) [![Install in VS Code](https://img.shields.io/badge/VS_Code-Install_server-007ACC?style=flat-square)](https://insiders.vscode.dev/redirect/mcp/install?name=onsystem&config=%7B%22command%22%3A%22npx%22%2C%22args%22%3A%5B%22-y%22%2C%22onsystem%22%2C%22--root%22%2C%22%24%7BworkspaceFolder%7D%22%5D%7D), or commit the [config](https://onsystem.vercel.app/docs/clients#cursor-and-vs-code). The agent gets the tools and runs `check_ui` on what it writes. diff --git a/docs/clients.md b/docs/clients.md index 3dc557e1..6992566e 100644 --- a/docs/clients.md +++ b/docs/clients.md @@ -2,11 +2,11 @@ The server speaks MCP over stdio, in both protocol eras: the 2026-07-28 revision (stateless, with `server/discover`) and the 2025 one (the `initialize` handshake), the era chosen by the client's first message. It finds the project from `--root`, a config file in the working directory, or the working directory itself when it holds a project (a `package.json` or `components.json`). MCP roots, which 2026-07-28 deprecates, are the fallback: when the working directory is none of those (Claude Desktop starts servers in `/`), a 2025-era client that reports roots has its first `file://` root used. When none of these gives a project (a client that starts servers in your home or the plugin's folder and reports no roots), each tool call's project is found from the absolute path it names, `path` (or `filename`, for `check_ui` with `code`): the nearest folder at or above it with a `package.json`, `components.json` or config, or the monorepo root above that, but never your home folder or the filesystem root. A project found this way loads a JSON config (`onsystem.config.json`) only: the server runs a JavaScript or TypeScript config only in a project it was started in. Until a call names one, the tools return an error that asks for it; after that, a call without one answers from the project found, as long as only one has been. Started at a [monorepo root](configuration.md#monorepo-roots), it serves every app and design-system package in it: the tools take the `path` of the file the agent is editing and answer for that file's project. -Every client gets the same five tools, including `check_ui`, which the agent runs on its own output. Only Claude Code has a hook that checks each edit as it happens, so there use the [plugin](plugin.md) instead; with other agents, add the [CI check](ci.md) so what the agent skips still gets caught. +Every client gets the same five tools, including `check_ui`, which the agent runs on its own output. Claude Code and Codex CLI also run the [plugin](plugin.md)'s hook, which checks each edit as it happens, so there use the plugin instead; with other agents, add the [CI check](ci.md) so what the agent skips still gets caught. ## Plugins for other agents -The skill and the MCP server also come packaged for other agents, pinned to the same release as the Claude Code plugin. Only the Claude Code plugin has the hook that checks each edit; elsewhere, the [CI check](ci.md) catches what the agent skips. +The skill and the MCP server also come packaged for other agents, pinned to the same release as the Claude Code plugin. Only the Claude Code plugin has the hook that checks each edit, and Codex runs it too; elsewhere, the [CI check](ci.md) catches what the agent skips. - **Any agent with Agent Skills** (Claude Code, Codex, Cursor, GitHub Copilot, Gemini CLI, OpenCode and others): the skill, from the repository. It goes to `.agents/skills/onsystem`, which most agents read, and is linked into `.claude/skills` for Claude Code. Add the MCP server with your client's config below. @@ -14,7 +14,7 @@ The skill and the MCP server also come packaged for other agents, pinned to the npx skills add dgesteves/onsystem ``` -- **Codex CLI:** Codex reads the same marketplace as Claude Code and installs that plugin's server and skill. It also lists the plugin's hook, which is written for Claude Code's `Write` and `Edit` tools. +- **Codex CLI:** Codex reads the same marketplace as Claude Code and installs that plugin: the server, the skill and the hook, which checks each `.tsx` or `.jsx` file an `apply_patch` adds or updates. Codex runs a plugin's hooks only after you review and trust them: open `/hooks` once after installing. ```sh codex plugin marketplace add dgesteves/onsystem @@ -266,4 +266,4 @@ To share it with your team, add `--scope project`, which writes `.mcp.json` at t Any other MCP client: run `npx -y onsystem --root /absolute/path/to/app` as a stdio server. On native Windows, wrap it as `cmd /c npx ...`. -The server sends usage instructions in its `initialize` result and in `server/discover`. Clients that ignore them benefit from one line in `CLAUDE.md`, `AGENTS.md` or `.cursor/rules`: _"Before writing UI, use the onsystem tools. Run check_ui on every file you change and fix all errors."_ +The server sends usage instructions in its `initialize` result and in `server/discover`. Clients that ignore them benefit from one line in `CLAUDE.md`, `AGENTS.md` or `.cursor/rules`: _"Before writing UI, use the onsystem tools. Run check_ui on every file you change and fix all errors."_ Run in a terminal, `onsystem init` offers to add a short section that says so to the project's `AGENTS.md` or `CLAUDE.md`, with `npx -y onsystem check ` for agents without the tools. It sits between `` and ``, which a later run updates in place. diff --git a/docs/plugin.md b/docs/plugin.md index e856f3b1..74052190 100644 --- a/docs/plugin.md +++ b/docs/plugin.md @@ -33,6 +33,7 @@ The plugin is the fastest way to keep Claude on your design system. It bundles t - **The design system's own files are left alone.** Like `check`, it skips them: an edit to `components/ui/button.tsx` changes the design system, which is a call for you and your reviewers rather than a lint error. - **So is what `check .` leaves out.** A file the config's `exclude` covers, tests and stories by default (`*.test.tsx`, `*.stories.tsx`, `__tests__` folders) or a folder you list such as `app/legacy`, is not checked: the hook runs `check --skip-excluded`, which leaves such a file out even though it is named. Add a folder to `exclude` to keep the hook off code you don't hold to the design system. A project's own install from before that flag checks the file, as it did before. - **It says when it can't check.** When the CLI can't run (npx can't fetch it because of a [min-release-age policy](troubleshooting.md#a-min-release-age-policy-blocks-npx), a private registry or no network, or the config is broken), it says so once per session and project, as a message to you, and never blocks the edit. When npm could not reach the registry, or npx didn't finish within the hook's 55 seconds, the hook doesn't start npx again for ten minutes, so an offline session waits for npm once, not on every edit. +- **Codex runs it too.** Codex installs this plugin from the same marketplace (`codex plugin add onsystem@dgesteves`) and runs the hook after each `apply_patch`, once you trust it in `/hooks`. It checks every `.tsx` or `.jsx` file the patch adds or updates, a moved file where it moved to, and holds an updated file to the lines its hunks change, as it holds an Edit. A patch's files are checked in one run per project. - **Monorepos work.** The hook finds the project from the edited file, not from where Claude started, and the MCP server started at the root serves every project, answering for the file's own project when the tools get its `path`. When the monorepo root has a config or a baseline, as `init` there writes them, the hook runs the check from the root, as `check .` there does: the root config's rules apply to every project without a config of its own, and the root baseline covers every project. Since every project without a config of its own takes the root config on, the hook also checks a project without design-system components there, as `check .` at the root does (with neither components nor color tokens, only the accessibility rule runs in it). A project with a baseline of its own, as a CI job per app keeps, is checked from its own folder, and so is every project when the root is one project with a design system of its own (its config sets `components`, or it has a `components.json` or a `components/ui` folder), so each project's own components are still its design system. ## What it runs, and what it touches diff --git a/plugins/onsystem/hooks/check-ui.mjs b/plugins/onsystem/hooks/check-ui.mjs index 9c16b3c9..0f701f1e 100644 --- a/plugins/onsystem/hooks/check-ui.mjs +++ b/plugins/onsystem/hooks/check-ui.mjs @@ -2,11 +2,17 @@ // Claude Code PostToolUse hook: after Claude writes or edits a .tsx/.jsx file, // run `onsystem check` on it and hand the findings back. // +// Codex runs it too, from the same plugin: its `apply_patch` matches `Write|Edit`, +// and carries the patch in `tool_input.command`. Every .tsx/.jsx file the patch +// adds or updates is checked, in one run per project, a moved file where it moved +// to. Other clients' tools that name the file `path` or `filePath` are read too, +// when their input carries the text they wrote. +// // - Errors: exit 2 with the findings on stderr, which Claude Code shows to -// Claude so it fixes them before moving on. For an Edit, only findings on -// the lines it changed are listed; older ones are counted, not pushed. -// When the hook cannot tell which lines an Edit changed, the findings it -// may have caused go back as a non-blocking note instead. +// Claude so it fixes them before moving on. For an Edit, or a patch's hunks, +// only findings on the lines it changed are listed; older ones are counted, +// not pushed. When the hook cannot tell which lines an Edit changed, the +// findings it may have caused go back as a non-blocking note instead. // - Warnings only: exit 0 with `additionalContext`, a non-blocking note. // - Clean file, other files, a file `check .` leaves out (what the config's // `exclude` covers, tests and stories by default: `check --skip-excluded`), or a @@ -94,128 +100,261 @@ const PRIMITIVES = const BIN = process.env.ONSYSTEM_BIN || process.env.DESIGN_SYSTEM_MCP_BIN || undefined; const input = await readInput(); -const filePath = input?.tool_input?.file_path; -if (typeof filePath !== 'string' || !UI_FILE.test(filePath)) process.exit(0); - const sessionDir = - typeof input.cwd === 'string' && input.cwd + typeof input?.cwd === 'string' && input.cwd ? input.cwd : (process.env.CLAUDE_PROJECT_DIR ?? process.cwd()); -const file = path.resolve(sessionDir, filePath); -const project = projectRoot(path.dirname(file)); -// Where `check` runs: the project's folder, or the monorepo root whose config or baseline covers it. -const root = (project && monorepoRoot(project)) ?? project ?? sessionDir; -// Forward slashes, as the CLI prints paths, on Windows too. -const name = path.relative(root, file).split(path.sep).join('/') || file; +// The .tsx and .jsx files the tool wrote, each with the edits it made there. +const targets = editedFiles(input).filter((t) => UI_FILE.test(t.path)); +if (!targets.length) process.exit(0); + +// One run of `check` per folder it runs from and CLI, with the files it checks there. +const jobs = new Map(); +for (const target of targets) { + const file = path.resolve(sessionDir, target.path); + const project = projectRoot(path.dirname(file)); + // Where `check` runs: the project's folder, or the monorepo root whose config or baseline covers it. + const root = (project && monorepoRoot(project)) ?? project ?? sessionDir; + // The project's own install, which may be in the project's node_modules rather than the root's. + const cli = resolveCli(project ?? root); + const key = `${root}\0${cli.argv?.join('\0')}`; + const job = jobs.get(key) ?? { root, cli, files: [] }; + jobs.set(key, job); + // Forward slashes, as the CLI prints paths, on Windows too. + const name = path.relative(root, file).split(path.sep).join('/') || file; + if (!job.files.some((f) => f.file === file)) job.files.push({ ...target, file, name }); +} if (!process.env.ONSYSTEM_BIN && process.env.DESIGN_SYSTEM_MCP_BIN) { // On stderr, the debug log, and only when the hook passes: a blocked edit's stderr is Claude's. const text = 'DESIGN_SYSTEM_MCP_BIN is the name ONSYSTEM_BIN had before the rename: rename it.'; + const [{ root }] = jobs.values(); process.on('exit', (code) => { - if (code === 0) once('env-notice', text, () => fs.writeSync(2, `${text}\n`)); + if (code === 0) once('env-notice', root, text, () => fs.writeSync(2, `${text}\n`)); }); } -// The project's own install, which may be in the project's node_modules rather than the root's. -const cli = resolveCli(project ?? root); -if (!cli.argv) cannotRun('npx was not found next to this Node.js.'); -// npm could not reach the registry, or npx ran out of time, less than OFFLINE_PAUSE ago: the -// notice has been shown, and npx would wait as long again. -const pause = markerPath('offline'); -if (cli.npx && paused()) process.exit(0); const deadline = Date.now() + TIMEOUT; -let run = check(cli.npx ? ['--prefer-offline'] : []); -let results = resultsOf(run); -// A run from the cache can fail where the registry would not: npm's cached list of versions can -// predate this release, after the plugin moves to it. When npm itself failed, then, once more with -// the registry, unless npm could not reach it, which a second run would not change. When the CLI -// ran and failed (a broken config, a crash), npm did its part: the registry would not change that. -if (results === undefined && cli.npx && npmFailed(run) && !unreachable(run)) { - run = check(['--prefer-online']); - results = resultsOf(run); -} -// Anything else (a config error, npx unable to fetch the package) is not Claude's to fix: tell -// the user once, and move on. -if (results === undefined) { - // The next edit would wait as long: npx is paused. - if (cli.npx && (timedOut(run) || unreachable(run))) { - try { - fs.mkdirSync(path.dirname(pause), { recursive: true }); - fs.writeFileSync(pause, ''); - } catch { - // Without the marker the next edit waits for npm again; it still never blocks. +// What each run found: errors that block, notes that don't, and notices for the user. +const blocks = []; +const notes = []; +const notices = []; +for (const job of jobs.values()) checkFiles(job); + +if (blocks.length) { + // Notes on the other files go with the errors: after exit 2, only stderr reaches the agent. + process.stderr.write(`${[...blocks, ...notes].join('\n\n')}\n`); + process.exit(2); +} +const output = {}; +for (const { root, text } of notices) { + once('hook-notice', root, text, () => { + process.stderr.write(`${text}\n`); + output.systemMessage = output.systemMessage ? `${output.systemMessage}\n${text}` : text; + }); +} +if (notes.length) { + output.hookSpecificOutput = { + hookEventName: 'PostToolUse', + additionalContext: notes.join('\n\n'), + }; +} +if (Object.keys(output).length) process.stdout.write(JSON.stringify(output)); +process.exit(0); + +/** Runs `check` on a job's files, and adds what it found to `blocks`, `notes` and `notices`. */ +function checkFiles(job) { + const { root, cli, files } = job; + if (!cli.argv) return cannotRun(job, 'npx was not found next to this Node.js.'); + // npm could not reach the registry, or npx ran out of time, less than OFFLINE_PAUSE ago: the + // notice has been shown, and npx would wait as long again. + const pause = markerPath('offline', root); + if (cli.npx && paused(pause)) return; + + let run = check(job, cli.npx ? ['--prefer-offline'] : []); + let results = resultsOf(run); + // A run from the cache can fail where the registry would not: npm's cached list of versions can + // predate this release, after the plugin moves to it. When npm itself failed, then, once more with + // the registry, unless npm could not reach it, which a second run would not change. When the CLI + // ran and failed (a broken config, a crash), npm did its part: the registry would not change that. + if (results === undefined && cli.npx && npmFailed(run) && !unreachable(run)) { + run = check(job, ['--prefer-online']); + results = resultsOf(run); + } + // Anything else (a config error, npx unable to fetch the package) is not Claude's to fix: tell + // the user once, and move on. + if (results === undefined) { + // The next edit would wait as long: npx is paused. + if (cli.npx && (timedOut(run) || unreachable(run))) { + try { + fs.mkdirSync(path.dirname(pause), { recursive: true }); + fs.writeFileSync(pause, ''); + } catch { + // Without the marker the next edit waits for npm again; it still never blocks. + } } + return cannotRun(job, failure(cli, run)); + } + if (!Array.isArray(results)) return; + for (const target of files) { + // A run of several files names each result's file relative to the folder it ran in. + const own = + files.length === 1 + ? results + : results.filter( + (r) => typeof r?.file === 'string' && path.resolve(root, r.file) === target.file, + ); + report( + target, + own.flatMap((r) => (Array.isArray(r?.diagnostics) ? r.diagnostics : [])), + ); } - cannotRun(failure(run)); -} -if (!Array.isArray(results)) process.exit(0); -const diagnostics = results.flatMap((r) => (Array.isArray(r?.diagnostics) ? r.diagnostics : [])); -if (!diagnostics.length) process.exit(0); - -// An Edit is held to the lines it changed; a Write wrote the whole file. -const edited = editedLines(input); -const inScope = edited?.lines ? diagnostics.filter((d) => edited.lines.has(d.line)) : diagnostics; -const elsewhere = diagnostics.length - inScope.length; -if (!inScope.length) process.exit(0); - -// Errors first, so a long list of warnings never hides one. -const ordered = [ - ...inScope.filter((d) => d.severity === 'error'), - ...inScope.filter((d) => d.severity !== 'error'), -]; -const errors = ordered.filter((d) => d.severity === 'error').length; -const shown = ordered.slice(0, MAX_FINDINGS).map(describe); -if (ordered.length > shown.length) { - shown.push( - `…and ${ordered.length - shown.length} more. Run check_ui on the file to see them all.`, - ); } -const note = elsewhere - ? `\n(${elsewhere} other ${elsewhere === 1 ? 'finding' : 'findings'} elsewhere in the file predate this edit; leave them unless asked.)` - : ''; - -// An Edit the hook cannot place never blocks: its findings, errors included, go back as a note. -const unplaced = edited?.placed === false; -if (errors && !unplaced) { - process.stderr.write( - `${name}: this change breaks the project's design system (onsystem check). ` + - 'Fix these errors before moving on; get_component and get_tokens list the valid props, variants and tokens.\n\n' + - `${shown.join('\n')}${note}\n`, - ); - process.exit(2); + +/** Adds the findings in one file to `blocks` (errors) or `notes` (warnings, or an unplaced edit). */ +function report({ file, name, edits, patch }, diagnostics) { + if (!diagnostics.length) return; + // An Edit, or a patch's hunks, is held to the lines it changed; a Write wrote the whole file. + const edited = editedLines(file, edits, patch); + const inScope = edited?.lines ? diagnostics.filter((d) => edited.lines.has(d.line)) : diagnostics; + const elsewhere = diagnostics.length - inScope.length; + if (!inScope.length) return; + + // Errors first, so a long list of warnings never hides one. + const ordered = [ + ...inScope.filter((d) => d.severity === 'error'), + ...inScope.filter((d) => d.severity !== 'error'), + ]; + const errors = ordered.filter((d) => d.severity === 'error').length; + const shown = ordered.slice(0, MAX_FINDINGS).map(describe); + if (ordered.length > shown.length) { + shown.push( + `…and ${ordered.length - shown.length} more. Run check_ui on the file to see them all.`, + ); + } + const note = elsewhere + ? `\n(${elsewhere} other ${elsewhere === 1 ? 'finding' : 'findings'} elsewhere in the file predate this edit; leave them unless asked.)` + : ''; + + // An Edit the hook cannot place never blocks: its findings, errors included, go back as a note. + const unplaced = edited?.placed === false; + if (errors && !unplaced) { + blocks.push( + `${name}: this change breaks the project's design system (onsystem check). ` + + 'Fix these errors before moving on; get_component and get_tokens list the valid props, variants and tokens.\n\n' + + `${shown.join('\n')}${note}`, + ); + return; + } + const lead = unplaced + ? `onsystem check: findings in ${name} that this edit may not have caused, since the hook could not tell which lines it changed. Fix the ones it introduced and leave the rest unless asked.` + : `onsystem check: warnings in ${name}. Prefer the suggested scale values and tokens.`; + notes.push(`${lead}\n${shown.join('\n')}${note}`); } -const lead = unplaced - ? `onsystem check: findings in ${name} that this edit may not have caused, since the hook could not tell which lines it changed. Fix the ones it introduced and leave the rest unless asked.` - : `onsystem check: warnings in ${name}. Prefer the suggested scale values and tokens.`; -process.stdout.write( - JSON.stringify({ - hookSpecificOutput: { - hookEventName: 'PostToolUse', - additionalContext: `${lead}\n${shown.join('\n')}${note}`, - }, - }), -); -process.exit(0); /** - * Says once per session and project that edits are not being checked, and why, - * on stderr (the debug log) and as a `systemMessage` the user sees. Never blocks. + * Adds a notice that the job's files are not being checked, and why, said once per session and + * project on stderr (the debug log) and as a `systemMessage` the user sees. Never blocks. */ -function cannotRun(reason) { +function cannotRun({ root, cli, files }, reason) { + const names = files.map((f) => f.name); const advice = /config|tsconfig|Token file/i.test(reason) ? 'Fix the config, or run `onsystem inspect` to see the problem.' : cli.npx ? `Install it in the project to run it without npx: npm install --save-dev ${PACKAGE}` - : `Run \`onsystem check ${name}\` to see why.`; - const text = `onsystem could not check ${name}, so edits are not being checked against the design system: ${reason} ${advice}`; - once('hook-notice', text, () => { - process.stderr.write(`${text}\n`); - process.stdout.write(JSON.stringify({ systemMessage: text })); - }); - process.exit(0); + : `Run \`onsystem check ${names.join(' ')}\` to see why.`; + const text = `onsystem could not check ${names.join(', ')}, so edits are not being checked against the design system: ${reason} ${advice}`; + notices.push({ root, text }); +} + +/** + * The files the tool wrote, as `{ path, edits, patch }`: `edits` are the `{ before, after }` texts + * it replaced, or undefined for a file written whole, and `patch` is Claude Code's diff of them. + * Claude Code's Write and Edit (old_string, new_string) name the file in `file_path`. A Codex + * apply_patch names its files in the patch, in `command`. Other clients' tools name it `path` or + * `filePath`, with the replaced text in `oldString` and `newString`, or `old_str` and `new_str`. + * Their tools that read a file name it the same way, and VS Code runs hooks after every tool, so + * a file named so counts only when the input carries the text written: new text, or the content. + */ +function editedFiles(payload) { + const tool = payload?.tool_input; + if (!tool || typeof tool !== 'object') return []; + if (payload.tool_name === 'apply_patch') { + return typeof tool.command === 'string' ? patchedFiles(tool.command) : []; + } + const fields = [ + ['old_string', 'new_string'], + ['oldString', 'newString'], + ['old_str', 'new_str'], + ].find(([, after]) => typeof tool[after] === 'string'); + const writes = + fields || ['content', 'file_text', 'code'].some((k) => typeof tool[k] === 'string'); + const file = [tool.file_path, ...(writes ? [tool.path, tool.filePath] : [])].find( + (f) => typeof f === 'string' && f, + ); + if (!file) return []; + const edits = fields && [{ before: tool[fields[0]], after: tool[fields[1]] }]; + return [{ path: file, edits, patch: payload.tool_response?.structuredPatch }]; +} + +/** + * The files a Codex apply_patch adds or updates, from its `*** Add File:`, `*** Update File:` and + * `*** Move to:` lines. An added file is written whole. An updated one is held to its hunks, each + * the text before (its context and `-` lines) and after (its context and `+` lines), and is + * checked where `*** Move to:` moved it. A deleted file leaves nothing to check. + */ +function patchedFiles(text) { + const files = new Map(); + let current; + let hunk; + for (const raw of text.split('\n')) { + const line = raw.replace(/\r$/, ''); + const marker = /^\*\*\* (Add File|Update File|Delete File|Move to): (.+)$/.exec(line); + if (marker) { + const kind = marker[1]; + const file = marker[2].trim(); + hunk = undefined; + if (kind === 'Move to') { + if (current) { + files.delete(current.path); + current.path = file; + files.set(file, current); + } + continue; + } + if (kind === 'Delete File') { + files.delete(file); + current = undefined; + continue; + } + current = files.get(file) ?? { path: file, edits: [] }; + if (kind === 'Add File') current.edits = undefined; + files.set(file, current); + continue; + } + // `*** Begin Patch`, `*** End Patch`, `*** End of File` and the like. + if (line.startsWith('***')) { + if (/^\*\*\* (Begin|End) Patch/.test(line)) current = undefined; + continue; + } + // An added file's lines, or none of a file's. + if (!current?.edits) continue; + if (line.startsWith('@@') || !hunk) { + hunk = { before: [], after: [] }; + current.edits.push(hunk); + if (line.startsWith('@@')) continue; + } + const [kind, rest] = [line[0], line.slice(1)]; + if (kind === '-' || kind === ' ' || !line) hunk.before.push(rest); + if (kind === '+' || kind === ' ' || !line) hunk.after.push(rest); + } + return [...files.values()].map(({ path: file, edits }) => ({ + path: file, + edits: edits?.map((h) => ({ before: h.before.join('\n'), after: h.after.join('\n') })), + })); } /** A marker file in the system temp folder, per session and project (per day without a session). */ -function markerPath(kind) { +function markerPath(kind, root) { const day = new Date().toISOString().slice(0, 10); const key = createHash('sha256') .update(`${input.session_id ?? day}\0${root}`) @@ -225,8 +364,8 @@ function markerPath(kind) { } /** Runs `say` once per session and project (per day without a session), through a marker file. */ -function once(kind, text, say) { - const marker = markerPath(kind); +function once(kind, root, text, say) { + const marker = markerPath(kind, root); if (fs.existsSync(marker)) return; try { fs.mkdirSync(path.dirname(marker), { recursive: true }); @@ -238,7 +377,7 @@ function once(kind, text, say) { } /** Whether npx was paused, in this session and project, less than OFFLINE_PAUSE ago. */ -function paused() { +function paused(pause) { try { return Date.now() - fs.statSync(pause).mtimeMs < OFFLINE_PAUSE; } catch { @@ -246,19 +385,21 @@ function paused() { } } -/** Runs `check` on the file, through npx with `npm` flags, in what is left of TIMEOUT. */ -function check(npm) { +/** Runs `check` on the job's files, through npx with `npm` flags, in what is left of TIMEOUT. */ +function check({ root, cli, files }, npm) { const [command, ...args] = cli.npx ? [...cli.argv, ...npm, '--yes', `${PACKAGE}@${VERSION}`] : cli.argv; // Relative to the folder it runs in, which the CLI sees by its real path: an absolute path // through a symlink (macOS's /tmp) would fall outside it, and outside every project in it. - const relative = path.relative(root, file); - const target = relative && !path.isAbsolute(relative) ? `.${path.sep}${relative}` : file; + const paths = files.map(({ file }) => { + const relative = path.relative(root, file); + return relative && !path.isAbsolute(relative) ? `.${path.sep}${relative}` : file; + }); const spawn = (flags) => spawnSync( command, - [...args, 'check', target, '--format', 'json', '--quiet-without-design-system', ...flags], + [...args, 'check', ...paths, '--format', 'json', '--quiet-without-design-system', ...flags], { cwd: root, encoding: 'utf8', @@ -319,7 +460,7 @@ function unreachable(result) { } /** Why the CLI did not run, in a sentence, without anything that could hold a credential. */ -function failure(result) { +function failure(cli, result) { if (result.error?.code === 'ENOENT') return 'npx was not found.'; if (timedOut(result)) { // npm may have been waiting for the registry, or the CLI for itself: it cannot be told apart. @@ -582,17 +723,18 @@ function installedCli(dir) { } /** - * The 1-based lines an Edit changed, or undefined for the whole file (a Write). Lines it - * repeats unchanged from old_string are not its own. `placed` is false when the hook cannot - * tell where the edit is: its new text is in the file more than once, or no longer as written - * (a formatter rewrote it); `lines` are then the lines it may be on, or undefined for any. + * The 1-based lines `edits` changed in `file`, or undefined for the whole file (a Write, or a + * file a patch adds). Each edit is the text it replaced and its new text: an Edit's old_string + * and new_string, or a patch hunk. Lines an edit repeats unchanged from the old text are not its + * own. `placed` is false when the hook cannot tell where an edit is: its new text is in the file + * more than once, or no longer as written (a formatter rewrote it); `lines` are then the lines it + * may be on, or undefined for any. */ -function editedLines(payload) { - if (payload.tool_name !== 'Edit') return undefined; - const { old_string: before, new_string: after } = payload.tool_input ?? {}; - if (typeof after !== 'string') return undefined; +function editedLines(file, edits, structuredPatch) { + if (!edits) return undefined; // Removing text adds no finding of its own. - if (!after.trim()) return { lines: new Set(), placed: true }; + const adding = edits.filter((edit) => edit.after.trim()); + if (!adding.length) return { lines: new Set(), placed: true }; let text; try { text = fs.readFileSync(file, 'utf8').replace(/\r\n/g, '\n'); @@ -600,26 +742,31 @@ function editedLines(payload) { return { lines: undefined, placed: false }; } // Claude Code's own diff of the edit, while the file still matches it. - const patched = patchLines(payload.tool_response?.structuredPatch, text.split('\n')); + const patched = patchLines(structuredPatch, text.split('\n')); if (patched) return { lines: patched, placed: true }; - const replacement = after.replace(/\r\n/g, '\n'); - const changed = - typeof before === 'string' - ? changedLines(before.replace(/\r\n/g, '\n').split('\n'), replacement.split('\n')) - : replacement.split('\n').map((_, i) => i); const lines = new Set(); - let matches = 0; - // One pass over the file: lines are counted from the previous match, not from the top. - for ( - let at = text.indexOf(replacement), line = 1, counted = 0; - at !== -1; - at = text.indexOf(replacement, at + 1) - ) { - for (; counted < at; counted++) if (text.charCodeAt(counted) === 10) line++; - matches++; - for (const offset of changed) lines.add(line + offset); + let placed = true; + for (const { before, after } of adding) { + const replacement = after.replace(/\r\n/g, '\n'); + const changed = + typeof before === 'string' + ? changedLines(before.replace(/\r\n/g, '\n').split('\n'), replacement.split('\n')) + : replacement.split('\n').map((_, i) => i); + let matches = 0; + // One pass over the file: lines are counted from the previous match, not from the top. + for ( + let at = text.indexOf(replacement), line = 1, counted = 0; + at !== -1; + at = text.indexOf(replacement, at + 1) + ) { + for (; counted < at; counted++) if (text.charCodeAt(counted) === 10) line++; + matches++; + for (const offset of changed) lines.add(line + offset); + } + if (!matches) return { lines: undefined, placed: false }; + if (matches > 1) placed = false; } - return matches ? { lines, placed: matches === 1 } : { lines: undefined, placed: false }; + return { lines, placed }; } /** The new-file lines a structuredPatch adds, if every line it shows is still in the file there. */ diff --git a/src/init.ts b/src/init.ts index e4f1b812..1307e40a 100644 --- a/src/init.ts +++ b/src/init.ts @@ -62,6 +62,9 @@ export const GUIDE_FILE = 'ONSYSTEM.md'; * every app that depends on the package reads it, and writes ONSYSTEM.md, which tells those * apps' teams how to add the agent tools and the CI check. In an app that takes on such a * package's config, it writes no config of its own unless --force says to. + * + * In a terminal, it offers to add a short section on onsystem to the project's AGENTS.md or + * CLAUDE.md, for coding agents. */ export async function init( configOptions: LoadConfigOptions, @@ -301,7 +304,8 @@ export async function init( io.stdout(`No baseline written; \`${NAME} check . --update-baseline\` records one later.`); } } - const commit = [...written, ...(baselineWritten ? [BASELINE_FILE] : [])]; + const instructed = await agentRules(root, ask, io); + const commit = [...written, ...(baselineWritten ? [BASELINE_FILE] : []), ...instructed]; const files = commit.length > 1 ? `${commit.slice(0, -1).join(', ')} and ${commit.at(-1) ?? ''}` : commit[0]; io.stdout( @@ -316,6 +320,82 @@ export async function init( const CONFIG_FILE = 'onsystem.config.json'; +/** The instruction files agents read, which `init` offers to add onsystem's section to. */ +const AGENT_FILES = ['AGENTS.md', 'CLAUDE.md']; +const RULES_BEGIN = ''; +const RULES_END = ''; +/** onsystem's section of AGENTS.md or CLAUDE.md: the MCP tools, and the CLI for agents without them. */ +const AGENT_RULES = [ + RULES_BEGIN, + '# Design system', + '', + `This project's UI is checked against its design system by ${NAME}.`, + '', + `- Before writing UI, look up its components, props, variants and tokens with the ${NAME} tools (\`list_components\`, \`get_component\`, \`get_tokens\`).`, + `- After changing a \`.tsx\` or \`.jsx\` file, run \`check_ui\` on it and fix every error. Without the ${NAME} tools, run \`npx -y ${NAME} check \` instead.`, + RULES_END, +].join('\n'); + +/** + * Offers to add onsystem's section to the AGENTS.md and CLAUDE.md in the root, between markers a + * later run updates in place, so agents that read them check the UI they write, through the CLI + * when they have no MCP config. A CLAUDE.md that imports AGENTS.md (an `@AGENTS.md` line) is left + * alone: Claude Code reads the section there. Asked only in a terminal, never with --yes; nothing + * is asked when neither file exists or the section is current. Returns the files it wrote. + */ +async function agentRules(root: string, ask: InitIo['prompt'], io: InitIo): Promise { + if (!ask) return []; + const found = AGENT_FILES.flatMap((name) => { + const file = path.join(root, name); + try { + return [{ name, file, text: fs.readFileSync(file, 'utf8') }]; + } catch { + return []; + } + }); + const imports = (text: string) => /^\s*@(?:\.\/)?AGENTS\.md\s*$/m.test(text); + const changes = found + .filter((f) => !(f.name === 'CLAUDE.md' && imports(f.text) && found.length > 1)) + .map((f) => ({ ...f, next: withAgentRules(f.text) })) + .filter((f) => f.next !== f.text); + if (!changes.length) return []; + const names = andList(changes.map((f) => f.name)); + const update = changes.every((f) => f.text.includes(RULES_BEGIN)); + const answer = ( + await ask( + update + ? `Update the onsystem section in ${names}? [Y/n] ` + : `Add a short onsystem section to ${names}, so coding agents check the UI they write? [Y/n] `, + ) + ) + .trim() + .toLowerCase(); + if (answer !== '' && !answer.startsWith('y')) { + io.stdout( + `${names} ${changes.length === 1 ? 'was' : 'were'} left as ${changes.length === 1 ? 'it is' : 'they are'}.`, + ); + return []; + } + for (const f of changes) fs.writeFileSync(f.file, f.next); + io.stdout( + update + ? `Updated the onsystem section in ${names}.` + : `Added an onsystem section to ${names}: it tells agents to run check_ui, or \`npx -y ${NAME} check \` without the ${NAME} tools, on the UI files they change.`, + ); + return changes.map((f) => f.name); +} + +/** `text` with onsystem's section: replaced between its markers, or added at the end. */ +function withAgentRules(text: string): string { + const eol = text.includes('\r\n') ? '\r\n' : '\n'; + const rules = AGENT_RULES.replaceAll('\n', eol); + const start = text.indexOf(RULES_BEGIN); + const end = start === -1 ? -1 : text.indexOf(RULES_END, start); + if (end !== -1) return `${text.slice(0, start)}${rules}${text.slice(end + RULES_END.length)}`; + if (!text.trim()) return `${rules}${eol}`; + return `${text.endsWith('\n') ? text : `${text}${eol}`}${eol}${rules}${eol}`; +} + /** A package.json as read, with the indentation to write it back in. */ interface Manifest { file: string; diff --git a/test/hook-apply-patch.test.ts b/test/hook-apply-patch.test.ts new file mode 100644 index 00000000..71f01b41 --- /dev/null +++ b/test/hook-apply-patch.test.ts @@ -0,0 +1,279 @@ +import { spawnSync } from 'node:child_process'; +import fs from 'node:fs'; +import path from 'node:path'; + +import { describe, expect, it } from 'vitest'; + +import { main, type Io } from '../src/cli.js'; +import { fixture } from './helpers.js'; + +const HOOK = path.resolve(import.meta.dirname, '../plugins/onsystem/hooks/check-ui.mjs'); + +// Stands in for the CLI: `check --format json` reports an error on every line of each +// file with an arbitrary color, named as the CLI names files, relative to the folder it runs in. +// With STUB_ANSWER set, it prints that answer instead, which a test gets from the real CLI. +const STUB = `import fs from 'node:fs'; +import path from 'node:path'; +fs.appendFileSync(path.join(import.meta.dirname, 'runs.log'), JSON.stringify(process.argv.slice(2)) + '\\n'); +if (process.env.STUB_ANSWER) { + const answer = JSON.parse(fs.readFileSync(process.env.STUB_ANSWER, 'utf8')); + process.stdout.write(answer.stdout); + process.exit(answer.code); +} +const args = process.argv.slice(3); +const files = args.slice(0, args.indexOf('--format')); +const results = files.map((target) => { + const diagnostics = fs.readFileSync(target, 'utf8').split(/\\r?\\n/).flatMap((text, i) => { + const at = text.indexOf('bg-[#'); + if (at === -1) return []; + const value = text.slice(at, text.indexOf(']', at) + 1); + return [{ ruleId: 'no-hardcoded-color', severity: 'error', message: 'Hardcoded color \`' + value + '\`.', line: i + 1, column: at + 1 }]; + }); + const file = path.relative(process.cwd(), path.resolve(target)).split(path.sep).join('/'); + return { file, diagnostics }; +}); +console.log(JSON.stringify(results)); +process.exit(results.some((r) => r.diagnostics.length) ? 1 : 0); +`; + +// An older finding on line 3; the button on line 5 is what the patch below changes. +const CARD = [ + 'export function SettingsCard() {', + ' return (', + '
', + '

Settings

', + ' ', + '
', + ' );', + '}', + '', +]; + +// Older findings on lines 2 to 4, each of which also holds `}`. +const TOOLBAR = [ + 'export function Toolbar() {', + ' const save = ;', + ' const undo = ;', + ' const redo = ;', + ' return
{save}{undo}{redo}
;', + '}', + '', +]; + +const BANNER = [ + 'export function Banner() {', + ' return

Sale

;', + '}', + '', +]; + +/** A Codex apply_patch, as its PostToolUse input carries it in `tool_input.command`. */ +const patch = (...sections: string[]) => + ['*** Begin Patch', ...sections, '*** End Patch'].join('\n'); +/** The hunk that turned the card's button from bg-primary into a hardcoded color. */ +const CARD_HUNK = [ + `@@ ${CARD[0]}`, + ` ${CARD[3]}`, + '- ', + `+${CARD[4]}`, + ` ${CARD[5]}`, +].join('\n'); +const add = (file: string, lines: string[]) => + [`*** Add File: ${file}`, ...lines.slice(0, -1).map((l) => `+${l}`)].join('\n'); + +describe('check-ui hook: a Codex apply_patch', () => { + const project = fixture({ + 'stub.mjs': STUB, + 'package.json': '{"name":"app"}', + 'app/settings-card.tsx': CARD.join('\n'), + 'app/toolbar.tsx': TOOLBAR.join('\n'), + 'app/banner.tsx': BANNER.join('\n'), + 'app/moved/banner.tsx': BANNER.join('\n'), + 'app/notes.md': 'bg-[#000000]\n', + // A design system, for the real CLI: components and the colors above as tokens. + 'components/ui/button.tsx': + 'export function Button(props: React.ComponentProps<"button">) { return ', + newString: CARD[4], + }, + 'replace_string_in_file', + ); + expect(edited.code).toBe(2); + expect(edited.stderr).toContain('5:26 error'); + expect(edited.stderr).not.toMatch(/^3:\d+ error/m); + // A tool that reads the file names it the same way: it is not checked. + const card = path.join(project, 'app/settings-card.tsx'); + expect(hook({ filePath: card, startLine: 1, endLine: 9 }, 'read_file')).toEqual(silent); + expect(hook({ path: card }, 'view')).toEqual(silent); + }); + + it("finds each file's findings in the real CLI's answer", async () => { + // The command the hook ran for a patch of two files, run by the real CLI in that folder. + const command = patch( + '*** Update File: app/settings-card.tsx', + CARD_HUNK, + add('app/banner.tsx', BANNER), + ); + const [args] = hook({ command }).started; + const out: string[] = []; + const io: Io = { cwd: project, color: false, stdout: (t) => out.push(t), stderr: () => {} }; + const code = await main([...(args ?? []), '--no-cache'], io); + const real = JSON.parse(out.join('\n')) as { file: string }[]; + // In its own order, not the order it was given them. + expect(real.map((r) => r.file)).toEqual(['app/banner.tsx', 'app/settings-card.tsx']); + const answer = path.join(project, 'answer.json'); + fs.writeFileSync(answer, JSON.stringify({ stdout: out.join('\n'), code })); + const { stderr } = hook({ command }, 'apply_patch', answer); + expect(stderr).toMatch(/^app\/settings-card\.tsx: this change breaks/); + expect(stderr).toMatch(/^5:\d+ error \[no-hardcoded-color\]/m); + expect(stderr).not.toMatch(/^3:\d+ error/m); + expect(stderr).toContain('\n\napp/banner.tsx: this change breaks'); + expect(stderr).toContain('2:24 error [no-hardcoded-color]'); + }); +}); diff --git a/test/init-agents-md.test.ts b/test/init-agents-md.test.ts new file mode 100644 index 00000000..071d8117 --- /dev/null +++ b/test/init-agents-md.test.ts @@ -0,0 +1,130 @@ +import fs from 'node:fs'; +import path from 'node:path'; + +import { describe, expect, it } from 'vitest'; + +import { main, type Io } from '../src/cli.js'; +import { fixture, TSCONFIG } from './helpers.js'; + +const json = (value: unknown) => JSON.stringify(value, null, 2); + +async function run(args: string[], cwd: string, answers?: string[]) { + const out: string[] = []; + const err: string[] = []; + const asked: string[] = []; + const io: Io = { + cwd, + color: false, + stdout: (t) => out.push(t), + stderr: (t) => err.push(t), + prompt: answers + ? (question) => { + asked.push(question); + return Promise.resolve(answers.shift() ?? ''); + } + : undefined, + }; + const code = await main(args, io); + return { code, stdout: out.join('\n'), stderr: err.join('\n'), asked }; +} + +/** A shadcn/ui app with a clean page, so init asks no baseline question. */ +const APP = { + 'tsconfig.json': TSCONFIG, + 'package.json': json({ name: 'app' }), + 'components.json': json({ + tailwind: { css: 'app/globals.css' }, + aliases: { ui: '@/components/ui' }, + }), + 'components/ui/button.tsx': + 'export function Button(props: { variant?: "default" | "outline"; children?: unknown }) { return \n', +}; + +// An AGENTS.md shaped like create-next-app 16.4's, with Next's rules, and a CLAUDE.md that imports it. +const NEXT_AGENTS = + '\n# This is NOT the Next.js you know\n\nRead the docs in node_modules/next/dist/docs/ before writing code.\n\n'; +const NEXT_CLAUDE = '@AGENTS.md\n'; + +const BEGIN = ''; +const END = ''; +const read = (root: string, file: string) => fs.readFileSync(path.join(root, file), 'utf8'); +/** The onsystem section of a file, markers included. */ +const section = (text: string) => text.slice(text.indexOf(BEGIN), text.indexOf(END) + END.length); + +describe('onsystem init: instructions for coding agents', () => { + it('adds a marked section to the AGENTS.md of a create-next-app, when asked in a terminal', async () => { + const root = fixture({ ...APP, 'AGENTS.md': NEXT_AGENTS, 'CLAUDE.md': NEXT_CLAUDE }); + const { code, stdout, asked } = await run(['init', '--no-cache'], root, ['']); + expect(code).toBe(0); + expect(asked).toEqual([ + 'Add a short onsystem section to AGENTS.md, so coding agents check the UI they write? [Y/n] ', + ]); + const agents = read(root, 'AGENTS.md'); + // Next's rules stay as they were, and the section follows them. + expect(agents.startsWith(`${NEXT_AGENTS}\n${BEGIN}\n`)).toBe(true); + expect(agents.endsWith(`${END}\n`)).toBe(true); + // Both forms: the MCP tools, and the CLI for agents without them. + expect(section(agents)).toContain('run `check_ui` on it and fix every error'); + expect(section(agents)).toContain('`npx -y onsystem check `'); + // CLAUDE.md imports AGENTS.md: the section is not written twice. + expect(read(root, 'CLAUDE.md')).toBe(NEXT_CLAUDE); + expect(stdout).toContain('Added an onsystem section to AGENTS.md'); + expect(stdout).toContain('Next: commit onsystem.config.json and AGENTS.md, and run'); + + // A later run leaves a current section alone, without asking. + const again = await run(['init', '--force', '--no-cache'], root, []); + expect(again.asked).toEqual([]); + expect(read(root, 'AGENTS.md')).toBe(agents); + + // An older section is replaced in place, once asked. + fs.writeFileSync( + path.join(root, 'AGENTS.md'), + agents.replace(section(agents), `${BEGIN}\nAn older section.\n${END}`) + '\n# Mine\n', + ); + const update = await run(['init', '--force', '--no-cache'], root, ['y']); + expect(update.asked).toEqual(['Update the onsystem section in AGENTS.md? [Y/n] ']); + const updated = read(root, 'AGENTS.md'); + expect(updated).toBe(`${agents}\n# Mine\n`); + expect(updated.split(BEGIN)).toHaveLength(2); + expect(update.stdout).toContain('Updated the onsystem section in AGENTS.md.'); + }); + + it('writes CLAUDE.md when it does not import AGENTS.md, and asks once for both', async () => { + const root = fixture({ ...APP, 'AGENTS.md': '# Agents\n', 'CLAUDE.md': '# Claude' }); + const { asked } = await run(['init', '--no-cache'], root, ['']); + expect(asked).toEqual([ + 'Add a short onsystem section to AGENTS.md and CLAUDE.md, so coding agents check the UI they write? [Y/n] ', + ]); + expect(read(root, 'AGENTS.md')).toMatch(/^# Agents\n\n\n/); + expect(read(root, 'CLAUDE.md')).toMatch(/^# Claude\n\n\n/); + expect(section(read(root, 'CLAUDE.md'))).toBe(section(read(root, 'AGENTS.md'))); + }); + + it('writes nothing when declined, with --yes, without a terminal, or with neither file', async () => { + const files = { ...APP, 'AGENTS.md': NEXT_AGENTS }; + const declined = fixture(files); + const no = await run(['init', '--no-cache'], declined, ['n']); + expect(no.asked).toHaveLength(1); + expect(no.stdout).toContain('AGENTS.md was left as it is.'); + expect(read(declined, 'AGENTS.md')).toBe(NEXT_AGENTS); + + for (const [args, answers] of [ + [['init', '--yes', '--no-cache'], []], + [['init', '--no-cache'], undefined], + ] as const) { + const root = fixture(files); + const { code, asked } = await run([...args], root, answers && [...answers]); + expect(code).toBe(0); + expect(asked).toEqual([]); + expect(read(root, 'AGENTS.md')).toBe(NEXT_AGENTS); + } + + const none = fixture(APP); + expect((await run(['init', '--no-cache'], none, [])).asked).toEqual([]); + expect(fs.existsSync(path.join(none, 'AGENTS.md'))).toBe(false); + }); +}); From e7217d4da2de95fec2d331fb929c6317d3d3f90b Mon Sep 17 00:00:00 2001 From: Diogo Esteves Date: Fri, 16 Oct 2026 09:38:58 +0100 Subject: [PATCH 2/2] fix(hook): leave code snippets alone, keep text after a stray BEGIN marker, and skip deletion-only hunks - The hook no longer treats a `code` field as a whole-file write. VS Code's insert_edit_into_file sends a snippet with `...existing code...` lines, and check_ui sends unsaved code, so such a call blocked on findings the edit did not cause. Only `content` and `file_text` count as the file. - init pairs the agent-rules END marker with the nearest BEGIN before it, so a BEGIN whose END was deleted no longer makes a later run replace the user's text after it. - A patch hunk that only deletes lines is not looked for when placing a file's edits, so its repeated context no longer turns the file's errors into a non-blocking note. - docs/clients.md and the changeset name the markers without `` and ``, which a later run updates in place. A `CLAUDE.md` that imports `AGENTS.md` is left alone, and `--yes` never writes it. +`init`, run in a terminal, offers to add a short section to the project's `AGENTS.md` (create-next-app writes one) or `CLAUDE.md`: use the onsystem tools before writing UI, and run `check_ui`, or `npx -y onsystem check ` without the tools, on every UI file changed. It sits between `BEGIN:onsystem-agent-rules` and `END:onsystem-agent-rules` comment markers, which a later run updates in place. A `CLAUDE.md` that imports `AGENTS.md` is left alone, and `--yes` never writes it. diff --git a/docs/clients.md b/docs/clients.md index 6992566e..64a26109 100644 --- a/docs/clients.md +++ b/docs/clients.md @@ -266,4 +266,4 @@ To share it with your team, add `--scope project`, which writes `.mcp.json` at t Any other MCP client: run `npx -y onsystem --root /absolute/path/to/app` as a stdio server. On native Windows, wrap it as `cmd /c npx ...`. -The server sends usage instructions in its `initialize` result and in `server/discover`. Clients that ignore them benefit from one line in `CLAUDE.md`, `AGENTS.md` or `.cursor/rules`: _"Before writing UI, use the onsystem tools. Run check_ui on every file you change and fix all errors."_ Run in a terminal, `onsystem init` offers to add a short section that says so to the project's `AGENTS.md` or `CLAUDE.md`, with `npx -y onsystem check ` for agents without the tools. It sits between `` and ``, which a later run updates in place. +The server sends usage instructions in its `initialize` result and in `server/discover`. Clients that ignore them benefit from one line in `CLAUDE.md`, `AGENTS.md` or `.cursor/rules`: _"Before writing UI, use the onsystem tools. Run check_ui on every file you change and fix all errors."_ Run in a terminal, `onsystem init` offers to add a short section that says so to the project's `AGENTS.md` or `CLAUDE.md`, with `npx -y onsystem check ` for agents without the tools. It sits between `BEGIN:onsystem-agent-rules` and `END:onsystem-agent-rules` comment markers, which a later run updates in place. diff --git a/plugins/onsystem/hooks/check-ui.mjs b/plugins/onsystem/hooks/check-ui.mjs index 0f701f1e..a281c295 100644 --- a/plugins/onsystem/hooks/check-ui.mjs +++ b/plugins/onsystem/hooks/check-ui.mjs @@ -273,7 +273,9 @@ function cannotRun({ root, cli, files }, reason) { * apply_patch names its files in the patch, in `command`. Other clients' tools name it `path` or * `filePath`, with the replaced text in `oldString` and `newString`, or `old_str` and `new_str`. * Their tools that read a file name it the same way, and VS Code runs hooks after every tool, so - * a file named so counts only when the input carries the text written: new text, or the content. + * a file named so counts only when the input carries the text written: new text, or the content + * (`content` or `file_text`). A `code` field is not one: VS Code's insert_edit_into_file sends a + * snippet with `...existing code...` lines in it, and check_ui sends code it has not saved. */ function editedFiles(payload) { const tool = payload?.tool_input; @@ -286,8 +288,7 @@ function editedFiles(payload) { ['oldString', 'newString'], ['old_str', 'new_str'], ].find(([, after]) => typeof tool[after] === 'string'); - const writes = - fields || ['content', 'file_text', 'code'].some((k) => typeof tool[k] === 'string'); + const writes = fields || ['content', 'file_text'].some((k) => typeof tool[k] === 'string'); const file = [tool.file_path, ...(writes ? [tool.path, tool.filePath] : [])].find( (f) => typeof f === 'string' && f, ); @@ -726,9 +727,10 @@ function installedCli(dir) { * The 1-based lines `edits` changed in `file`, or undefined for the whole file (a Write, or a * file a patch adds). Each edit is the text it replaced and its new text: an Edit's old_string * and new_string, or a patch hunk. Lines an edit repeats unchanged from the old text are not its - * own. `placed` is false when the hook cannot tell where an edit is: its new text is in the file - * more than once, or no longer as written (a formatter rewrote it); `lines` are then the lines it - * may be on, or undefined for any. + * own, and an edit that only removes lines, which changes none, is not looked for. `placed` is + * false when the hook cannot tell where an edit is: its new text is in the file more than once, or + * no longer as written (a formatter rewrote it); `lines` are then the lines it may be on, or + * undefined for any. */ function editedLines(file, edits, structuredPatch) { if (!edits) return undefined; @@ -752,6 +754,8 @@ function editedLines(file, edits, structuredPatch) { typeof before === 'string' ? changedLines(before.replace(/\r\n/g, '\n').split('\n'), replacement.split('\n')) : replacement.split('\n').map((_, i) => i); + // A patch hunk that only deletes lines keeps its context, which may well be in the file twice. + if (!changed.length) continue; let matches = 0; // One pass over the file: lines are counted from the previous match, not from the top. for ( diff --git a/src/init.ts b/src/init.ts index 1307e40a..3c1fad36 100644 --- a/src/init.ts +++ b/src/init.ts @@ -360,7 +360,7 @@ async function agentRules(root: string, ask: InitIo['prompt'], io: InitIo): Prom .filter((f) => f.next !== f.text); if (!changes.length) return []; const names = andList(changes.map((f) => f.name)); - const update = changes.every((f) => f.text.includes(RULES_BEGIN)); + const update = changes.every((f) => rulesSection(f.text) !== undefined); const answer = ( await ask( update @@ -389,13 +389,25 @@ async function agentRules(root: string, ask: InitIo['prompt'], io: InitIo): Prom function withAgentRules(text: string): string { const eol = text.includes('\r\n') ? '\r\n' : '\n'; const rules = AGENT_RULES.replaceAll('\n', eol); - const start = text.indexOf(RULES_BEGIN); - const end = start === -1 ? -1 : text.indexOf(RULES_END, start); - if (end !== -1) return `${text.slice(0, start)}${rules}${text.slice(end + RULES_END.length)}`; + const found = rulesSection(text); + if (found) return `${text.slice(0, found.start)}${rules}${text.slice(found.end)}`; if (!text.trim()) return `${rules}${eol}`; return `${text.endsWith('\n') ? text : `${text}${eol}`}${eol}${rules}${eol}`; } +/** + * Where onsystem's section is in `text`, from its BEGIN marker to the end of its END marker: the + * first END with a BEGIN before it, and the BEGIN nearest that END. A BEGIN whose END was deleted + * is then not a section, so the text after it, which may be the user's, is never replaced. + */ +function rulesSection(text: string): { start: number; end: number } | undefined { + for (let end = text.indexOf(RULES_END); end !== -1; end = text.indexOf(RULES_END, end + 1)) { + const start = text.lastIndexOf(RULES_BEGIN, end); + if (start !== -1) return { start, end: end + RULES_END.length }; + } + return undefined; +} + /** A package.json as read, with the indentation to write it back in. */ interface Manifest { file: string; diff --git a/test/hook-apply-patch.test.ts b/test/hook-apply-patch.test.ts index 71f01b41..40723a2d 100644 --- a/test/hook-apply-patch.test.ts +++ b/test/hook-apply-patch.test.ts @@ -230,6 +230,26 @@ describe('check-ui hook: a Codex apply_patch', () => { expect(context(brace.stdout)).toContain('could not tell which lines it changed'); }); + it('holds a patch to its placed hunks when another hunk only deletes lines', () => { + // The second hunk's context, `}`, is on lines 2 to 6, but it changes no line of its own. + const { code, stderr } = hook({ + command: patch( + '*** Update File: app/toolbar.tsx', + '@@', + '- const redo = ;', + `+${TOOLBAR[3]}`, + '@@', + ` ${TOOLBAR[5]}`, + '-// An old comment.', + ), + }); + expect(code).toBe(2); + expect(stderr).toMatch(/^app\/toolbar\.tsx: this change breaks/); + expect(stderr).toMatch(/^4:\d+ error \[no-hardcoded-color\] Hardcoded color `bg-\[#dc2626\]`/m); + expect(stderr).not.toMatch(/^[23]:\d+ error/m); + expect(stderr).toContain('(2 other findings elsewhere in the file predate this edit'); + }); + it('reads the file from path or filePath, as other clients name it', () => { // A file written whole. const created = hook({ path: path.join(project, 'app/banner.tsx'), file_text: '' }, 'create'); @@ -253,6 +273,27 @@ describe('check-ui hook: a Codex apply_patch', () => { expect(hook({ path: card }, 'view')).toEqual(silent); }); + it('leaves a code snippet alone: it is not the file', () => { + // VS Code's insert_edit_into_file sends the lines it changes, with the rest elided. The file + // has older findings; the hook runs after every tool there, and must not blame this edit. + const toolbar = path.join(project, 'app/toolbar.tsx'); + const snippet = [ + '// ...existing code...', + ' return
{save}{undo}{redo}
;', + '// ...existing code...', + ].join('\n'); + expect( + hook( + { explanation: 'Widen the gap', filePath: toolbar, code: snippet }, + 'insert_edit_into_file', + ), + ).toEqual(silent); + // Nor is check_ui's code, which it checks unsaved, under the path it will be saved to. + expect( + hook({ code: TOOLBAR.join('\n'), path: toolbar }, 'mcp__plugin_onsystem_onsystem__check_ui'), + ).toEqual(silent); + }); + it("finds each file's findings in the real CLI's answer", async () => { // The command the hook ran for a patch of two files, run by the real CLI in that folder. const command = patch( diff --git a/test/init-agents-md.test.ts b/test/init-agents-md.test.ts index 071d8117..c3ee443b 100644 --- a/test/init-agents-md.test.ts +++ b/test/init-agents-md.test.ts @@ -104,6 +104,32 @@ describe('onsystem init: instructions for coding agents', () => { expect(section(read(root, 'CLAUDE.md'))).toBe(section(read(root, 'AGENTS.md'))); }); + it('keeps what follows a BEGIN marker whose END was deleted', async () => { + const mine = `# Agents\n\n${BEGIN}\n## User notes\n\nkeep me\n`; + const root = fixture({ ...APP, 'AGENTS.md': mine }); + // Without its END, the old BEGIN is no section: a new one is added, after the user's notes. + const first = await run(['init', '--no-cache'], root, ['']); + expect(first.asked).toEqual([ + 'Add a short onsystem section to AGENTS.md, so coding agents check the UI they write? [Y/n] ', + ]); + const added = read(root, 'AGENTS.md'); + expect(added.startsWith(`${mine}\n${BEGIN}\n# Design system\n`)).toBe(true); + expect(added.endsWith(`${END}\n`)).toBe(true); + // The next run finds the new section current: nothing is asked, and the notes stay. + const again = await run(['init', '--force', '--no-cache'], root, []); + expect(again.asked).toEqual([]); + expect(read(root, 'AGENTS.md')).toBe(added); + + // An older section after the stray BEGIN is the one updated, and the notes stay. + fs.writeFileSync( + path.join(root, 'AGENTS.md'), + `${mine}\n${BEGIN}\nAn older section.\n${END}\n`, + ); + const update = await run(['init', '--force', '--no-cache'], root, ['']); + expect(update.asked).toEqual(['Update the onsystem section in AGENTS.md? [Y/n] ']); + expect(read(root, 'AGENTS.md')).toBe(added); + }); + it('writes nothing when declined, with --yes, without a terminal, or with neither file', async () => { const files = { ...APP, 'AGENTS.md': NEXT_AGENTS }; const declined = fixture(files);