fix(issue-ref): read a bare #1234 only when the number stands alone

core/src/host/work-dispatch.js

@@ -3,7 +3,7 @@
 // that member's card and the drive wakes it in its own chat (#1205); no group part opens. A
 // question, a remark, a line naming two members or an `@all` keeps the group turn it had — this
 // module only answers the shape of one line, the room decides who may dispatch.
-import { issueRefPattern } from '../shared/issue-ref.js'
+import { issueRefPattern, refOf } from '../shared/issue-ref.js'
 
 export { cardNamesIssue, issueRefs, namesIssue } from '../shared/issue-ref.js'
 
@@ -29,7 +29,7 @@ export function workDispatch(text, { callees = [] } = {}) {
   for (const match of line.matchAll(issueRefPattern())) {
     const near = line.slice(Math.max(0, match.index - NEAR), match.index + match[0].length + NEAR)
     if (!IMPERATIVE.test(near)) continue
-    const issue = `#${match[1] ?? match[2]}`
+    const issue = refOf(match)
     const note = oneLine(line)
     return { botId: callee.id, issue, title: note, note }
   }

core/src/shared/issue-ref.js

@@ -2,13 +2,22 @@
 // The dispatch, the task store and the board all call these. A second pattern would
 // hang a record the host does not treat as that issue's card.
 
-const ISSUE_REF = /#(\d{1,7})(?![0-9])|github\.com\/[\w.-]+\/[\w.-]+\/(?:issues|pull)\/(\d{1,7})(?![0-9])/gi
+// A bare `#12` counts only when the number stands alone: a word character, `.`, `-` or `/` in
+// 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 = /(?<![\w./-])#(\d{1,7})(?![\w])|(?:github\.com\/)?([\w.-]+)\/([\w.-]+)#(\d{1,7})(?![\w])|github\.com\/([\w.-]+)\/([\w.-]+)\/(?:issues|pull)\/(\d{1,7})(?![\w])/gi
+
+// The id a match of `ISSUE_REF` names, normalised to `#1234`. The three shapes carry their number in
+// different groups (1, 4, 7); every reader takes it from here, so no caller can pick the wrong one.
+export const refOf = (match) => `#${match[1] ?? match[4] ?? match[7]}`
 
 // The issue/PR numbers a line names, normalised to `#1234`, in the order they appear.
 export function issueRefs(text) {
   const keys = []
   for (const match of String(text ?? '').matchAll(new RegExp(ISSUE_REF.source, 'gi'))) {
-    const key = `#${match[1] ?? match[2]}`
+    const key = refOf(match)
     if (!keys.includes(key)) keys.push(key)
   }
   return keys
@@ -33,7 +42,7 @@ export const issueRefPattern = () => new RegExp(ISSUE_REF.source, 'gi')
 // The same mentions as ISSUE_REF, with the repository when the text writes one beside the number
 // (`owner/repo#12`, or a github.com issues/pull URL). A bare `#12` carries no repository. The
 // number is the same one `issueRefs` returns; the repository is not a second decision.
-const MENTION = /(?:github\.com\/)?([\w.-]+)\/([\w.-]+)#(\d{1,7})(?![0-9])|github\.com\/([\w.-]+)\/([\w.-]+)\/(?:issues|pull)\/(\d{1,7})(?![0-9])|#(\d{1,7})(?![0-9])/gi
+const MENTION = /(?:github\.com\/)?([\w.-]+)\/([\w.-]+)#(\d{1,7})(?![\w])|github\.com\/([\w.-]+)\/([\w.-]+)\/(?:issues|pull)\/(\d{1,7})(?![\w])|(?<![\w./-])#(\d{1,7})(?![\w])/gi
 
 const mentionOf = (match) => {
   if (match[3]) return { repo: `${match[1]}/${match[2]}`, number: match[3] }

core/test/issue-ref.test.js

@@ -0,0 +1,57 @@
+// A `#1234` is a reference only when the number stands on its own (P-14, from #1360's dispatch).
+// A hex colour, a language version, a version string, an identifier and a URL fragment all used
+// to read as one, and each of them dispatches a card that names an issue which does not exist;
+// the shapes that do name an issue have to keep working beside them.
+import assert from 'node:assert/strict'
+import { test } from 'node:test'
+import { cardMentions, issueRefs, namesIssue } from '../src/shared/issue-ref.js'
+
+const rejected = [
+  ['a hex colour', '@Dev-2 set the accent colour to #3366ff'],
+  ['a language version', 'fix C#7 codegen'],
+  ['a version string', 'version in 1.2#3'],
+  ['an identifier', 'the room abc#12 resolved'],
+  ['a number running into letters', '#1360abc'],
+  ['a URL fragment', 'see https://example.com/help/#123'],
+]
+
+for (const [what, text] of rejected) {
+  test(`${what} is not an issue reference`, () => {
+    assert.deepEqual(issueRefs(text), [], text)
+  })
+}
+
+const accepted = [
+  ['a bare number', '#1360', ['#1360']],
+  ['a repository-qualified number', 'owner/repo#1360', ['#1360']],
+  ['a github issue URL', 'https://github.com/o/r/issues/1360', ['#1360']],
+  ['a github pull URL', 'https://github.com/o/r/pull/1360', ['#1360']],
+  ['a number after a Bot mention', '@Dev-2 rebase #1360', ['#1360']],
+  ['a number in a sentence', 'please rebase #1360 onto main', ['#1360']],
+  ['numbers in brackets and punctuation', '(#1360), #1361.', ['#1360', '#1361']],
+]
+
+for (const [what, text, expected] of accepted) {
+  test(`${what} still names the issue: ${text}`, () => {
+    assert.deepEqual(issueRefs(text), expected)
+  })
+}
+
+test('namesIssue answers the same question, so a colour cannot name an issue', () => {
+  assert.equal(namesIssue('accent #3366ff', 3366), false)
+  assert.equal(namesIssue('fix C#7', 7), false)
+  assert.equal(namesIssue('1.2#3', 3), false)
+  assert.equal(namesIssue('abc#12', 12), false)
+  assert.equal(namesIssue('#1360abc', 1360), false)
+  assert.equal(namesIssue('https://example.com/help/#123', 123), false)
+  assert.equal(namesIssue('owner/repo#1360', 1360), true)
+  assert.equal(namesIssue('@Dev-2 rebase #1360', 1360), true)
+})
+
+test('a card whose title only carries a colour names no issue', () => {
+  assert.deepEqual(cardMentions({ title: 'accent colour #3366ff', target: { href: '' } }), [])
+  assert.deepEqual(
+    cardMentions({ title: 'owner/repo#1360', target: { href: '' } }),
+    [{ repo: 'owner/repo', number: '1360' }],
+  )
+})

core/test/work-dispatch.test.js

@@ -9,7 +9,7 @@ import { tmpdir } from 'node:os'
 import { join } from 'node:path'
 import { test } from 'node:test'
 import { createRuntime } from '../index.js'
-import { workDispatch } from '../src/host/work-dispatch.js'
+import { cardNamesIssue, workDispatch } from '../src/host/work-dispatch.js'
 import { fakeContext } from './fake-ctx.js'
 
 const team = (room = {}) => ({
@@ -131,7 +131,10 @@ test('the dispatch shape is an issue id and an imperative, never any @ (#1254)',
     botId: 'a', issue: '#1234', title: '#1234 rebase and push', note: '#1234 rebase and push',
   })
   assert.ok(workDispatch('@Dev1 please fix #1234', { callees: [one[0]] }), 'the verb may come first')
-  assert.ok(workDispatch('@Dev1 https://github.com/o/r/pull/1234 land it', { callees: [one[0]] }), 'a pull link is an id too')
+  assert.equal(workDispatch('@Dev1 https://github.com/o/r/pull/1234 land it', { callees: [one[0]] })?.issue, '#1234', 'a pull link names its number, not undefined')
+  assert.equal(workDispatch('@Dev1 owner/repo#1234 rebase', { callees: [one[0]] })?.issue, '#1234', 'a repository-qualified mention names the number, not the repository')
+  assert.equal(workDispatch('@Dev1 set the accent colour to #3366ff', { callees: [one[0]] }), undefined, 'a hex colour is not a dispatch')
+  assert.equal(workDispatch('@Dev1 https://github.com/o/r/issues/1234 rebase', { callees: [one[0]] })?.issue, '#1234', 'a URL mention names the number, not undefined')
   assert.ok(workDispatch('@Dev1 请按 #1254 修一下 dispatch', { callees: [one[0]] }), 'Chinese imperatives count')
   assert.equal(workDispatch('@Dev1 #1234 looks good to me', { callees: [one[0]] }), undefined, 'a remark is not a dispatch')
   assert.equal(workDispatch('@Dev1 #1234 which file?', { callees: [one[0]] }), undefined, 'a question stays a question')
@@ -173,6 +176,23 @@ test('a dispatch on a card that exists updates that card instead of writing a se
   assert.ok(again.updatedAt > updatedAt, 'the update moves the card, so the drive wakes it')
 })
 
+test('two dispatches on one issue through a qualified or URL id write one card (#1254, #1331)', async (t) => {
+  const app = await start(t)
+  await app.relay([app.post('@Dev1 owner/repo#1234 rebase onto main')]).done
+  const [first] = app.cards()
+  assert.ok(first, 'a repository-qualified id writes one card')
+  await app.relay([app.post('@Dev1 https://github.com/o/r/issues/1234 then push the branch')]).done
+  assert.equal(app.cards().length, 1, 'the same issue through a URL id reuses that card, so the id is the number')
+  assert.equal(app.cards()[0].id, first.id, 'one issue, one card')
+})
+
+test('a card names an issue through the same number, whichever shape wrote it (#1331)', () => {
+  assert.equal(cardNamesIssue({ title: 'owner/repo#1360 rebase' }, '#1360'), true, 'a qualified id names the number')
+  assert.equal(cardNamesIssue({ title: 'see https://github.com/o/r/issues/1360' }, '#1360'), true, 'a URL id names the number')
+  assert.equal(cardNamesIssue({ title: 'rebase #1360' }, '#1360'), true, 'a bare id names the number')
+  assert.equal(cardNamesIssue({ title: 'accent #3366ff' }, '#3366'), false, 'a colour names no issue')
+})
+
 test('a question to a member is still its group turn (#1254)', async (t) => {
   const app = await start(t)
   const run = app.relay([app.post('@Dev1 #1234 which file should I change?')])

patches/ds-bot/files/src/host/work-dispatch.js

@@ -3,7 +3,7 @@
 // that member's card and the drive wakes it in its own chat (#1205); no group part opens. A
 // question, a remark, a line naming two members or an `@all` keeps the group turn it had — this
 // module only answers the shape of one line, the room decides who may dispatch.
-import { issueRefPattern } from '../shared/issue-ref.js'
+import { issueRefPattern, refOf } from '../shared/issue-ref.js'
 
 export { cardNamesIssue, issueRefs, namesIssue } from '../shared/issue-ref.js'
 
@@ -29,7 +29,7 @@ export function workDispatch(text, { callees = [] } = {}) {
   for (const match of line.matchAll(issueRefPattern())) {
     const near = line.slice(Math.max(0, match.index - NEAR), match.index + match[0].length + NEAR)
     if (!IMPERATIVE.test(near)) continue
-    const issue = `#${match[1] ?? match[2]}`
+    const issue = refOf(match)
     const note = oneLine(line)
     return { botId: callee.id, issue, title: note, note }
   }

patches/ds-bot/files/src/shared/issue-ref.js

@@ -2,13 +2,22 @@
 // The dispatch, the task store and the board all call these. A second pattern would
 // hang a record the host does not treat as that issue's card.
 
-const ISSUE_REF = /#(\d{1,7})(?![0-9])|github\.com\/[\w.-]+\/[\w.-]+\/(?:issues|pull)\/(\d{1,7})(?![0-9])/gi
+// A bare `#12` counts only when the number stands alone: a word character, `.`, `-` or `/` in
+// 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 = /(?<![\w./-])#(\d{1,7})(?![\w])|(?:github\.com\/)?([\w.-]+)\/([\w.-]+)#(\d{1,7})(?![\w])|github\.com\/([\w.-]+)\/([\w.-]+)\/(?:issues|pull)\/(\d{1,7})(?![\w])/gi
+
+// The id a match of `ISSUE_REF` names, normalised to `#1234`. The three shapes carry their number in
+// different groups (1, 4, 7); every reader takes it from here, so no caller can pick the wrong one.
+export const refOf = (match) => `#${match[1] ?? match[4] ?? match[7]}`
 
 // The issue/PR numbers a line names, normalised to `#1234`, in the order they appear.
 export function issueRefs(text) {
   const keys = []
   for (const match of String(text ?? '').matchAll(new RegExp(ISSUE_REF.source, 'gi'))) {
-    const key = `#${match[1] ?? match[2]}`
+    const key = refOf(match)
     if (!keys.includes(key)) keys.push(key)
   }
   return keys
@@ -33,7 +42,7 @@ export const issueRefPattern = () => new RegExp(ISSUE_REF.source, 'gi')
 // The same mentions as ISSUE_REF, with the repository when the text writes one beside the number
 // (`owner/repo#12`, or a github.com issues/pull URL). A bare `#12` carries no repository. The
 // number is the same one `issueRefs` returns; the repository is not a second decision.
-const MENTION = /(?:github\.com\/)?([\w.-]+)\/([\w.-]+)#(\d{1,7})(?![0-9])|github\.com\/([\w.-]+)\/([\w.-]+)\/(?:issues|pull)\/(\d{1,7})(?![0-9])|#(\d{1,7})(?![0-9])/gi
+const MENTION = /(?:github\.com\/)?([\w.-]+)\/([\w.-]+)#(\d{1,7})(?![\w])|github\.com\/([\w.-]+)\/([\w.-]+)\/(?:issues|pull)\/(\d{1,7})(?![\w])|(?<![\w./-])#(\d{1,7})(?![\w])/gi
 
 const mentionOf = (match) => {
   if (match[3]) return { repo: `${match[1]}/${match[2]}`, number: match[3] }

patches/ds-bot/files/test/issue-ref.test.js

@@ -0,0 +1,57 @@
+// A `#1234` is a reference only when the number stands on its own (P-14, from #1360's dispatch).
+// A hex colour, a language version, a version string, an identifier and a URL fragment all used
+// to read as one, and each of them dispatches a card that names an issue which does not exist;
+// the shapes that do name an issue have to keep working beside them.
+import assert from 'node:assert/strict'
+import { test } from 'node:test'
+import { cardMentions, issueRefs, namesIssue } from '../src/shared/issue-ref.js'
+
+const rejected = [
+  ['a hex colour', '@Dev-2 set the accent colour to #3366ff'],
+  ['a language version', 'fix C#7 codegen'],
+  ['a version string', 'version in 1.2#3'],
+  ['an identifier', 'the room abc#12 resolved'],
+  ['a number running into letters', '#1360abc'],
+  ['a URL fragment', 'see https://example.com/help/#123'],
+]
+
+for (const [what, text] of rejected) {
+  test(`${what} is not an issue reference`, () => {
+    assert.deepEqual(issueRefs(text), [], text)
+  })
+}
+
+const accepted = [
+  ['a bare number', '#1360', ['#1360']],
+  ['a repository-qualified number', 'owner/repo#1360', ['#1360']],
+  ['a github issue URL', 'https://github.com/o/r/issues/1360', ['#1360']],
+  ['a github pull URL', 'https://github.com/o/r/pull/1360', ['#1360']],
+  ['a number after a Bot mention', '@Dev-2 rebase #1360', ['#1360']],
+  ['a number in a sentence', 'please rebase #1360 onto main', ['#1360']],
+  ['numbers in brackets and punctuation', '(#1360), #1361.', ['#1360', '#1361']],
+]
+
+for (const [what, text, expected] of accepted) {
+  test(`${what} still names the issue: ${text}`, () => {
+    assert.deepEqual(issueRefs(text), expected)
+  })
+}
+
+test('namesIssue answers the same question, so a colour cannot name an issue', () => {
+  assert.equal(namesIssue('accent #3366ff', 3366), false)
+  assert.equal(namesIssue('fix C#7', 7), false)
+  assert.equal(namesIssue('1.2#3', 3), false)
+  assert.equal(namesIssue('abc#12', 12), false)
+  assert.equal(namesIssue('#1360abc', 1360), false)
+  assert.equal(namesIssue('https://example.com/help/#123', 123), false)
+  assert.equal(namesIssue('owner/repo#1360', 1360), true)
+  assert.equal(namesIssue('@Dev-2 rebase #1360', 1360), true)
+})
+
+test('a card whose title only carries a colour names no issue', () => {
+  assert.deepEqual(cardMentions({ title: 'accent colour #3366ff', target: { href: '' } }), [])
+  assert.deepEqual(
+    cardMentions({ title: 'owner/repo#1360', target: { href: '' } }),
+    [{ repo: 'owner/repo', number: '1360' }],
+  )
+})

patches/ds-bot/files/test/work-dispatch.test.js

@@ -9,7 +9,7 @@ import { tmpdir } from 'node:os'
 import { join } from 'node:path'
 import { test } from 'node:test'
 import { createRuntime } from '../index.js'
-import { workDispatch } from '../src/host/work-dispatch.js'
+import { cardNamesIssue, workDispatch } from '../src/host/work-dispatch.js'
 import { fakeContext } from './fake-ctx.js'
 
 const team = (room = {}) => ({
@@ -131,7 +131,10 @@ test('the dispatch shape is an issue id and an imperative, never any @ (#1254)',
     botId: 'a', issue: '#1234', title: '#1234 rebase and push', note: '#1234 rebase and push',
   })
   assert.ok(workDispatch('@Dev1 please fix #1234', { callees: [one[0]] }), 'the verb may come first')
-  assert.ok(workDispatch('@Dev1 https://github.com/o/r/pull/1234 land it', { callees: [one[0]] }), 'a pull link is an id too')
+  assert.equal(workDispatch('@Dev1 https://github.com/o/r/pull/1234 land it', { callees: [one[0]] })?.issue, '#1234', 'a pull link names its number, not undefined')
+  assert.equal(workDispatch('@Dev1 owner/repo#1234 rebase', { callees: [one[0]] })?.issue, '#1234', 'a repository-qualified mention names the number, not the repository')
+  assert.equal(workDispatch('@Dev1 set the accent colour to #3366ff', { callees: [one[0]] }), undefined, 'a hex colour is not a dispatch')
+  assert.equal(workDispatch('@Dev1 https://github.com/o/r/issues/1234 rebase', { callees: [one[0]] })?.issue, '#1234', 'a URL mention names the number, not undefined')
   assert.ok(workDispatch('@Dev1 请按 #1254 修一下 dispatch', { callees: [one[0]] }), 'Chinese imperatives count')
   assert.equal(workDispatch('@Dev1 #1234 looks good to me', { callees: [one[0]] }), undefined, 'a remark is not a dispatch')
   assert.equal(workDispatch('@Dev1 #1234 which file?', { callees: [one[0]] }), undefined, 'a question stays a question')
@@ -173,6 +176,23 @@ test('a dispatch on a card that exists updates that card instead of writing a se
   assert.ok(again.updatedAt > updatedAt, 'the update moves the card, so the drive wakes it')
 })
 
+test('two dispatches on one issue through a qualified or URL id write one card (#1254, #1331)', async (t) => {
+  const app = await start(t)
+  await app.relay([app.post('@Dev1 owner/repo#1234 rebase onto main')]).done
+  const [first] = app.cards()
+  assert.ok(first, 'a repository-qualified id writes one card')
+  await app.relay([app.post('@Dev1 https://github.com/o/r/issues/1234 then push the branch')]).done
+  assert.equal(app.cards().length, 1, 'the same issue through a URL id reuses that card, so the id is the number')
+  assert.equal(app.cards()[0].id, first.id, 'one issue, one card')
+})
+
+test('a card names an issue through the same number, whichever shape wrote it (#1331)', () => {
+  assert.equal(cardNamesIssue({ title: 'owner/repo#1360 rebase' }, '#1360'), true, 'a qualified id names the number')
+  assert.equal(cardNamesIssue({ title: 'see https://github.com/o/r/issues/1360' }, '#1360'), true, 'a URL id names the number')
+  assert.equal(cardNamesIssue({ title: 'rebase #1360' }, '#1360'), true, 'a bare id names the number')
+  assert.equal(cardNamesIssue({ title: 'accent #3366ff' }, '#3366'), false, 'a colour names no issue')
+})
+
 test('a question to a member is still its group turn (#1254)', async (t) => {
   const app = await start(t)
   const run = app.relay([app.post('@Dev1 #1234 which file should I change?')])