From eed6068ba46a95d60f8c9db0dce7acb1adaaf4b2 Mon Sep 17 00:00:00 2001 From: HugoXK Date: Thu, 5 Nov 2026 08:52:37 +0000 Subject: [PATCH] fix(issue-ref): do not read a URL fragment as owner/repo#12 The repository-qualified shape had no boundary in front of it, so any link whose path carried a segment before the `#` matched as `/#12`: `https://example.com/page#123` named issue #123 and the dispatch wrote a card for an issue nobody meant. The qualified shape now needs the same boundary as the bare number, so a URL fragment stays a fragment while `owner/repo#12`, a bare `#12` and GitHub issue/pull URLs still name the issue. Test: core/test/issue-ref-url-fragment.test.js (red on b38ec011: 5 of 8 tests fail). Docs-not-needed: core/src/shared/issue-ref.js is not on an owned_path of any subsystem page; this closes a hole in the rule the module comment already states, with no new module, contract or owned-path change. --- .../2435-issue-ref-url-fragment.fixed.md | 1 + core/src/shared/issue-ref.js | 6 ++- core/test/issue-ref-url-fragment.test.js | 42 +++++++++++++++++++ patches/ds-bot/files/src/shared/issue-ref.js | 6 ++- .../files/test/issue-ref-url-fragment.test.js | 42 +++++++++++++++++++ 5 files changed, 93 insertions(+), 4 deletions(-) create mode 100644 changelog.d/2435-issue-ref-url-fragment.fixed.md create mode 100644 core/test/issue-ref-url-fragment.test.js create mode 100644 patches/ds-bot/files/test/issue-ref-url-fragment.test.js diff --git a/changelog.d/2435-issue-ref-url-fragment.fixed.md b/changelog.d/2435-issue-ref-url-fragment.fixed.md new file mode 100644 index 000000000..7699c575e --- /dev/null +++ b/changelog.d/2435-issue-ref-url-fragment.fixed.md @@ -0,0 +1 @@ +Reading an issue number out of a line no longer turns a link's URL fragment into an issue reference: `https://example.com/page#123` used to name issue #123 and write a card for an issue nobody meant, while `owner/repo#12`, a bare `#12` and GitHub issue and pull-request links keep naming the issue they point at ([#2435](https://github.com/ThinkFlowLab/FlyBot/issues/2435)). diff --git a/core/src/shared/issue-ref.js b/core/src/shared/issue-ref.js index 015f85595..0779541af 100644 --- a/core/src/shared/issue-ref.js +++ b/core/src/shared/issue-ref.js @@ -6,8 +6,10 @@ // front makes it part of an identifier, a version, a hex colour or a URL fragment (`C#7`, `1.2#3`, // `#3366ff`, `help/#123`), and a word character behind means the number keeps going (`#1360abc`). // `owner/repo#12` is its own shape, tried separately, so this separation cannot swallow it: there -// the `#` follows a repo name rather than a space. -const ISSUE_REF = /(?/#12`: `https://example.com/page#123` named issue #123 and the dispatch wrote a +// card for an issue nobody meant (#1360). The boundary and the number are asserted apart: a +// fragment must not hand over digits, and a real GitHub URL with a fragment must still yield exactly +// one number. +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { issueRefs, namesIssue } from '../src/shared/issue-ref.js' + +const fragments = [ + ['a path segment before the #', 'see https://example.com/page#123 for details'], + ['a two-segment path', 'https://example.com/docs/guide#12'], + ['a nested path', 'read https://docs.example.org/a/b#42 then'], + ['a fragment of seven digits', 'see https://example.com/page#1234567 for details'], + ['a fragment longer than an id', 'https://example.com/page#12345678'], +] + +for (const [what, text] of fragments) { + test(`a URL fragment names no issue (${what})`, () => { + assert.deepEqual(issueRefs(text), [], text) + }) +} + +test('a fragment does not keep the digits of the link it follows', () => { + assert.equal(namesIssue('https://example.com/page#123', 123), false) + assert.equal(namesIssue('https://example.com/page#1234', 123), false) +}) + +test('one GitHub URL yields exactly one issue number, fragment and all', () => { + assert.deepEqual(issueRefs('https://github.com/o/r/issues/123#issuecomment-4567'), ['#123']) + assert.deepEqual(issueRefs('https://github.com/o/r/pull/9#discussion_r123456'), ['#9']) +}) + +test('the shapes that do name an issue keep working beside a URL', () => { + assert.deepEqual(issueRefs('owner/repo#1360'), ['#1360']) + assert.deepEqual(issueRefs('see owner/repo#1360 for the fix'), ['#1360']) + assert.deepEqual(issueRefs('please rebase #1360 onto main'), ['#1360']) + assert.deepEqual(issueRefs('https://github.com/o/r/issues/1360'), ['#1360']) + assert.deepEqual(issueRefs('@Dev-2 rebase owner/repo#1360'), ['#1360']) + assert.equal(namesIssue('owner/repo#1360', 1360), true) +}) diff --git a/patches/ds-bot/files/src/shared/issue-ref.js b/patches/ds-bot/files/src/shared/issue-ref.js index 015f85595..0779541af 100644 --- a/patches/ds-bot/files/src/shared/issue-ref.js +++ b/patches/ds-bot/files/src/shared/issue-ref.js @@ -6,8 +6,10 @@ // front makes it part of an identifier, a version, a hex colour or a URL fragment (`C#7`, `1.2#3`, // `#3366ff`, `help/#123`), and a word character behind means the number keeps going (`#1360abc`). // `owner/repo#12` is its own shape, tried separately, so this separation cannot swallow it: there -// the `#` follows a repo name rather than a space. -const ISSUE_REF = /(?/#12`: `https://example.com/page#123` named issue #123 and the dispatch wrote a +// card for an issue nobody meant (#1360). The boundary and the number are asserted apart: a +// fragment must not hand over digits, and a real GitHub URL with a fragment must still yield exactly +// one number. +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { issueRefs, namesIssue } from '../src/shared/issue-ref.js' + +const fragments = [ + ['a path segment before the #', 'see https://example.com/page#123 for details'], + ['a two-segment path', 'https://example.com/docs/guide#12'], + ['a nested path', 'read https://docs.example.org/a/b#42 then'], + ['a fragment of seven digits', 'see https://example.com/page#1234567 for details'], + ['a fragment longer than an id', 'https://example.com/page#12345678'], +] + +for (const [what, text] of fragments) { + test(`a URL fragment names no issue (${what})`, () => { + assert.deepEqual(issueRefs(text), [], text) + }) +} + +test('a fragment does not keep the digits of the link it follows', () => { + assert.equal(namesIssue('https://example.com/page#123', 123), false) + assert.equal(namesIssue('https://example.com/page#1234', 123), false) +}) + +test('one GitHub URL yields exactly one issue number, fragment and all', () => { + assert.deepEqual(issueRefs('https://github.com/o/r/issues/123#issuecomment-4567'), ['#123']) + assert.deepEqual(issueRefs('https://github.com/o/r/pull/9#discussion_r123456'), ['#9']) +}) + +test('the shapes that do name an issue keep working beside a URL', () => { + assert.deepEqual(issueRefs('owner/repo#1360'), ['#1360']) + assert.deepEqual(issueRefs('see owner/repo#1360 for the fix'), ['#1360']) + assert.deepEqual(issueRefs('please rebase #1360 onto main'), ['#1360']) + assert.deepEqual(issueRefs('https://github.com/o/r/issues/1360'), ['#1360']) + assert.deepEqual(issueRefs('@Dev-2 rebase owner/repo#1360'), ['#1360']) + assert.equal(namesIssue('owner/repo#1360', 1360), true) +})