Skip to content

fix(#11324): recognise closing keywords on non-default-branch PRs #11332

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ken-talltree-io wants to merge 3 commits into
base: master
Choose a base branch
from
+299 −8
Open

fix(#11324): recognise closing keywords on non-default-branch PRs #11332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e7a5db
fix(#11324): recognise closing keywords on non-default-branch PRs
ken-talltree-io Aug 6, 2026
8088802
fix(#11324): let unexpected lookup failures fail the job
ken-talltree-io Aug 7, 2026
13dcf4e
refactor(#11324): build the closing-keyword regex with String.raw
ken-talltree-io Aug 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 97 additions & 7 deletions scripts/ci/andra-bot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,86 @@ 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 = String.raw`[\w.-]+`;

@sugat009 sugat009 Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question (if-minor): should REPO_NAME accept . and ..?

[\w.-]+ matches .., so Closes medic/..#1 parses as owner=medic repo=.. num=1 (verified) and passes isInOrg, since the owner really is medic. It then reaches issues.get with repo: '..'.

I haven't checked what the client does with that path, so I'm not claiming an impact beyond a stray 404. It's contributor-controlled text going into an API path in a privileged workflow, so requiring at least one non-dot character seems worth the two characters it costs.

const CLOSING_REFERENCE_REGEX = new RegExp(
String.raw`\b(?:${CLOSING_KEYWORDS})\b\s*:?\s+` +

@sugat009 sugat009 Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

issue (non-blocking): \s*:?\s+ backtracks quadratically

Both quantifiers match whitespace, so the alternation is ambiguous. Measured against this exact regex, with closes followed by N spaces and a non-matching character:

N=2000   ->     4.2 ms
N=20000  ->   460 ms
N=65000  ->  4763 ms

GitHub's body limit is 65536, this runs under pull_request_target, and every edited event re-triggers it. Dropping the leading \s* gives 0.27 ms at N=65000 and still accepts both Closes #1 and Closes: #1 (verified both).

Same line, much smaller: \s+ matches newlines, so a keyword ending one line binds to a #N starting the next, which GitHub doesn't link. Zero occurrences in 4711 real cht-core PR bodies, so purely over-matching. [^\S\n]+ if you're touching the line anyway.

String.raw`(?:https?://github\.com/(${REPO_NAME})/(${REPO_NAME})/issues/(\d+)` +
String.raw`|(?:(${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);

@sugat009 sugat009 Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

issue (blocking): code spans aren't excluded, so the gate can pass an unlinked PR

stripComments removes HTML comments but nothing else. I ran this regex over this PR's own body and it returns four references rather than one:

"Closes: #10"
"Closes Medic/cht-android#99"
"Closes #1234"
"Closes #11324"

The first three are prose inside backticks. GitHub links none of them (checked against POST /markdown with mode: gfm).

Since getLinkedIssueFailure uses .some(), a PR with no real link passes as soon as its author is assigned to any org issue that happens to be mentioned in prose. That's the inverse of #11324: the bot green-lights something it should hold. Extending stripComments at :36 to drop fenced blocks and inline code spans covers it.

Side note: blockquotes are stripped, but GitHub does linkify inside them. Ignoring quoted text seems deliberate and it's defensible, it just isn't stated anywhere.

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),

@sugat009 sugat009 Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

issue (non-blocking): Number() on the raw digit run

Two verified behaviours: Number('01234') is 1234, so Closes #01234 resolves to #1234 even though GitHub doesn't autolink a leading-zero reference. And Number('9'.repeat(23)) stringifies to the literal '1e+23', which goes straight into the request path as /issues/1e+23.

What I did not verify is what GitHub returns for a non-integer path segment. If it's anything other than 404/410 the status isn't in MISSING_ISSUE_STATUSES, the error escapes getFailures, and the job goes red with no comment and no label change. Worth a Number.isSafeInteger guard regardless, since it's cheap.

}));

// 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 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) => {
const issues = await Promise.all(references.map(async (reference) => {
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 (!MISSING_ISSUE_STATUSES.has(err.status)) {

@sugat009 sugat009 Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question (non-blocking): does 404 always mean "doesn't exist"?

GitHub also returns 404 when the token can't see the repository, and GITHUB_TOKEN only has public-level access outside this repo. So Closes medic/<private-repo>#42 would be dropped with a core.info line and the PR reported unlinked, which is the false-fail class this PR exists to remove, with the reason visible only in the job log.

I didn't test this, since it needs a private in-org repo. If it holds, surfacing dropped references in the comment rather than only the log would make it diagnosable.

throw err;
}
const name = `${reference.owner}/${reference.repo}#${reference.number.toString()}`;
core.info(`Ignoring referenced issue ${name}: not found.`);
return null;
}
}));
return issues.filter(Boolean);
};

const getLinkedIssues = async (github, context, core) => {
const query = `
query ($owner: String!, $repo: String!, $number: Int!) {
repository(owner: $owner, name: $repo) {
Expand All @@ -121,9 +200,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) {

@sugat009 sugat009 Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

issue (blocking): the fallback exits one condition too early

This short-circuits on closingIssuesReferences being non-empty, but the gate is "linked and assigned to the author". A non-empty result isn't necessarily a passing one, so the body gets discarded in the case where it still matters.

The field has two independent feeds: closing keywords, which don't register on non-default branches, and the Development sidebar, which works on any branch. So on a non-default-branch PR a sidebar-linked epic makes this non-empty while the contributor's Closes #B is never read, and they get "You are not assigned to the linked issue (#epic)" with no edit that clears it. Same unfixable shape as #11324, relocated.

Being straight about evidence: this is a mechanism, not something I've seen happen.

Switching the condition costs nothing in the common case:

// today
if (linkedIssues.length) {
  return linkedIssues;
}

// instead
if (linkedIssues.some(isAssignedToAuthor)) {
  return linkedIssues;      // happy path: body never parsed, no extra lookups
}

The body is then read only when the bot is about to fail the PR anyway. Needs pr.user.login threaded in, since the assignment check currently lives in getLinkedIssueFailure.

return linkedIssues;
}

const references = parseClosingReferences(context.payload.pull_request.body, context)
.filter(reference => isInOrg(reference.owner))

@sugat009 sugat009 Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question (non-blocking): should the org gate run before resolution?

This filters on the owner string the contributor typed, whereas the GraphQL path at :204 filters after resolution. Since toIssueNode takes nameWithOwner from repository_url, an issue that has moved out of the org would come back under its new owner and never be re-filtered.

I couldn't test this: it needs a repo that has actually transferred out of medic. The staging difference between the two paths is real in the code, the exploitable consequence is my inference. Re-applying isInOrg to the resolved node would make the paths agree either way.

.slice(0, MAX_LINKED_ISSUES);

@sugat009 sugat009 Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

issue (non-blocking): truncation is silent and untested

References past the 20th are dropped with no log line. If the issue the author is assigned to sits 21st in document order they get "You are not assigned to the linked issue" listing 20 issues they aren't assigned to, and only reordering the body clears it.

Nothing exercises this either, so the .slice could be removed or moved before the org filter without a test failing. Also MAX_LINKED_ISSUES at :98 and the two hardcoded first: 20 values in the query are the same limit written three times.

if (!references.length) {
return [];
}
return resolveReferencedIssues(github, context, core, references);
};

const getLinkedIssueFailure = (pr, linkedIssues) => {
Expand All @@ -144,7 +234,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 = [];

Expand All @@ -157,7 +247,7 @@ const getFailures = async (github, context) => {
failures.push(getMessage('license-changed'));
}

const linkedIssues = await getLinkedIssues(github, context);
const linkedIssues = await getLinkedIssues(github, context, core);
const linkedIssueFailure = getLinkedIssueFailure(pr, linkedIssues);
if (linkedIssueFailure) {
failures.push(linkedIssueFailure);
Expand Down Expand Up @@ -278,7 +368,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) {
Expand Down
203 changes: 202 additions & 1 deletion webapp/tests/mocha/unit/testingtests/andra-bot.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@ describe('AndraBot', () => {
.replace('<!-- DESCRIPTION -->', 'Fixes the date conversion by using the local format.')
.replace('<!-- ISSUE NUMBER -->', '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('<!-- DESCRIPTION -->', 'Fixes the date conversion by using the local format.')
.replace('<!-- ISSUE NUMBER -->', '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] } },
Expand All @@ -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);
Expand All @@ -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 })),
},
},
};
Expand Down Expand Up @@ -305,14 +335,15 @@ 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;
expect(commentBody).to.contain(getMessage('missing-linked-issue'));
expect(commentBody).to.not.contain(templateMismatchMessage());
});


it('should query the PR from the event payload', async () => {
await run(getPr({ body: filledTemplate }));

Expand All @@ -334,12 +365,182 @@ 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('<!-- Closes #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'));
});

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 () => {

@sugat009 sugat009 Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

issue (non-blocking): this test and the 410 one pass vacuously

Both rely on the beforeEach stub rejecting and assert only that the comment contains missing-linked-issue, never that a lookup was attempted.

I checked by mutation: at PR head the suite is 67 passing; making parseClosingReferences return [] so body parsing does nothing turns 14 tests red, but these two stay green:

✔ should ignore a reference to an issue that does not exist
✔ should ignore a reference to an issue that was deleted or transferred

That's the same class as the issue_number: NaN regression the description says an earlier draft hit. expect(github.rest.issues.get.called).to.be.true; fixes both, the way the outside-the-org test at :436 already does in the negative direction.

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' }));
});

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 }));

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'));
});

/*
* 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);

await expect(run(getPr({ body: filledTemplate }))).to.be.rejectedWith('Server Error');

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;
});
});
});

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', () => {
Expand Down