diff --git a/.changeset/verify-attach-evidence-parts.md b/.changeset/verify-attach-evidence-parts.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/verify-attach-evidence-parts.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/.claude/skills/verify-clerk-expo/SKILL.md b/.claude/skills/verify-clerk-expo/SKILL.md index 683b6839710..4ac5d43e450 100644 --- a/.claude/skills/verify-clerk-expo/SKILL.md +++ b/.claude/skills/verify-clerk-expo/SKILL.md @@ -238,7 +238,7 @@ For screenshots, show each screen the change touches, and each state of a screen Some states are out of reach for a test user on the standard settings with the default roles. Try `host.useSettings` in the spec, or test data that the spec seeds, first. When neither reaches it, force the state with a small local patch to the build. Never commit or push the patch. Put the same patch on the before build and the after build, and run both on a local device, because `attach` refuses a run on a session build with uncommitted changes. In the pull request, say exactly what the patch changed and why no test user could reach the state without it. -`attach` puts the files in the description of the pull request with `gh pr edit --attach`. It writes one block for the platform of the run, between the comments `` and ``, or the same two with `android`. The block is a line and then one or more tables. The line leads with the result and names the run, the device, the commit, and how many tests passed. With `--before` it also names the before run, its commit, and how many of its tests passed. With `--before`, one table has a `Before` and an `After` column, and a row for each screenshot or one row for the video. A `--new-screen` row has `New screen` in place of its before. Without it, the screenshots are two across, each under its label in code font, and the video is alone in a table under its name. Each screenshot is 280 pixels wide. +`attach` puts the files in the description of the pull request with `gh pr edit --attach`. It writes one block for the platform of the run, between the comments `` and ``, or the same two with `android`. The block is a line and then one or more tables. The line leads with the result and names the run, the device, the commit, and how many tests passed. With `--before` it also names the before run, its commit, and how many of its tests passed. With `--before`, one table has a `Before` and an `After` column, and a row for each screenshot or one row for the video. A `--new-screen` row has `New screen` in place of its before. Without it, the screenshots are two across, each under its label in code font, and the video is alone in a table under its name. Each screenshot is 280 pixels wide. Under a video's table, a numbered list names the parts the run marked with `host.evidence`, in the order they started, so a reviewer, and CodeRabbit, which cannot watch a video, can tell which states the video shows. With `--before`, one list says each video shows those parts when both runs marked the same ones, and otherwise the block has a list for the before video and one for the after video. A part whose test failed inside its block says it stopped before it finished. Past 20 parts, the last line says how many more there are. The first `attach` of a platform adds its block after the description. A later `attach` of that platform replaces its block, so the description holds the latest run of each platform and the media does not pile up. `attach` changes nothing outside the block. Keep both comments of a block or remove both: `attach` refuses a description that has one without the other, or either one twice. A comment counts only when it is a whole line outside a code fence, so a description can quote one in a sentence or show a whole block as an example. diff --git a/scripts/verify-attach.mjs b/scripts/verify-attach.mjs index 65483065014..3e090c52d93 100644 --- a/scripts/verify-attach.mjs +++ b/scripts/verify-attach.mjs @@ -22,7 +22,9 @@ const LIMITS = { const ARTIFACT = 'verify-evidence'; const MANIFEST_FILE = 'manifest.json'; const SESSION_WORKFLOW = '.github/workflows/verify-remote.yml'; -const MANIFEST_VERSION = 2; +const MANIFEST_VERSION = 3; +// Version 2 came before the parts of the video, and sessions that started on an older commit still write it. +const VERSIONS_TAKEN = [2, MANIFEST_VERSION]; const SHAPE = { run: /^r\d{8}-\d{6}-[0-9a-f]{4}$/, @@ -35,8 +37,25 @@ const SHAPE = { actor: /^[A-Za-z0-9][A-Za-z0-9_-]{0,99}(\[bot\])?$/, }; -const MANIFEST_KEYS = ['v', 'pr', 'run', 'platform', 'device', 'commit', 'passed', 'flaky', 'total', 'files', 'before']; -const BEFORE_KEYS = ['run', 'commit', 'passed', 'flaky', 'total', 'files']; +const MANIFEST_KEYS = [ + 'v', + 'pr', + 'run', + 'platform', + 'device', + 'commit', + 'passed', + 'flaky', + 'total', + 'files', + 'before', + 'parts', +]; +const KEYS_OF_VERSION_2 = MANIFEST_KEYS.filter(key => key !== 'parts'); +const PART = /^[^\u0000-\u001f<>`|]{1,200}$/; +const MOST_PARTS = 20; +const BEFORE_KEYS = ['run', 'commit', 'passed', 'flaky', 'total', 'files', 'parts']; +const BEFORE_KEYS_OF_VERSION_2 = BEFORE_KEYS.filter(key => key !== 'parts'); const FILE_KEYS = ['name', 'bytes', 'sha256']; class Refused extends Error {} @@ -251,8 +270,8 @@ function parseFiles(list, what) { return files; } -function parseBefore(value, run, files) { - const raw = record(value, BEFORE_KEYS, 'before'); +function parseBefore(value, run, files, withParts) { + const raw = record(value, withParts ? BEFORE_KEYS : BEFORE_KEYS_OF_VERSION_2, 'before'); const before = { run: shaped(raw.run, SHAPE.run, 'the run of before'), commit: shaped(raw.commit, SHAPE.commit, 'the commit of before'), @@ -260,6 +279,7 @@ function parseBefore(value, run, files) { flaky: whole(raw.flaky, 0, LIMITS.tests, 'flaky of before'), total: whole(raw.total, 1, LIMITS.tests, 'total of before'), files: parseBeforeFiles(raw.files, files), + parts: withParts ? parseParts(raw.parts) : [], }; if (before.run === run) { refuse('before is the run itself'); @@ -286,6 +306,35 @@ function parseBeforeFiles(list, files) { return paired; } +function parseParts(list) { + if (!Array.isArray(list) || list.length > MOST_PARTS) { + refuse(`parts must be a list of at most ${MOST_PARTS}`); + } + return list.map((part, i) => { + if (typeof part !== 'string' || !PART.test(part)) { + refuse(`part ${i + 1} must be 1 to 200 characters of one line, without <, >, \` or |`); + } + return part; + }); +} + +const listed = (heading, parts) => + [`${heading} these parts, in order:`, ...parts.map((part, at) => `${at + 1}. ${part}`)].join('\n'); + +// The before video is cut to the before run's own parts, which differ when that run failed partway or seeded other users. +function partsSection(parts, beforeParts) { + if (beforeParts === null) { + return parts.length === 0 ? [] : [listed('The video shows', parts)]; + } + if (parts.length > 0 && parts.length === beforeParts.length && parts.every((part, at) => part === beforeParts[at])) { + return [listed('Each video shows', parts)]; + } + return [ + ...(beforeParts.length === 0 ? [] : [listed('The before video shows', beforeParts)]), + ...(parts.length === 0 ? [] : [listed('The after video shows', parts)]), + ]; +} + function parseManifest(text) { if (Buffer.byteLength(text) > LIMITS.manifestBytes) { refuse(`the manifest is over ${LIMITS.manifestBytes} bytes`); @@ -297,16 +346,17 @@ function parseManifest(text) { return refuse('the manifest is not JSON'); } const { v } = anObject(parsed, 'the manifest'); - if (v !== MANIFEST_VERSION) { + if (!VERSIONS_TAKEN.includes(v)) { const found = Number.isInteger(v) && v >= 0 && v <= 999 ? `version ${v}` : 'of no version this script knows'; - refuse(`the manifest is ${found}, and this script publishes only version ${MANIFEST_VERSION}`); + refuse(`the manifest is ${found}, and this script publishes only versions ${VERSIONS_TAKEN.join(' and ')}`); } - const raw = record(parsed, MANIFEST_KEYS, 'the manifest'); + const raw = record(parsed, v === MANIFEST_VERSION ? MANIFEST_KEYS : KEYS_OF_VERSION_2, 'the manifest'); + const parts = v === MANIFEST_VERSION ? parseParts(raw.parts) : []; if (raw.platform !== 'ios' && raw.platform !== 'android') { refuse('platform must be ios or android'); } const files = parseFiles(raw.files, 'files'); - const before = raw.before === null ? null : parseBefore(raw.before, raw.run, files); + const before = raw.before === null ? null : parseBefore(raw.before, raw.run, files, v === MANIFEST_VERSION); const all = everyFile({ files, before }); if (all.length > LIMITS.files) { refuse(`at most ${LIMITS.files} files in all`); @@ -329,6 +379,7 @@ function parseManifest(text) { total: whole(raw.total, 1, LIMITS.tests, 'total'), files, before, + parts, }; if (manifest.passed + manifest.flaky < 1 || manifest.passed + manifest.flaky > manifest.total) { refuse('passed and flaky must count at least one test and at most total'); @@ -486,11 +537,14 @@ function evidenceBlock(manifest, session, onDisk = file => file.name, uploaded = const media = uploaded === null ? everyFile(manifest).map(file => `![${isVideo(file.name) ? file.name : stem(file.name)}](./${onDisk(file)})`) - : mediaTables( - manifest.files.map(file => file.name), - before === null ? null : before.files.map(file => file !== null), - uploaded, - ); + : [ + ...mediaTables( + manifest.files.map(file => file.name), + before === null ? null : before.files.map(file => file !== null), + uploaded, + ), + ...partsSection(manifest.parts, before === null ? null : before.parts), + ]; return [start, '', line, ...media.flatMap(part => ['', part]), '', end].join('\n'); } diff --git a/scripts/verify-attach.test.mjs b/scripts/verify-attach.test.mjs index 2a6f2eb1a30..6d824d0e4e2 100644 --- a/scripts/verify-attach.test.mjs +++ b/scripts/verify-attach.test.mjs @@ -37,7 +37,7 @@ const JPG = Buffer.concat([Buffer.from([0xff, 0xd8, 0xff, 0xe0]), Buffer.from('t const MP4 = Buffer.concat([Buffer.from([0, 0, 0, 0x18]), Buffer.from('ftypmp42'), Buffer.alloc(4096, 'v')]); const manifestFor = (files, change = {}) => ({ - v: 2, + v: 3, pr: 669, run: 'r20261008-052713-253e', platform: 'ios', @@ -48,6 +48,7 @@ const manifestFor = (files, change = {}) => ({ total: 3, files: files.map(([name, data]) => ({ name, bytes: data.length, sha256: sha256(data) })), before: null, + parts: [], ...change, }); @@ -151,6 +152,7 @@ const beforeFor = (files, change = {}) => ({ flaky: 0, total: 3, files: files.map(([name, data]) => ({ name, bytes: data.length, sha256: sha256(data) })), + parts: [], ...change, }); const WITH_BEFORE = manifestFor(MEDIA, { before: beforeFor(WAS_MEDIA) }); @@ -557,25 +559,54 @@ describe('the manifest', () => { const { before: _, ...versionOne } = { ...GOOD, v: 1 }; assert.equal( refusal(() => parseManifest(JSON.stringify(versionOne))), - 'the manifest is version 1, and this script publishes only version 2', + 'the manifest is version 1, and this script publishes only versions 2 and 3', ); assert.equal( - refusal(() => parseManifest(JSON.stringify({ ...GOOD, v: 3, layout: 'a key of a later version' }))), - 'the manifest is version 3, and this script publishes only version 2', + refusal(() => parseManifest(JSON.stringify({ ...GOOD, v: 4, layout: 'a key of a later version' }))), + 'the manifest is version 4, and this script publishes only versions 2 and 3', ); for (const v of ['2', 2.5, -1, 1000, null, '\n::error::pwned']) { assert.equal( refusal(() => parse({ v })), - 'the manifest is of no version this script knows, and this script publishes only version 2', + 'the manifest is of no version this script knows, and this script publishes only versions 2 and 3', JSON.stringify(v), ); } assert.equal( refusal(() => parseManifest(JSON.stringify({ device: 'x' }))), - 'the manifest is of no version this script knows, and this script publishes only version 2', + 'the manifest is of no version this script knows, and this script publishes only versions 2 and 3', ); }); + it('reads a version 2 manifest, which has no parts, as one with no parts', () => { + const { parts: _, ...versionTwo } = { ...GOOD, v: 2 }; + assert.deepEqual(parseManifest(JSON.stringify(versionTwo)), { ...GOOD, parts: [] }); + assert.match( + refusal(() => parseManifest(JSON.stringify({ ...versionTwo, parts: [] }))), + /the manifest must have exactly the keys/, + ); + }); + + it('refuses parts that are not a short list of one-line labels without markup', () => { + assert.deepEqual(parseManifest(JSON.stringify({ ...GOOD, parts: ['search members'] })).parts, ['search members']); + for (const parts of [ + 'a label', + [''], + ['two\nlines'], + ['bold'], + ['a `tick`'], + ['a | pipe'], + ['x'.repeat(201)], + Array.from({ length: 21 }, () => 'a part'), + ]) { + assert.match( + refusal(() => parseManifest(JSON.stringify({ ...GOOD, parts }))), + /^part|^parts/, + JSON.stringify(parts).slice(0, 30), + ); + } + }); + it('accepts a before run, which may have passed nothing, with one file of the same kind for each file of the run', () => { assert.deepEqual(parseManifest(JSON.stringify(WITH_BEFORE)), WITH_BEFORE); }); @@ -1466,8 +1497,11 @@ describe('the step that edits the description', () => { const WAS_SHOT = [['before-profile.png', WAS_PNG]]; const CLIP = [['video.mp4', MP4]]; const WAS_CLIP = [['before-video.mp4', WAS_MP4]]; - const published = async (media, wasMedia) => { - const manifest = manifestFor(media, wasMedia === undefined ? {} : { before: beforeFor(wasMedia) }); + const published = async (media, wasMedia, parts = [], beforeParts = parts) => { + const manifest = manifestFor(media, { + parts, + ...(wasMedia === undefined ? {} : { before: beforeFor(wasMedia, { parts: beforeParts }) }), + }); const fake = fakeGh({ ...PULL, body: '' }); const { outcome } = publish(await fetched(manifest, [...media, ...(wasMedia ?? [])]), fake); assert.equal(outcome, 'published'); @@ -1515,6 +1549,65 @@ describe('the step that edits the description', () => { ); }); + it('lists the parts of a video under its table, in the order the run marked them', async () => { + const parts = ['search members for a +clerk_test address', 'search members for a name nobody has']; + assert.equal( + await published(CLIP, WAS_CLIP, parts), + laidOut( + `${SAID}${WAS}`, + '', + '| Before | After |', + '| :---: | :---: |', + `| ${asset(2)} | ${asset(1)} |`, + '', + 'Each video shows these parts, in order:', + `1. ${parts[0]}`, + `2. ${parts[1]}`, + ), + ); + assert.equal( + await published(CLIP, undefined, parts), + laidOut( + SAID, + '', + '| `video` |', + '| :---: |', + `| ${asset(1)} |`, + '', + 'The video shows these parts, in order:', + `1. ${parts[0]}`, + `2. ${parts[1]}`, + ), + ); + assert.equal( + await published(CLIP, WAS_CLIP, parts, [parts[0]]), + laidOut( + `${SAID}${WAS}`, + '', + '| Before | After |', + '| :---: | :---: |', + `| ${asset(2)} | ${asset(1)} |`, + '', + 'The before video shows these parts, in order:', + `1. ${parts[0]}`, + '', + 'The after video shows these parts, in order:', + `1. ${parts[0]}`, + `2. ${parts[1]}`, + ), + ); + }); + + it('reads the before of a version 2 manifest as having no parts', () => { + const { parts: _, ...versionTwo } = { ...WITH_BEFORE, v: 2 }; + const { parts: __, ...beforeTwo } = WITH_BEFORE.before; + assert.deepEqual(parseManifest(JSON.stringify({ ...versionTwo, before: beforeTwo })), { + ...WITH_BEFORE, + parts: [], + before: { ...WITH_BEFORE.before, parts: [] }, + }); + }); + it('lays out a flow that is new as its video alone, in a table that keeps it from the full width', async () => { assert.equal(await published(CLIP), laidOut(SAID, '', '| `video` |', '| :---: |', `| ${asset(1)} |`)); });