Skip to content

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

Open
ken-talltree-io wants to merge 3 commits into
medic:masterfrom
ken-talltree-io:11324-andra-bot-linked-issue-fallback
Open

fix(#11324): recognise closing keywords on non-default-branch PRs#11332
ken-talltree-io wants to merge 3 commits into
medic:masterfrom
ken-talltree-io:11324-andra-bot-linked-issue-fallback

Conversation

@ken-talltree-io

Copy link
Copy Markdown

Description

The linked-issue check reads GitHub's GraphQL closingIssuesReferences, which GitHub only populates for PRs targeting the repository's default branch. On any other base the field is empty even when the contributor linked the issue correctly — so AndraBot failed the PR and asked them to add a closing keyword they had already added, with no way to clear it. That's what happened to @megha1807 on #11320.

When GitHub reports no linkage, getLinkedIssues now falls back to parsing the PR body for closing keywords and resolving each reference through the issues API. The results are shaped like closingIssuesReferences nodes, so the existing same-org filter and the assignee check work on them unchanged.

Recognised: #123, owner/repo#123, and full issue URLs, with GitHub's documented keyword set (close/closes/closed, fix/fixes/fixed, resolve/resolves/resolved).

Details worth a look

Prior art. nearform-actions/github-action-check-linked-issues (MIT) tackles the same GitHub limitation, which was useful confirmation the fallback is the right shape. @sugat009 read its source on the issue and found three reasons not to adopt it that are stronger than my own: its loose-matching is a mode switch rather than a fallback, so it ignores closingIssuesReferences entirely and a sidebar-linked PR reads as unlinked; it cannot do the assignee half of this check, so we would still fetch every issue ourselves on top of the GraphQL call; and its per-lookup bare catch turns a rate limit or 5xx into "not a valid issue", which is the bug this PR is fixing. Two things were worth taking from it: the optional colon in the keyword regex (Closes: #10 links on GitHub, so the fallback must accept it), and a no-issue skip label as a possible future escape hatch — out of scope here.

Case-insensitivity. GitHub owner and repo names are case-insensitive, so Closes Medic/cht-android#99 is a valid link. The org filter, the dedupe key and the same-repo comparison all compare case-insensitively, and the issue's canonical names are taken from the API response's repository_url rather than from what the contributor typed — otherwise a same-repo issue referenced as MEDIC/CHT-Core#1234 would render as MEDIC/CHT-Core#1234 instead of #1234 in the not-assigned message.

Failed lookups. Only 404 and 410 count as "the issue is not there". Anything else 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. An earlier draft warned and skipped the check instead; @sugat009 pointed out on the issue that this was the one path that could hand a genuinely unlinked PR its Ready for review label, which is right, and it is fixed in the second commit.

Scope decisions, both confirmed on the issue: the fallback runs whenever closingIssuesReferences is empty rather than being gated on the base branch, so it behaves the same as the linkage path beside it and the existing same-org filter (andra-bot.js:124) applies unchanged. Matching is keyword-anchored only, so a "related to #123" aside does not register as a link — which also makes the comment stripping belt-and-braces rather than load-bearing.

One thing I did not do: code fences and blockquotes are not stripped, so a body documenting the linking syntax inside a fenced block would match. It is largely self-limiting, since the issue must exist and be assigned to the PR author.

Changed existing tests — worth reviewing that hunk closely. should fail when no issue is linked and should not count an issue linked from a repo outside the org both used a body containing Closes #1234. Under the new behaviour their names no longer describe what they test, so I repointed them at a new bodyWithoutIssue fixture. The stub for issues.get also matches owner/repo case-insensitively, because an exact-match double is stricter than the real API.

Closes #11324

Code review checklist

  • Readable: Concise, well named, follows the style guide
  • Tested: Unit and/or e2e where appropriate — 24 new cases in andra-bot.spec.js (441 → 465 in webapp's mocha suite) covering each reference form, the case-insensitive paths, PR-vs-issue references, comment stripping, dedupe, the 404/410 paths, and propagation of other lookup failures.
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes. — the fallback only runs when GitHub reports no linkage, so PRs that already pass are unaffected; no configuration or workflow changes.
  • AI disclosure: Please disclose use of AI per the guidelines.

AI Disclosure: This PR was written with Claude Code (Claude Opus) — exploring the code path, drafting the fix and its tests, and running the suite. I reviewed the result before submitting, and that review caught two defects in the first draft, both now fixed: a case-sensitive org comparison that silently dropped valid Owner/Repo#N references, and error handling that reported "not linked" for any failed lookup including transient 5xx — reintroducing the same false-fail class this issue is about. An earlier draft also had a capturing group in the keyword alternation that shifted every match index and produced issue_number: NaN; the API call 404'd, the error was swallowed, and the check reported "not linked" exactly as before, so it looked like a working no-op until the tests caught it. I'm accountable for the change and have verified the results reported below.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

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.

- `medic#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
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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AndraBot: linked-issue check false-fails valid PRs on non-default branches (depends on GitHub's default-branch-only keyword linkage)

1 participant