diff --git a/src/services/githubApi.js b/src/services/githubApi.js index 9e4376f..038742e 100644 --- a/src/services/githubApi.js +++ b/src/services/githubApi.js @@ -104,10 +104,11 @@ export function buildGithubReleaseListApiUrl(owner, repo, perPage = 1) { return `${API_BASE}/repos/${owner}/${repo}/releases?per_page=${safePerPage}`; } -export function buildGithubCommitListApiUrl(owner, repo, defaultBranch = '', perPage = 1) { +export function buildGithubCommitListApiUrl(owner, repo, defaultBranch = '', perPage = 1, page = null) { const safePerPage = Number.isInteger(perPage) ? Math.min(Math.max(perPage, 1), 100) : 1; const branchFilter = defaultBranch ? `&sha=${encodeURIComponent(defaultBranch)}` : ''; - return `${API_BASE}/repos/${owner}/${repo}/commits?per_page=${safePerPage}${branchFilter}`; + const pageFilter = Number.isInteger(page) && page > 1 ? `&page=${page}` : ''; + return `${API_BASE}/repos/${owner}/${repo}/commits?per_page=${safePerPage}${branchFilter}${pageFilter}`; } export function buildGithubBranchApiUrl(owner, repo, branch) { diff --git a/src/services/githubImporter.js b/src/services/githubImporter.js index 3ce8ddf..933cd1f 100644 --- a/src/services/githubImporter.js +++ b/src/services/githubImporter.js @@ -33,6 +33,7 @@ import { resolveContributorFallbackLimit, } from './githubImporterContributors.js'; import { dedupeAuthors } from './githubImporterAuthors.js'; +import { fetchCommitAuthors } from './githubImporterCommitAuthors.js'; import { addCitationConsistencyWarnings, mergeMetadata } from './githubImporterMerge.js'; export { addCitationConsistencyWarnings, mergeMetadata }; import { @@ -549,7 +550,7 @@ export async function importGithubMetadata(repoUrl, options = {}) { ); const releaseData = Array.isArray(releaseList) && releaseList.length > 0 ? releaseList[0] : null; const recentCommitPayload = await fetchOptionalJson( - buildGithubCommitListApiUrl(owner, repo, defaultBranch, 10), + buildGithubCommitListApiUrl(owner, repo, defaultBranch, 100), buildGithubRequestConfig({ authToken, source: 'commits', @@ -716,10 +717,17 @@ export async function importGithubMetadata(repoUrl, options = {}) { fetchOptionalJson, extractOrcidFromGithubProfile, }); + const commitAuthors = await fetchCommitAuthors({ + owner, repo, defaultBranch, initialCommits: recentCommitPayload, + knownGithubLogins: contributorResult.githubLogins, + warnings, authToken, cleanString, normalizeAuthor, normalizeAuthors, + addWarning, fetchOptionalJson, + }); const coAuthorAuthors = normalizeAuthors(commitCoAuthorNames.map((name) => normalizeAuthor({ name }))); const contributors = dedupeAuthors([ ...coAuthorAuthors, ...contributorResult.fallbackAuthors.filter(Boolean), + ...commitAuthors, ]); const contributorLookupAuthors = dedupeAuthors([ ...coAuthorAuthors, @@ -815,4 +823,3 @@ export async function importGithubMetadata(repoUrl, options = {}) { return { metadata, warnings, errors, review, healthScan, comparisons }; } - diff --git a/src/services/githubImporterCommitAuthors.js b/src/services/githubImporterCommitAuthors.js new file mode 100644 index 0000000..92155b9 --- /dev/null +++ b/src/services/githubImporterCommitAuthors.js @@ -0,0 +1,74 @@ +import { buildGithubCommitListApiUrl, buildGithubRequestConfig } from './githubApi.js'; +import { dedupeAuthors } from './githubImporterAuthors.js'; +import { isAutomatedContributorIdentity } from './githubImporterContributors.js'; + +const COMMIT_PAGE_SIZE = 100; +const UNAUTHENTICATED_PAGE_LIMIT = 1; +const AUTHENTICATED_PAGE_LIMIT = 10; + +function isUsableCommitName(name, login, knownLogins, cleanString) { + const normalized = name.toLowerCase(); + if (!name || normalized === String(login ?? '').toLowerCase() || knownLogins.has(normalized)) { + return false; + } + if ( + /\d/.test(name) + || /[_@]/.test(name) + || /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(name) + || isAutomatedContributorIdentity(name, cleanString) + || isAutomatedContributorIdentity(login, cleanString) + ) { + return false; + } + if (!/\s/.test(name) && name.includes('-') && !name.split('-').every((segment) => /^[A-Z][a-z]+$/.test(segment))) { + return false; + } + return true; +} + +export async function fetchCommitAuthors({ + owner, repo, defaultBranch, initialCommits = [], knownGithubLogins = [], + warnings, authToken = '', cleanString, normalizeAuthor, normalizeAuthors, + addWarning, fetchOptionalJson, +}) { + const knownLogins = new Set(knownGithubLogins.map((login) => cleanString(login).toLowerCase())); + const discoveredAuthors = []; + let commits = Array.isArray(initialCommits) ? initialCommits : []; + let page = 1; + const maxPages = authToken ? AUTHENTICATED_PAGE_LIMIT : UNAUTHENTICATED_PAGE_LIMIT; + + while (true) { + for (const commit of commits) { + const name = cleanString(commit?.commit?.author?.name ?? ''); + const login = cleanString(commit?.author?.login ?? ''); + if (login) knownLogins.add(login.toLowerCase()); + if (commit?.author?.type && commit.author.type !== 'User') continue; + discoveredAuthors.push({ name, login }); + } + + if (commits.length < COMMIT_PAGE_SIZE) break; + if (page >= maxPages) { + addWarning(warnings, 'commit-authors', 'commit-author-scan-limited', + `Scanned the first ${page * COMMIT_PAGE_SIZE} commits for contributor author names.`, + { owner, repo, scannedPages: page, scannedCommits: page * COMMIT_PAGE_SIZE }); + break; + } + + page += 1; + commits = await fetchOptionalJson( + buildGithubCommitListApiUrl(owner, repo, defaultBranch, COMMIT_PAGE_SIZE, page), + buildGithubRequestConfig({ + authToken, + source: 'commit-authors', + label: `commit authors page ${page}`, + onWarning: (source, code, message, details = {}) => addWarning(warnings, source, code, message, details), + }), + ) || []; + if (!Array.isArray(commits) || commits.length === 0) break; + } + + const authorNames = discoveredAuthors + .filter(({ name, login }) => isUsableCommitName(name, login, knownLogins, cleanString)) + .map(({ name }) => normalizeAuthor({ name })); + return dedupeAuthors(normalizeAuthors(authorNames)); +} \ No newline at end of file diff --git a/src/services/githubImporterContributors.js b/src/services/githubImporterContributors.js index 3c63d18..d5cba16 100644 --- a/src/services/githubImporterContributors.js +++ b/src/services/githubImporterContributors.js @@ -10,7 +10,7 @@ const TOP_CONTRIBUTOR_FALLBACK_LIMIT = 4; const MAX_CONTRIBUTOR_FALLBACK_LIMIT = 20; const GITHUB_PAGE_SIZE = 100; -function isAutomatedContributorIdentity(value, cleanString) { +export function isAutomatedContributorIdentity(value, cleanString) { const text = cleanString(value ?? '').trim(); if (!text) { return false; @@ -375,5 +375,6 @@ export async function fetchContributorAuthors({ return { fallbackAuthors: dedupeAuthors(normalizeAuthors(fallbackAuthors)), lookupAuthors: dedupeAuthors(normalizeAuthors(lookupAuthors)), + githubLogins: contributors.map((contributor) => cleanString(contributor?.login ?? '').toLowerCase()).filter(Boolean), }; } diff --git a/tests/services/commitAuthors.test.js b/tests/services/commitAuthors.test.js new file mode 100644 index 0000000..eb4758f --- /dev/null +++ b/tests/services/commitAuthors.test.js @@ -0,0 +1,105 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; + +import { fetchCommitAuthors } from '../../src/services/githubImporterCommitAuthors.js'; +import { cleanString, normalizeAuthor, normalizeAuthors } from '../../src/services/githubImporterUtils.js'; + +function commit(name, login = '') { + return { commit: { author: { name } }, author: login ? { login, type: 'User' } : null }; +} + +test('authenticated scans continue to page two for human author names', async () => { + const urls = []; + const warnings = []; + const result = await fetchCommitAuthors({ + owner: 'test-owner', repo: 'test-repo', defaultBranch: 'feature/new', authToken: 'token', + initialCommits: Array.from({ length: 100 }, () => commit('known-login')), + knownGithubLogins: ['known-login'], warnings, + cleanString, normalizeAuthor, normalizeAuthors, + addWarning: (items, source, code) => items.push({ source, code }), + fetchOptionalJson: async (url) => { + urls.push(url); + return [commit('Ada Lovelace'), commit('bot-123'), commit('name-handle'), commit('Alice Example', 'Alice Example')]; + }, + }); + assert.equal(urls.length, 1); + assert.equal(urls[0].endsWith('commits?per_page=100&sha=feature%2Fnew&page=2'), true); + assert.deepEqual(result.map((author) => `${author.givenNames} ${author.familyNames}`), ['Ada Lovelace']); + assert.equal(warnings.length, 0); +}); + +test('commit names matching logins on any scanned page are excluded', async () => { + const initialCommits = [ + commit('Jane-Doe'), + commit('Anna-Maria'), + commit('Mary Smith-Jones'), + ...Array.from({ length: 97 }, () => commit('Repeated Person')), + ]; + const result = await fetchCommitAuthors({ + owner: 'test-owner', + repo: 'test-repo', + defaultBranch: 'main', + authToken: 'token', + initialCommits, + knownGithubLogins: [], + warnings: [], + cleanString, + normalizeAuthor, + normalizeAuthors, + addWarning: () => {}, + fetchOptionalJson: async () => [ + commit('Real Author', 'jane-doe'), + commit('Another Contributor', 'ANNA-MARIA'), + commit('Jean-Luc'), + commit('Full Name', 'jean-luc'), + ], + }); + + assert.deepEqual(result.map((author) => `${author.givenNames} ${author.familyNames}`), [ + 'Mary Smith Jones', + 'Repeated Person', + 'Real Author', + 'Another Contributor', + 'Full Name', + ]); +}); + +test('unauthenticated scans stop after the first 100 commits', async () => { + const warnings = []; + const result = await fetchCommitAuthors({ + owner: 'test-owner', repo: 'test-repo', initialCommits: Array.from({ length: 100 }, () => commit('Dana Example')), + warnings, cleanString, normalizeAuthor, normalizeAuthors, + addWarning: (items, source, code) => items.push({ source, code }), + fetchOptionalJson: async () => { throw new Error('Should not request another page'); }, + }); + assert.equal(result.length, 1); + assert.equal(warnings.some((warning) => warning.code === 'commit-author-scan-limited'), true); +}); + +test('commit history excludes automated identities and aliases but accepts full hyphenated names', async () => { + const result = await fetchCommitAuthors({ + owner: 'test-owner', + repo: 'test-repo', + initialCommits: [ + commit('Jean-Pierre Dupont'), + commit('Mary Smith-Jones'), + commit('Claude Code'), + commit('GitHub Actions'), + commit('GitHub Actions', 'actions-bot'), + commit('Claude Fable'), + commit('alice_dev'), + commit('dev@example.com'), + ], + warnings: [], + cleanString, + normalizeAuthor, + normalizeAuthors, + addWarning: () => {}, + fetchOptionalJson: async () => [], + }); + + assert.deepEqual(result.map((author) => `${author.givenNames} ${author.familyNames}`), [ + 'Jean Pierre Dupont', + 'Mary Smith Jones', + ]); +}); \ No newline at end of file diff --git a/tests/services/githubImporter.test.js b/tests/services/githubImporter.test.js index 83fbd12..c866b04 100644 --- a/tests/services/githubImporter.test.js +++ b/tests/services/githubImporter.test.js @@ -996,7 +996,7 @@ test('importGithubMetadata ignores username-like contributors when no real profi return new Response(JSON.stringify({ message: 'Not Found' }), { status: 404, headers: { 'Content-Type': 'application/json' } }); } - if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=10&sha=main')) { + if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=100&sha=main')) { return Response.json([ { commit: { @@ -1075,7 +1075,7 @@ test('importGithubMetadata excludes AI bot co-authors and contributor accounts w return new Response(JSON.stringify({ message: 'Not Found' }), { status: 404, headers: { 'Content-Type': 'application/json' } }); } - if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=10&sha=main')) { + if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=100&sha=main')) { return Response.json([ { commit: { @@ -1238,7 +1238,7 @@ test('importGithubMetadata ignores GitHub usernames in co-author names and prefe return new Response(JSON.stringify({ message: 'Not Found' }), { status: 404, headers: { 'Content-Type': 'application/json' } }); } - if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=10&sha=main')) { + if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=100&sha=main')) { return Response.json([ { commit: { @@ -1318,7 +1318,7 @@ test('importGithubMetadata prefers commit co-author names over username fallback return new Response(JSON.stringify({ message: 'Not Found' }), { status: 404, headers: { 'Content-Type': 'application/json' } }); } - if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=10&sha=main')) { + if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=100&sha=main')) { return Response.json([ { commit: { @@ -1397,7 +1397,7 @@ test('importGithubMetadata includes co-authored contributor names from commit me return new Response(JSON.stringify({ message: 'Not Found' }), { status: 404, headers: { 'Content-Type': 'application/json' } }); } - if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=10&sha=main')) { + if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=100&sha=main')) { return Response.json([ { commit: { @@ -1475,7 +1475,7 @@ test('importGithubMetadata includes co-authored contributor names from recent hi return new Response(JSON.stringify({ message: 'Not Found' }), { status: 404, headers: { 'Content-Type': 'application/json' } }); } - if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=10&sha=main')) { + if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=100&sha=main')) { return Response.json([ { commit: { @@ -1554,7 +1554,7 @@ test('importGithubMetadata includes co-authored contributor names when a release }); } - if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=10&sha=main')) { + if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=100&sha=main')) { return Response.json([ { commit: { @@ -1614,8 +1614,15 @@ test('importGithubMetadata includes co-authored contributor names when a release } }); -test('importGithubMetadata orders imported authors by contributor rank', async () => { +test('importGithubMetadata does not reorder metadata authors based on history-only commits', async () => { const originalFetch = globalThis.fetch; + const jsonResponse = (data) => ({ + ok: true, + status: 200, + statusText: 'OK', + headers: { get: () => null }, + text: async () => JSON.stringify(data), + }); const citationText = `cff-version: 1.2.0\ntitle: "OpenCite"\nversion: "1.0.0"\ndate-released: "2025-01-02"\nrepository-code: "https://github.com/test-owner/test-repo"\nauthors:\n - family-names: "Doe"\n given-names: "Jane"\n - family-names: "Smith"\n given-names: "John"\n`; const citationBase64 = Buffer.from(citationText, 'utf8').toString('base64'); @@ -1624,7 +1631,7 @@ test('importGithubMetadata orders imported authors by contributor rank', async ( const value = String(url); if (value.endsWith('/repos/test-owner/test-repo')) { - return Response.json({ + return jsonResponse({ name: 'test-repo', html_url: 'https://github.com/test-owner/test-repo', default_branch: 'main', @@ -1639,55 +1646,37 @@ test('importGithubMetadata orders imported authors by contributor rank', async ( } if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=1&sha=main')) { - return Response.json([{ commit: { committer: { date: '2025-01-02T00:00:00Z' } } }]); + return jsonResponse([{ commit: { committer: { date: '2025-01-02T00:00:00Z' } } }]); + } + + if (value.endsWith('/repos/test-owner/test-repo/commits?per_page=100&sha=main')) { + return jsonResponse([ + { commit: { author: { name: 'John Smith' } }, author: null }, + { commit: { author: { name: 'Ada Lovelace' } }, author: null }, + { commit: { author: { name: 'Ada Lovelace' } }, author: null }, + ]); } if (value.endsWith('/repos/test-owner/test-repo/branches/main')) { - return Response.json({ name: 'main' }); + return jsonResponse({ name: 'main' }); } if (value.includes('/repos/test-owner/test-repo/contents/CITATION.cff?ref=main')) { - return Response.json({ encoding: 'base64', content: citationBase64 }); + return jsonResponse({ encoding: 'base64', content: citationBase64 }); } if (value.includes('/repos/test-owner/test-repo/contents/')) { - return new Response(JSON.stringify({ message: 'Not Found' }), { status: 404, headers: { 'Content-Type': 'application/json' } }); + return { + ok: false, + status: 404, + statusText: 'Not Found', + headers: { get: () => null }, + text: async () => JSON.stringify({ message: 'Not Found' }), + }; } if (value.includes('/repos/test-owner/test-repo/contributors?')) { - // John appears first => highest contributor rank. - return Response.json([ - { login: 'johnsmith', type: 'User' }, - { login: 'janedoe', type: 'User' }, - ]); - } - - if (value.endsWith('/users/janedoe')) { - return Response.json({ - login: 'janedoe', - type: 'User', - name: 'Jane Doe', - company: 'Imageomics', - html_url: 'https://github.com/janedoe', - }); - } - - if (value.endsWith('/users/johnsmith')) { - return Response.json({ - login: 'johnsmith', - type: 'User', - name: 'John Smith', - company: 'Imageomics', - html_url: 'https://github.com/johnsmith', - }); - } - - if (value.endsWith('/users/janedoe/social_accounts') || value.endsWith('/users/johnsmith/social_accounts')) { - return Response.json([]); - } - - if (value === 'https://github.com/janedoe' || value === 'https://github.com/johnsmith') { - return new Response('', { status: 200, headers: { 'Content-Type': 'text/html' } }); + return jsonResponse([]); } throw new Error(`Unexpected fetch URL: ${value}`); @@ -1698,10 +1687,11 @@ test('importGithubMetadata orders imported authors by contributor rank', async ( contributorFallbackLimit: 5, }); - assert.equal(result.errors.length, 0); - assert.equal(result.metadata.authors.length >= 2, true); - assert.equal(result.metadata.authors[0].givenNames, 'John'); - assert.equal(result.metadata.authors[0].familyNames, 'Smith'); + assert.equal(result.errors.length, 0, JSON.stringify(result.errors)); + assert.deepEqual( + result.metadata.authors.map(({ givenNames, familyNames }) => `${givenNames} ${familyNames}`), + ['Jane Doe', 'John Smith', 'Ada Lovelace'], + ); } finally { globalThis.fetch = originalFetch; }