check-links: one suggestion per fix, synced with findings; one fact per line in reports

.github/workflows/check-links.yml

@@ -97,24 +97,13 @@ jobs:
                   fi
 
             - name: Suggest fixes as review comments
-              # One suggested change per added line with a fix. Suggestions already on
-              # the PR (same file, line, and text) are not posted again.
-              if: steps.check.outputs.broken == 'true' && github.event.pull_request.head.repo.full_name == github.repository
+              # One suggested change per finding with a fix, kept in sync with the
+              # findings; see dev/sync-review-comments.sh
+              if: github.event.pull_request.head.repo.full_name == github.repository
               env:
                   GH_TOKEN: ${{ github.token }}
                   PR_NUMBER: ${{ github.event.pull_request.number }}
-              run: |
-                  gh api "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/comments" --paginate \
-                      --jq '.[] | {path, line, body}' | jq -s . > "$RUNNER_TEMP/posted.json"
-                  jq --slurpfile posted "$RUNNER_TEMP/posted.json" \
-                      '.comments |= map(select(. as $comment | $posted[0] | index({path: $comment.path, line: $comment.line, body: $comment.body}) | not))' \
-                      "$RUNNER_TEMP/review.json" > "$RUNNER_TEMP/review-new.json"
-
-                  if [ "$(jq '.comments | length' "$RUNNER_TEMP/review-new.json")" -gt 0 ]; then
-                      gh api --method POST "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/reviews" \
-                          --input "$RUNNER_TEMP/review-new.json" > /dev/null \
-                          || echo "::warning::Could not post the suggested fixes; they are in the report above"
-                  fi
+              run: dev/sync-review-comments.sh '<!-- check-links-finding:' "$RUNNER_TEMP/review.json"
 
             - name: Fail when this PR introduces broken links
               if: steps.check.outputs.broken == 'true'

cspell-allow-list.txt

@@ -40,6 +40,7 @@ appendonly
 appengine
 appgw
 ARGF
+argjson # jq option
 atoburl
 atoi
 attnum
@@ -545,6 +546,7 @@ toolcall
 topk
 topsecretorg
 topsecretproject
+tostring # jq builtin
 transactionally
 transformchanges
 transformchangesgroup

dev/check-links.mjs

@@ -307,15 +307,16 @@ function validateSelfLink(url, currentFile, maps) {
 	const visited = new Set();
 	let candidate = relative;
 	while (true) {
-		const moved = candidate === relative ? '' : ' to a moved page';
+		const error = candidate === relative ? 'Absolute link to this site' : 'Absolute link to a moved page';
 		const problem = validateLink({ url: candidate }, currentFile, maps);
 		if (!problem) {
-			return { error: `Absolute self-link${moved}; use "${candidate}" instead`, fix: candidate };
+			return { error, fix: candidate };
 		}
 		const destination = maps.redirects.get(candidate.split('#')[0]);
 		if (!destination || visited.has(destination)) {
-			const replaced = moved ? `; "${candidate}" replaced it, but` : ', and';
-			return { error: `Absolute self-link${moved}${replaced} ${problem[0].toLowerCase()}${problem.slice(1)}` };
+			const message = problem.error ?? problem;
+			const replaced = candidate === relative ? ', and' : `; "${candidate}" replaced it, but`;
+			return { error: `${error}${replaced} ${message[0].toLowerCase()}${message.slice(1)}` };
 		}
 		visited.add(destination);
 		candidate = isSelfLink(destination) ? relativeSelfLink(destination) : destination;
@@ -413,7 +414,7 @@ function validateLink(link, currentFile, maps) {
 		resolvedPath.replace(/\/$/, '').toLowerCase()
 	);
 	if (realPath) {
-		return `Case mismatch: "${resolvedPath}" should be "${realPath}"`;
+		return { error: 'Path case mismatch: works on macOS, 404s on the Linux build', fix: anchor ? `${realPath}#${anchor}` : realPath };
 	}
 
 	// Check if it's a file with extension (like .png, .pdf)
@@ -543,9 +544,9 @@ function formatText(findings) {
 	];
 	for (const [file, fileFindings] of byFile) {
 		lines.push(`\n📄 ${file}`);
-		for (const { line, url, error } of fileFindings) {
+		for (const { line, url, error, fix } of fileFindings) {
 			lines.push(`   Line ${line}: ${url}`);
-			lines.push(`   └─ ${error}`);
+			lines.push(`   └─ ${error}${fix ? `; use ${fix}` : ''}`);
 		}
 	}
 	return lines.join('\n') + '\n';
@@ -555,67 +556,67 @@ function linkTo(text, url) {
 	return url ? `[${text}](${url})` : text;
 }
 
-// Markdown list of findings grouped by file, linked to the source when --link-base is set
+// Markdown list of findings grouped by file, one line per fact, linked to the
+// source when --link-base is set
 function markdownFindingList(findings) {
 	const lines = [];
 	for (const [file, fileFindings] of groupByFile(findings)) {
 		// ?plain=1 opens GitHub's code view, where #L<n> anchors work; the rendered
 		// Markdown preview ignores them
 		const fileUrl = LINK_BASE && `${LINK_BASE}/${file}?plain=1`;
 		lines.push(linkTo(`**\`${file}\`**`, fileUrl));
-		for (const { line, url, error } of fileFindings) {
-			lines.push(`- ${linkTo(`line ${line}`, fileUrl && `${fileUrl}#L${line}`)}: \`${url}\` — ${error}`);
+		for (const { line, url, error, fix } of fileFindings) {
+			lines.push(
+				`- ${linkTo(`line ${line}`, fileUrl && `${fileUrl}#L${line}`)}`,
+				`  - Link: \`${url}\``,
+				`  - Problem: ${error}`,
+				...(fix ? [`  - Fix: \`${fix}\``] : [])
+			);
 		}
 		lines.push('');
 	}
 	return lines;
 }
 
+const ABSOLUTE_LINKS_ADVICE =
+	'Write links on this site as relative paths (`/admin/config/site-config`), ' +
+	'not `https://sourcegraph.com/docs/…`: absolute links leave the preview ' +
+	'deployment and local dev server, and hide moved pages behind redirects.';
+
 // Body for a pull request comment. With --diff, findings are split into
-// outbound (in a file this PR changed: the PR added or edited a bad link) and
-// inbound (in a file it did not: the PR renamed or removed a link target).
+// outbound (in a file this PR changed: the PR added or edited a bad link),
+// absolute links to this site, and inbound (in a file the PR did not change:
+// the PR renamed or removed a link target).
 function formatMarkdown(findings) {
 	if (findings.length === 0) {
 		return '### ✅ This PR introduces no broken links\n';
 	}
 
 	const lines = [`### ❌ This PR introduces ${findings.length} broken link(s)`, ''];
-	if (DIFF) {
-		const outbound = findings.filter(finding => DIFF.files.has(finding.file));
-		const inbound = findings.filter(finding => !DIFF.files.has(finding.file));
-		if (outbound.length > 0) {
-			lines.push(
-				'### Outbound',
-				'',
-				'Your PR includes links to pages or anchors that do not exist, or absolute links to this site.',
-				'',
-				...markdownFindingList(outbound)
-			);
-		}
-		if (inbound.length > 0) {
-			lines.push(
-				'### Inbound',
-				'',
-				'A change your PR made broke inbound links from elsewhere. ' +
-					'Please fix the inbound links on the other pages.',
-				'',
-				...markdownFindingList(inbound)
-			);
+	const section = (heading, intro, sectionFindings) => {
+		if (sectionFindings.length > 0) {
+			lines.push(`### ${heading}`, '', intro, '', ...markdownFindingList(sectionFindings));
 		}
+	};
+	if (DIFF) {
+		const absolute = findings.filter(finding => isSelfLink(finding.url));
+		const outbound = findings.filter(finding => !isSelfLink(finding.url) && DIFF.files.has(finding.file));
+		const inbound = findings.filter(finding => !isSelfLink(finding.url) && !DIFF.files.has(finding.file));
+		section('Outbound', 'Your PR includes links to pages or anchors that do not exist.', outbound);
+		section('Absolute links', ABSOLUTE_LINKS_ADVICE, absolute);
+		section(
+			'Inbound',
+			'A change your PR made broke inbound links from these other files. Please fix the inbound links in these other files.',
+			inbound
+		);
 	} else {
 		lines.push(...markdownFindingList(findings));
-	}
-	if (findings.some(finding => isSelfLink(finding.url))) {
-		lines.push(
-			'Write links to this site as relative paths (`/admin/config/site-config`), ' +
-				'not `https://sourcegraph.com/docs/…` or `https://docs.sourcegraph.com/…`: ' +
-				'absolute links leave the preview deployment and local dev server, and ' +
-				'hide moved pages behind redirects.',
-			''
-		);
+		if (findings.some(finding => isSelfLink(finding.url))) {
+			lines.push(ABSOLUTE_LINKS_ADVICE, '');
+		}
 	}
 	lines.push(
-		'Reproduce locally with `pnpm check-links --check-anchors` ' +
+		'Reproduce locally with `pnpm check links --check-anchors --check-self-links` ' +
 			'(see `dev/check-links.mjs`).',
 		'',
 		'Adding a redirect in `src/data/redirects.ts` does not satisfy this ' +
@@ -624,38 +625,32 @@ function formatMarkdown(findings) {
 	return lines.join('\n') + '\n';
 }
 
+// First line of a review comment, so the workflow can match the comments it
+// posted earlier to the findings still present and delete the rest
+const REVIEW_MARKER = '<!-- check-links-finding:';
+
 // Body for POST /repos/{owner}/{repo}/pulls/{n}/reviews: one suggested change per
-// added line that has fixes, so the author can apply them from the PR. The comment
-// lists every finding on the line, so the ones the suggestion cannot fix are not
-// mistaken for accepted. Review comments must sit on a line of the diff, hence the
-// added-line restriction.
+// finding that has a fix, so the author can apply each from the PR. Review comments
+// must sit on a line of the diff, hence the added-line restriction. No review body:
+// a submitted review cannot be deleted, so a body would outlive the comments once
+// the links are fixed.
 function reviewRequest(findings) {
-	const findingsByLine = new Map();
-	for (const finding of findings) {
-		if (!isAddedLine(finding.file, finding.line)) continue;
-		const key = `${finding.file}:${finding.line}`;
-		if (!findingsByLine.has(key)) findingsByLine.set(key, []);
-		findingsByLine.get(key).push(finding);
-	}
-
-	const comments = [...findingsByLine.values()]
-		.filter(lineFindings => lineFindings.some(finding => finding.fix))
-		.map(lineFindings => {
-			const { file, line } = lineFindings[0];
+	const comments = findings
+		.filter(({ file, line, fix }) => fix && isAddedLine(file, line))
+		.map(({ file, line, url, error, fix }) => {
 			const source = fs.readFileSync(path.join(ROOT_DIR, file), 'utf-8').split('\n')[line - 1];
-			const fixed = lineFindings
-				.filter(finding => finding.fix)
-				.reduce((text, { url, fix }) => text.split(url).join(fix), source);
-			const notes = lineFindings.map(
-				({ url, error, fix }) => `- \`${url}\`: ${error}${fix ? '' : ' (not fixed by this suggestion)'}`
-			);
-			return { path: file, line, side: 'RIGHT', body: [...notes, '```suggestion', fixed, '```'].join('\n') };
+			const body = [
+				`${REVIEW_MARKER} ${url} -->`,
+				`Link: \`${url}\``,
+				`Problem: ${error}`,
+				`Fix: \`${fix}\``,
+				'````suggestion',
+				source.split(url).join(fix),
+				'````'
+			];
+			return { path: file, line, side: 'RIGHT', body: body.join('\n') };
 		});
-	return {
-		event: 'COMMENT',
-		body: 'Suggested fixes for the links this PR adds; details in the check-links comment.',
-		comments
-	};
+	return { event: 'COMMENT', body: '', comments };
 }
 
 const FORMATTERS = {

dev/sync-review-comments.sh

@@ -0,0 +1,40 @@
+#!/usr/bin/env sh
+# Make a check's suggested-change review comments on a PR match a review.json
+# ({comments: [{path, line, start_line?, body}]}): post the new ones, update
+# the ones whose text changed, and delete the ones whose finding is gone.
+# GitHub sets line to null on comments it could not carry to the new revision,
+# so those are deleted too. Comments are matched by file, line, and the
+# marker comment on their first line, e.g. "<!-- check-links-finding: <url> -->".
+#
+# Usage: dev/sync-review-comments.sh '<!-- check-links-finding:' review.json
+# Needs GH_TOKEN, GITHUB_REPOSITORY, and PR_NUMBER.
+set -eu
+marker=$1
+review=$2
+key='(.path + ":" + (.line | tostring) + ":" + (.body | split("\n")[0]))'
+pulls="repos/$GITHUB_REPOSITORY/pulls"
+
+posted=$(gh api "$pulls/$PR_NUMBER/comments" --paginate \
+    --jq ".[] | select(.body | startswith(\"$marker\")) | {id, body, key: $key}" | jq -s .)
+
+printf '%s' "$posted" | jq -c --slurpfile review "$review" ".[]
+    | .key as \$key
+    | (\$review[0].comments | map(select($key == \$key)) | first) as \$wanted
+    | if \$wanted == null then {id, method: \"DELETE\"}
+      elif \$wanted.body != .body then {id, method: \"PATCH\", body: \$wanted.body}
+      else empty end" \
+| while read -r change; do
+    comment="$pulls/comments/$(printf '%s' "$change" | jq -r .id)"
+    if [ "$(printf '%s' "$change" | jq -r .method)" = DELETE ]; then
+        gh api --method DELETE "$comment" < /dev/null
+    else
+        printf '%s' "$change" | jq '{body}' | gh api --method PATCH "$comment" --input - > /dev/null
+    fi
+done
+
+fresh=$(jq --argjson posted "$posted" \
+    ".comments |= map(select($key as \$key | \$posted | any(.key == \$key) | not))" "$review")
+if [ "$(printf '%s' "$fresh" | jq '.comments | length')" -gt 0 ]; then
+    printf '%s' "$fresh" | gh api --method POST "$pulls/$PR_NUMBER/reviews" --input - > /dev/null \
+        || echo "::warning::Could not post the suggested fixes; they are in the report above"
+fi