From 208eb1889ae462b9bf901b9e92fee3162da581f5 Mon Sep 17 00:00:00 2001 From: Oto Macenauer Date: Wed, 23 Sep 2026 11:46:13 +0200 Subject: [PATCH] feat(actions): check-docs, the contract checks on a docs repo's pull request A third composite action. It runs the same checker publish-docs runs at release time (actions/lib/check.js, rules in contract/RULES.md) on a docs repo's manifest and built headless output, without packing or uploading anything, so a finding that would fail the publish fails the pull request instead, one release earlier. - Errors fail the check; warnings only with strict: true. - One annotation per rule, with a count and the first example: a docs template repeats one finding on every page, and GitHub shows only the first few annotations of a step. The job summary tables every rule and lists every finding. - Paths go in as the repo wrote them, so findings name kb-docs.json and dist/..., not runner paths. Outputs: errors, warnings. - The private-registry inputs the other actions have. The CLI's manifest/output resolution becomes checkWorkspace() and the build's by-rule grouping becomes summarise(), both in check.js, shared by the CLI, this action and the knowledge base build. annotate() can emit titled warnings. The private-registry spec now discovers every actions/*/action.yml instead of listing two, so a new action cannot ship without those inputs. CI runs ./actions/check-docs itself, strict, on the clean single-page fixture, next to its self-test. The kb-docs-add audit report offers the PR check, and writes it only if the user asks. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/ci.yml | 15 +++ CLAUDE.md | 1 + README.md | 7 ++ actions/check-docs/README.md | 61 ++++++++++ actions/check-docs/action.yml | 108 ++++++++++++++++++ actions/check-docs/src/index.js | 77 +++++++++++++ actions/check-docs/src/selftest.js | 151 +++++++++++++++++++++++++ actions/lib/check-cli.js | 39 ++----- actions/lib/check.js | 62 +++++++++- actions/lib/runner.js | 10 +- actions/package.json | 5 +- contract/RULES.md | 7 +- scripts/check-artifact.js | 21 +--- skills/kb-docs-add/references/audit.md | 3 + tests/private-registry.spec.js | 14 ++- 15 files changed, 516 insertions(+), 65 deletions(-) create mode 100644 actions/check-docs/README.md create mode 100644 actions/check-docs/action.yml create mode 100644 actions/check-docs/src/index.js create mode 100644 actions/check-docs/src/selftest.js diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7ab17e6..84a8c85 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -162,6 +162,21 @@ jobs: ' - run: npm run selftest working-directory: actions + # The self-tests call check-docs' entry point directly; this runs the + # composite action itself, the way a docs repo's pull request does, on a + # fixture that must come out clean. + - name: Run check-docs as a docs repo would + id: check-docs + uses: ./actions/check-docs + with: + manifest: tests/fixtures/single-page-bundle/kb-docs.json + dist: tests/fixtures/single-page-bundle + strict: 'true' + - name: check-docs reported no findings + env: + KB_ERRORS: ${{ steps.check-docs.outputs.errors }} + KB_WARNINGS: ${{ steps.check-docs.outputs.warnings }} + run: test "$KB_ERRORS" = 0 && test "$KB_WARNINGS" = 0 # ── 5. Deployment workflow dry run ───────────────────────────────────────── # diff --git a/CLAUDE.md b/CLAUDE.md index e876a27..ceaebdd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -85,6 +85,7 @@ Orchestrator: `scripts/build-vite.js`. Flags: `--local`, `--headless`. - `scripts/artifacts.js` — Safe tarball extraction + tree copy, shared by both fetch paths. Validates archive members (no traversal, no absolute paths, no symlinks) before anything is written, and replaces the old `cp -r`/`tar` shell-outs so the build runs on Windows - `scripts/check-artifact.js` — Runs the publish action's contract checker (`actions/lib/check.js`) on every installed artifact, before hoisting; logs findings grouped by rule, and fails a strict build on an error. `check.js` resolves its parsers from `actions/node_modules` or the root, which pins the same versions - `scripts/hoist-inline-scripts.js` — Moves inline `'), + 'guide/index.html': page(''), + })); + assert.equal(r.code, 0, r.stdout); + assert.deepEqual(r.outputs, { errors: '0', warnings: '2' }); + const annotations = r.stdout.match(/^::warning title=KB-HTML-004::.*$/gm) ?? []; + assert.equal(annotations.length, 1, r.stdout); + assert.match(annotations[0], /KB-HTML-004 ×2, e\.g\. demo\//); +}); + +check('the job summary tables every rule and lists every finding', () => { + const r = action(workspace('summary', { + 'index.html': page(''), + 'guide/index.html': page(''), + })); + assert.match(r.summary, /\| KB-HTML-004 \| warning \| 2 \|/); + assert.match(r.summary, /\| KB-HTML-005 \| warning \| 1 \|/); + assert.match(r.summary, /All 3 finding\(s\)/); + assert.match(r.summary, /contract\/RULES\.md\]\(https:\/\/github\.com\/AbsaOSS\/knowledge-base\/blob\/master\/contract\/RULES\.md\)/); +}); + +check('strict fails on warnings', () => { + const ws = workspace('strict', { 'index.html': page('') }); + const r = action(ws, { KB_CHECK_STRICT: 'true' }); + assert.equal(r.code, 1); + assert.match(r.stdout, /::error::1 warning finding\(s\), and "strict" is on/); + assert.match(r.summary, /The check fails \(strict: warnings fail too\)/); +}); + +check('an error fails the check, annotated as an error with its rule', () => { + const r = action(workspace('error', { 'index.html': page('', 'lang="en"') })); + assert.equal(r.code, 1); + assert.equal(r.outputs.errors, '1'); + assert.match(r.stdout, /^::error title=KB-HTML-001::KB-HTML-001 demo\/index\.html: missing data-kb-headless/m); + assert.match(r.stdout, /::error::1 error finding\(s\): the publish-docs action would refuse this output/); +}); + +check('a missing manifest is a KB-MAN-001 finding, named as the user wrote it', () => { + const r = action(workspace('no-manifest', { 'index.html': page() }, null)); + assert.equal(r.code, 1); + assert.match(r.stdout, /^::error title=KB-MAN-001::KB-MAN-001 kb-docs\.json: No manifest at/m); +}); + +check('custom manifest and dist inputs are honoured, relative to the workspace', () => { + const ws = workspace('custom', {}); + mkdirSync(join(ws, 'docs-meta'), { recursive: true }); + writeFileSync(join(ws, 'docs-meta', 'kb.json'), JSON.stringify(MANIFEST)); + mkdirSync(join(ws, 'site'), { recursive: true }); + writeFileSync(join(ws, 'site', 'index.html'), page()); + const r = action(ws, { KB_MANIFEST: 'docs-meta/kb.json', KB_DIST: 'site' }); + assert.equal(r.code, 0, r.stdout); + const missing = action(ws, { KB_MANIFEST: 'docs-meta/kb.json', KB_DIST: 'build' }); + assert.equal(missing.code, 1); + assert.match(missing.stdout, /KB-ART-001 build: the built output directory does not exist/); +}); + +rmSync(root, { recursive: true, force: true }); +if (failures > 0) { + console.log(`\n\x1b[31m${failures} check(s) failed\x1b[0m`); + process.exit(1); +} +console.log('\nAll checks passed'); diff --git a/actions/lib/check-cli.js b/actions/lib/check-cli.js index 080bcf2..abfd82b 100644 --- a/actions/lib/check-cli.js +++ b/actions/lib/check-cli.js @@ -2,10 +2,10 @@ /** * check-cli.js — runs the contract checks outside a release. * - * The publish-docs action runs the same checks at release time; this is for - * everywhere else: a docs repo's pull-request CI, a local build, an agent - * fixing a repo. It reads nothing but the manifest and the built output, and - * never packs or uploads. + * The publish-docs action runs the same checks at release time and the + * check-docs action on pull requests; this is for everywhere else: a local + * build, an agent fixing a repo. It reads nothing but the manifest and the + * built output, and never packs or uploads. * * node actions/lib/check-cli.js [--manifest kb-docs.json] [--dist dist] [--json] [--strict] * @@ -14,12 +14,8 @@ * caller that wants to act on rule IDs rather than read prose. */ -import { existsSync } from 'node:fs'; -import { resolve } from 'node:path'; - -import { appDirResolver, checkApps } from './check.js'; -import { PublishError, readManifestFile } from './manifest.js'; -import { RULES_DOC, finding, formatFinding } from './rules.js'; +import { checkWorkspace } from './check.js'; +import { RULES_DOC, formatFinding } from './rules.js'; function parseArgs(argv) { const args = { manifest: 'kb-docs.json', dist: 'dist', json: false, strict: false }; @@ -34,27 +30,6 @@ function parseArgs(argv) { return args; } -function collect({ manifest: manifestPath, dist }) { - let manifest; - try { - manifest = readManifestFile(resolve(manifestPath)); - } catch (err) { - if (!(err instanceof PublishError)) throw err; - return [finding('KB-MAN-001', manifestPath, err.message.replace(/^KB-MAN-001 /, ''))]; - } - const distDir = resolve(dist); - if (!existsSync(distDir)) { - return [finding('KB-ART-001', dist, `the built output directory does not exist. Build the site first, or pass --dist.`)]; - } - const appDirFor = appDirResolver(manifest, distDir); - const missing = manifest.apps.filter((app) => !existsSync(appDirFor(app.slug))); - if (missing.length > 0) { - return missing.map((app) => finding('KB-ART-001', app.slug, - `no built output at ${appDirFor(app.slug)} — with several apps, --dist holds one subdirectory per slug.`)); - } - return checkApps(manifest, appDirFor); -} - function main() { const args = parseArgs(process.argv.slice(2)); if (args.help) { @@ -62,7 +37,7 @@ function main() { return 0; } - const findings = collect(args); + const findings = checkWorkspace(args); const errors = findings.filter((f) => f.severity === 'error').length; if (args.json) { diff --git a/actions/lib/check.js b/actions/lib/check.js index b65dc1d..1ff6bc0 100644 --- a/actions/lib/check.js +++ b/actions/lib/check.js @@ -2,8 +2,8 @@ * check.js — checks a built docs site against the contract, rule by rule. * * Runs in the publishing repo: at publish time inside the publish-docs action, - * and on demand through check-cli.js (a repo's own CI, or an agent fixing the - * repo). That is the only place anyone can act on a finding. The knowledge base + * on pull requests inside the check-docs action, and on demand through + * check-cli.js (a local build, or an agent fixing the repo). That is the only place anyone can act on a finding. The knowledge base * build repairs some of the same things when it re-hosts a page — it hoists * inline scripts, strips a theme bootstrap, absolutises CSS URLs — but by then * the artifact is released and the person who can fix it has moved on. @@ -18,13 +18,13 @@ */ import { readdirSync, readFileSync, existsSync } from 'node:fs'; -import { join, relative } from 'node:path'; +import { join, relative, resolve } from 'node:path'; import { parseDocument } from 'htmlparser2'; import postcss from 'postcss'; import { isThemeBootstrap } from '../../src/utils/theme.js'; -import { PublishError } from './manifest.js'; +import { PublishError, readManifestFile } from './manifest.js'; import { RULES_DOC, finding, formatFinding } from './rules.js'; /** The marker the knowledge base looks for on ``. */ @@ -314,6 +314,60 @@ export function checkApps(manifest, appDirFor) { return manifest.apps.flatMap((app) => checkApp(appDirFor(app.slug), app)); } +/** + * Findings for a docs repo as it sits on disk: its manifest and its built output. + * + * What the CLI and the check-docs action run. A manifest that cannot be read or + * fails the schema, and output that is missing, are findings too, rather than + * a crash, so a caller always gets one list to report. + * + * @param {{manifest: string, dist: string}} paths - as the user gave them + */ +export function checkWorkspace({ manifest: manifestPath, dist }) { + let manifest; + try { + manifest = readManifestFile(resolve(manifestPath)); + } catch (err) { + if (!(err instanceof PublishError)) throw err; + return [finding('KB-MAN-001', manifestPath, err.message.replace(/^KB-MAN-001 /, ''))]; + } + const distDir = resolve(dist); + if (!existsSync(distDir)) { + return [finding('KB-ART-001', dist, 'the built output directory does not exist. Build the site first, or point "dist" at its output.')]; + } + const appDirFor = appDirResolver(manifest, distDir); + const missing = manifest.apps.filter((app) => !existsSync(appDirFor(app.slug))); + if (missing.length > 0) { + return missing.map((app) => finding('KB-ART-001', app.slug, + `no built output at ${appDirFor(app.slug)} — with several apps, "dist" holds one subdirectory per slug.`)); + } + return checkApps(manifest, appDirFor); +} + +/** + * Groups findings by rule: one entry per rule with its count and first example, + * because a docs site repeats the same template on every page and forty + * identical warnings bury the one that differs. + */ +export function summarise(findings) { + const byRule = new Map(); + for (const f of findings) { + if (!byRule.has(f.id)) byRule.set(f.id, []); + byRule.get(f.id).push(f); + } + return [...byRule.values()].map((group) => { + const [first] = group; + const count = group.length > 1 ? ` ×${group.length}, e.g.` : ''; + return { + id: first.id, + severity: first.severity, + count: group.length, + first, + line: `${first.id}${count} ${first.where}: ${first.message}`, + }; + }); +} + /** * Checks every app in a manifest for the publish step: warnings are annotated, * and any error stops the publish with every error listed at once. diff --git a/actions/lib/runner.js b/actions/lib/runner.js index 861d9f7..177250c 100644 --- a/actions/lib/runner.js +++ b/actions/lib/runner.js @@ -10,10 +10,14 @@ import { appendFileSync } from 'node:fs'; import { PublishError } from './manifest.js'; -/** Emits an error annotation. Newlines must be percent-encoded to survive. */ -export function annotate(message) { +/** + * Emits an annotation — an error unless `level` says otherwise, titled when a + * `title` is given. Newlines must be percent-encoded to survive. + */ +export function annotate(message, { level = 'error', title } = {}) { const encoded = String(message).replace(/%/g, '%25').replace(/\r/g, '%0D').replace(/\n/g, '%0A'); - process.stdout.write(`::error::${encoded}\n`); + const props = title ? ` title=${String(title).replace(/[,:]/g, ' ')}` : ''; + process.stdout.write(`::${level}${props}::${encoded}\n`); } /** Appends `key=value` to the runner's step-output file when running in CI. */ diff --git a/actions/package.json b/actions/package.json index c6a50e2..76cd325 100644 --- a/actions/package.json +++ b/actions/package.json @@ -6,12 +6,13 @@ "type": "module", "license": "Apache-2.0", "scripts": { - "selftest": "node publish-single-page-docs/src/selftest.js && node publish-docs/src/selftest.js && node lib/release.selftest.js && node lib/check.selftest.js", + "selftest": "node publish-single-page-docs/src/selftest.js && node publish-docs/src/selftest.js && node lib/release.selftest.js && node lib/check.selftest.js && node check-docs/src/selftest.js", "selftest:check": "node lib/check.selftest.js", "check": "node lib/check-cli.js", "selftest:single-page": "node publish-single-page-docs/src/selftest.js", "selftest:docs": "node publish-docs/src/selftest.js", - "selftest:release": "node lib/release.selftest.js" + "selftest:release": "node lib/release.selftest.js", + "selftest:check-docs": "node check-docs/src/selftest.js" }, "dependencies": { "ajv": "8.17.1", diff --git a/contract/RULES.md b/contract/RULES.md index 0849ccd..aae0d93 100644 --- a/contract/RULES.md +++ b/contract/RULES.md @@ -18,9 +18,10 @@ it has not seen before. ## Running the checks -The `publish-docs` action runs them before it packs anything. To run them earlier — on -a pull request, or while fixing a repo — use the same code from a checkout of this -repository: +The `publish-docs` action runs them before it packs anything. On a pull request, the +[`check-docs`](../actions/check-docs) action runs the same checks without releasing +anything: add it next to the publish workflow, with the same build step and `dist`. +Locally — or while fixing a repo — use the same code from a checkout of this repository: ```bash git clone --depth 1 https://github.com/AbsaOSS/knowledge-base.git /tmp/knowledge-base diff --git a/scripts/check-artifact.js b/scripts/check-artifact.js index 00d0992..0ef17f3 100644 --- a/scripts/check-artifact.js +++ b/scripts/check-artifact.js @@ -17,27 +17,10 @@ * (tests/artifact-checks.spec.js holds the two in step). */ +import { summarise } from '../actions/lib/check.js'; import { RULES_DOC, formatFinding } from '../actions/lib/rules.js'; -export { checkApp } from '../actions/lib/check.js'; - -/** - * Groups findings by rule: one line per rule with its count and first example, - * because a docs site repeats the same template on every page and forty - * identical warnings bury the one that differs. - */ -export function summarise(findings) { - const byRule = new Map(); - for (const f of findings) { - if (!byRule.has(f.id)) byRule.set(f.id, []); - byRule.get(f.id).push(f); - } - return [...byRule.values()].map((group) => { - const [first] = group; - const count = group.length > 1 ? ` ×${group.length}, e.g.` : ''; - return { id: first.id, severity: first.severity, line: `${first.id}${count} ${first.where}: ${first.message}` }; - }); -} +export { checkApp, summarise } from '../actions/lib/check.js'; /** * Reports one artifact's findings through the build's logger. diff --git a/skills/kb-docs-add/references/audit.md b/skills/kb-docs-add/references/audit.md index d948c87..4c796b3 100644 --- a/skills/kb-docs-add/references/audit.md +++ b/skills/kb-docs-add/references/audit.md @@ -130,3 +130,6 @@ Report: release. - The one manual step: publish a release so the fixed output reaches the knowledge base. The registry entry does not change. +- If the repo has no pull-request check yet, say that `AbsaOSS/knowledge-base/actions/check-docs@v1` + runs these same checks on every PR (packaged sites only). Offer it, and write that + workflow only if the user asks. diff --git a/tests/private-registry.spec.js b/tests/private-registry.spec.js index c0632fb..ed18f98 100644 --- a/tests/private-registry.spec.js +++ b/tests/private-registry.spec.js @@ -17,7 +17,7 @@ import { test, expect } from '@playwright/test'; import { spawnSync } from 'node:child_process'; -import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -28,7 +28,11 @@ const SCRIPT = join(ROOT, 'actions', 'lib', 'npm-registry.sh'); const LOCKFILES = ['package-lock.json', 'actions/package-lock.json']; -const ACTIONS = ['actions/publish-docs/action.yml', 'actions/publish-single-page-docs/action.yml']; +// Every action under actions/, discovered rather than listed, so a new one +// cannot ship without the inputs a private-network runner needs. +const ACTIONS = readdirSync(join(ROOT, 'actions'), { withFileTypes: true }) + .filter((d) => d.isDirectory() && existsSync(join(ROOT, 'actions', d.name, 'action.yml'))) + .map((d) => `actions/${d.name}/action.yml`); const WORKFLOW = '.github/workflows/build-image.yml'; /** The inputs every consumer-facing manifest has to offer, by the same names. */ @@ -59,6 +63,12 @@ test.describe('lockfiles resolve to the public registry', () => { }); test.describe('the private-registry inputs exist on every shared CI piece', () => { + test('every action is found, including check-docs', () => { + expect(ACTIONS).toEqual(expect.arrayContaining([ + 'actions/check-docs/action.yml', 'actions/publish-docs/action.yml', 'actions/publish-single-page-docs/action.yml', + ])); + }); + for (const rel of ACTIONS) { test(`${rel} declares the inputs and installs through the shared script`, () => { const text = read(rel);