diff --git a/.github/scripts/show-dont-tell.mjs b/.github/scripts/show-dont-tell.mjs new file mode 100644 index 0000000..5a6886d --- /dev/null +++ b/.github/scripts/show-dont-tell.mjs @@ -0,0 +1,115 @@ +// show-dont-tell: a pull request that touches a rendered path must show it. +// Node built-ins only. Used by workflows/show-dont-tell.yml. +// +// node show-dont-tell.mjs (env: GH_TOKEN, REPO, PR) +// +// Exits 1 (red) when a changed file matches `paths:` and the PR body carries +// neither an image nor a preview URL; 0 otherwise. Writes markdown to stdout. +import { execFileSync } from 'node:child_process'; +import { existsSync, readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; + +// Glob -> RegExp. `**` crosses `/`, `*` and `?` do not; `**/` may match nothing. +export function globToRegExp(glob) { + let re = ''; + for (let i = 0; i < glob.length; i++) { + const c = glob[i]; + if (c === '*') { + if (glob[i + 1] === '*') { + i++; + if (glob[i + 1] === '/') { i++; re += '(?:.*/)?'; } else re += '.*'; + } else re += '[^/]*'; + } else if (c === '?') re += '[^/]'; + else re += c.replace(/[.+^${}()|[\]\\]/g, '\\$&'); + } + return new RegExp(`^${re}$`); +} + +export function matchPaths(files, globs) { + const res = globs.map(globToRegExp); + return files.filter((f) => res.some((r) => r.test(f))); +} + +// The opt-in file: top-level `paths:` and optional `urls:`, each a "- item" +// list. Comments (#) and quotes are tolerated; nothing else is YAML. +export function parseConfig(text) { + const out = { paths: [], urls: [] }; + let key = null; + for (const raw of text.split(/\r?\n/)) { + const line = raw.replace(/(^|\s)#.*$/, '').trimEnd(); + if (!line.trim()) continue; + const head = /^(paths|urls):\s*$/.exec(line); + if (head) { key = head[1]; continue; } + const item = /^\s*-\s+(.*)$/.exec(line); + if (item && key) out[key].push(item[1].trim().replace(/^(["'])(.*)\1$/, '$2')); + else key = null; + } + return out; +} + +const IMAGE = [ + /!\[[^\]]*\]\(\s*]+/, // markdown image + /]*\bsrc\s*=/i, // html image + /https:\/\/github\.com\/user-attachments\/(?:assets|files)\/[\w-]+/, // dragged-in upload + /https:\/\/user-images\.githubusercontent\.com\/[^\s)"'>]+/, // legacy upload +]; + +// What the body shows: {kind: 'image'|'url'|null, found}. A URL counts only +// when `urls` names its prefix: "any https link" was satisfied by the agent +// attribution link (https://claude.com/claude-code) in every agent-written body. +export function scanBody(body, urls = []) { + const text = body || ''; + // Code spans, fences and HTML comments (a template's "drop a screenshot + // here" hint) only describe a picture; they do not show one. + const shown = text.replace(/```[\s\S]*?```|`[^`\n]*`|/g, ''); + for (const re of IMAGE) { + const m = re.exec(shown); + if (m) return { kind: 'image', found: m[0] }; + } + const all = urls.length ? shown.match(/https:\/\/[^\s)<>"'\]]+/g) || [] : []; + const hit = all.find((u) => urls.some((p) => u.startsWith(p))); + return hit ? { kind: 'url', found: hit } : { kind: null, found: '' }; +} + +export function report({ matched, shown, urls }) { + if (!matched.length) return { ok: true, md: '### show-dont-tell\n\nNo rendered path touched: nothing to show.\n' }; + const list = matched.slice(0, 20).map((f) => `- \`${f}\``).join('\n') + (matched.length > 20 ? `\n- ... and ${matched.length - 20} more` : ''); + if (shown.kind) return { ok: true, md: `### show-dont-tell\n\nRendered paths touched (${matched.length}) and the body shows ${shown.kind === 'image' ? 'an image' : 'a preview URL'}: \`${shown.found}\`\n\n${list}\n` }; + const what = urls.length ? ` or a link starting with one of ${urls.map((u) => `\`${u}\``).join(', ')}` : ''; + return { ok: false, md: `### show-dont-tell: red\n\nThis pull request touches rendered paths:\n\n${list}\n\nIts description carries no picture. Edit the description to add an inline image (\`![what it shows](url)\`, or drag a screenshot in)${what}; the check re-runs when the description is edited.\n` }; +} + +function gh(...args) { return execFileSync('gh', args, { encoding: 'utf8', maxBuffer: 1 << 28 }); } + +function main() { + const [cfgPath] = process.argv.slice(2); + const { REPO, PR, BASE_SHA } = process.env; + // The opt-in file is read at the PR's base, so a PR cannot exempt itself by + // deleting or emptying it; a change to it takes effect once it merges. + let text = null; + if (BASE_SHA) { + try { + text = gh('api', `repos/${REPO}/contents/${cfgPath}?ref=${BASE_SHA}`, '-H', 'Accept: application/vnd.github.raw'); + } catch (e) { + if (!/HTTP 404/.test(String(e.stderr || e.message))) throw e; + } + } else if (existsSync(cfgPath)) text = readFileSync(cfgPath, 'utf8'); + if (text === null) { + console.log(`### show-dont-tell\n\nNo \`${cfgPath}\` in this repository${BASE_SHA ? ' at the base commit' : ''}: not opted in, nothing checked.`); + return 0; + } + const cfg = parseConfig(text); + if (!cfg.paths.length) { + console.log(`### show-dont-tell\n\n\`${cfgPath}\` lists no \`paths:\`: nothing checked.`); + return 0; + } + const files = gh('api', `repos/${REPO}/pulls/${PR}/files`, '--paginate', '--jq', '.[] | .filename, (.previous_filename // empty)').split('\n').filter(Boolean); + const matched = matchPaths(files, cfg.paths); + const body = matched.length ? gh('api', `repos/${REPO}/pulls/${PR}`, '--jq', '.body // ""') : ''; + const r = report({ matched, shown: scanBody(body, cfg.urls), urls: cfg.urls }); + console.log(r.md); + if (!r.ok) console.error('::error::show-dont-tell: rendered paths touched, but the PR description has no image or preview URL.'); + return r.ok ? 0 : 1; +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) process.exit(main()); diff --git a/.github/scripts/show-dont-tell.test.mjs b/.github/scripts/show-dont-tell.test.mjs new file mode 100644 index 0000000..596e9ba --- /dev/null +++ b/.github/scripts/show-dont-tell.test.mjs @@ -0,0 +1,83 @@ +// Tests for show-dont-tell.mjs. Run: node --test .github/scripts/show-dont-tell.test.mjs +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { globToRegExp, matchPaths, parseConfig, report, scanBody } from './show-dont-tell.mjs'; + +test('globs: ** crosses directories, * and ? do not', () => { + const m = (g, p) => globToRegExp(g).test(p); + assert.ok(m('ui/**', 'ui/a/b.tsx')); + assert.ok(m('**/golden/**', 'x/y/golden/a.png')); + assert.ok(m('**/golden/**', 'golden/a.png')); + assert.ok(m('src/*.css', 'src/a.css')); + assert.ok(!m('src/*.css', 'src/deep/a.css')); + assert.ok(m('a?.md', 'ab.md')); + assert.ok(!m('a?.md', 'a/.md')); + assert.ok(!m('ui/**', 'other/ui/a.tsx')); + assert.ok(m('a.b', 'a.b')); + assert.ok(!m('a.b', 'axb')); +}); + +test('matchPaths returns only matching files', () => { + assert.deepEqual(matchPaths(['ui/a.tsx', 'README.md', 'docs/x.md'], ['ui/**', 'docs/*.md']), ['ui/a.tsx', 'docs/x.md']); + assert.deepEqual(matchPaths(['README.md'], ['ui/**']), []); +}); + +test('parseConfig reads paths and urls, tolerates comments and quotes', () => { + const c = parseConfig('# rendered\npaths:\n - "ui/**" # the app\n - docs/*.md\nurls:\n - https://preview.example.com/\n'); + assert.deepEqual(c, { paths: ['ui/**', 'docs/*.md'], urls: ['https://preview.example.com/'] }); + assert.deepEqual(parseConfig(''), { paths: [], urls: [] }); +}); + +test('parseConfig accepts unindented list items', () => { + assert.deepEqual(parseConfig('paths:\n- web/**\n'), { paths: ['web/**'], urls: [] }); +}); + +test('scanBody: an image only named in a code span or fence is not shown', () => { + assert.equal(scanBody('Satisfied by an inline image (`![](..)`)').kind, null); + assert.equal(scanBody('add `![](url)`').kind, null); + assert.equal(scanBody('```\n![x](y.png)\n```').kind, null); + assert.equal(scanBody('`code` then ![x](y.png)').kind, 'image'); + assert.equal(scanBody('').kind, null); + assert.equal(scanBody('').kind, null); + assert.equal(scanBody('\n![x](y.png)').kind, 'image'); + const urls = ['https://preview.example.com/']; + assert.equal(scanBody('urls: `https://preview.example.com/`', urls).kind, null); + assert.equal(scanBody('```\nhttps://preview.example.com/\n```', urls).kind, null); + assert.equal(scanBody('`code` then https://preview.example.com/', urls).kind, 'url'); + assert.equal(scanBody('', urls).kind, null); +}); + +test('scanBody: images count anywhere', () => { + assert.equal(scanBody('hi ![shot](https://x.test/a.png)').kind, 'image'); + assert.equal(scanBody('').kind, 'image'); + assert.equal(scanBody('https://github.com/user-attachments/assets/1234-abcd').kind, 'image'); + assert.equal(scanBody('https://user-images.githubusercontent.com/1/a.png').kind, 'image'); +}); + +test('scanBody: without urls, no link counts, the agent attribution link least of all', () => { + assert.equal(scanBody('see https://pr-4.preview.example.com/x').kind, null); + assert.equal(scanBody('🤖 Generated with [Claude Code](https://claude.com/claude-code)').kind, null); + assert.equal(scanBody('see https://github.com/o/r/actions/runs/1').kind, null); + assert.equal(scanBody('').kind, null); + assert.equal(scanBody(null).kind, null); +}); + +test('scanBody: with urls, only those prefixes count', () => { + const urls = ['https://preview.example.com/']; + assert.equal(scanBody('https://other.example.org/x', urls).kind, null); + assert.equal(scanBody('https://claude.com/claude-code', urls).kind, null); + assert.equal(scanBody('see http://preview.example.com/pr/4', urls).kind, null); + assert.equal(scanBody('https://preview.example.com/pr/4', urls).kind, 'url'); +}); + +test('report: green with no match, green when shown, red when not', () => { + assert.ok(report({ matched: [], shown: { kind: null }, urls: [] }).ok); + assert.match(report({ matched: [], shown: { kind: null }, urls: [] }).md, /No rendered path touched/); + assert.ok(report({ matched: ['ui/a'], shown: { kind: 'image', found: 'x' }, urls: [] }).ok); + const red = report({ matched: ['ui/a'], shown: { kind: null, found: '' }, urls: [] }); + assert.ok(!red.ok); + assert.match(red.md, /`ui\/a`/); + assert.match(red.md, /inline image/); + assert.doesNotMatch(red.md, /link starting with/); + assert.match(report({ matched: ['ui/a'], shown: { kind: null, found: '' }, urls: ['https://p.example/'] }).md, /link starting with one of `https:\/\/p\.example\/`/); +}); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ba88f5a..e6f1dae 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -98,6 +98,16 @@ jobs: - run: npm ci --prefix .github/scripts/golden-diff --no-audit --no-fund - run: node --test .github/scripts/golden-diff.test.mjs + # The show-dont-tell script's suite (glob matcher, body scanner); no + # network, node built-ins only; seconds. + show-dont-tell-tests: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + with: + persist-credentials: false + - run: node --test .github/scripts/show-dont-tell.test.mjs + # The review-metrics suite; parsing and classing only, no network; seconds. review-metrics-tests: runs-on: ubuntu-latest @@ -110,7 +120,7 @@ jobs: # The single required status check. `always()` lets it evaluate when a # dependency failed or was skipped instead of inheriting the skip. ci-gate: - needs: [nonempty, secret-scan, complexity-tests, duplication-tests, golden-diff-tests, review-metrics-tests] + needs: [nonempty, secret-scan, complexity-tests, duplication-tests, golden-diff-tests, show-dont-tell-tests, review-metrics-tests] if: always() permissions: {} uses: ./.github/workflows/ci-gate.yml diff --git a/.github/workflows/show-dont-tell.yml b/.github/workflows/show-dont-tell.yml new file mode 100644 index 0000000..2ee8962 --- /dev/null +++ b/.github/workflows/show-dont-tell.yml @@ -0,0 +1,91 @@ +name: show-dont-tell + +# A pull request that touches a rendered path must show it: an image or a +# preview URL in its description, else this check is red. Why: a change to +# what people see is judged by looking, and a description that only tells +# ("tweaked the header spacing") makes the reviewer rebuild the page to find +# out. Opt-in per repository; a repository without the file below passes +# with a note. +# +# Caller, in the repository's own workflow (the `edited` type is what makes +# a description edit re-run the check, so keep it): +# +# on: +# pull_request: +# types: [opened, edited, synchronize, reopened] +# jobs: +# show-dont-tell: +# permissions: { contents: read, pull-requests: read } +# uses: mark-brannan/.github/.github/workflows/show-dont-tell.yml@main +# +# Opt-in file, `.github/show-dont-tell.yml` in the calling repository, read +# at the PR's base commit (a PR cannot exempt itself by editing it): +# +# paths: # globs of rendered files; ** crosses /, * and ? do not +# - "web/**" +# - "screenshots/goldens/**" +# urls: # optional: prefixes of this repo's preview URLs +# - https://preview.example.com/ +# +# What satisfies the check: an inline image anywhere in the description +# (`![..](..)`, ``, a github.com/user-attachments upload), or a +# link starting with one of the `urls:` prefixes. With `urls:` absent only an +# image counts: "any https link" was met by the agent attribution link +# (https://claude.com/claude-code) in every agent-written body, so the check +# would never have fired on one. Green with "no rendered path +# touched" when no changed file matches. The description is read live from +# the API, not the event payload, so an edit is judged on its new text. +# Renamed files match on both old and new name. Needs only read scopes. +# (The caller checks nothing out; the job reads the file through the API.) +# +# The script is .github/scripts/show-dont-tell.mjs in this repository (node +# built-ins only, so the job is seconds); it is fetched from `engine-ref`, +# main by default, as golden-diff.yml does, so a caller can try a branch. + +on: + workflow_call: + inputs: + config: + description: "Path of the opt-in file in the calling repository" + type: string + default: ".github/show-dont-tell.yml" + engine-ref: + description: "Ref of mark-brannan/.github to fetch the script from; override to try a branch from a caller before it lands" + type: string + default: main + +jobs: + show-dont-tell: + runs-on: ubuntu-latest + if: github.event_name == 'pull_request' + env: + SCRIPT: ${{ github.repository == 'mark-brannan/.github' && '.github/scripts' || '.show-dont-tell-engine/.github/scripts' }} + steps: + - if: github.repository == 'mark-brannan/.github' + uses: actions/checkout@v7 + with: + persist-credentials: false + - if: github.repository != 'mark-brannan/.github' + uses: actions/checkout@v7 + with: + repository: mark-brannan/.github + ref: ${{ inputs.engine-ref }} + sparse-checkout: | + /.github/scripts/show-dont-tell.mjs + sparse-checkout-cone-mode: false + path: .show-dont-tell-engine + persist-credentials: false + - name: The description shows what the rendered paths changed + env: + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + PR: ${{ github.event.pull_request.number }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + CONFIG: ${{ inputs.config }} + run: | + set -euo pipefail + rc=0 + node "$SCRIPT/show-dont-tell.mjs" "$CONFIG" > "$RUNNER_TEMP/sdt.md" || rc=$? + cat "$RUNNER_TEMP/sdt.md" >> "$GITHUB_STEP_SUMMARY" + cat "$RUNNER_TEMP/sdt.md" + exit "$rc"