Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions .github/workflows/check-links.yml
Original file line number Diff line number Diff line change
Expand Up @@ -97,19 +97,29 @@ 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. Suggestions already on the
# PR (same file, line, and link) are not posted again; suggestions for
# findings that are gone are deleted. GitHub sets line to null on comments
# it could not carry to the new revision, so those are deleted too.
if: github.event.pull_request.head.repo.full_name == github.repository
env:
GH_TOKEN: ${{ github.token }}
PR_NUMBER: ${{ github.event.pull_request.number }}
run: |
key='(.path + ":" + (.line | tostring) + ":" + (.body | split("\n")[0]))'
gh api "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/comments" --paginate \
--jq '.[] | {path, line, body}' | jq -s . > "$RUNNER_TEMP/posted.json"
--jq ".[] | select(.body | startswith(\"<!-- check-links-finding:\")) | {id, key: $key}" \
| 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))' \
".comments |= map(select($key as \$key | \$posted[0] | any(.key == \$key) | not))" \
"$RUNNER_TEMP/review.json" > "$RUNNER_TEMP/review-new.json"

jq -r --slurpfile review "$RUNNER_TEMP/review.json" \
".[] | select(.key as \$key | \$review[0].comments | any($key == \$key) | not) | .id" \
"$RUNNER_TEMP/posted.json" | while read -r comment_id; do
gh api --method DELETE "repos/$GITHUB_REPOSITORY/pulls/comments/$comment_id"
done

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 \
Expand Down
133 changes: 133 additions & 0 deletions .github/workflows/check-redirects.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
name: Check redirects

# Reports redirects in src/data/redirects.ts that this PR breaks, compared with
# the merge base: destinations that no longer exist, #fragments whose heading
# was renamed, and new redirects that shadow an existing page. Pre-existing
# broken redirects on the base branch are ignored.

on:
pull_request:

# A new push supersedes the run for the previous one
concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number }}
cancel-in-progress: true

permissions:
contents: read
pull-requests: write

jobs:
check-redirects:
name: Broken redirects introduced by this PR
runs-on: ubuntu-latest
steps:
- name: Check out pull request head
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 0

- name: Install github-slugger, the only dependency of dev/check-redirects.mjs
# Into a scratch prefix, not the repo: `npm install <pkg>` next to
# package.json would install every dependency of the site
run: |
npm install --prefix "$RUNNER_TEMP/deps" --no-package-lock --no-audit --no-fund \
"github-slugger@$(node -p 'require("./package.json").dependencies["github-slugger"]')"
ln -s "$RUNNER_TEMP/deps/node_modules" node_modules

- name: Check out merge base
env:
BASE_SHA: ${{ github.event.pull_request.base.sha }}
run: |
merge_base=$(git merge-base "$BASE_SHA" HEAD)
git worktree add "$RUNNER_TEMP/base" "$merge_base"
git diff -U0 "$merge_base" HEAD -- src/data/redirects.ts > "$RUNNER_TEMP/changes.diff"

- name: Record broken redirects already present on the base branch
# Exit 1 means findings, which is expected here
run: |
node dev/check-redirects.mjs --format json \
--root "$RUNNER_TEMP/base" > "$RUNNER_TEMP/base-redirects.json" \
|| [ $? -eq 1 ]

- name: Find redirects broken by this PR
id: check
env:
# Line links in the report open redirects.ts on the PR branch
LINK_BASE: ${{ github.event.pull_request.head.repo.html_url }}/blob/${{ github.event.pull_request.head.ref }}
run: |
if node dev/check-redirects.mjs --format markdown \
--baseline "$RUNNER_TEMP/base-redirects.json" \
--diff "$RUNNER_TEMP/changes.diff" \
--review "$RUNNER_TEMP/review.json" \
--link-base "$LINK_BASE" > "$RUNNER_TEMP/report.md"; then
echo "broken=false" >> "$GITHUB_OUTPUT"
else
echo "broken=true" >> "$GITHUB_OUTPUT"
fi
cat "$RUNNER_TEMP/report.md"

- name: Comment on the pull request
# Fork PRs get a read-only token; the report is still in the job log
if: github.event.pull_request.head.repo.full_name == github.repository
env:
GH_TOKEN: ${{ github.token }}
PR_NUMBER: ${{ github.event.pull_request.number }}
BROKEN: ${{ steps.check.outputs.broken }}
run: |
marker='<!-- check-redirects-report -->'
existing_comment=$(gh api "repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments" \
--paginate --jq ".[] | select(.body | startswith(\"$marker\")) | .id" | head -n 1)

# Comment only when there is something to report, or an earlier report to resolve
if [ "$BROKEN" = true ]; then
{ echo "$marker"; cat "$RUNNER_TEMP/report.md"; } > "$RUNNER_TEMP/comment.md"
elif [ -n "$existing_comment" ]; then
printf '%s\n### ✅ The redirects an earlier revision of this PR broke are fixed\n' \
"$marker" > "$RUNNER_TEMP/comment.md"
else
exit 0
fi

if [ -n "$existing_comment" ]; then
gh api --method PATCH "repos/$GITHUB_REPOSITORY/issues/comments/$existing_comment" \
--field body=@"$RUNNER_TEMP/comment.md"
else
gh pr comment "$PR_NUMBER" --body-file "$RUNNER_TEMP/comment.md"
fi

- name: Suggest fixes as review comments
# One suggested change per fixable entry this PR added. Suggestions
# already on the PR (same lines and entry) are not posted again;
# suggestions for findings that are gone are deleted. GitHub sets line
# to null on comments it could not carry to the new revision, so those
# are deleted too.
if: github.event.pull_request.head.repo.full_name == github.repository
env:
GH_TOKEN: ${{ github.token }}
PR_NUMBER: ${{ github.event.pull_request.number }}
run: |
key='(.path + ":" + (.line | tostring) + ":" + (.body | split("\n")[0]))'
gh api "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/comments" --paginate \
--jq ".[] | select(.body | startswith(\"<!-- check-redirects-finding:\")) | {id, key: $key}" \
| jq -s . > "$RUNNER_TEMP/posted.json"
jq --slurpfile posted "$RUNNER_TEMP/posted.json" \
".comments |= map(select($key as \$key | \$posted[0] | any(.key == \$key) | not))" \
"$RUNNER_TEMP/review.json" > "$RUNNER_TEMP/review-new.json"

jq -r --slurpfile review "$RUNNER_TEMP/review.json" \
".[] | select(.key as \$key | \$review[0].comments | any($key == \$key) | not) | .id" \
"$RUNNER_TEMP/posted.json" | while read -r comment_id; do
gh api --method DELETE "repos/$GITHUB_REPOSITORY/pulls/comments/$comment_id"
done

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

- name: Fail when this PR breaks redirects
if: steps.check.outputs.broken == 'true'
run: exit 1
1 change: 1 addition & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
- **Lint**: `npm run lint`
- **Checks**: `npm run check` runs every `dev/check-*.mjs` (links, filenames, images); `npm run build` runs them first, so any finding fails a deploy
- **Check links**: `npm run check -- links --check-anchors --check-self-links` (CI comments on PRs that break links; see `dev/check-links.mjs`; the build runs it without flags, so only dead page links fail a deploy). When moving a page or renaming a heading, update every link to it; a redirect in `src/data/redirects.ts` does not satisfy the check. Link to this site with relative paths (`/admin/config/site-config`), never `https://sourcegraph.com/docs/…` or `https://docs.sourcegraph.com/…`. To also probe the external links you added: `npm run check -- links --check-anchors --check-self-links --check-external --diff <(git diff -U0 origin/main)`
- **Check redirects**: `node dev/check-redirects.mjs` reports broken entries in `src/data/redirects.ts` (CI comments on PRs that break redirects; see the script header for what it checks). Not part of `npm run check`: main has hundreds of pre-existing findings, and CI only reports the ones a PR adds
- **Prove changed links resolve on a deploy**: `node dev/verify-links-live.mjs --site <vercel-preview-url>` prints a Markdown table for the PR description

## AI Chat Integration
Expand Down
2 changes: 2 additions & 0 deletions cspell-allow-list.txt
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,7 @@ toolcall
topk
topsecretorg
topsecretproject
tostring # jq builtin
transactionally
transformchanges
transformchangesgroup
Expand Down Expand Up @@ -597,3 +598,4 @@ YOURUSERNAME
Zaporizhzhia
Zoekt
zoomable
aardvark # test entry for the sorted-list check (will be reverted)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aardvark is out of alphabetical order: move it above acmeco on line 26.

137 changes: 66 additions & 71 deletions dev/check-links.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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';
Expand All @@ -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 ' +
Expand All @@ -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 = {
Expand Down
Loading
Loading