Build: Skip the canary PR body update when the description has no markers

.github/scripts/replace-pr-body-markers.ts

@@ -3,17 +3,24 @@ import { readFileSync } from 'node:fs';
 import { resolve } from 'node:path';
 import { fileURLToPath } from 'node:url';
 
-export function replaceMarker(body: string, marker: string, replacement: string): string {
-  const pattern = new RegExp(
+function markerPattern(marker: string): RegExp {
+  return new RegExp(
     `(<!-- ${escapeRegExp(marker)} -->)(.*?)(<!-- ${escapeRegExp(marker)} -->)`,
     's'
   );
-  const matches = body.match(new RegExp(pattern.source, 'gs'));
-  if (matches?.length !== 1) {
-    throw new Error(`expected 1 ${marker} pair, found ${matches?.length ?? 0}`);
+}
+
+export function countMarkerPairs(body: string, marker: string): number {
+  return body.match(new RegExp(markerPattern(marker).source, 'gs'))?.length ?? 0;
+}
+
+export function replaceMarker(body: string, marker: string, replacement: string): string {
+  const count = countMarkerPairs(body, marker);
+  if (count !== 1) {
+    throw new Error(`expected 1 ${marker} pair, found ${count}`);
   }
   return body.replace(
-    pattern,
+    markerPattern(marker),
     (_match, open, _inner, close) => `${open}\n${replacement}\n${close}`
   );
 }

.github/scripts/update-canary-pr-body-from-run.test.ts

@@ -11,6 +11,7 @@ import {
   decideCanaryPrBodyAction,
   findCanaryRefFromJobs,
   findPullRequestNumber,
+  missingCanaryMarkers,
   PUBLISH_JOB_NAME,
   shortSha,
   updateCanaryPrBodyFromRun,
@@ -331,3 +332,151 @@ test('updateCanaryPrBodyFromRun ignores a PR number that does not match the run
     { action: 'skip', reason: `no open PR with head ${SHA}` }
   );
 });
+
+const NO_MARKERS = `A description that replaced the template.
+
+## What I did
+
+Nothing that keeps the canary markers.
+`;
+
+test('missingCanaryMarkers names the pairs a description does not have', () => {
+  assert.deepEqual(missingCanaryMarkers(TEMPLATE, 'released'), []);
+  assert.deepEqual(missingCanaryMarkers(TEMPLATE, 'failed'), []);
+  assert.deepEqual(missingCanaryMarkers(NO_MARKERS, 'released'), [
+    'CANARY_RELEASE_HEADING',
+    'CANARY_RELEASE_SECTION',
+  ]);
+  assert.deepEqual(missingCanaryMarkers(NO_MARKERS, 'failed'), ['CANARY_RELEASE_HEADING']);
+});
+
+test('missingCanaryMarkers reports a marker that appears more than once', () => {
+  const duplicated = TEMPLATE + TEMPLATE;
+  assert.deepEqual(missingCanaryMarkers(duplicated, 'released'), [
+    'CANARY_RELEASE_HEADING',
+    'CANARY_RELEASE_SECTION',
+  ]);
+  assert.equal(applyCanaryPrBodyUpdate(duplicated, { action: 'released', sha: SHA }), duplicated);
+});
+
+test('applyCanaryPrBodyUpdate leaves a description without markers untouched', () => {
+  assert.equal(applyCanaryPrBodyUpdate(NO_MARKERS, { action: 'released', sha: SHA }), NO_MARKERS);
+  assert.equal(applyCanaryPrBodyUpdate(NO_MARKERS, { action: 'failed', sha: SHA }), NO_MARKERS);
+});
+
+test('updateCanaryPrBodyFromRun still comments on failure when the description has no markers', () => {
+  const calls: { args: string[]; input?: string }[] = [];
+  const gh: GhClient = (args, input) => {
+    calls.push({ args, input });
+    if (args[0] === 'run' && args[1] === 'view') {
+      return JSON.stringify({
+        event: 'pull_request',
+        conclusion: 'failure',
+        headSha: SHA,
+        jobs: [{ name: PUBLISH_JOB_NAME, conclusion: 'failure' }],
+      });
+    }
+    if (args[0] === 'api' && args[1]?.includes('/commits/')) {
+      return JSON.stringify([{ number: 36155, state: 'open', head: { sha: SHA } }]);
+    }
+    if (
+      args[0] === 'api' &&
+      args[1] === 'repos/storybookjs/storybook/pulls/36155' &&
+      !args.includes('PATCH')
+    ) {
+      return NO_MARKERS;
+    }
+    return '';
+  };
+
+  const result = updateCanaryPrBodyFromRun(
+    { repo: 'storybookjs/storybook', runId: '99', actor: 'jeppe' },
+    gh
+  );
+
+  assert.deepEqual(result, { action: 'failed', pr: 36155 });
+  const comment = calls.find((call) => call.args[0] === 'pr' && call.args[1] === 'comment');
+  assert.equal(
+    comment?.args.at(-1),
+    buildFailureComment({ actor: 'jeppe', repo: 'storybookjs/storybook', runId: '99' })
+  );
+  assert.equal(
+    calls.findIndex((call) => call.args.includes('PATCH')),
+    -1
+  );
+});
+
+test('updateCanaryPrBodyFromRun skips the patch on a release when the description has no markers', () => {
+  const calls: { args: string[]; input?: string }[] = [];
+  const gh: GhClient = (args, input) => {
+    calls.push({ args, input });
+    if (args[0] === 'run' && args[1] === 'view') {
+      return JSON.stringify({
+        event: 'pull_request',
+        conclusion: 'success',
+        headSha: SHA,
+        jobs: [{ name: PUBLISH_JOB_NAME, conclusion: 'success' }],
+      });
+    }
+    if (args[0] === 'api' && args[1]?.includes('/commits/')) {
+      return JSON.stringify([{ number: 36155, state: 'open', head: { sha: SHA } }]);
+    }
+    if (
+      args[0] === 'api' &&
+      args[1] === 'repos/storybookjs/storybook/pulls/36155' &&
+      !args.includes('PATCH')
+    ) {
+      return NO_MARKERS;
+    }
+    return '';
+  };
+
+  const result = updateCanaryPrBodyFromRun(
+    { repo: 'storybookjs/storybook', runId: '99', actor: 'jeppe' },
+    gh
+  );
+
+  assert.deepEqual(result, { action: 'released', pr: 36155 });
+  assert.equal(
+    calls.findIndex((call) => call.args.includes('PATCH')),
+    -1
+  );
+  assert.equal(
+    calls.findIndex((call) => call.args[0] === 'pr' && call.args[1] === 'comment'),
+    -1
+  );
+});
+
+test('updateCanaryPrBodyFromRun comments before it patches the body on failure', () => {
+  const calls: { args: string[]; input?: string }[] = [];
+  const gh: GhClient = (args, input) => {
+    calls.push({ args, input });
+    if (args[0] === 'run' && args[1] === 'view') {
+      return JSON.stringify({
+        event: 'pull_request',
+        conclusion: 'failure',
+        headSha: SHA,
+        jobs: [{ name: PUBLISH_JOB_NAME, conclusion: 'failure' }],
+      });
+    }
+    if (args[0] === 'api' && args[1]?.includes('/commits/')) {
+      return JSON.stringify([{ number: 34799, state: 'open', head: { sha: SHA } }]);
+    }
+    if (
+      args[0] === 'api' &&
+      args[1] === 'repos/storybookjs/storybook/pulls/34799' &&
+      !args.includes('PATCH')
+    ) {
+      return TEMPLATE;
+    }
+    return '';
+  };
+
+  updateCanaryPrBodyFromRun({ repo: 'storybookjs/storybook', runId: '99', actor: 'jeppe' }, gh);
+
+  const commentAt = calls.findIndex((call) => call.args[0] === 'pr' && call.args[1] === 'comment');
+  const patchAt = calls.findIndex((call) => call.args.includes('PATCH'));
+  assert.notEqual(commentAt, -1);
+  assert.notEqual(patchAt, -1);
+  assert.ok(commentAt < patchAt);
+});

.github/scripts/update-canary-pr-body-from-run.ts

@@ -3,7 +3,7 @@ import { resolve } from 'node:path';
 import { fileURLToPath } from 'node:url';
 
 import { parseCanaryRefStepName, type ResolvedCanaryRef } from './resolve-canary-ref.ts';
-import { replaceMarker } from './replace-pr-body-markers.ts';
+import { countMarkerPairs, replaceMarker } from './replace-pr-body-markers.ts';
 
 export const PUBLISH_JOB_NAME = 'Publish Canary Packages';
 export const PKG_PR_NEW_DASHBOARD = 'https://pkg.pr.new/~/storybookjs/storybook';
@@ -122,18 +122,26 @@ export function buildReleasedSection(sha: string): string {
   ].join('\n');
 }
 
+export function missingCanaryMarkers(body: string, action: 'released' | 'failed'): string[] {
+  const markers = action === 'released' ? [HEADING_MARKER, SECTION_MARKER] : [HEADING_MARKER];
+  return markers.filter((marker) => countMarkerPairs(body, marker) !== 1);
+}
+
 export function applyCanaryPrBodyUpdate(
   body: string,
   update: { action: 'released' | 'failed'; sha: string }
 ): string {
+  const replaceIfPresent = (current: string, marker: string, replacement: string) =>
+    countMarkerPairs(current, marker) === 1 ? replaceMarker(current, marker, replacement) : current;
+
   if (update.action === 'released') {
-    return replaceMarker(
-      replaceMarker(body, HEADING_MARKER, buildReleasedHeading(update.sha)),
+    return replaceIfPresent(
+      replaceIfPresent(body, HEADING_MARKER, buildReleasedHeading(update.sha)),
       SECTION_MARKER,
       buildReleasedSection(update.sha)
     );
   }
-  return replaceMarker(body, HEADING_MARKER, buildFailedHeading(update.sha));
+  return replaceIfPresent(body, HEADING_MARKER, buildFailedHeading(update.sha));
 }
 
 export function buildFailureComment(input: { actor: string; repo: string; runId: string }): string {
@@ -193,17 +201,8 @@ export function updateCanaryPrBodyFromRun(
     }
   }
 
-  let body = gh(['api', `repos/${input.repo}/pulls/${pr}`, '--jq', '.body']);
-  if (body.endsWith('\n')) {
-    body = body.slice(0, -1);
-  }
-
-  const nextBody = applyCanaryPrBodyUpdate(body, { action: decision.action, sha });
-  gh(
-    ['api', '--method', 'PATCH', `repos/${input.repo}/pulls/${pr}`, '--input', '-'],
-    JSON.stringify({ body: nextBody })
-  );
-
+  // The failure notice goes out before the body edit, so a description we cannot patch
+  // cannot swallow it.
   if (decision.action === 'failed') {
     gh([
       'pr',
@@ -216,6 +215,26 @@ export function updateCanaryPrBodyFromRun(
     ]);
   }
 
+  let body = gh(['api', `repos/${input.repo}/pulls/${pr}`, '--jq', '.body']);
+  if (body.endsWith('\n')) {
+    body = body.slice(0, -1);
+  }
+
+  const missing = missingCanaryMarkers(body, decision.action);
+  if (missing.length > 0) {
+    console.log(
+      `PR #${pr} does not have exactly one ${missing.join(' or ')} pair, leaving that part of the body`
+    );
+  }
+
+  const nextBody = applyCanaryPrBodyUpdate(body, { action: decision.action, sha });
+  if (nextBody !== body) {
+    gh(
+      ['api', '--method', 'PATCH', `repos/${input.repo}/pulls/${pr}`, '--input', '-'],
+      JSON.stringify({ body: nextBody })
+    );
+  }
+
   return { action: decision.action, pr };
 }