From 2e7a5db1cc132841877e75a1a2b75ac509f7e372 Mon Sep 17 00:00:00 2001 From: Ken Britton Date: Thu, 6 Aug 2026 08:19:43 -0700 Subject: [PATCH 1/7] fix(#11324): recognise closing keywords on non-default-branch PRs The linked-issue check read GitHub's closingIssuesReferences, which is only populated for PRs targeting the default branch. On any other base the field is empty even when the contributor linked the issue correctly, so the check failed and told them to add a closing keyword they had already added. When GitHub reports no linkage, the PR body is now parsed for closing keywords and each reference is resolved through the issues API, so the existing same-org filter and assignee check keep working unchanged. - `#123`, `owner/repo#123` and full issue URLs are all recognised - comparisons are case-insensitive, since GitHub owner and repo names are - HTML comments are stripped first, so an unfilled template does not read as linked - a reference to a pull request is not a link - only a 404 means "no such issue"; any other lookup failure is reported as a warning rather than blaming the contributor for it --- scripts/ci/andra-bot.js | 110 +++++++++- .../mocha/unit/testingtests/andra-bot.spec.js | 201 +++++++++++++++++- 2 files changed, 303 insertions(+), 8 deletions(-) diff --git a/scripts/ci/andra-bot.js b/scripts/ci/andra-bot.js index a47b216232d..f5943d4e156 100644 --- a/scripts/ci/andra-bot.js +++ b/scripts/ci/andra-bot.js @@ -95,7 +95,85 @@ const matchesLicense = (prBody, template) => { return bodyLicenseSections.every(section => section.content.startsWith(templateLicenseSection.content)); }; -const getLinkedIssues = async (github, context) => { +const MAX_LINKED_ISSUES = 20; + +// GitHub's documented closing keywords, and the three reference forms they accept: +// `#123`, `owner/repo#123`, and a full issue URL. +// https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue +const CLOSING_KEYWORDS = 'close[sd]?|fix(?:e[sd])?|resolve[sd]?'; +const REPO_NAME = '[\\w.-]+'; +const CLOSING_REFERENCE_REGEX = new RegExp( + `\\b(?:${CLOSING_KEYWORDS})\\b\\s*:?\\s+` + + `(?:https?://github\\.com/(${REPO_NAME})/(${REPO_NAME})/issues/(\\d+)` + + `|(?:(${REPO_NAME})/(${REPO_NAME}))?#(\\d+))`, + 'gi' +); + +// GitHub only populates closingIssuesReferences from keywords when the PR targets the +// repository's default branch; on any other base the field is empty even though the +// contributor linked the issue correctly. Parsing the body covers that case. +const parseClosingReferences = (body, context) => { + const matches = stripComments(body || '').matchAll(CLOSING_REFERENCE_REGEX); + const references = [...matches].map(([, urlOwner, urlRepo, urlNumber, owner, repo, number]) => ({ + owner: urlOwner || owner || context.repo.owner, + repo: urlRepo || repo || context.repo.repo, + number: Number(urlNumber || number), + })); + + // Owner and repo names are case-insensitive on GitHub, so the key is too. + const seen = new Set(); + return references.filter(reference => { + const key = `${reference.owner}/${reference.repo}#${reference.number.toString()}`.toLowerCase(); + if (seen.has(key)) { + return false; + } + seen.add(key); + return true; + }); +}; + +// Shapes a REST issue like a closingIssuesReferences node so the callers can't tell them apart. +// The names come from repository_url where possible, so they carry GitHub's canonical casing +// rather than whatever the contributor typed. +const toIssueNode = (issue, reference) => { + const [, owner, repo] = /\/repos\/([^/]+)\/([^/]+)$/.exec(issue.repository_url || '') || []; + const nameWithOwner = `${owner || reference.owner}/${repo || reference.repo}`; + return { + number: issue.number, + repository: { nameWithOwner, owner: { login: owner || reference.owner } }, + assignees: { nodes: (issue.assignees || []).map(assignee => ({ login: assignee.login })) }, + }; +}; + +// A referenced issue that genuinely does not exist is not a link. Any other failure means we +// could not tell, and blaming the contributor for that is the very false-fail this check is +// meant to avoid — so those are reported separately rather than folded into "not linked". +const resolveReferencedIssues = async (github, context, core, references) => { + let inconclusive = false; + const issues = await Promise.all(references.map(async (reference) => { + const name = `${reference.owner}/${reference.repo}#${reference.number.toString()}`; + try { + const { data } = await github.rest.issues.get({ + owner: reference.owner, + repo: reference.repo, + issue_number: reference.number, + }); + // A pull request is also an issue on this endpoint, but referencing one is not a link. + return data.pull_request ? null : toIssueNode(data, reference); + } catch (err) { + if (err.status === 404) { + core.info(`Ignoring referenced issue ${name}: not found.`); + } else { + inconclusive = true; + core.warning(`Could not read referenced issue ${name}: ${err.message}`); + } + return null; + } + })); + return { issues: issues.filter(Boolean), inconclusive }; +}; + +const getLinkedIssues = async (github, context, core) => { const query = ` query ($owner: String!, $repo: String!, $number: Int!) { repository(owner: $owner, name: $repo) { @@ -121,9 +199,20 @@ const getLinkedIssues = async (github, context) => { number: context.payload.pull_request.number, }); // Issues linked from repositories outside the org don't count as linked. - return result.repository.pullRequest.closingIssuesReferences.nodes.filter( - issue => issue.repository.owner.login === context.repo.owner - ); + const isInOrg = owner => owner.toLowerCase() === context.repo.owner.toLowerCase(); + const linkedIssues = result.repository.pullRequest.closingIssuesReferences.nodes + .filter(issue => isInOrg(issue.repository.owner.login)); + if (linkedIssues.length) { + return { issues: linkedIssues, inconclusive: false }; + } + + const references = parseClosingReferences(context.payload.pull_request.body, context) + .filter(reference => isInOrg(reference.owner)) + .slice(0, MAX_LINKED_ISSUES); + if (!references.length) { + return { issues: [], inconclusive: false }; + } + return resolveReferencedIssues(github, context, core, references); }; const getLinkedIssueFailure = (pr, linkedIssues) => { @@ -144,7 +233,7 @@ const getLinkedIssueFailure = (pr, linkedIssues) => { return getMessage('not-assigned', { issueList }); }; -const getFailures = async (github, context) => { +const getFailures = async (github, context, core) => { const pr = context.payload.pull_request; const failures = []; @@ -157,7 +246,14 @@ const getFailures = async (github, context) => { failures.push(getMessage('license-changed')); } - const linkedIssues = await getLinkedIssues(github, context); + const { issues: linkedIssues, inconclusive } = await getLinkedIssues(github, context, core); + if (inconclusive && !linkedIssues.length) { + // Failing the PR here would blame the contributor for a GitHub outage. The warning keeps + // the run visible without stranding a correctly-linked PR behind an infrastructure blip. + core.warning('Could not verify the linked issue, skipping that check.'); + return failures; + } + const linkedIssueFailure = getLinkedIssueFailure(pr, linkedIssues); if (linkedIssueFailure) { failures.push(linkedIssueFailure); @@ -278,7 +374,7 @@ const runAndraBot = async ({ github, context, core }) => { return; } - const failures = await getFailures(github, context); + const failures = await getFailures(github, context, core); const existingComment = await findExistingComment(github, context); if (!failures.length) { diff --git a/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js b/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js index 06a78bf8e7b..be3cabc73c7 100644 --- a/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js +++ b/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js @@ -64,6 +64,14 @@ describe('AndraBot', () => { .replace('', 'Fixes the date conversion by using the local format.') .replace('', 'Closes #1234'); + // Same as filledTemplate but with no issue reference at all, for the cases that need a body + // the closing-keyword fallback cannot resolve. + const bodyWithoutIssue = TEMPLATE + .replace('', 'Fixes the date conversion by using the local format.') + .replace('', 'No issue for this one.'); + + const withIssueReference = (reference) => filledTemplate.replace('Closes #1234', reference); + const linkedIssue = (number, assigneeLogins, repo = 'medic/cht-core') => ({ number, repository: { nameWithOwner: repo, owner: { login: repo.split('/')[0] } }, @@ -76,6 +84,26 @@ describe('AndraBot', () => { }); }; + // The REST shape returned by issues.get, which the fallback normalizes. + // GitHub resolves owner/repo case-insensitively, so the stub matches the same way — the + // action passes through whatever the contributor typed. + const matchesIssue = (owner, repo, number) => sinon.match(args => { + return args.owner.toLowerCase() === owner.toLowerCase() && + args.repo.toLowerCase() === repo.toLowerCase() && + args.issue_number === number; + }); + + const setReferencedIssue = ({ owner = 'medic', repo = 'cht-core', number, assignees = [], isPr = false }) => { + return github.rest.issues.get + .withArgs(matchesIssue(owner, repo, number)) + .resolves({ data: { + number, + repository_url: `https://api.github.com/repos/${owner}/${repo}`, + assignees: assignees.map(login => ({ login })), + ...(isPr ? { pull_request: { url: 'https://api.github.com/pulls/1' } } : {}), + } }); + }; + const setComments = (comments) => github.paginate .withArgs(github.rest.issues.listComments) .resolves(comments); @@ -98,6 +126,8 @@ describe('AndraBot', () => { listLabelsOnIssue: sinon.stub(), addLabels: sinon.stub().resolves(), removeLabel: sinon.stub().resolves(), + // Not found by default; tests that exercise the fallback opt in via setReferencedIssue. + get: sinon.stub().rejects(Object.assign(new Error('Not Found'), { status: 404 })), }, }, }; @@ -305,7 +335,7 @@ describe('AndraBot', () => { describe('linked issue check', () => { it('should fail when no issue is linked', async () => { - await run(getPr({ body: filledTemplate })); + await run(getPr({ body: bodyWithoutIssue })); expect(core.setFailed.calledOnce).to.be.true; const commentBody = github.rest.issues.createComment.args[0][0].body; @@ -313,6 +343,7 @@ describe('AndraBot', () => { expect(commentBody).to.not.contain(templateMismatchMessage()); }); + it('should query the PR from the event payload', async () => { await run(getPr({ body: filledTemplate })); @@ -334,12 +365,180 @@ describe('AndraBot', () => { it('should not count an issue linked from a repo outside the org', async () => { setLinkedIssues([linkedIssue(1234, ['external-dev'], 'external-dev/cht-core')]); + await run(getPr({ body: bodyWithoutIssue })); + + expect(core.setFailed.calledOnce).to.be.true; + const commentBody = github.rest.issues.createComment.args[0][0].body; + expect(commentBody).to.contain(getMessage('missing-linked-issue')); + }); + }); + + // GitHub only populates closingIssuesReferences for PRs targeting the default branch, so a + // correctly keyword-linked PR on any other base arrives here with an empty list. + describe('closing-keyword fallback for non-default-branch PRs', () => { + it('should accept a keyword-linked issue when GitHub reports no linkage', async () => { + setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + + await run(getPr({ body: filledTemplate })); + + expect(core.setFailed.called).to.be.false; + expect(github.rest.issues.get.calledOnceWithExactly({ + owner: 'medic', + repo: 'cht-core', + issue_number: 1234, + })).to.be.true; + }); + + it('should still report the assignee failure for a keyword-linked issue', async () => { + setReferencedIssue({ number: 1234, assignees: ['someone-else'] }); + + await run(getPr({ body: filledTemplate })); + + expect(core.setFailed.calledOnce).to.be.true; + const commentBody = github.rest.issues.createComment.args[0][0].body; + expect(commentBody).to.contain(getMessage('not-assigned', { issueList: '#1234' })); + }); + + ['Closes #1234', 'closes: #1234', 'Fixes #1234', 'resolved #1234'].forEach(reference => { + it(`should recognise "${reference}"`, async () => { + setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + + await run(getPr({ body: withIssueReference(reference) })); + + expect(core.setFailed.called).to.be.false; + }); + }); + + it('should recognise an owner/repo#number reference in the same org', async () => { + setReferencedIssue({ repo: 'cht-android', number: 99, assignees: ['external-dev'] }); + + await run(getPr({ body: withIssueReference('Closes medic/cht-android#99') })); + + expect(core.setFailed.called).to.be.false; + }); + + it('should recognise a full issue URL', async () => { + setReferencedIssue({ repo: 'cht-android', number: 99, assignees: ['external-dev'] }); + + await run(getPr({ body: withIssueReference('Closes https://github.com/medic/cht-android/issues/99') })); + + expect(core.setFailed.called).to.be.false; + }); + + it('should ignore a reference to a repo outside the org', async () => { + await run(getPr({ body: withIssueReference('Closes external-dev/cht-core#1234') })); + + expect(core.setFailed.calledOnce).to.be.true; + const commentBody = github.rest.issues.createComment.args[0][0].body; + expect(commentBody).to.contain(getMessage('missing-linked-issue')); + // Reaching out to a repo outside the org is itself the thing to avoid, not just an + // implementation detail — the token has no business reading it. + expect(github.rest.issues.get.called).to.be.false; + }); + + it('should ignore references inside HTML comments', async () => { + await run(getPr({ body: withIssueReference('') })); + + expect(core.setFailed.calledOnce).to.be.true; + const commentBody = github.rest.issues.createComment.args[0][0].body; + expect(commentBody).to.contain(getMessage('missing-linked-issue')); + }); + + it('should ignore the example reference in the unfilled template', async () => { + // The template's own comment block contains "feat(#1234): add hat wobble"; an empty + // template must not read as a linked PR. + setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + + await run(getPr({ body: TEMPLATE })); + + expect(core.setFailed.calledOnce).to.be.true; + const commentBody = github.rest.issues.createComment.args[0][0].body; + expect(commentBody).to.contain(getMessage('missing-linked-issue')); + }); + + it('should ignore a reference that points at a pull request', async () => { + setReferencedIssue({ number: 1234, assignees: ['external-dev'], isPr: true }); + await run(getPr({ body: filledTemplate })); expect(core.setFailed.calledOnce).to.be.true; const commentBody = github.rest.issues.createComment.args[0][0].body; expect(commentBody).to.contain(getMessage('missing-linked-issue')); }); + + it('should ignore a reference to an issue that does not exist', async () => { + await run(getPr({ body: filledTemplate })); + + expect(core.setFailed.calledOnce).to.be.true; + const commentBody = github.rest.issues.createComment.args[0][0].body; + expect(commentBody).to.contain(getMessage('missing-linked-issue')); + }); + + // GitHub owner and repo names are case-insensitive, so a reference that differs only in + // case is still a valid link and must not be dropped. + ['Closes Medic/cht-core#1234', 'Closes MEDIC/CHT-Core#1234'].forEach(reference => { + it(`should accept "${reference}" regardless of case`, async () => { + setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + + await run(getPr({ body: withIssueReference(reference) })); + + expect(core.setFailed.called).to.be.false; + }); + }); + + it('should report a same-repo issue as #number even when referenced with different case', async () => { + setReferencedIssue({ number: 1234, assignees: ['someone-else'] }); + + await run(getPr({ body: withIssueReference('Closes MEDIC/CHT-Core#1234') })); + + const commentBody = github.rest.issues.createComment.args[0][0].body; + expect(commentBody).to.contain(getMessage('not-assigned', { issueList: '#1234' })); + }); + + describe('when the issue cannot be read', () => { + const failGet = (status) => github.rest.issues.get + .rejects(Object.assign(new Error('Server Error'), { status })); + + [500, 403].forEach(status => { + it(`should not claim the PR is unlinked after a ${status.toString()}`, async () => { + failGet(status); + + await run(getPr({ body: filledTemplate })); + + expect(core.setFailed.called).to.be.false; + expect(core.warning.called).to.be.true; + }); + }); + + it('should still run the template check when the lookup fails', async () => { + failGet(500); + + // Body has a closing reference (so the lookup is attempted) but no template sections. + await run(getPr({ body: 'Closes #1234' })); + + expect(core.setFailed.calledOnce).to.be.true; + const commentBody = github.rest.issues.createComment.args[0][0].body; + expect(commentBody).to.contain(templateMismatchMessage()); + expect(commentBody).to.not.contain(getMessage('missing-linked-issue')); + }); + }); + + it('should look each referenced issue up only once', async () => { + setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + + await run(getPr({ body: withIssueReference('Closes #1234, closes #1234') })); + + expect(github.rest.issues.get.calledOnce).to.be.true; + }); + + it('should not fall back when GitHub already reports a linked issue', async () => { + setLinkedIssues([linkedIssue(1234, ['external-dev'])]); + + await run(getPr({ body: filledTemplate })); + + expect(github.rest.issues.get.called).to.be.false; + expect(core.setFailed.called).to.be.false; + }); }); describe('labelling', () => { From 8088802d9d1f62417b8ddbd9731913b328f6ebdb Mon Sep 17 00:00:00 2001 From: Ken Britton Date: Fri, 7 Aug 2026 08:53:14 -0700 Subject: [PATCH 2/7] fix(#11324): let unexpected lookup failures fail the job Per review on the issue: only 404 and 410 mean the referenced issue is not there. Everything else now throws, so the job goes red with no comment and no label change, matching how the script already treats every other API call, and the next synchronize re-runs it. The previous warn-and-skip behaviour was the one path that could hand a genuinely unlinked PR its "Ready for review" label. --- scripts/ci/andra-bot.js | 36 +++++++--------- .../mocha/unit/testingtests/andra-bot.spec.js | 42 ++++++++++--------- 2 files changed, 37 insertions(+), 41 deletions(-) diff --git a/scripts/ci/andra-bot.js b/scripts/ci/andra-bot.js index f5943d4e156..d427902aac9 100644 --- a/scripts/ci/andra-bot.js +++ b/scripts/ci/andra-bot.js @@ -145,13 +145,15 @@ const toIssueNode = (issue, reference) => { }; }; -// A referenced issue that genuinely does not exist is not a link. Any other failure means we -// could not tell, and blaming the contributor for that is the very false-fail this check is -// meant to avoid — so those are reported separately rather than folded into "not linked". +// A referenced issue that does not exist is not a link, so 404 (and 410, for issues that were +// transferred or deleted) means the reference simply does not count. Anything else is left to +// throw: the job goes red without a comment or a label change, exactly as it already does for +// every other API call here, and the next `synchronize` re-runs it. Swallowing those would be +// the one path that can hand a genuinely unlinked PR its `Ready for review` label. +const MISSING_ISSUE_STATUSES = new Set([404, 410]); + const resolveReferencedIssues = async (github, context, core, references) => { - let inconclusive = false; const issues = await Promise.all(references.map(async (reference) => { - const name = `${reference.owner}/${reference.repo}#${reference.number.toString()}`; try { const { data } = await github.rest.issues.get({ owner: reference.owner, @@ -161,16 +163,15 @@ const resolveReferencedIssues = async (github, context, core, references) => { // A pull request is also an issue on this endpoint, but referencing one is not a link. return data.pull_request ? null : toIssueNode(data, reference); } catch (err) { - if (err.status === 404) { - core.info(`Ignoring referenced issue ${name}: not found.`); - } else { - inconclusive = true; - core.warning(`Could not read referenced issue ${name}: ${err.message}`); + if (!MISSING_ISSUE_STATUSES.has(err.status)) { + throw err; } + const name = `${reference.owner}/${reference.repo}#${reference.number.toString()}`; + core.info(`Ignoring referenced issue ${name}: not found.`); return null; } })); - return { issues: issues.filter(Boolean), inconclusive }; + return issues.filter(Boolean); }; const getLinkedIssues = async (github, context, core) => { @@ -203,14 +204,14 @@ const getLinkedIssues = async (github, context, core) => { const linkedIssues = result.repository.pullRequest.closingIssuesReferences.nodes .filter(issue => isInOrg(issue.repository.owner.login)); if (linkedIssues.length) { - return { issues: linkedIssues, inconclusive: false }; + return linkedIssues; } const references = parseClosingReferences(context.payload.pull_request.body, context) .filter(reference => isInOrg(reference.owner)) .slice(0, MAX_LINKED_ISSUES); if (!references.length) { - return { issues: [], inconclusive: false }; + return []; } return resolveReferencedIssues(github, context, core, references); }; @@ -246,14 +247,7 @@ const getFailures = async (github, context, core) => { failures.push(getMessage('license-changed')); } - const { issues: linkedIssues, inconclusive } = await getLinkedIssues(github, context, core); - if (inconclusive && !linkedIssues.length) { - // Failing the PR here would blame the contributor for a GitHub outage. The warning keeps - // the run visible without stranding a correctly-linked PR behind an infrastructure blip. - core.warning('Could not verify the linked issue, skipping that check.'); - return failures; - } - + const linkedIssues = await getLinkedIssues(github, context, core); const linkedIssueFailure = getLinkedIssueFailure(pr, linkedIssues); if (linkedIssueFailure) { failures.push(linkedIssueFailure); diff --git a/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js b/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js index be3cabc73c7..b504c627e27 100644 --- a/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js +++ b/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js @@ -495,31 +495,33 @@ describe('AndraBot', () => { expect(commentBody).to.contain(getMessage('not-assigned', { issueList: '#1234' })); }); - describe('when the issue cannot be read', () => { - const failGet = (status) => github.rest.issues.get - .rejects(Object.assign(new Error('Server Error'), { status })); + it('should ignore a reference to an issue that was deleted or transferred', async () => { + github.rest.issues.get.rejects(Object.assign(new Error('Gone'), { status: 410 })); - [500, 403].forEach(status => { - it(`should not claim the PR is unlinked after a ${status.toString()}`, async () => { - failGet(status); - - await run(getPr({ body: filledTemplate })); + await run(getPr({ body: filledTemplate })); - expect(core.setFailed.called).to.be.false; - expect(core.warning.called).to.be.true; - }); - }); + expect(core.setFailed.calledOnce).to.be.true; + const commentBody = github.rest.issues.createComment.args[0][0].body; + expect(commentBody).to.contain(getMessage('missing-linked-issue')); + }); - it('should still run the template check when the lookup fails', async () => { - failGet(500); + /* + * Anything other than "the issue is not there" is left to throw, so the job goes red with + * no comment and no label change and the next synchronize re-runs it. Swallowing these is + * the one path that could hand a genuinely unlinked PR its Ready for review label. + */ + describe('when the issue lookup fails for another reason', () => { + [500, 403].forEach(status => { + it(`should propagate a ${status.toString()} rather than treat it as unlinked`, async () => { + const err = Object.assign(new Error('Server Error'), { status }); + github.rest.issues.get.rejects(err); - // Body has a closing reference (so the lookup is attempted) but no template sections. - await run(getPr({ body: 'Closes #1234' })); + await expect(run(getPr({ body: filledTemplate }))).to.be.rejectedWith('Server Error'); - expect(core.setFailed.calledOnce).to.be.true; - const commentBody = github.rest.issues.createComment.args[0][0].body; - expect(commentBody).to.contain(templateMismatchMessage()); - expect(commentBody).to.not.contain(getMessage('missing-linked-issue')); + expect(github.rest.issues.createComment.called).to.be.false; + expect(github.rest.issues.addLabels.called).to.be.false; + expect(github.rest.issues.removeLabel.called).to.be.false; + }); }); }); From 13dcf4e715b5f0ca2d44aa263f450ab22b520988 Mon Sep 17 00:00:00 2001 From: Ken Britton Date: Fri, 7 Aug 2026 10:33:36 -0700 Subject: [PATCH 3/7] refactor(#11324): build the closing-keyword regex with String.raw Escaping every backslash twice made the pattern hard to read against GitHub's documented reference forms. The compiled regex is unchanged (verified byte-identical), so this is readability only. Fixes the SonarCloud javascript:S7780 findings. --- scripts/ci/andra-bot.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/ci/andra-bot.js b/scripts/ci/andra-bot.js index d427902aac9..8cbe3fcebba 100644 --- a/scripts/ci/andra-bot.js +++ b/scripts/ci/andra-bot.js @@ -101,11 +101,11 @@ const MAX_LINKED_ISSUES = 20; // `#123`, `owner/repo#123`, and a full issue URL. // https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue const CLOSING_KEYWORDS = 'close[sd]?|fix(?:e[sd])?|resolve[sd]?'; -const REPO_NAME = '[\\w.-]+'; +const REPO_NAME = String.raw`[\w.-]+`; const CLOSING_REFERENCE_REGEX = new RegExp( - `\\b(?:${CLOSING_KEYWORDS})\\b\\s*:?\\s+` + - `(?:https?://github\\.com/(${REPO_NAME})/(${REPO_NAME})/issues/(\\d+)` + - `|(?:(${REPO_NAME})/(${REPO_NAME}))?#(\\d+))`, + String.raw`\b(?:${CLOSING_KEYWORDS})\b\s*:?\s+` + + String.raw`(?:https?://github\.com/(${REPO_NAME})/(${REPO_NAME})/issues/(\d+)` + + String.raw`|(?:(${REPO_NAME})/(${REPO_NAME}))?#(\d+))`, 'gi' ); From bab0474da46fbf788c80432833e20387fd6f9626 Mon Sep 17 00:00:00 2001 From: Ken Britton Date: Mon, 10 Aug 2026 09:00:34 -0700 Subject: [PATCH 4/7] fix(#11324): don't read closing references out of code or prose links Review findings on #11332. Two that could pass or fail a PR wrongly: - Code spans and fenced blocks were parsed. Because the assignment check uses `.some()`, a PR with no real link passed as soon as its author was assigned to any org issue merely mentioned in prose. Run over this PR's own body the parser found four references where GitHub links one. - The fallback exited on `closingIssuesReferences` being non-empty, but the gate is "linked and assigned". That field is also fed by the Development sidebar, which works on any base branch, so a sidebar-linked epic could hide a contributor's own keyword link behind a failure they could not clear. It now exits only on a link that would actually pass, and the two sources are merged rather than one replacing the other. Smaller ones from the same review: - `\s*:?\s+` was ambiguous and backtracked quadratically, ~4s at GitHub's body limit in a workflow re-run on every edit. `:?[^\S\n]+` is 0.37ms and no longer binds a keyword across a newline. - Issue numbers with leading zeros or beyond safe-integer range are rejected rather than reaching the API as `1234` or `1e+23`. - A repo name of only dots can no longer reach the API path. - References past the limit are logged instead of silently dropped, and the limit is defined once rather than three times. - Resolved issues are re-checked against the org, matching the GraphQL path, so an issue transferred out cannot slip through. - A dropped reference warns rather than infos: 404 also means the token cannot see the repository. --- scripts/ci/andra-bot.js | 94 ++++++++++++++----- .../mocha/unit/testingtests/andra-bot.spec.js | 83 ++++++++++++++++ 2 files changed, 156 insertions(+), 21 deletions(-) diff --git a/scripts/ci/andra-bot.js b/scripts/ci/andra-bot.js index 8cbe3fcebba..78014e6d34c 100644 --- a/scripts/ci/andra-bot.js +++ b/scripts/ci/andra-bot.js @@ -42,6 +42,15 @@ const stripComments = (text) => { return text; }; +// GitHub does not linkify inside code, so neither should the fallback: a body that +// merely documents the syntax must not read as a link. Fences go first so that a +// backtick inside one can't pair with a later one and swallow real prose. +const stripCode = (text) => { + return text + .replace(/^[^\S\n]*(```+|~~~+)[\s\S]*?^[^\S\n]*\1[^\S\n]*$/gm, '') + .replace(/(`+)[^`]*?\1/g, ''); +}; + const HEADING_PREFIX = '# '; const HEADING_REGEX = new RegExp(`^${HEADING_PREFIX}.+$`, 'gm'); @@ -101,9 +110,15 @@ const MAX_LINKED_ISSUES = 20; // `#123`, `owner/repo#123`, and a full issue URL. // https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue const CLOSING_KEYWORDS = 'close[sd]?|fix(?:e[sd])?|resolve[sd]?'; -const REPO_NAME = String.raw`[\w.-]+`; +// At least one character that isn't a dot, so `..` can't reach the API as a path segment. +const REPO_NAME = String.raw`[\w.-]*[\w-][\w.-]*`; +// `:?[^\S\n]+` rather than `\s*:?\s+`: the latter is ambiguous between its two +// whitespace quantifiers and backtracks quadratically on a long run of spaces +// (~4s at GitHub's 65536 body limit, re-run on every `edited` event). Excluding +// newlines also stops a keyword ending one line binding to a `#N` on the next, +// which GitHub does not link. const CLOSING_REFERENCE_REGEX = new RegExp( - String.raw`\b(?:${CLOSING_KEYWORDS})\b\s*:?\s+` + + String.raw`\b(?:${CLOSING_KEYWORDS})\b:?[^\S\n]+` + String.raw`(?:https?://github\.com/(${REPO_NAME})/(${REPO_NAME})/issues/(\d+)` + String.raw`|(?:(${REPO_NAME})/(${REPO_NAME}))?#(\d+))`, 'gi' @@ -113,12 +128,18 @@ const CLOSING_REFERENCE_REGEX = new RegExp( // repository's default branch; on any other base the field is empty even though the // contributor linked the issue correctly. Parsing the body covers that case. const parseClosingReferences = (body, context) => { - const matches = stripComments(body || '').matchAll(CLOSING_REFERENCE_REGEX); - const references = [...matches].map(([, urlOwner, urlRepo, urlNumber, owner, repo, number]) => ({ - owner: urlOwner || owner || context.repo.owner, - repo: urlRepo || repo || context.repo.repo, - number: Number(urlNumber || number), - })); + const matches = stripCode(stripComments(body || '')).matchAll(CLOSING_REFERENCE_REGEX); + const references = [...matches] + .map(([, urlOwner, urlRepo, urlNumber, owner, repo, number]) => ({ + owner: urlOwner || owner || context.repo.owner, + repo: urlRepo || repo || context.repo.repo, + // Leading zeros are not autolinked by GitHub, and a digit run long enough to + // lose precision would reach the API as exponential notation. + number: /^[1-9]\d*$/.test(urlNumber || number) + ? Number(urlNumber || number) + : Number.NaN, + })) + .filter(reference => Number.isSafeInteger(reference.number)); // Owner and repo names are case-insensitive on GitHub, so the key is too. const seen = new Set(); @@ -166,27 +187,34 @@ const resolveReferencedIssues = async (github, context, core, references) => { if (!MISSING_ISSUE_STATUSES.has(err.status)) { throw err; } + // 404 also covers "the token cannot see this repository", so a reference to a + // private in-org repo lands here. Warn rather than info so a dropped reference is + // visible in the job summary instead of only deep in the log. const name = `${reference.owner}/${reference.repo}#${reference.number.toString()}`; - core.info(`Ignoring referenced issue ${name}: not found.`); + core.warning(`Ignoring referenced issue ${name}: not found or not visible.`); return null; } })); return issues.filter(Boolean); }; +const isAssignedTo = (issue, login) => { + return issue.assignees.nodes.some(assignee => assignee.login === login); +}; + const getLinkedIssues = async (github, context, core) => { const query = ` - query ($owner: String!, $repo: String!, $number: Int!) { + query ($owner: String!, $repo: String!, $number: Int!, $limit: Int!) { repository(owner: $owner, name: $repo) { pullRequest(number: $number) { - closingIssuesReferences(first: 20) { + closingIssuesReferences(first: $limit) { nodes { number repository { nameWithOwner owner { login } } - assignees(first: 20) { + assignees(first: $limit) { nodes { login } } } @@ -198,38 +226,62 @@ const getLinkedIssues = async (github, context, core) => { owner: context.repo.owner, repo: context.repo.repo, number: context.payload.pull_request.number, + limit: MAX_LINKED_ISSUES, }); // Issues linked from repositories outside the org don't count as linked. const isInOrg = owner => owner.toLowerCase() === context.repo.owner.toLowerCase(); + const author = context.payload.pull_request.user.login; const linkedIssues = result.repository.pullRequest.closingIssuesReferences.nodes .filter(issue => isInOrg(issue.repository.owner.login)); - if (linkedIssues.length) { + + // Only short-circuit on a link that would actually pass. closingIssuesReferences is + // fed by both closing keywords and the Development sidebar, and the sidebar works on + // any base branch — so a sidebar-linked epic can fill this while the contributor's own + // keyword link goes unread, leaving them a failure they cannot clear from the PR. + if (linkedIssues.some(issue => isAssignedTo(issue, author))) { return linkedIssues; } - const references = parseClosingReferences(context.payload.pull_request.body, context) - .filter(reference => isInOrg(reference.owner)) - .slice(0, MAX_LINKED_ISSUES); + const parsed = parseClosingReferences(context.payload.pull_request.body, context) + .filter(reference => isInOrg(reference.owner)); + if (parsed.length > MAX_LINKED_ISSUES) { + core.warning( + `Only the first ${MAX_LINKED_ISSUES.toString()} referenced issues are checked; ` + + `${(parsed.length - MAX_LINKED_ISSUES).toString()} later reference(s) were ignored.` + ); + } + const references = parsed.slice(0, MAX_LINKED_ISSUES); if (!references.length) { - return []; + return linkedIssues; } - return resolveReferencedIssues(github, context, core, references); + + const resolved = await resolveReferencedIssues(github, context, core, references); + // Re-filter: an issue transferred out of the org comes back under its new owner. + return [...linkedIssues, ...resolved.filter(issue => isInOrg(issue.repository.owner.login))]; }; const getLinkedIssueFailure = (pr, linkedIssues) => { if (!linkedIssues.length) { return getMessage('missing-linked-issue'); } - const isAssigned = linkedIssues - .some(issue => issue.assignees.nodes.some(assignee => assignee.login === pr.user.login)); - if (isAssigned) { + if (linkedIssues.some(issue => isAssignedTo(issue, pr.user.login))) { return null; } + // The GraphQL and parsed sets can name the same issue; list it once. + const seen = new Set(); const issueList = linkedIssues .map(issue => issue.repository.nameWithOwner === pr.base.repo.full_name ? `#${issue.number}` : `${issue.repository.nameWithOwner}#${issue.number}`) + .filter(name => { + const key = name.toLowerCase(); + if (seen.has(key)) { + return false; + } + seen.add(key); + return true; + }) .join(', '); return getMessage('not-assigned', { issueList }); }; diff --git a/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js b/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js index b504c627e27..91073462e82 100644 --- a/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js +++ b/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js @@ -353,6 +353,7 @@ describe('AndraBot', () => { owner: 'medic', repo: 'cht-core', number: 42, + limit: 20, }); }); @@ -469,6 +470,7 @@ describe('AndraBot', () => { it('should ignore a reference to an issue that does not exist', async () => { await run(getPr({ body: filledTemplate })); + expect(github.rest.issues.get.called).to.be.true; expect(core.setFailed.calledOnce).to.be.true; const commentBody = github.rest.issues.createComment.args[0][0].body; expect(commentBody).to.contain(getMessage('missing-linked-issue')); @@ -500,6 +502,7 @@ describe('AndraBot', () => { await run(getPr({ body: filledTemplate })); + expect(github.rest.issues.get.called).to.be.true; expect(core.setFailed.calledOnce).to.be.true; const commentBody = github.rest.issues.createComment.args[0][0].body; expect(commentBody).to.contain(getMessage('missing-linked-issue')); @@ -800,4 +803,84 @@ describe('AndraBot', () => { expect(github.rest.issues.removeLabel.args[0][0].name).to.equal(FAILURE_LABEL); }); }); + + describe('review follow-ups on #11332', () => { + it('does not treat a reference inside a code span as a link', async () => { + setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + + await run(getPr({ body: withIssueReference('Write `Closes #1234` in the description.') })); + + expect(github.rest.issues.get.called).to.be.false; + expect(core.setFailed.calledOnce).to.be.true; + const commentBody = github.rest.issues.createComment.args[0][0].body; + expect(commentBody).to.contain(getMessage('missing-linked-issue')); + }); + + it('does not treat a reference inside a fenced block as a link', async () => { + setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + const body = withIssueReference('Example:\n\n```\nCloses #1234\n```'); + + await run(getPr({ body })); + + expect(github.rest.issues.get.called).to.be.false; + expect(core.setFailed.calledOnce).to.be.true; + }); + + it('reads the body when the linked issue is assigned to someone else', async () => { + // A sidebar-linked epic fills closingIssuesReferences on any base branch. Short- + // circuiting on it would hide the contributor's own keyword link behind a failure + // they cannot clear. + setLinkedIssues([linkedIssue(999, ['someone-else'])]); + setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + + await run(getPr({ body: filledTemplate })); + + expect(github.rest.issues.get.called).to.be.true; + expect(core.setFailed.called).to.be.false; + }); + + it('lists an issue once when both sources name it', async () => { + setLinkedIssues([linkedIssue(1234, ['someone-else'])]); + setReferencedIssue({ number: 1234, assignees: ['someone-else'] }); + + await run(getPr({ body: filledTemplate })); + + const commentBody = github.rest.issues.createComment.args[0][0].body; + expect(commentBody).to.contain(getMessage('not-assigned', { issueList: '#1234' })); + }); + + it('does not bind a keyword across a newline', async () => { + await run(getPr({ body: withIssueReference('Closes\n#1234') })); + + expect(github.rest.issues.get.called).to.be.false; + expect(core.setFailed.calledOnce).to.be.true; + }); + + ['Closes #01234', 'Closes #99999999999999999999999'].forEach(reference => { + it(`ignores the malformed number in "${reference}"`, async () => { + await run(getPr({ body: withIssueReference(reference) })); + + expect(github.rest.issues.get.called).to.be.false; + expect(core.setFailed.calledOnce).to.be.true; + }); + }); + + it('ignores a repo name made only of dots', async () => { + await run(getPr({ body: withIssueReference('Closes medic/..#1234') })); + + expect(github.rest.issues.get.called).to.be.false; + expect(core.setFailed.calledOnce).to.be.true; + }); + + it('warns rather than silently dropping references past the limit', async () => { + const refs = Array.from({ length: 22 }, (_, i) => `Closes #${(i + 1).toString()}`).join(' '); + github.rest.issues.get.rejects(Object.assign(new Error('Not Found'), { status: 404 })); + + await run(getPr({ body: withIssueReference(refs) })); + + expect(github.rest.issues.get.callCount).to.equal(20); + expect(core.warning.args.some(([msg]) => msg.includes('2 later reference(s) were ignored'))) + .to.be.true; + }); + }); }); From bd382eb5988290106e4d3318195f63dd757e5410 Mon Sep 17 00:00:00 2001 From: Ken Britton Date: Mon, 10 Aug 2026 09:22:27 -0700 Subject: [PATCH 5/7] fix(#11324): rewrite the code stripper as a line scanner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The regex version I pushed had three bugs, two of which reintroduced the failure classes it was added to prevent: - One unbalanced backtick paired with the next one anywhere below it, including the PR template's own, deleting a real `Closes #1234` in between and failing a correctly linked PR. - A fence with no matching close, or a `~~~` block closed with backticks, was parsed as prose. GitHub renders both as code and links nothing, so an unlinked PR could pass. - Removing a region spliced the prose either side together, so "does not fix `anything` #1234" read as a reference. Every rule here is a line-level rule, so a line scanner states each one directly instead of encoding it in a backreference. Removals leave a newline, which the closing-reference regex will not cross — that also makes a single pass over comments sufficient. Fences do not nest, so one open marker is tracked rather than a stack. --- scripts/ci/andra-bot.js | 45 +++++++++++++++--- .../mocha/unit/testingtests/andra-bot.spec.js | 46 ++++++++++++++++++- 2 files changed, 83 insertions(+), 8 deletions(-) diff --git a/scripts/ci/andra-bot.js b/scripts/ci/andra-bot.js index 78014e6d34c..9df8e42d0e6 100644 --- a/scripts/ci/andra-bot.js +++ b/scripts/ci/andra-bot.js @@ -42,13 +42,44 @@ const stripComments = (text) => { return text; }; -// GitHub does not linkify inside code, so neither should the fallback: a body that -// merely documents the syntax must not read as a link. Fences go first so that a -// backtick inside one can't pair with a later one and swallow real prose. -const stripCode = (text) => { +const FENCE_REGEX = /^\s*(```+|~~~+)/; +const CODE_SPAN_REGEX = /(`+)[^`\n]*?\1/g; + +// GitHub does not linkify inside comments or code, so neither should the fallback: a body +// that merely documents the syntax must not read as a link. Handled line by line, because +// every subtlety here is a line-level rule — an unclosed fence runs to the end of the +// document, a `~~~` block is not closed by a ``` one, and a code span cannot span lines. +// +// Every removal leaves a newline behind. Deleting outright splices the prose either side +// together, so `does not fix ` + `#1234` reads as a reference; a space doesn't help since +// the closing-reference regex spans those. A newline is the one separator it won't cross. +// +// One pass over comments is enough here, unlike stripComments above: that one loops +// because deleting can splice a new marker out of the remains (`- y -->`), +// and the newline left behind is what stops that. +const stripNonProse = (text) => { + let openFence = null; + // Fences do not nest — the first matching close ends the block, so this tracks one open + // marker rather than a stack. A stack would need two closes to exit an inner marker and + // would swallow every real reference after the block. return text - .replace(/^[^\S\n]*(```+|~~~+)[\s\S]*?^[^\S\n]*\1[^\S\n]*$/gm, '') - .replace(/(`+)[^`]*?\1/g, ''); + .replace(//g, '\n') + .split('\n') + .map(line => { + const fence = FENCE_REGEX.exec(line)?.[1]; + if (openFence) { + if (fence?.startsWith(openFence)) { + openFence = null; + } + return ''; + } + if (fence) { + openFence = fence; + return ''; + } + return line.replace(CODE_SPAN_REGEX, '\n'); + }) + .join('\n'); }; const HEADING_PREFIX = '# '; @@ -128,7 +159,7 @@ const CLOSING_REFERENCE_REGEX = new RegExp( // repository's default branch; on any other base the field is empty even though the // contributor linked the issue correctly. Parsing the body covers that case. const parseClosingReferences = (body, context) => { - const matches = stripCode(stripComments(body || '')).matchAll(CLOSING_REFERENCE_REGEX); + const matches = stripNonProse(body || '').matchAll(CLOSING_REFERENCE_REGEX); const references = [...matches] .map(([, urlOwner, urlRepo, urlNumber, owner, repo, number]) => ({ owner: urlOwner || owner || context.repo.owner, diff --git a/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js b/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js index 91073462e82..a4b99ea0b86 100644 --- a/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js +++ b/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js @@ -343,7 +343,6 @@ describe('AndraBot', () => { expect(commentBody).to.not.contain(templateMismatchMessage()); }); - it('should query the PR from the event payload', async () => { await run(getPr({ body: filledTemplate })); @@ -872,6 +871,51 @@ describe('AndraBot', () => { expect(core.setFailed.calledOnce).to.be.true; }); + it('is not fooled by an unclosed fence', async () => { + setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + + // Raw body, not the filled template: the template's own backticks can pair with the + // fence marker and strip the reference by accident, which would make this pass for + // the wrong reason. + await run(getPr({ body: 'How to link:\n\n```\nCloses #1234' })); + + expect(github.rest.issues.get.called).to.be.false; + expect(core.setFailed.calledOnce).to.be.true; + }); + + it('is not fooled by a ~~~ block closed with backticks', async () => { + setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + + await run(getPr({ body: '~~~\nCloses #1234\n```' })); + + expect(github.rest.issues.get.called).to.be.false; + expect(core.setFailed.calledOnce).to.be.true; + }); + + it('does not splice prose either side of a removed span into a reference', async () => { + setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + + await run(getPr({ body: 'This does not fix `anything` #1234 related.' })); + + expect(github.rest.issues.get.called).to.be.false; + expect(core.setFailed.calledOnce).to.be.true; + }); + + it('still finds a real link when an unbalanced backtick appears above it', async () => { + // One stray backtick used to pair with the next one anywhere below and delete the + // reference in between — a false failure the contributor could not clear. + setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + const body = filledTemplate.replace( + 'Fixes the date conversion by using the local format.', + 'Switch to `Intl.DateTimeFormat for parsing.' + ); + + await run(getPr({ body })); + + expect(github.rest.issues.get.called).to.be.true; + expect(core.setFailed.called).to.be.false; + }); + it('warns rather than silently dropping references past the limit', async () => { const refs = Array.from({ length: 22 }, (_, i) => `Closes #${(i + 1).toString()}`).join(' '); github.rest.issues.get.rejects(Object.assign(new Error('Not Found'), { status: 404 })); From b82b28e772745864a2b2922e9565dadb58858455 Mon Sep 17 00:00:00 2001 From: Ken Britton Date: Mon, 10 Aug 2026 10:03:16 -0700 Subject: [PATCH 6/7] fix(#11324): bound the code stripper to fences that close Review found four ways the line scanner discarded a genuine issue link, all of them false negatives that fail a contributor with no way to clear the failure. Three shared a root cause: an unmatched fence reached to the end of the body, so one misread line hid every reference below it. Only a fence that actually closes now delimits a block, which leaves an unpaired marker stripping nothing and replaces the scanner with three bounded replacements. That removes the need to encode CommonMark's rules for info strings, indented fences and closing markers, since each could only ever cause this to strip more. Order matters: a `` and delete the link between them. parseSections masks blocks for the same reason. Restore the linear repo-name pattern. Requiring a non-dot character made its quantifiers ambiguous and the failed match cubic: 68s on a 8k body and hours at GitHub's 65536-character limit, on a pull_request_target body re-parsed on every edit. `..` is rejected after parsing instead. Two tests asserted the old unbounded behaviour and now assert the new contract. Assertions moved from `issues.get.called` to the outcome the contributor sees. A test pinning the fence indent bound was dropped as vacuous: the pair requirement already subsumes it. Co-Authored-By: Claude Opus 5 --- scripts/ci/andra-bot.js | 83 +++++++++++-------- .../mocha/unit/testingtests/andra-bot.spec.js | 43 +++++++--- 2 files changed, 78 insertions(+), 48 deletions(-) diff --git a/scripts/ci/andra-bot.js b/scripts/ci/andra-bot.js index 9df8e42d0e6..e3f02c902d3 100644 --- a/scripts/ci/andra-bot.js +++ b/scripts/ci/andra-bot.js @@ -42,51 +42,55 @@ const stripComments = (text) => { return text; }; -const FENCE_REGEX = /^\s*(```+|~~~+)/; +// Only a fence that is actually closed delimits a block. An opener with no matching close +// matches nothing and so strips nothing, which is what keeps this bounded: the alternative — +// running an unclosed fence to the end of the document, as a renderer does — means one +// misread line silently discards every reference below it, and the contributor cannot tell +// why the link they wrote is not being seen. The failure directions are not equal. Reading +// code as prose lets a PR that only *documents* a link past a check that still requires the +// author be assigned to that issue; reading prose as code blocks a real contributor with no +// way to clear it. Everything ambiguous here is therefore resolved toward stripping less. +// +// That asymmetry is also why no CommonMark subtleties are encoded: an info string with a +// backtick (``` ```sh make test``` ```, an inline span, not a block), a four-space indent +// making a fence literal, a close carrying its own info string. Each would only ever cause +// this to strip *more*, and unpaired markers already strip nothing. +const FENCED_BLOCK_REGEX = /^ {0,3}(```+|~~~+)[^\n]*\n[\s\S]*?^ {0,3}\1[^\n]*$/gm; const CODE_SPAN_REGEX = /(`+)[^`\n]*?\1/g; +const COMMENT_REGEX = //g; +// Carries no comment marker and starts no heading, but is not empty. +const CODE_BLOCK_PLACEHOLDER = '\ncode\n'; // GitHub does not linkify inside comments or code, so neither should the fallback: a body -// that merely documents the syntax must not read as a link. Handled line by line, because -// every subtlety here is a line-level rule — an unclosed fence runs to the end of the -// document, a `~~~` block is not closed by a ``` one, and a code span cannot span lines. +// that merely documents the syntax must not read as a link. +// +// The order is load-bearing. A `` below +// it — in practice the PR template's own — and delete the issue link in between. An XML or +// HTML sample in a cht-core PR body makes that an ordinary body, not a contrived one. // // Every removal leaves a newline behind. Deleting outright splices the prose either side // together, so `does not fix ` + `#1234` reads as a reference; a space doesn't help since // the closing-reference regex spans those. A newline is the one separator it won't cross. // -// One pass over comments is enough here, unlike stripComments above: that one loops -// because deleting can splice a new marker out of the remains (`- y -->`), -// and the newline left behind is what stops that. -const stripNonProse = (text) => { - let openFence = null; - // Fences do not nest — the first matching close ends the block, so this tracks one open - // marker rather than a stack. A stack would need two closes to exit an inner marker and - // would swallow every real reference after the block. - return text - .replace(//g, '\n') - .split('\n') - .map(line => { - const fence = FENCE_REGEX.exec(line)?.[1]; - if (openFence) { - if (fence?.startsWith(openFence)) { - openFence = null; - } - return ''; - } - if (fence) { - openFence = fence; - return ''; - } - return line.replace(CODE_SPAN_REGEX, '\n'); - }) - .join('\n'); -}; +// One pass over comments is enough here, unlike stripComments above: that one loops because +// deleting can splice a new marker out of the remains (`- y -->`), and the +// newline left behind is what stops that. +const stripNonProse = (text) => text + .replace(FENCED_BLOCK_REGEX, '\n') + .replace(CODE_SPAN_REGEX, '\n') + .replace(COMMENT_REGEX, '\n'); const HEADING_PREFIX = '# '; const HEADING_REGEX = new RegExp(`^${HEADING_PREFIX}.+$`, 'gm'); const parseSections = (text) => { - text = stripComments(text); + // A fenced block's content is literal, so neither the `` and swallows a heading, and the second reads as one. Reducing each block to a + // placeholder keeps both out of the scan while still counting as content, so a section + // filled only with a code sample is not then judged empty. + text = stripComments(text.replace(FENCED_BLOCK_REGEX, CODE_BLOCK_PLACEHOLDER)); const headings = [...text.matchAll(HEADING_REGEX)]; return headings.map((match, index) => ({ heading: match[0].trim().replace(HEADING_PREFIX, ''), @@ -141,8 +145,12 @@ const MAX_LINKED_ISSUES = 20; // `#123`, `owner/repo#123`, and a full issue URL. // https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue const CLOSING_KEYWORDS = 'close[sd]?|fix(?:e[sd])?|resolve[sd]?'; -// At least one character that isn't a dot, so `..` can't reach the API as a path segment. -const REPO_NAME = String.raw`[\w.-]*[\w-][\w.-]*`; +const PATH_SEGMENT_REGEX = /^\.+$/; +// Kept unambiguous. Splitting this to require a non-dot character (`[\w.-]*[\w-][\w.-]*`) +// makes the two quantifiers able to divide a word run between them; with two names either +// side of a `/` the failure path is cubic, and a 65KB body — the API's own limit — takes +// hours rather than the 29ms this does. `..` is rejected after parsing instead. +const REPO_NAME = String.raw`[\w.-]+`; // `:?[^\S\n]+` rather than `\s*:?\s+`: the latter is ambiguous between its two // whitespace quantifiers and backtracks quadratically on a long run of spaces // (~4s at GitHub's 65536 body limit, re-run on every `edited` event). Excluding @@ -170,7 +178,12 @@ const parseClosingReferences = (body, context) => { ? Number(urlNumber || number) : Number.NaN, })) - .filter(reference => Number.isSafeInteger(reference.number)); + .filter(reference => Number.isSafeInteger(reference.number)) + // `.` and `..` are legal in the character class but are path segments, not names, so + // they would traverse in the request URL rather than 404. Nothing else needs excluding: + // any other name is merely one that does not exist. + .filter(reference => !PATH_SEGMENT_REGEX.test(reference.owner) && + !PATH_SEGMENT_REGEX.test(reference.repo)); // Owner and repo names are case-insensitive on GitHub, so the key is too. const seen = new Set(); diff --git a/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js b/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js index a4b99ea0b86..cad6f9e1472 100644 --- a/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js +++ b/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js @@ -871,25 +871,42 @@ describe('AndraBot', () => { expect(core.setFailed.calledOnce).to.be.true; }); - it('is not fooled by an unclosed fence', async () => { - setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + // Each of these is a body whose issue link is genuine but which an over-eager code + // stripper discarded, failing a contributor with no way to clear it — the link they are + // told to add is the link already there. Only a fence that is actually closed delimits a + // block, so an unpaired marker cannot reach past itself to swallow any of them. + [ + ['an unbalanced /g; // Carries no comment marker and starts no heading, but is not empty. diff --git a/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js b/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js index cad6f9e1472..70746979793 100644 --- a/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js +++ b/webapp/tests/mocha/unit/testingtests/andra-bot.spec.js @@ -891,6 +891,19 @@ describe('AndraBot', () => { }); }); + it('still passes a PR whose link sits between markers of unequal length', async () => { + setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); + // A closing fence is at least as long as its opener, so these two do not pair and the + // line between them is ordinary prose. Letting the opening run be retried shorter finds + // a pair anyway and deletes the link — and costs a rescan of the line per candidate + // length, which is what makes a body of backticks quadratic. + + await run(getPr({ body: withIssueReference('`````\nCloses #1234\n```') })); + + expect(core.setFailed.called).to.be.false; + expect(github.rest.issues.addLabels.args[0][0].labels).to.deep.equal([SUCCESS_LABEL]); + }); + it('still passes a PR whose body reaches GitHub\'s size limit', async () => { setReferencedIssue({ number: 1234, assignees: ['external-dev'] }); // Two 32k path segments either side of a `/`, and deliberately *no* trailing `#number`: