diff --git a/.github/workflows/link-check-internal.yml b/.github/workflows/link-check-internal.yml index 5674853ff35a..6a6b1ffa800a 100644 --- a/.github/workflows/link-check-internal.yml +++ b/.github/workflows/link-check-internal.yml @@ -212,6 +212,8 @@ jobs: steps: - name: Checkout uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - uses: ./.github/actions/node-npm-setup + - name: Download all artifacts uses: actions/download-artifact@70fc10c6e5e1ce46ad2ea6f2b72d43f7d47b13c3 # v8.0.0 with: @@ -221,23 +223,23 @@ jobs: - name: Combine reports id: combine + env: + ACTION_RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + # A version with no broken links uploads no report, so the files on disk undercount + # what was checked. Pass the matrix so the report can say "broken in all versions" + # and mean it. + MATRIX: ${{ needs.setup-matrix.outputs.matrix }} run: | - # Check if any reports exist - if ls reports/*.md 1> /dev/null 2>&1; then + # Merge the per-version JSON rather than concatenating the rendered Markdown. + # A link broken in every version is one problem, not one per version. + if ls reports/*.json 1> /dev/null 2>&1; then echo "has_reports=true" >> $GITHUB_OUTPUT - - # Combine all markdown reports - echo "# Internal Links Report" > combined-report.md - echo "" >> combined-report.md - echo "Generated: $(date -u +'%Y-%m-%d %H:%M UTC')" >> combined-report.md - echo "[Action run](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})" >> combined-report.md - echo "" >> combined-report.md - - for report in reports/*.md; do - echo "---" >> combined-report.md - cat "$report" >> combined-report.md - echo "" >> combined-report.md - done + VERSIONS=$(echo "$MATRIX" | jq -r '[.include[] | "\(.version) \(.language)"] | join(",")') + npm run combine-link-reports -- \ + --input reports \ + --output combined-report.md \ + --versions "$VERSIONS" \ + --action-url "$ACTION_RUN_URL" else echo "has_reports=false" >> $GITHUB_OUTPUT echo "No broken link reports generated - all links valid!" diff --git a/package.json b/package.json index a0f1c20cbf31..5aa05ce4d1ad 100644 --- a/package.json +++ b/package.json @@ -91,6 +91,7 @@ "liquid-tags": "tsx src/content-render/scripts/liquid-tags.ts", "check-links-pr": "tsx src/links/scripts/check-links-pr.ts", "check-links-internal": "tsx src/links/scripts/check-links-internal.ts", + "combine-link-reports": "tsx src/links/scripts/combine-link-reports.ts", "check-links-external": "tsx src/links/scripts/check-links-external.ts", "rest-dev": "tsx src/rest/scripts/update-files.ts", "show-action-deps": "echo 'Action Dependencies:' && rg '^[\\s|-]*(uses:.*)$' .github -I -N --no-heading -r '$1$2' | sort | uniq | cut -c 7-", diff --git a/src/links/lib/link-report.ts b/src/links/lib/link-report.ts index 4ce641d0cab1..adedb620c92d 100644 --- a/src/links/lib/link-report.ts +++ b/src/links/lib/link-report.ts @@ -22,8 +22,20 @@ export interface BrokenLink { * `update-internal-links` looks the href up exactly as written, so it can't fix these. */ requiresVersionContext?: boolean + /** + * Two checked versions resolved this href to genuinely different destinations, so no + * single rewrite is correct for all of them. Merging keeps one target and drops the + * rest, which would otherwise let the report name a destination that is only right for + * one version. + */ + hasConflictingRedirectTargets?: boolean statusCode?: number errorMessage?: string + /** + * The versions this link is broken in. Only set on a merged report, where the same link + * usually breaks in every version checked. + */ + versions?: string[] } /** @@ -55,6 +67,8 @@ export interface LinkReport { totalOccurrences: number timestamp: string actionUrl?: string + /** Every version this report covers. Only set on a merged report. */ + versionsChecked?: string[] } // ============================================================================ @@ -237,6 +251,15 @@ function isVersionOnlyRedirect(target: string, redirectTarget: string): boolean return withoutVersion === target } +/** + * Two redirect targets that differ only by version prefix are the same rename seen from + * two versions, not a disagreement. `/enterprise-server@3.21/new` and + * `/enterprise-server@3.17/new` both mean "the page moved to /new". + */ +function sameDestination(a: string, b: string): boolean { + return a.replace(VERSION_PREFIX_RE, '') === b.replace(VERSION_PREFIX_RE, '') +} + /** * Create a suggestion message for a redirect */ @@ -368,6 +391,84 @@ function createSummary(errorCount: number, warningCount: number, totalOccurrence return `Found ${parts.join(' and ')} across ${totalOccurrences} occurrence${plural}.` } +/** + * Describe which versions a link breaks in, but only when that is news. + * + * Nearly every broken link breaks in every version, so printing the full list on every + * group is noise that also blows past the issue body size limit. Say something only when a + * link is version-specific. + */ +export function describeVersions( + versions: string[] | undefined, + versionsChecked: string[] | undefined, +): string | undefined { + if (!versions?.length || !versionsChecked?.length) return undefined + if (versionsChecked.length === 1) return undefined + if (versions.length >= versionsChecked.length) return undefined + return versions.join(', ') +} + +/** + * Merge one report per version into a single report. + * + * The workflow used to concatenate each version's rendered Markdown, so a link broken in + * every version produced an identical section per version. Merging on the link itself means + * one section per real problem, with the versions recorded on the occurrence. + */ +export function mergeInternalLinkReports( + reports: { version: string; report: LinkReport }[], + options: { actionUrl?: string; versionsChecked?: string[] } = {}, +): LinkReport { + const merged = new Map() + + for (const { version, report } of reports) { + for (const group of report.groups) { + for (const occurrence of group.occurrences) { + const href = occurrence.href || group.target + const key = `${href}\u0000${occurrence.file}` + const existing = merged.get(key) + if (existing) { + existing.lines = [...new Set([...existing.lines, ...occurrence.lines])].sort( + (a, b) => a - b, + ) + existing.versions = [...new Set([...(existing.versions ?? []), version])] + // A link that redirects in any version is still worth rewriting everywhere. + existing.isRedirect = existing.isRedirect || occurrence.isRedirect + existing.requiresVersionContext = + existing.requiresVersionContext || occurrence.requiresVersionContext + // Keeping the first target and dropping the rest is only safe while every + // version agrees on where the page went. Today they always do, but if that ever + // stops being true the report would confidently name a destination that is + // right for one version and wrong for the others. Flag it instead. + if ( + existing.redirectTarget && + occurrence.redirectTarget && + !sameDestination(existing.redirectTarget, occurrence.redirectTarget) + ) { + existing.hasConflictingRedirectTargets = true + } + existing.redirectTarget = existing.redirectTarget ?? occurrence.redirectTarget + } else { + merged.set(key, { ...occurrence, href, versions: [version] }) + } + } + } + } + + // A version with no broken links writes no report, so the files on disk undercount what + // was actually checked. Callers that know the full matrix pass it in, otherwise fall back + // to what was found. + const versionsChecked = options.versionsChecked?.length + ? options.versionsChecked + : reports.map((r) => r.version) + const report = generateInternalLinkReport([...merged.values()], options) + const scope = + versionsChecked.length > 1 + ? `\n\nChecked ${versionsChecked.length} versions: ${versionsChecked.join(', ')}. A link listed without a version breaks in all of them.` + : '' + return { ...report, versionsChecked, summary: report.summary + scope } +} + /** * Generate a report for internal links */ @@ -472,6 +573,10 @@ export function classifyFixStrategy(group: GroupedBrokenLinks): FixStrategy { if (group.occurrences.some((occ) => occ.requiresVersionContext)) { return 'decide' } + // Versions disagree about where the page went, so there is no single correct rewrite. + if (group.occurrences.some((occ) => occ.hasConflictingRedirectTargets)) { + return 'decide' + } return 'codemod' } // The link carries a fragment, so the stale part is likely a renamed heading. @@ -501,15 +606,30 @@ function codemodPaths(groups: GroupedBrokenLinks[]): string[] { return [...paths].sort() } +/** The union of versions across a group's occurrences. */ +function groupVersions(group: GroupedBrokenLinks): string[] { + const versions = new Set() + for (const occ of group.occurrences) { + for (const version of occ.versions ?? []) versions.add(version) + } + return [...versions] +} + function occurrenceCount(groups: GroupedBrokenLinks[]): number { return groups.reduce((sum, g) => sum + g.occurrences.length, 0) } -function renderCodemodSection(groups: GroupedBrokenLinks[]): string { +function renderCodemodSection(groups: GroupedBrokenLinks[], versionsChecked?: string[]): string { + const versionFor = (group: GroupedBrokenLinks) => + describeVersions(groupVersions(group), versionsChecked) + const showVersions = groups.some((group) => versionFor(group)) + const rows = groups .map((group) => { const target = group.occurrences.find((occ) => occ.redirectTarget)?.redirectTarget ?? '' - return `| \`${group.target}\` | \`${target}\` | ${group.occurrences.length} |` + const cells = [`\`${group.target}\``, `\`${target}\``, `${group.occurrences.length}`] + if (showVersions) cells.push(versionFor(group) ?? 'all') + return `| ${cells.join(' | ')} |` }) .join('\n') @@ -542,8 +662,8 @@ Review the diff, then open a pull request.
The ${groups.length} link${plural} this fixes -| From | To | Occurrences | -|------|-----|-------------| +| From | To | Occurrences |${showVersions ? ' Versions |' : ''} +|------|-----|-------------|${showVersions ? '----------|' : ''} ${rows}
` @@ -591,8 +711,15 @@ function renderManualSection( blurb: string, groups: GroupedBrokenLinks[], isExternal: boolean, + versionsChecked?: string[], ): string { - const sections = groups.map((group) => TEMPLATES.group(group, isExternal)).join('\n\n') + const sections = groups + .map((group) => { + const versions = describeVersions(groupVersions(group), versionsChecked) + const note = versions ? `\n\n**Only in:** ${versions}` : '' + return TEMPLATES.group(group, isExternal) + note + }) + .join('\n\n') return `## ${heading} (${groups.length} link${groups.length === 1 ? '' : 's'}, ${occurrenceCount(groups)} occurrence${occurrenceCount(groups) === 1 ? '' : 's'}) ${blurb} @@ -604,7 +731,11 @@ ${sections}` * Render an internal report as four buckets ordered by how much work each one costs, from * one command down to nothing at all. */ -function renderByFixStrategy(groups: GroupedBrokenLinks[], isExternal: boolean): string { +function renderByFixStrategy( + groups: GroupedBrokenLinks[], + isExternal: boolean, + versionsChecked?: string[], +): string { const codemod = groups.filter((g) => classifyFixStrategy(g) === 'codemod') const versionless = groups.filter((g) => classifyFixStrategy(g) === 'versionless') const anchors = groups.filter((g) => classifyFixStrategy(g) === 'anchor') @@ -631,7 +762,7 @@ ${summaryRows.join('\n')} Work top to bottom. Bucket 1 is usually most of the report and costs one command.`, ] - if (codemod.length > 0) parts.push(renderCodemodSection(codemod)) + if (codemod.length > 0) parts.push(renderCodemodSection(codemod, versionsChecked)) if (anchors.length > 0) { parts.push( renderManualSection( @@ -639,6 +770,7 @@ Work top to bottom. Bucket 1 is usually most of the report and costs one command 'The `#fragment` does not match a heading on the target page. Usually a heading was renamed: find it and repoint the link, or drop the fragment if the section is gone. Check that the page itself still exists first, since a missing page with a fragment also lands here.', anchors, isExternal, + versionsChecked, ), ) } @@ -649,6 +781,7 @@ Work top to bottom. Bucket 1 is usually most of the report and costs one command 'The codemod looks each link up exactly as written, and for these that lookup finds nothing: either no redirect exists at all, or the redirect only exists under a version prefix the link does not carry. Choose a destination, or add a redirect from the path as written.', decide, isExternal, + versionsChecked, ), ) } @@ -725,7 +858,7 @@ export function reportToMarkdown(report: LinkReport, isExternal = false): string parts.push( isExternal ? renderGroups(report.groups, isExternal) - : renderByFixStrategy(report.groups, isExternal), + : renderByFixStrategy(report.groups, isExternal, report.versionsChecked), ) } diff --git a/src/links/scripts/combine-link-reports.ts b/src/links/scripts/combine-link-reports.ts new file mode 100644 index 000000000000..af4c72c4a6f0 --- /dev/null +++ b/src/links/scripts/combine-link-reports.ts @@ -0,0 +1,79 @@ +#!/usr/bin/env tsx + +/** + * Combine every version's link report into one deduplicated Markdown report. + * + * The workflow used to `cat` each version's rendered Markdown together, so a link broken in + * every version produced an identical section per version. That multiplied the report by the + * size of the matrix and pushed it past the issue body limit, where it got truncated. + */ + +import fs from 'fs' +import path from 'path' +import { program } from 'commander' + +import { + mergeInternalLinkReports, + reportToMarkdown, + type LinkReport, +} from '@/links/lib/link-report' + +// `link-report-free-pro-team@latest-en.json` -> `free-pro-team@latest en` +const REPORT_FILE = /^link-report-(.+)-([a-z]{2})\.json$/ + +interface VersionedReport { + version: string + report: LinkReport +} + +export function readReports(directory: string): VersionedReport[] { + if (!fs.existsSync(directory)) return [] + + const reports: VersionedReport[] = [] + for (const file of fs.readdirSync(directory).sort()) { + const match = REPORT_FILE.exec(file) + if (!match) continue + + const [, version, language] = match + const raw = fs.readFileSync(path.join(directory, file), 'utf8') + reports.push({ version: `${version} ${language}`, report: JSON.parse(raw) as LinkReport }) + } + return reports +} + +async function main() { + program + .description('Combine per-version link reports into one deduplicated report') + .option('-i, --input ', 'Directory holding the report JSON files', 'reports') + .option('-o, --output ', 'Where to write the combined Markdown', 'combined-report.md') + .option('--action-url ', 'Link back to the workflow run') + .option( + '--versions ', + 'Comma-separated list of every version checked, including the ones that came back clean', + ) + .parse() + + const { input, output, actionUrl, versions } = program.opts() + const versionsChecked = versions + ? String(versions) + .split(',') + .map((v: string) => v.trim()) + .filter(Boolean) + : undefined + const reports = readReports(input) + + if (reports.length === 0) { + console.log(`No report JSON found in ${input}.`) + process.exit(1) + } + + const merged = mergeInternalLinkReports(reports, { actionUrl, versionsChecked }) + fs.writeFileSync(output, reportToMarkdown(merged)) + + const before = reports.reduce((sum, r) => sum + r.report.groups.length, 0) + console.log( + `Combined ${reports.length} report(s): ${before} sections before, ${merged.groups.length} after.`, + ) +} + +main() diff --git a/src/links/tests/link-report.ts b/src/links/tests/link-report.ts index 5df737dc6db1..23a5665e3fd4 100644 --- a/src/links/tests/link-report.ts +++ b/src/links/tests/link-report.ts @@ -10,6 +10,8 @@ import { generatePRComment, generateSampleReports, classifyFixStrategy, + mergeInternalLinkReports, + describeVersions, } from '../lib/link-report' describe('groupBrokenLinks', () => { @@ -621,6 +623,103 @@ describe('codemod command scoping', () => { }) }) +describe('mergeInternalLinkReports', () => { + const reportFor = (version: string, links: BrokenLink[]) => ({ + version, + report: generateInternalLinkReport(links, { version }), + }) + + const shared: BrokenLink = { + href: '/old', + file: 'actions/a.md', + lines: [1], + isRedirect: true, + redirectTarget: '/new', + } + + test('a link broken in every version becomes one group, not one per version', () => { + const merged = mergeInternalLinkReports([ + reportFor('fpt', [shared]), + reportFor('ghes', [shared]), + ]) + + expect(merged.groups).toHaveLength(1) + expect(merged.groups[0].occurrences).toHaveLength(1) + expect(merged.groups[0].occurrences[0].versions).toEqual(['fpt', 'ghes']) + }) + + test('keeps links that only break in one version', () => { + const merged = mergeInternalLinkReports([ + reportFor('fpt', [shared]), + reportFor('ghes', [shared, { href: '/ghes-only', file: 'admin/b.md', lines: [2] }]), + ]) + + expect(merged.groups).toHaveLength(2) + const ghesOnly = merged.groups.find((g) => g.target === '/ghes-only') + expect(ghesOnly?.occurrences[0].versions).toEqual(['ghes']) + }) + + test('unions line numbers for the same link in the same file', () => { + const merged = mergeInternalLinkReports([ + reportFor('fpt', [{ ...shared, lines: [3, 1] }]), + reportFor('ghes', [{ ...shared, lines: [2] }]), + ]) + + expect(merged.groups[0].occurrences[0].lines).toEqual([1, 2, 3]) + }) + + test('a link that redirects in any version keeps its destination', () => { + const merged = mergeInternalLinkReports([ + reportFor('fpt', [{ href: '/old', file: 'actions/a.md', lines: [1] }]), + reportFor('ghes', [shared]), + ]) + + expect(merged.groups[0].isWarning).toBe(true) + expect(merged.groups[0].occurrences[0].redirectTarget).toBe('/new') + }) + + test('records every version checked', () => { + const merged = mergeInternalLinkReports([ + reportFor('fpt', [shared]), + reportFor('ghes', [shared]), + ]) + + expect(merged.versionsChecked).toEqual(['fpt', 'ghes']) + expect(merged.summary).toContain('Checked 2 versions') + }) + + test('flags version-specific links in the rendered report', () => { + const merged = mergeInternalLinkReports([ + reportFor('fpt', [shared]), + reportFor('ghes', [shared, { href: '/ghes-only', file: 'admin/b.md', lines: [2] }]), + ]) + const markdown = reportToMarkdown(merged) + + expect(markdown).toContain('**Only in:** ghes') + // The shared link breaks everywhere, so saying so on every group would be noise. + expect(markdown).not.toContain('**Only in:** fpt, ghes') + }) +}) + +describe('describeVersions', () => { + test('says nothing when the link breaks in every version checked', () => { + expect(describeVersions(['fpt', 'ghes'], ['fpt', 'ghes'])).toBeUndefined() + }) + + test('says nothing when only one version was checked', () => { + expect(describeVersions(['fpt'], ['fpt'])).toBeUndefined() + }) + + test('names the versions when a link is version-specific', () => { + expect(describeVersions(['ghes'], ['fpt', 'ghes'])).toBe('ghes') + }) + + test('says nothing without version data, as on a single-version report', () => { + expect(describeVersions(undefined, ['fpt', 'ghes'])).toBeUndefined() + expect(describeVersions(['fpt'], undefined)).toBeUndefined() + }) +}) + describe('version-only redirects', () => { const versionOnly: BrokenLink[] = [ { @@ -721,6 +820,39 @@ describe('version-only classification across versions', () => { }) }) +describe('versions checked when some come back clean', () => { + const report = (href: string) => + generateInternalLinkReport([{ href, file: 'actions/a.md', lines: [1] }]) + + test('a caller-supplied version list wins over what was found on disk', () => { + // A clean version uploads no report, so counting files undercounts the matrix. + const merged = mergeInternalLinkReports( + [ + { version: 'free-pro-team@latest en', report: report('/a#gone') }, + { version: 'enterprise-cloud@latest en', report: report('/b#gone') }, + ], + { versionsChecked: ['free-pro-team@latest en', 'enterprise-cloud@latest en', 'ghes en'] }, + ) + + expect(merged.versionsChecked).toHaveLength(3) + expect(merged.summary).toContain('Checked 3 versions') + // Two of three versions is now worth saying out loud, where two of two was not. + expect(reportToMarkdown(merged)).toContain('**Only in:**') + }) + + test('falls back to the reports on disk when no list is given', () => { + const merged = mergeInternalLinkReports([ + { version: 'free-pro-team@latest en', report: report('/a#gone') }, + { version: 'enterprise-cloud@latest en', report: report('/b#gone') }, + ]) + + expect(merged.versionsChecked).toEqual([ + 'free-pro-team@latest en', + 'enterprise-cloud@latest en', + ]) + }) +}) + describe('redirects the codemod cannot resolve', () => { const occurrence = (extra: Partial = {}): BrokenLink => ({ href: '/admin/old', @@ -790,3 +922,56 @@ describe('rename advice and inherited version prefixes', () => { expect(s).toContain('Leave the link versionless') }) }) + +describe('merging redirect targets across versions', () => { + const reportFor = (version: string, links: BrokenLink[]) => ({ + version, + report: generateInternalLinkReport(links, { version }), + }) + const occ = (redirectTarget: string, extra: Partial = {}): BrokenLink => ({ + href: '/old', + file: 'actions/a.md', + lines: [1], + isRedirect: true, + redirectTarget, + ...extra, + }) + const mergedOccurrence = (targets: string[], extra: Partial = {}) => + mergeInternalLinkReports(targets.map((t, i) => reportFor(`v${i}`, [occ(t, extra)]))).groups[0] + .occurrences[0] + + test('treats targets that differ only by version prefix as the same destination', () => { + const merged = mergedOccurrence(['/enterprise-server@3.22/new', '/enterprise-server@3.17/new']) + expect(merged.hasConflictingRedirectTargets).toBeUndefined() + }) + + test('flags genuinely different destinations between versions', () => { + const merged = mergedOccurrence(['/new-a', '/new-b']) + expect(merged.hasConflictingRedirectTargets).toBe(true) + }) + + test('sends conflicting destinations to a human instead of the codemod', () => { + const report = mergeInternalLinkReports([ + reportFor('v0', [occ('/new-a')]), + reportFor('v1', [occ('/new-b')]), + ]) + expect(classifyFixStrategy(report.groups[0])).toBe('decide') + }) + + test('still routes an agreed rename to the codemod', () => { + const report = mergeInternalLinkReports([ + reportFor('v0', [occ('/enterprise-server@3.22/new')]), + reportFor('v1', [occ('/enterprise-server@3.17/new')]), + ]) + expect(classifyFixStrategy(report.groups[0])).toBe('codemod') + }) + + test('keeps requiresVersionContext when only a later version sets it', () => { + const report = mergeInternalLinkReports([ + reportFor('v0', [occ('/new')]), + reportFor('v1', [occ('/new', { requiresVersionContext: true })]), + ]) + expect(report.groups[0].occurrences[0].requiresVersionContext).toBe(true) + expect(classifyFixStrategy(report.groups[0])).toBe('decide') + }) +}) diff --git a/src/search/scripts/aggregate-search-index-failures.ts b/src/search/scripts/aggregate-search-index-failures.ts index ffd0c802948d..122d8b6548ff 100644 --- a/src/search/scripts/aggregate-search-index-failures.ts +++ b/src/search/scripts/aggregate-search-index-failures.ts @@ -35,6 +35,55 @@ export interface FailuresSummary { interface PageFailure { versions: Set languages: Set + // Full error text to the number of failures reporting it, so the report can + // lead with the dominant error rather than an alphabetically lucky one. + errors: Map +} + +// A page usually fails identically across every version and language it appears +// in, so the same error repeats many times. Show a few distinct ones per page, +// keep each short, and keep the whole report inside the limits of the places it +// gets posted. A GitHub issue body is rejected outright over 65536 characters, +// which would lose the entire alert during the largest incidents. +const MAX_ERRORS_PER_PAGE = 3 +const MAX_ERROR_LENGTH = 200 +const MAX_MESSAGE_LENGTH = 30000 + +/** + * Renders a failure as a single line of `errorType: error`, collapsing any + * whitespace so one failure can never span multiple lines of the report. + */ +function formatError(failure: Failure): string { + const normalize = (value: unknown) => + typeof value === 'string' ? value.replace(/\s+/g, ' ').trim() : '' + + const detail = normalize(failure.error) + const errorType = normalize(failure.errorType) + + return errorType && detail ? `${errorType}: ${detail}` : errorType || detail +} + +/** + * Escapes the characters Slack treats as control syntax, so error text lifted + * from an API response cannot inject a mention such as `` into the + * notification. The slack-alert action escapes its own interpolated fields for + * this reason, but passes a caller-supplied message through verbatim. + * + * The same string is also posted as a GitHub issue body, where these entities + * render back to the original characters. + */ +function escapeSlackControlCharacters(text: string): string { + return text.replace(/&/g, '&').replace(//g, '>') +} + +/** + * Truncates on code points so a multi-byte character is never split in half. + * Docs content is translated, so error text routinely carries non-ASCII. + */ +function truncate(text: string, maxLength: number): string { + const characters = Array.from(text) + if (characters.length <= maxLength) return text + return `${characters.slice(0, maxLength - 3).join('')}...` } export interface AggregationResult { @@ -67,12 +116,16 @@ export function aggregateFailures( pageFailures.set(pagePath, { versions: new Set(), languages: new Set(), + errors: new Map(), }) } const pageData = pageFailures.get(pagePath)! pageData.versions.add(langFailures.indexVersion) pageData.languages.add(langFailures.languageCode) + + const error = formatError(failure) + if (error) pageData.errors.set(error, (pageData.errors.get(error) || 0) + 1) } } } @@ -91,17 +144,83 @@ export function aggregateFailures( // Sort pages alphabetically and format each const sortedPages = Array.from(pageFailures.entries()).sort((a, b) => a[0].localeCompare(b[0])) - for (const [pagePath, data] of sortedPages) { + const renderedPages = sortedPages.map(([pagePath, data]) => { const versions = Array.from(data.versions).sort().join(', ') const languages = Array.from(data.languages).sort().join(', ') - lines.push(`• \`${pagePath}\` (versions: ${versions}, languages: ${languages})`) + const bullet = `• \`${escapeSlackControlCharacters(pagePath)}\` (versions: ${versions}, languages: ${languages})` + + // Truncate before escaping so an entity is never cut in half, and so the + // limit stays a limit on the error itself rather than on its encoding. + // Merge counts after rendering: two errors that differ only past the + // truncation point would otherwise print as two identical lines. + const renderedErrors = new Map() + for (const [error, count] of data.errors) { + const rendered = escapeSlackControlCharacters(truncate(error, MAX_ERROR_LENGTH)) + renderedErrors.set(rendered, (renderedErrors.get(rendered) || 0) + count) + } + + // Most frequent error first, breaking ties alphabetically so the report is + // stable across runs on the same input. + const errors = Array.from(renderedErrors.entries()).sort( + (a, b) => b[1] - a[1] || a[0].localeCompare(b[0]), + ) + + const errorLines = errors + .slice(0, MAX_ERRORS_PER_PAGE) + .map(([error, count]) => ` ↳ ${error}${count > 1 ? ` (×${count})` : ''}`) + if (errors.length > MAX_ERRORS_PER_PAGE) { + errorLines.push(` ↳ ...and ${errors.length - MAX_ERRORS_PER_PAGE} more distinct error(s)`) + } + + return { bullet, errorLines } + }) + + const truncatedPagesLine = (count: number) => + `...and ${count} more page(s) not listed. See the workflow run for the full set.` + const footerLines = workflowUrl ? ['', `Workflow: ${workflowUrl}`] : [] + + // Reserve room for the footer up front, using the longest the truncation + // notice could get, so MAX_MESSAGE_LENGTH bounds the whole message rather + // than just the part written inside the loop. + const footerReserve = + truncatedPagesLine(sortedPages.length).length + + 1 + + footerLines.reduce((total, line) => total + line.length + 1, 0) + const budget = MAX_MESSAGE_LENGTH - footerReserve + + let usedLength = lines.join('\n').length + + // Which pages get listed is decided before any error text is added, since the + // page list is the report and the errors are the hint. Otherwise a handful of + // long errors would crowd out most of the pages. + const shownPages: { bullet: string; errorLines: string[]; shownErrorLines: string[] }[] = [] + for (const page of renderedPages) { + const bulletLength = page.bullet.length + 1 + // Always show at least one page, even if that page alone blows the budget. + if (shownPages.length > 0 && usedLength + bulletLength > budget) break + usedLength += bulletLength + shownPages.push({ ...page, shownErrorLines: [] }) + } + + errorLineBudget: for (const page of shownPages) { + for (const errorLine of page.errorLines) { + const errorLineLength = errorLine.length + 1 + if (usedLength + errorLineLength > budget) break errorLineBudget + usedLength += errorLineLength + page.shownErrorLines.push(errorLine) + } + } + + for (const page of shownPages) { + lines.push(page.bullet, ...page.shownErrorLines) } - if (workflowUrl) { - lines.push('') - lines.push(`Workflow: ${workflowUrl}`) + if (shownPages.length < sortedPages.length) { + lines.push(truncatedPagesLine(sortedPages.length - shownPages.length)) } + lines.push(...footerLines) + const message = lines.join('\n') return { hasFailures: true, message, totalCount: uniquePageCount } diff --git a/src/search/tests/aggregate-search-index-failures.ts b/src/search/tests/aggregate-search-index-failures.ts index 08c5e31fca02..3a17988afba8 100644 --- a/src/search/tests/aggregate-search-index-failures.ts +++ b/src/search/tests/aggregate-search-index-failures.ts @@ -166,4 +166,356 @@ describe('aggregateFailures', () => { const result = aggregateFailures(failures) expect(result.message).toContain('unknown') }) + + test('includes the error type and message for each page', () => { + const failures: FailuresSummary[] = [ + { + totalFailedPages: 1, + failures: [ + { + indexName: 'github-docs-dotcom-pt', + languageCode: 'pt', + indexVersion: 'dotcom', + failures: [ + { + relativePath: 'codespaces/index.md', + error: 'tag "endif" not found, line:1, col:131', + errorType: 'API Error', + }, + ], + }, + ], + }, + ] + + const result = aggregateFailures(failures) + expect(result.message).toContain('API Error: tag "endif" not found, line:1, col:131') + }) + + test('shows an identical error once per page rather than once per failure', () => { + const versions = ['dotcom', 'ghes-3.19', 'ghes-3.20'] + const failures: FailuresSummary[] = [ + { + totalFailedPages: versions.length, + failures: versions.map((indexVersion) => ({ + indexName: `github-docs-${indexVersion}-ko`, + languageCode: 'ko', + indexVersion, + failures: [ + { + relativePath: 'organizations/index.md', + error: 'tag "else" not found, line:1, col:16', + errorType: 'API Error', + }, + ], + })), + }, + ] + + const result = aggregateFailures(failures) + const occurrences = result.message.split('tag "else" not found').length - 1 + expect(occurrences).toBe(1) + expect(result.message).toContain('API Error: tag "else" not found, line:1, col:16 (×3)') + }) + + test('keeps distinct errors for the same page and caps the list', () => { + const failures: FailuresSummary[] = [ + { + totalFailedPages: 4, + failures: [ + { + indexName: 'github-docs-dotcom-en', + languageCode: 'en', + indexVersion: 'dotcom', + failures: [ + { relativePath: 'content/page.md', error: 'first', errorType: 'API Error' }, + { relativePath: 'content/page.md', error: 'second', errorType: 'API Error' }, + { relativePath: 'content/page.md', error: 'third', errorType: 'Timeout' }, + { relativePath: 'content/page.md', error: 'fourth', errorType: 'Network Error' }, + ], + }, + ], + }, + ] + + const result = aggregateFailures(failures) + expect(result.message).toContain('and 1 more distinct error(s)') + expect(result.totalCount).toBe(1) + }) + + test('shows the most frequent error first', () => { + const rare = { relativePath: 'content/page.md', error: 'aaa rare', errorType: 'API Error' } + const common = { relativePath: 'content/page.md', error: 'zzz common', errorType: 'API Error' } + const failures: FailuresSummary[] = [ + { + totalFailedPages: 1, + failures: [ + { + indexName: 'github-docs-dotcom-en', + languageCode: 'en', + indexVersion: 'dotcom', + failures: [rare, common, common, common], + }, + ], + }, + ] + + const result = aggregateFailures(failures) + // Alphabetically 'aaa rare' sorts first, so ordering by count is what puts + // the common error above it. + expect(result.message.indexOf('zzz common')).toBeLessThan(result.message.indexOf('aaa rare')) + }) + + test('merges errors that render identically after truncation and counts them', () => { + const prefix = 'x'.repeat(300) + const failures: FailuresSummary[] = [ + { + totalFailedPages: 2, + failures: [ + { + indexName: 'github-docs-dotcom-en', + languageCode: 'en', + indexVersion: 'dotcom', + failures: [ + { relativePath: 'content/page.md', error: `${prefix}-one`, errorType: 'API Error' }, + { relativePath: 'content/page.md', error: `${prefix}-two`, errorType: 'API Error' }, + ], + }, + ], + }, + ] + + const result = aggregateFailures(failures) + const errorLines = result.message.split('\n').filter((line) => line.includes('↳')) + expect(errorLines).toHaveLength(1) + expect(errorLines[0]).toContain('(×2)') + }) + + test('omits the count for an error reported only once', () => { + const failures: FailuresSummary[] = [ + { + totalFailedPages: 1, + failures: [ + { + indexName: 'github-docs-dotcom-en', + languageCode: 'en', + indexVersion: 'dotcom', + failures: [{ relativePath: 'content/page.md', error: 'boom', errorType: 'API Error' }], + }, + ], + }, + ] + + const result = aggregateFailures(failures) + expect(result.message).toContain('↳ API Error: boom') + expect(result.message).not.toContain('(×') + }) + + test('escapes Slack control characters in the page path too', () => { + const failures: FailuresSummary[] = [ + { + totalFailedPages: 1, + failures: [ + { + indexName: 'github-docs-dotcom-en', + languageCode: 'en', + indexVersion: 'dotcom', + failures: [ + { relativePath: 'content/.md', error: 'boom', errorType: 'API Error' }, + ], + }, + ], + }, + ] + + const result = aggregateFailures(failures) + expect(result.message).not.toContain('') + expect(result.message).toContain('content/<!channel>.md') + }) + + test('ignores malformed non-string error fields instead of throwing', () => { + const failures = [ + { + totalFailedPages: 1, + failures: [ + { + indexName: 'github-docs-dotcom-en', + languageCode: 'en', + indexVersion: 'dotcom', + failures: [{ relativePath: 'content/page.md', error: { nested: true }, errorType: 42 }], + }, + ], + }, + ] as unknown as FailuresSummary[] + + const result = aggregateFailures(failures) + expect(result.hasFailures).toBe(true) + expect(result.message).toContain('content/page.md') + expect(result.message).not.toContain('↳') + }) + + test('caps the overall message and says how many pages were left out', () => { + const failures: FailuresSummary[] = [ + { + totalFailedPages: 2000, + failures: [ + { + indexName: 'github-docs-dotcom-en', + languageCode: 'en', + indexVersion: 'dotcom', + failures: Array.from({ length: 2000 }, (_, index) => ({ + relativePath: `content/page-${String(index).padStart(4, '0')}.md`, + error: 'tag "endif" not found, line:1, col:131', + errorType: 'API Error', + })), + }, + ], + }, + ] + + const workflowUrl = 'https://github.com/github/docs-internal/actions/runs/12345678901' + const result = aggregateFailures(failures, workflowUrl) + expect(result.totalCount).toBe(2000) + // The footer is reserved for up front, so the cap holds for the whole + // message rather than just the page list. + expect(result.message.length).toBeLessThanOrEqual(30000) + expect(result.message).toContain(workflowUrl) + expect(result.message).toMatch(/and \d+ more page\(s\) not listed/) + }) + + test('lists as many pages as fit before spending the budget on error text', () => { + const pageCount = 2000 + const failures: FailuresSummary[] = [ + { + totalFailedPages: pageCount, + failures: [ + { + indexName: 'github-docs-dotcom-en', + languageCode: 'en', + indexVersion: 'dotcom', + failures: Array.from({ length: pageCount }, (_, index) => ({ + relativePath: `content/page-${String(index).padStart(4, '0')}.md`, + error: 'x'.repeat(200), + errorType: 'API Error', + })), + }, + ], + }, + ] + + const result = aggregateFailures(failures) + const bullets = result.message.split('\n').filter((line) => line.startsWith('•')).length + const errorLines = result.message.split('\n').filter((line) => line.includes('↳')).length + + // Errors are only worth showing for the pages that fit, so the long ones + // must not push pages out of the list. + expect(bullets).toBeGreaterThan(400) + expect(errorLines).toBeLessThan(bullets) + }) + + test('collapses newlines so an error cannot span multiple lines', () => { + const failures: FailuresSummary[] = [ + { + totalFailedPages: 1, + failures: [ + { + indexName: 'github-docs-dotcom-en', + languageCode: 'en', + indexVersion: 'dotcom', + failures: [ + { + relativePath: 'content/page.md', + error: 'first line\nsecond line', + errorType: 'API Error', + }, + ], + }, + ], + }, + ] + + const result = aggregateFailures(failures) + expect(result.message).toContain('API Error: first line second line') + expect(result.message.split('\n').filter((line) => line.includes('↳'))).toHaveLength(1) + }) + + test('truncates a very long error', () => { + const failures: FailuresSummary[] = [ + { + totalFailedPages: 1, + failures: [ + { + indexName: 'github-docs-dotcom-en', + languageCode: 'en', + indexVersion: 'dotcom', + failures: [ + { relativePath: 'content/page.md', error: 'x'.repeat(500), errorType: 'API Error' }, + ], + }, + ], + }, + ] + + const result = aggregateFailures(failures) + const errorLine = result.message.split('\n').find((line) => line.includes('↳'))! + expect(errorLine.length).toBeLessThan(250) + expect(errorLine).toContain('...') + }) + + test('truncates on code point boundaries rather than splitting a character', () => { + const failures: FailuresSummary[] = [ + { + totalFailedPages: 1, + failures: [ + { + indexName: 'github-docs-dotcom-ja', + languageCode: 'ja', + indexVersion: 'dotcom', + failures: [ + { relativePath: 'content/page.md', error: '🚀'.repeat(300), errorType: 'API Error' }, + ], + }, + ], + }, + ] + + const result = aggregateFailures(failures) + const errorLine = result.message.split('\n').find((line) => line.includes('↳'))! + + // A lone surrogate in either direction means a rocket was cut in half. + expect(errorLine).not.toMatch(/[\uD800-\uDBFF](?![\uDC00-\uDFFF])/) + expect(errorLine).not.toMatch(/(? { + const failures: FailuresSummary[] = [ + { + totalFailedPages: 1, + failures: [ + { + indexName: 'github-docs-dotcom-en', + languageCode: 'en', + indexVersion: 'dotcom', + failures: [ + { + relativePath: 'content/page.md', + error: 'unexpected near <@U012ABCDEF> & ', + errorType: 'API Error', + }, + ], + }, + ], + }, + ] + + const result = aggregateFailures(failures) + expect(result.message).not.toContain('') + expect(result.message).not.toContain('<@U012ABCDEF>') + expect(result.message).toContain('<!channel>') + expect(result.message).toContain('&') + }) })