diff --git a/.github/workflows/skills-paths.yml b/.github/workflows/skills-paths.yml new file mode 100644 index 0000000000..18648eaad3 --- /dev/null +++ b/.github/workflows/skills-paths.yml @@ -0,0 +1,70 @@ +name: Skills Paths + +# Why this is its own workflow instead of a step in `ci.yml` or `lint.yml`: this +# gate's ENTIRE scan surface is markdown (`skills/**`, guide prose), and both of +# those workflows list `'**/*.md'`, `content/**` and `docs/**` under the +# `paths-ignore` of their `push` trigger, with no per-job path filter available in +# GitHub Actions. A push that only edits a guide would therefore start neither — +# and editing only a guide is the single most likely way a stated path goes dead. +# +# This is the fourth instance of the shape in this repo, and the reasoning is +# borrowed rather than invented: `docs-links.yml`'s header records that the link +# check spent from #3213 to #3448 inside `ci.yml`'s `docs` job, unable to see the +# one class of PR most likely to break a link, and `control-bytes.yml`'s header +# names the consequence — a gate that cannot see a markdown-only change +# "rebuilds the hole it exists to close". `changeset-guard.yml` is the third. +# +# Hence: no `paths` and no `paths-ignore` here, deliberately. +# `scripts/__tests__/check-skills-paths.test.ts` fails if either is ever added, +# and fails too if a second workflow starts running the same script — one gate, +# one home. +# +# It needs no install and no build — a checkout plus one `node` call over 18 +# markdown files, a couple of seconds — so keep it that way if you add checks to +# it. + +on: + pull_request: + branches: [main, develop] + push: + branches: [main, develop] + # Merge queue (objectui#3523 — see `ci.yml`'s trigger block for the full note + # and the measurements behind it). A required check that does not report on a + # queue build stalls the queue until the ruleset's 60-minute timeout fails it, + # so an unfiltered gate that could become required subscribes here from the + # start. `types:` is named although `checks_requested` is currently the only + # activity type GitHub defines for `merge_group`. + merge_group: + types: [checks_requested] + workflow_dispatch: + +concurrency: + group: skills-paths-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + skills-paths: + name: Skill Guide Path Check + runs-on: ubuntu-latest + timeout-minutes: 5 + + steps: + - name: Checkout code + uses: actions/checkout@v7 + + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version: '22.x' + + # The guides under `skills/` are a direct input to every agent that writes + # code here, and their prose gives in-repo paths as coordinates. A dead one + # produces no compile error — just "file not found" from a Read, and a + # wasted lap re-locating a symbol that does exist (#3713 and #3730 were + # 13+ of these in one guide, both rounds found by eye). Reads the checkout + # and nothing else, so no install is required. + - name: Check paths stated in the skill guides + run: node scripts/check-skills-paths.mjs diff --git a/content/docs/guide/ci-cd-pipeline.md b/content/docs/guide/ci-cd-pipeline.md index 85b3948cc9..4ed3da6805 100644 --- a/content/docs/guide/ci-cd-pipeline.md +++ b/content/docs/guide/ci-cd-pipeline.md @@ -29,6 +29,7 @@ one has its own section below. | `changeset-presence.yml` | Changeset Declaration | PR to `main`, `develop` — **no path filter**; merge-queue builds | **Yes** — when a released package's `src/` changed and no changeset was added | | `control-bytes.yml` | Control Byte Scan | Push / PR to `main`, `develop` — **no path filter**; merge-queue builds; manual | **Yes** | | `docs-links.yml` | Internal Docs Link Check | Push / PR to `main`, `develop` — **no path filter**; merge-queue builds; manual | **Yes** | +| `skills-paths.yml` | Skill Guide Path Check | Push / PR to `main`, `develop` — **no path filter**; merge-queue builds; manual | **Yes** — when a path stated in a `skills/` guide does not exist | | `performance-budget.yml` | Bundle Analysis | Push / PR touching `packages/**`, `apps/console/**`, `pnpm-lock.yaml` | **Yes** — the console entry gzip budget | | `live-e2e.yml` | Live E2E (informational) | PR to `main`, `develop` (code paths); nightly cron `30 6 * * *`; manual | No — informational lane, `continue-on-error` | | `labeler.yml` | Auto Label PRs | PR `opened`, `synchronize`, `reopened` | No | @@ -453,6 +454,62 @@ page it points at has moved or been renamed — fix the link, or restore the tar checked as *routes*, so `/docs/guide/foo` is what belongs in the markdown, not `content/docs/guide/foo.md`. Run it locally with `pnpm docs:check-links`. +## Skill Guide Paths (`skills-paths.yml`) + +**Triggers:** Push and PR to `main`/`develop`, merge-queue builds, plus manual dispatch — with **no +path filter at all**, for the same reason as the two sections above: this gate's entire scan surface +is markdown, and `ci.yml` still lists `'**/*.md'` under the `paths-ignore` of its `push` trigger. It +appears in the checks list as **Skill Guide Path Check**. + +Runs `scripts/check-skills-paths.mjs`, which reads every markdown file under `skills/` and asks, of +each in-repo path the prose states inside a backtick code span, whether it exists on disk. Those +guides are a direct input to every agent that writes code in this repository, and their prose gives +paths as coordinates. + +**Why a dead coordinate costs more than its size suggests:** the symbol named next to it is usually +real and only the location is wrong, so nobody gets a compile error — an agent gets "file not found" +from a `Read`, assumes its own search was clumsy, and spends a full lap re-locating something the +guide claimed to have located for it. Two rounds were found by eye while reading: +[#3713](https://github.com/objectstack-ai/objectui/issues/3713) (PR #3729) and +[#3730](https://github.com/objectstack-ai/objectui/issues/3730) (PR #3734), the second one 13 real +symbols at coordinates that did not exist. It also recurs by construction — the app-shell extraction +commits moved code with nothing anywhere to say the guides had gone stale +([#3735](https://github.com/objectstack-ai/objectui/issues/3735)). + +**What counts as a stated path:** a backtick span that opens with one of five top-level directories +(`apps/`, `packages/`, `examples/`, `scripts/`, `content/`) and contains no whitespace. Three +exclusions, each a *rule* rather than an exemption, because none of them claims that a file exists: + +| Excluded | Example in the guides today | Why | +|---|---|---| +| whitespace inside the span | a `grep -rn … packages/app-shell/src` self-check command line | prose, a command line or a type — not a path | +| glob or placeholder segment | the protected-primitive glob under `packages/components/src/ui`, a schema path with a placeholder domain segment | a shape, not a location; `existsSync` on it would mean nothing | +| fenced code blocks | a `bash` block that creates a file | a worked example may legitimately name a file the reader is about to create | + +Measured on `main@6422aa891`: 18 guide files, 91 candidate spans, 5 of them patterns — **86 stated +paths, of which 85 resolve**. + +**The one exemption, and why it cannot rot.** `scripts/skills-path-baseline.json` lists paths a guide +states *deliberately as absent*. Today there is exactly one: the Key contexts section of +`console-development.md` exists to correct a recurring wrong guess and says there is no +`apps/console/src/context/` directory at all. That entry is a ratchet, red in **both** directions — +if the path ever appears on disk the gate fails and names it (the sentence has become false), and if +the scan stops meeting the entry the gate fails too (the prose was rewritten, so the entry is dead +weight). Entries are keyed by file and token, never by line number, because guide prose moves +constantly. + +**Scope, stated so it is not mistaken for an oversight.** `content/docs/**` carries backtick paths +too and is **not** scanned here. Widening a scan surface arrives with its own batch of red to clear, +which `check-doc-links.mjs` learned three times over (#3479, #3490, #3545) — measure it first, in its +own change. The five-prefix list is the same kind of decision: adding this repository's other five +top-level directories was measured at +2 candidates and 0 new red, so it is cheap, but it stays +deliberate rather than assumed. + +**If it fails:** it prints every `file:line — token`. Fix the prose. Add a baseline entry only when +the sentence's whole point is that the path does not exist. Run it locally with +`pnpm check:skills-paths`, or `node scripts/check-skills-paths.mjs --list` to see every candidate and +how it was classified. + ## Link Checking (`check-links.yml`) **Trigger:** Weekly cron (`17 4 * * 0` — Sundays, off the top of the hour, when the scheduled-run diff --git a/package.json b/package.json index 7fb98876ef..4696947e9b 100644 --- a/package.json +++ b/package.json @@ -39,6 +39,7 @@ "check:control-bytes": "node scripts/check-control-bytes.mjs", "check:i18n-keys": "node scripts/check-i18n-call-site-keys.mjs", "check:i18n-drift": "node scripts/check-i18n-en-drift.mjs", + "check:skills-paths": "node scripts/check-skills-paths.mjs", "cli": "node packages/cli/dist/cli.js", "objectui": "node packages/cli/dist/cli.js", "create-plugin": "node packages/create-plugin/dist/index.js", diff --git a/scripts/__tests__/check-skills-paths.test.ts b/scripts/__tests__/check-skills-paths.test.ts new file mode 100644 index 0000000000..73c5e1735e --- /dev/null +++ b/scripts/__tests__/check-skills-paths.test.ts @@ -0,0 +1,429 @@ +import { describe, expect, it } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +// Plain-JS CI helper. Its types are INFERRED from the .mjs source by +// `tsconfig.scripts.json` (`allowJs`), so no `@ts-expect-error` here — +// re-adding one is now itself an error (TS2578). See objectui#3494. +import { extractPathTokens, scan, PATH_PREFIXES, readBaseline, BASELINE_FILE } from '../check-skills-paths.mjs'; + +/** + * objectui#3735 — the test for `scripts/check-skills-paths.mjs`. + * + * The guides under `skills/objectui/` are read by every agent that writes code + * here, and they give in-repo paths as coordinates. Nothing checked those paths + * existed, so two rounds of dead ones (#3713 / PR #3729 and #3730 / PR #3734, + * 13+ in one guide) were found by eye while reading. This suite holds the gate + * that makes the third round mechanical. + * + * The fixtures are temporary trees built here, never the real `skills/` + * directory: a committed fixture guide would have to contain a deliberately dead + * path, and something else in the repo would eventually scan it. + */ + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); + +/** No exemptions — the shape most fixtures want. */ +const NO_BASELINE = { allowedMissing: {} }; + +const baselineFor = (file: string, token: string, reason = 'fixture') => ({ + allowedMissing: { [file]: { [token]: { reason, issue: 'objectui#3735' } } }, +}); + +/** Builds a throwaway tree and runs the REAL `scan()` over it. */ +function withTree(build: (write: (rel: string, contents: string) => void) => void, run: (dir: string) => T): T { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'check-skills-paths-')); + const write = (rel: string, contents: string) => { + const full = path.join(dir, rel); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, contents); + }; + try { + build(write); + return run(dir); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +type Hit = { line: number; token: string; pattern: boolean }; +const tokensOf = (source: string): string[] => extractPathTokens(source).map((h: Hit) => h.token); + +describe('extractPathTokens — what a guide is taken to be asserting', () => { + it('reads inline code spans in prose, and reports the line', () => { + const hits: Hit[] = extractPathTokens( + ['# Guide', '', 'The sidebar lives in `packages/app-shell/src/layout/UnifiedSidebar.tsx`.', ''].join('\n'), + ); + expect(hits).toEqual([ + { line: 3, token: 'packages/app-shell/src/layout/UnifiedSidebar.tsx', pattern: false }, + ]); + }); + + it('reads several spans on one line', () => { + expect(tokensOf('Both `apps/console/src/main.tsx` and `packages/core/src/index.ts` matter.')).toEqual([ + 'apps/console/src/main.tsx', + 'packages/core/src/index.ts', + ]); + }); + + it('ignores spans that are not in-repo paths at all', () => { + // Package specifiers, symbols, skill-relative links and site routes. None of + // them is a repo-root-relative path, and treating any as one would produce + // permanent false red. + expect( + tokensOf( + [ + 'Import from `@object-ui/app-shell`, call `useNavigation()`, read `rules/protocol.md`,', + 'link to `/docs/guide/plugins` or `./architecture.md`.', + ].join('\n'), + ), + ).toEqual([]); + }); + + it('rejects a span containing whitespace — prose and command lines are not paths', () => { + // The live instance: PR #3856 added a self-check command line to + // `console-development.md` naming `packages/app-shell`. It is out of scope by + // this rule alone, with no baseline entry anywhere. + expect( + tokensOf('Run `grep -rn "UnifiedSidebar" packages/app-shell/src` to confirm, or read `apps/console 目录`.'), + ).toEqual([]); + }); + + it('classifies globs and placeholders as patterns, not as paths', () => { + const hits: Hit[] = extractPathTokens( + [ + 'Protected: `packages/components/src/ui/**`.', + 'Schemas go in `packages/spec/src//`.', + 'Apps are `packages/platform-objects/src/apps/*.app.ts`.', + 'Docs are `content/docs/guide/*.md`.', + 'Real one: `packages/core/src/index.ts`.', + ].join('\n'), + ); + expect(hits.filter((h) => h.pattern).map((h) => h.token)).toEqual([ + 'packages/components/src/ui/**', + 'packages/spec/src//', + 'packages/platform-objects/src/apps/*.app.ts', + 'content/docs/guide/*.md', + ]); + expect(hits.filter((h) => !h.pattern).map((h) => h.token)).toEqual(['packages/core/src/index.ts']); + }); + + it('never reads inside a fenced block, backticks or tildes', () => { + // A fence is a worked example; a tutorial fence may name a file the reader is + // about to create. Only prose asserts where code already lives. + expect( + tokensOf( + [ + 'Prose says `packages/core/src/index.ts`.', + '```bash', + 'mkdir -p `packages/brand-new/src`', + '```', + '~~~ts', + "import x from `packages/also-new/src/x.ts`;", + '~~~', + ].join('\n'), + ), + ).toEqual(['packages/core/src/index.ts']); + }); + + it('keeps a longer fence open until a fence at least as long closes it', () => { + expect( + tokensOf( + ['````md', '```', 'inner `packages/nope/src` stays hidden', '```', '````', 'after: `packages/yes/src`'].join( + '\n', + ), + ), + ).toEqual(['packages/yes/src']); + }); + + it('reads a multi-backtick span', () => { + expect(tokensOf('Doubled: ``packages/core/src/index.ts``.')).toEqual(['packages/core/src/index.ts']); + }); + + it('covers each declared prefix', () => { + for (const prefix of PATH_PREFIXES as string[]) { + expect(tokensOf(`See \`${prefix}/x/y.ts\`.`)).toEqual([`${prefix}/x/y.ts`]); + } + }); +}); + +describe('scan — a seeded dead path turns the gate red and gets named', () => { + it('reports the guide, the line and the token', () => { + const result = withTree( + (write) => { + write('skills/objectui/guides/demo.md', 'The sidebar lives in `packages/app-shell/src/layout/Gone.tsx`.\n'); + }, + (dir) => scan(dir, NO_BASELINE), + ); + expect(result.missing).toEqual([ + { file: 'skills/objectui/guides/demo.md', line: 1, token: 'packages/app-shell/src/layout/Gone.tsx' }, + ]); + expect(result.checked).toBe(1); + expect(result.resolved).toBe(0); + }); + + it('finds the dead one among the live ones, in a nested guide tree', () => { + const result = withTree( + (write) => { + write('packages/core/src/index.ts', 'export {};\n'); + write('apps/console/src/main.tsx', 'export {};\n'); + write('skills/objectui/SKILL.md', 'Entry: `packages/core/src/index.ts`.\n'); + write( + 'skills/objectui/guides/console-development.md', + ['Boot: `apps/console/src/main.tsx`.', '', 'Contexts: `apps/console/src/context/`.'].join('\n'), + ); + }, + (dir) => scan(dir, NO_BASELINE), + ); + expect(result.missing.map((m: { file: string; line: number }) => `${m.file}:${m.line}`)).toEqual([ + 'skills/objectui/guides/console-development.md:3', + ]); + expect(result.files).toBe(2); + expect(result.checked).toBe(3); + expect(result.resolved).toBe(2); + }); + + it('stays green when every stated path exists, directories included', () => { + const result = withTree( + (write) => { + write('packages/app-shell/src/layout/UnifiedSidebar.tsx', 'export {};\n'); + write('packages/app-shell/src/providers/AppProvider.tsx', 'export {};\n'); + write( + 'skills/objectui/guides/architecture.md', + [ + 'Sidebar: `packages/app-shell/src/layout/UnifiedSidebar.tsx`.', + 'Providers live under `packages/app-shell/src/providers/`.', + ].join('\n'), + ); + }, + (dir) => scan(dir, NO_BASELINE), + ); + expect(result.missing).toEqual([]); + expect(result.resolved).toBe(2); + }); + + it('does not count pattern tokens as assertions', () => { + const result = withTree( + (write) => { + write('skills/objectui/rules/no-touch-zones.md', 'Never touch `packages/components/src/ui/**`.\n'); + }, + (dir) => scan(dir, NO_BASELINE), + ); + expect(result.missing).toEqual([]); + expect(result.checked).toBe(0); + expect(result.patterns.map((p: { token: string }) => p.token)).toEqual(['packages/components/src/ui/**']); + }); +}); + +describe('scan — the baseline is a ratchet, red in both directions', () => { + const GUIDE = 'skills/objectui/guides/console-development.md'; + const TOKEN = 'apps/console/src/context/'; + const negativeSentence = `There is no \`${TOKEN}\` directory at all.\n`; + + it('lets a declared negative statement through without hiding it', () => { + const result = withTree( + (write) => write(GUIDE, negativeSentence), + (dir) => scan(dir, baselineFor(GUIDE, TOKEN, 'deliberate negative statement')), + ); + expect(result.missing).toEqual([]); + // Baselined is not the same as invisible: it is still reported, with reason. + expect(result.exempt).toHaveLength(1); + expect(result.exempt[0].token).toBe(TOKEN); + expect(result.exempt[0].reason).toBe('deliberate negative statement'); + expect(result.exempt[0].issue).toBe('objectui#3735'); + expect(result.staleNowExists).toEqual([]); + expect(result.staleUnseen).toEqual([]); + }); + + it('goes red, naming the entry, when the exempted path APPEARS on disk', () => { + // The prose was written around "this does not exist". The day it exists, the + // sentence is false — and a plain existence check would have gone quietly + // green, which is how an exemption list rots. + const result = withTree( + (write) => { + write(GUIDE, negativeSentence); + write(`${TOKEN}NavigationContext.tsx`, 'export {};\n'); + }, + (dir) => scan(dir, baselineFor(GUIDE, TOKEN)), + ); + expect(result.missing).toEqual([]); + expect(result.exempt).toEqual([]); + expect(result.staleNowExists).toHaveLength(1); + expect(result.staleNowExists[0].file).toBe(GUIDE); + expect(result.staleNowExists[0].token).toBe(TOKEN); + expect(result.staleNowExists[0].issue).toBe('objectui#3735'); + }); + + it('goes red when the scan never meets a baseline entry', () => { + // The sentence was rewritten (or the guide moved). The entry is dead weight, + // and an entry nobody removes is how a baseline becomes a skip-list. + const result = withTree( + (write) => write(GUIDE, 'The five contexts live in `packages/app-shell/src/context/`.\n'), + (dir) => scan(dir, baselineFor(GUIDE, TOKEN)), + ); + expect(result.staleUnseen).toEqual([`${GUIDE} ${TOKEN}`]); + // The one real path in the rewritten sentence is still judged normally. + expect(result.missing.map((m: { token: string }) => m.token)).toEqual(['packages/app-shell/src/context/']); + }); + + it('goes red when the baselined guide file is gone entirely', () => { + const result = withTree( + (write) => write('skills/objectui/guides/other.md', '# Other\n'), + (dir) => scan(dir, baselineFor(GUIDE, TOKEN)), + ); + expect(result.staleUnseen).toEqual([`${GUIDE} ${TOKEN}`]); + }); + + it('exempts one token in one file, not the token everywhere', () => { + // An exemption is a statement about a sentence, so it must not travel: the + // same dead path stated as fact in another guide is still a defect. + const result = withTree( + (write) => { + write(GUIDE, negativeSentence); + write('skills/objectui/guides/app-composition.md', `Contexts live in \`${TOKEN}\`.\n`); + }, + (dir) => scan(dir, baselineFor(GUIDE, TOKEN)), + ); + expect(result.exempt).toHaveLength(1); + expect(result.missing).toEqual([ + { file: 'skills/objectui/guides/app-composition.md', line: 1, token: TOKEN }, + ]); + }); +}); + +describe('repo state — the gate is green on this tree', () => { + const result = scan(repoRoot); + + it('has no dead path stated in any guide', () => { + expect( + result.missing.map((m: { file: string; line: number; token: string }) => `${m.file}:${m.line} ${m.token}`), + 'Run `pnpm check:skills-paths` for the full report and the fix guidance.', + ).toEqual([]); + }); + + it('has no stale baseline entry in either direction', () => { + expect( + result.staleNowExists.map((s: { file: string; token: string }) => `${s.file} ${s.token}`), + `These paths now exist — fix the prose, then delete the entry from ${BASELINE_FILE}.`, + ).toEqual([]); + expect(result.staleUnseen, `These entries are no longer stated in any guide — delete them.`).toEqual([]); + }); + + it('actually read the guides rather than silently matching nothing', () => { + // The empty-verdict trap: a broken extractor or a moved scan root satisfies + // both assertions above by finding nothing. Floors, not exact counts, so + // ordinary guide edits do not touch this file — measured at 18 files and + // 86 assertions on main@6422aa891. + expect(result.files).toBeGreaterThanOrEqual(15); + expect(result.checked).toBeGreaterThan(50); + expect(result.resolved).toBe(result.checked - result.exempt.length); + }); + + it('keeps every baseline entry attached to a reason and an issue', () => { + const baseline = readBaseline(repoRoot) as { + allowedMissing: Record>; + }; + for (const [file, tokens] of Object.entries(baseline.allowedMissing)) { + for (const [token, entry] of Object.entries(tokens)) { + expect(entry.issue, `${file} ${token} must name the issue that granted the exemption`).toMatch(/#\d+/); + expect( + entry.reason?.length ?? 0, + `${file} ${token} must say WHY the prose states a path that does not exist`, + ).toBeGreaterThan(20); + } + } + }); + + it('exits 0 when run as the CLI', () => { + const out = execFileSync('node', ['scripts/check-skills-paths.mjs'], { cwd: repoRoot, encoding: 'utf8' }); + expect(out).toMatch(/check-skills-paths: OK/); + }); +}); + +describe('objectui#3713 / #3730 — the guide those two rounds corrected by hand', () => { + const GUIDE = 'skills/objectui/guides/console-development.md'; + const result = scan(repoRoot); + const inGuide = (rows: T[]) => rows.filter((r) => r.file === GUIDE); + + it('states no dead path any more', () => { + expect(inGuide(result.missing).map((m: { line: number; token: string }) => `${m.line} ${m.token}`)).toEqual([]); + }); + + it('is still the densest guide, so the assertion above is not vacuous', () => { + // #3730 alone corrected 13 coordinates in this one file. If its path-bearing + // prose ever collapses to nothing, the pin above would pass for the wrong + // reason. + const stated = extractPathTokens(fs.readFileSync(path.join(repoRoot, GUIDE), 'utf8')).filter( + (h: Hit) => !h.pattern, + ); + expect(stated.length).toBeGreaterThan(40); + }); + + it('carries exactly the one exemption, the deliberate negative sentence', () => { + expect(inGuide(result.exempt).map((e: { token: string }) => e.token)).toEqual(['apps/console/src/context/']); + }); +}); + +describe('wiring — the gate is reachable and a markdown-only PR starts it', () => { + const SCRIPT = 'scripts/check-skills-paths.mjs'; + const workflowDir = path.join(repoRoot, '.github/workflows'); + const workflowPath = path.join(workflowDir, 'skills-paths.yml'); + const workflowFiles = fs.readdirSync(workflowDir).filter((f) => f.endsWith('.yml')); + + /** + * A workflow's YAML with whole-line comments removed — this file's own header + * discusses `paths` and `paths-ignore` in prose, and `docs-links.yml` / + * `control-bytes.yml` name each other's scripts in theirs. A scan that counted + * comments would report filters and duplicate homes that no file has. + */ + const yamlOf = (file: string) => + fs + .readFileSync(path.join(workflowDir, file), 'utf8') + .split('\n') + .filter((line) => !/^\s*#/.test(line)) + .join('\n'); + + it('is exposed as a root package script', () => { + const pkg = JSON.parse(fs.readFileSync(path.join(repoRoot, 'package.json'), 'utf8')); + expect(pkg.scripts['check:skills-paths']).toBe(`node ${SCRIPT}`); + }); + + it('has a workflow that gates pull requests, not just pushes', () => { + expect(fs.existsSync(workflowPath), 'a check nothing runs is not a gate').toBe(true); + const yaml = yamlOf('skills-paths.yml'); + expect(yaml).toMatch(new RegExp(`run:\\s*node\\s+${SCRIPT.replace(/[.]/g, '\\.')}`)); + expect(yaml).toMatch(/^\s*pull_request:/m); + expect(yaml).toMatch(/^\s*push:/m); + }); + + it('runs it in NO path-filtered workflow — the scan surface is entirely markdown', () => { + // The whole reason this is its own workflow. `ci.yml` lists `'**/*.md'` under + // the `paths-ignore` of its `push` trigger and GitHub has no per-job path + // filter, so a push that only edits guides would never start it. That is + // exactly the hole #3448 (docs links) and control-bytes.yml were split out + // to close; a gate whose entire surface is markdown cannot afford to + // rebuild it. + expect(workflowFiles.length, 'the workflow directory scan returned implausibly few files').toBeGreaterThan(5); + for (const file of workflowFiles) { + const yaml = yamlOf(file); + if (!yaml.includes(SCRIPT)) continue; + expect(yaml, `${file} runs ${SCRIPT} behind a paths-ignore — a guide-only change would not start it`).not.toMatch( + /paths-ignore:/, + ); + expect(yaml, `${file} runs ${SCRIPT} behind a paths filter — see objectui#3448`).not.toMatch(/^\s+paths:/m); + } + }); + + it('has exactly one home', () => { + // A second copy in a path-filtered workflow is how a gate ends up looking + // covered while the change it exists for still slips past. + expect(workflowFiles.filter((f) => yamlOf(f).includes(SCRIPT))).toEqual(['skills-paths.yml']); + }); + + it('ships its baseline next to the script', () => { + expect(fs.existsSync(path.join(repoRoot, BASELINE_FILE))).toBe(true); + }); +}); diff --git a/scripts/__tests__/merge-queue-reporting.test.ts b/scripts/__tests__/merge-queue-reporting.test.ts index de12292a59..f3e8345a50 100644 --- a/scripts/__tests__/merge-queue-reporting.test.ts +++ b/scripts/__tests__/merge-queue-reporting.test.ts @@ -66,6 +66,12 @@ const MUST_SUBSCRIBE_MERGE_GROUP = new Map([ 'reason this list exists: it reports on every pull request, so it is requirable, and a ' + 'requirable context that skips the queue build stalls it', ], + [ + 'skills-paths.yml', + 'produces Skill Guide Path Check — added by objectui#3735, same shape as the two above: its ' + + 'entire scan surface is markdown, so it carries no path filter, reports on every pull ' + + 'request, and is therefore requirable', + ], ]); /** Workflows whose path filtering had to move from the trigger into the jobs. */ diff --git a/scripts/check-skills-paths.mjs b/scripts/check-skills-paths.mjs new file mode 100644 index 0000000000..4fbef99cca --- /dev/null +++ b/scripts/check-skills-paths.mjs @@ -0,0 +1,370 @@ +#!/usr/bin/env node +/** + * Every in-repo path an agent skill guide states in a backtick code span must + * exist on disk. + * + * Run: node scripts/check-skills-paths.mjs (also `pnpm check:skills-paths`) + * node scripts/check-skills-paths.mjs --list # every candidate + verdict + * Exit: 0 = every stated path resolves (or is baselined), 1 = one does not, or + * the baseline below has gone stale + * + * ## The gap this closes (objectui#3735) + * + * The guides under `skills/objectui/` are a direct INPUT to every agent that + * writes code in this repo, and their prose gives in-repo paths as coordinates: + * "the five contexts live in ...", "declare the route in ...". Nothing checked + * that any of those paths existed. + * + * `scripts/check-doc-links.mjs` does not reach them twice over. Its `SCAN_ROOTS` + * table (see that file) lists `content/docs`, `examples`, three root markdown + * files, `docs` and the package READMEs — no `skills` row. And it judges + * MARKDOWN LINKS; a bare path inside a code span is not a link, so widening its + * scan roots alone would still have seen none of this. + * + * The cost was paid twice before this gate existed, both times found by eye + * while reading: + * + * - #3713 / PR #3729 — the directory-tree and metadata-registry sections of + * `skills/objectui/guides/console-development.md`. + * - #3730 / PR #3734 — the same file's Key contexts (5 rows), Key hooks + * (7 rows) and UnifiedSidebar sections: 13 real symbols given at + * coordinates that do not exist. + * + * This failure mode is expensive out of proportion to its size, and the reason + * is worth stating: the SYMBOL is usually real and only the path is wrong, so an + * agent does not get a compile error — it gets "file not found" from a `Read`, + * assumes its own search was clumsy, and spends a full lap re-locating something + * the guide claimed to have located for it. It also recurs by construction: the + * app-shell extraction commits (`c1e105793`, `28ffe4033`, `b279d80d6`, + * `cccdf84d7`) moved code with nothing anywhere to say the guides had gone + * stale. + * + * ## What counts as a stated path, and what deliberately does not + * + * Measured on `main@6422aa891`, over the 18 markdown files under `skills`: + * 91 backtick tokens open with one of `PATH_PREFIXES` and contain no + * whitespace. Of those, 5 are patterns rather than paths and 86 are literal + * assertions — 85 resolve, 1 does not, and that one is a deliberate negative + * sentence (the sole baseline entry). So the signal-to-noise ratio is what makes + * this checkable at all: one exemption for 86 assertions. + * + * Three exclusions, each a rule rather than a baseline entry, because none of + * them is a claim that a file exists: + * + * 1. **Whitespace.** A span containing a space is prose, or a command line, or + * a type — not a path. (PR #3856 added a self-check command line to + * `console-development.md` in exactly this shape; it is out of scope by + * this rule alone, with no entry needed anywhere.) + * 2. **Patterns.** A token carrying a glob metacharacter or a placeholder + * segment is a shape, not a location: the guides legitimately write the + * protected-primitive glob under `packages/components/src/ui`, the guide + * glob under `content/docs/guide`, and a Zod-schema path with a + * placeholder domain segment. `existsSync` on any of them is meaningless. + * See `PATTERN_RE`. + * 3. **Fenced blocks.** Only inline code spans in PROSE are read. A fence is a + * worked example, and a tutorial fence may legitimately name a file the + * reader is about to create. Measured: this removes 0 tokens from today's + * tree, so it buys nothing yet and is a scope statement for later — the + * inverse boundary `check-doc-links.mjs` draws for itself, and for the same + * reason (that file's `stripCode` section spells it out). + * + * ## The prefix allow-list is a decision, not an accident + * + * `PATH_PREFIXES` holds the five top-level directories the finding measured. + * This repo has eleven: `docs`, `e2e`, `eslint-rules`, `public` and `patches` + * are deliberately NOT in the list today. That was measured too, so a later + * widening starts from a number instead of a guess: adding all five moves the + * reading from 86 to 88 checked assertions, both new ones resolve, and nothing + * turns red. Cheap — but it is a separate decision from this gate's existence, + * and `check-doc-links.mjs` learned the discipline the hard way (#3479 / #3490 / + * #3545: every scan-surface widening arrives with its own batch of red to + * clear). Widen it on purpose, with the measurement re-run, not as a rider. + * + * Two near-misses that the pattern rule catches today by luck rather than by + * design, recorded so the next reader is not surprised: the guides cite the + * SIBLING framework repo's `packages/spec/src` and `packages/platform-objects` + * trees, neither of which exists in objectui. Both are spelled with a wildcard + * or a placeholder, so rule 2 removes them. A cross-repo path spelled literally + * would be a false red here, and the baseline — with its `reason` field — is + * where that gets recorded if it ever happens. + * + * ## Existence only, not kind + * + * `existsSync`, deliberately: a trailing slash is not required to resolve to a + * directory, nor a `.tsx` suffix to a file. The defect class is "this + * coordinate does not exist", and stretching the gate to judge kind would add a + * second, weaker claim to every green. + */ + +import { existsSync, readdirSync, readFileSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** The scan surface, relative to the repo root: markdown under this directory. */ +export const SCAN_ROOT = 'skills'; + +/** Where the exemptions live. A ratchet — see `readBaseline`. */ +export const BASELINE_FILE = 'scripts/skills-path-baseline.json'; + +/** + * Top-level directories whose name at the start of a code span means "a path in + * this repository". See the docblock section above before adding one. + */ +export const PATH_PREFIXES = ['apps', 'packages', 'examples', 'scripts', 'content']; + +const PREFIX_RE = new RegExp(`^(${PATH_PREFIXES.join('|')})/`); + +/** + * A shape rather than a location: glob metacharacters, an angle-bracket or + * brace placeholder segment, a shell variable, or an elided middle. None of + * these can be handed to `existsSync` and mean anything. + */ +const PATTERN_RE = /[*?[\]{}<>$]|\.\.\./; + +/** Opening or closing fence, matched the way `check-doc-links.mjs` matches it. */ +const FENCE_RE = /^\s*(`{3,}|~{3,})(.*)$/; + +/** An inline code span. Multi-backtick delimiters included, contents captured. */ +const INLINE_CODE_RE = /(`+)([^`\n]*)\1/g; + +/** + * `file token` — the baseline's identity for one exemption. + * + * A SPACE joins them, and that is checkable rather than lucky: a candidate token + * is rejected outright if it contains whitespace, and no path under `skills` + * has a space in it either. (Not a NUL, and not any other control byte: one raw + * U+0000 makes grep classify this whole file as binary — see + * `scripts/check-control-bytes.mjs`, which would also fail on it.) + */ +const keyOf = (file, token) => `${file} ${token}`; + +/** Every markdown file under `dir`, recursively. Absolute paths, sorted. */ +export function markdownFiles(dir, files = []) { + let entries; + try { + entries = readdirSync(dir, { withFileTypes: true }); + } catch { + return files; // caller decides whether a missing surface is an error + } + for (const entry of entries.sort((a, b) => a.name.localeCompare(b.name))) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) markdownFiles(full, files); + else if (entry.name.endsWith('.md')) files.push(full); + } + return files; +} + +/** + * Path-shaped tokens stated in one markdown file's prose. + * + * @returns `{ line, token, pattern }[]` — `pattern: true` marks the tokens rule + * 2 above excludes. They are returned rather than dropped so `--list` + * and the tests can see what the extractor classified, instead of + * having to infer it from an absence. + */ +export function extractPathTokens(source) { + /** @type {{ line: number, token: string, pattern: boolean }[]} */ + const found = []; + let openFence = null; + + source.split('\n').forEach((line, index) => { + const fence = FENCE_RE.exec(line); + + if (openFence) { + const closes = + fence && fence[1][0] === openFence[0] && fence[1].length >= openFence.length && fence[2].trim() === ''; + if (closes) openFence = null; + return; + } + if (fence) { + openFence = fence[1]; + return; + } + + for (const match of line.matchAll(INLINE_CODE_RE)) { + const token = match[2]; + if (!token || /\s/.test(token) || !PREFIX_RE.test(token)) continue; + found.push({ line: index + 1, token, pattern: PATTERN_RE.test(token) }); + } + }); + + return found; +} + +/** + * The exemption list. + * + * A RATCHET, not an escape hatch, and `scan()` enforces that in BOTH directions: + * an entry whose path has appeared on disk is as red as a new dead path (the + * prose now says something false), and so is an entry the scan never met (the + * sentence was rewritten or the file renamed, so the entry is dead weight). + * Without the second direction a baseline degrades into a skip-list nobody dares + * delete from; without the first it silently licenses a claim that has become + * wrong. `KNOWN_OFFENDERS` in `check-control-bytes.mjs` and + * `scripts/i18n-call-site-key-baseline.json` are the two in-repo precedents. + */ +export function readBaseline(root) { + const file = path.join(root, BASELINE_FILE); + const parsed = JSON.parse(readFileSync(file, 'utf8')); + return { allowedMissing: parsed.allowedMissing ?? {} }; +} + +/** Every `file token` key the baseline declares. */ +export function baselineKeys(baseline) { + return Object.entries(baseline.allowedMissing).flatMap(([file, tokens]) => + Object.keys(tokens).map((token) => keyOf(file, token)), + ); +} + +/** + * The one scan. `main()`, `--list` and the test suite all go through here, so + * the tests exercise the real code path rather than a parallel imitation. + * + * Reads the directory, not `git ls-files`: the surface is "markdown on disk + * under `skills`", which lets the tests run the real `scan()` over a fixture + * tree with no git repository in it. + * + * @param root directory to treat as the repository root + * @param baseline `{ allowedMissing: { [file]: { [token]: { reason, issue } } } }` + */ +export function scan(root, baseline = readBaseline(root)) { + const files = markdownFiles(path.join(root, SCAN_ROOT)); + + /** Dead paths nobody declared. Red. */ + const missing = []; + /** Declared dead paths, still dead. Green, but reported. */ + const exempt = []; + /** Declared dead paths that now exist — the prose is now wrong. Red. */ + const staleNowExists = []; + /** Tokens rule 2 excluded. Reported by `--list` only. */ + const patterns = []; + const seen = new Set(); + let checked = 0; + let resolved = 0; + + for (const full of files) { + const file = path.relative(root, full); + for (const hit of extractPathTokens(readFileSync(full, 'utf8'))) { + if (hit.pattern) { + patterns.push({ file, ...hit }); + continue; + } + checked++; + + const entry = baseline.allowedMissing[file]?.[hit.token]; + if (entry) seen.add(keyOf(file, hit.token)); + const record = { file, line: hit.line, token: hit.token }; + + if (existsSync(path.join(root, hit.token))) { + resolved++; + if (entry) staleNowExists.push({ ...record, ...entry }); + continue; + } + if (entry) { + exempt.push({ ...record, ...entry }); + continue; + } + missing.push(record); + } + } + + /** Declared exemptions the scan never met. Red. */ + const staleUnseen = baselineKeys(baseline).filter((key) => !seen.has(key)); + + return { missing, exempt, staleNowExists, staleUnseen, patterns, files: files.length, checked, resolved }; +} + +function repoRoot() { + return path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); +} + +const describe = (r) => `${r.file}:${r.line} — ${r.token}`; + +function main() { + const root = repoRoot(); + const result = scan(root); + const { missing, exempt, staleNowExists, staleUnseen } = result; + + // The empty-verdict trap: a broken extractor, a renamed scan root or a moved + // guide tree would satisfy every assertion above by finding nothing at all. + // A gate that passes because it looked at zero files is not a gate. + if (result.files === 0 || result.checked === 0) { + console.error( + `❌ check-skills-paths: scanned ${result.files} markdown file(s) under ${SCAN_ROOT}/ and found ${result.checked} path assertion(s).\n\n` + + `Nothing to judge means this gate reports OK for the wrong reason. Either the\n` + + `scan surface moved (see SCAN_ROOT) or the extractor stopped matching (see\n` + + `extractPathTokens and PATH_PREFIXES). objectui#3735 measured 18 files and\n` + + `86 assertions on main@6422aa891, so a reading of zero is a broken gate, not\n` + + `a clean tree.`, + ); + process.exit(1); + } + + if (missing.length === 0 && staleNowExists.length === 0 && staleUnseen.length === 0) { + const waived = exempt.length ? `; ${exempt.length} baselined` : ''; + console.log( + `✅ check-skills-paths: OK (${result.resolved}/${result.checked} stated path(s) resolve across ` + + `${result.files} guide file(s)${waived}).`, + ); + process.exit(0); + } + + if (missing.length > 0) { + const phrase = missing.length === 1 ? 'stated path does' : 'stated paths do'; + console.error(`❌ check-skills-paths: ${missing.length} ${phrase} not exist\n`); + for (const r of missing) console.error(` • ${describe(r)}`); + console.error(` +These are coordinates an agent will follow. The symbol named next to one of them +is usually real and only the location is wrong, so nobody gets a compile error — +they get "file not found" from a Read and spend a lap re-locating it (#3713, +#3730 were 13+ of these in one guide). + +Fix the prose. If the path is deliberately named as NOT existing — a sentence +whose whole point is "there is no such directory" — add it to +${BASELINE_FILE} with a reason, and expect the gate to +go red again the day that path appears on disk.`); + } + + if (staleNowExists.length > 0) { + const phrase = staleNowExists.length === 1 ? 'baselined path now EXISTS' : 'baselined paths now EXIST'; + console.error(`\n❌ check-skills-paths: ${staleNowExists.length} ${phrase} on disk:\n`); + for (const r of staleNowExists) console.error(` • ${describe(r)} [${r.issue}] ${r.reason}`); + console.error(` +The exemption said this path does not exist and the prose was written around +that. It exists now, so the sentence is wrong — fix the prose first, then delete +the entry from ${BASELINE_FILE}.`); + } + + if (staleUnseen.length > 0) { + const phrase = staleUnseen.length === 1 ? 'baseline entry' : 'baseline entries'; + console.error(`\n❌ check-skills-paths: ${staleUnseen.length} ${phrase} the scan never met:\n`); + for (const key of staleUnseen) console.error(` • ${key}`); + console.error(` +Written as "file token". The guide no longer states that path — the sentence was +rewritten, or the file moved. Delete the entry from ${BASELINE_FILE}: an +exemption nobody removes is how a baseline turns into a permanent skip-list.`); + } + + process.exit(1); +} + +// Run only when invoked directly — the test suite imports `scan()` and the +// extractor from here and must not trigger a repo scan (or a `process.exit`) on +// import. Same guard shape as `scripts/check-control-bytes.mjs`. +const invokedDirectly = process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url); + +if (invokedDirectly) { + if (process.argv.includes('--list')) { + const result = scan(repoRoot()); + for (const r of result.patterns) console.log(`pattern ${describe(r)}`); + for (const r of result.exempt) console.log(`baselined ${describe(r)} [${r.issue}]`); + for (const r of result.missing) console.log(`MISSING ${describe(r)}`); + console.log( + `\n${result.files} file(s), ${result.checked} assertion(s) checked ` + + `(${result.resolved} resolve, ${result.exempt.length} baselined), ` + + `${result.patterns.length} pattern(s) excluded.`, + ); + } else { + main(); + } +} diff --git a/scripts/skills-path-baseline.json b/scripts/skills-path-baseline.json new file mode 100644 index 0000000000..68fe3c4f7e --- /dev/null +++ b/scripts/skills-path-baseline.json @@ -0,0 +1,32 @@ +{ + "note": [ + "Exemptions for scripts/check-skills-paths.mjs: in-repo paths a guide states in a", + "backtick code span that deliberately do NOT exist on disk. Measured on", + "main@6422aa891: 86 stated paths across the 18 guide files, 85 resolve, and this", + "is the one that does not.", + "", + "A RATCHET, not an allowlist, red in BOTH directions (scan() in that script):", + " - the path APPEARS on disk -> red. The sentence built around 'there is no", + " such directory' has become false; fix the prose, then delete the entry.", + " - the scan never MEETS it -> red. The prose was rewritten or the file", + " moved, so the entry is dead weight; delete it.", + "Entries are keyed by guide file, then by the exact token as written in the", + "prose. Line numbers are deliberately NOT part of the key: guide prose is edited", + "constantly (PR #3856 moved this very file's paragraphs) and a line-keyed", + "exemption would go stale on every unrelated edit.", + "", + "Not everything unresolvable needs an entry. A token containing whitespace, a", + "glob metacharacter or a placeholder segment is excluded by RULE, because it is", + "not a claim that a file exists -- see the script's docblock. Reach for this file", + "only when the prose really does assert the absence of a real coordinate." + ], + + "allowedMissing": { + "skills/objectui/guides/console-development.md": { + "apps/console/src/context/": { + "reason": "Deliberate negative statement. The Key contexts section exists to correct the recurring wrong guess that the five contexts live in the console app: 'there is no apps/console/src/context/ directory at all'. If this directory is ever created, that sentence must be rewritten before the entry is removed.", + "issue": "objectui#3735" + } + } + } +}