From 0a0887327804685aae28a977fde7a26d0833f894 Mon Sep 17 00:00:00 2001 From: Day Dreamer <87640074+HugoXK@users.noreply.github.com> Date: Tue, 27 Oct 2026 17:14:57 +0800 Subject: [PATCH] fix(dispatch): read the issue id through one helper, not the match's group positions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three shapes carry their number in groups 1, 4 and 7, but `work-dispatch.js` still built the id from `match[1] ?? match[2]`: a repository-qualified mention named `#owner` and a GitHub issue URL named `#undefined`. `cardNamesIssue` refuses a non-numeric id, so a second dispatch on the same issue wrote a second card, which is the invariant #1254/#1331 exists to keep. `issue-ref.js` now exports `refOf(match)` and both `issueRefs` and `workDispatch` call it, so no reader repeats the group numbers. The new tests pin the exact id for a bare, a repository-qualified and a URL mention, refuse `#3366ff`, and pin one card for two dispatches on one issue. Red first, same command: with the old source and these tests, `work-dispatch.test.js` is 2 red / 19 (`'#owner' !== '#1234'`, `2 !== 1`); with the fix it is 19/19 green. `issue-ref.test.js` stays 15/15. Docs-not-needed: this is the matcher's byte boundary and one internal helper extraction — `core-host.md` describes `issue-ref.js`'s API and that a dispatch names an issue/PR id, and both stay true; no caller outside this repository changes. --- core/src/host/work-dispatch.js | 4 +- core/src/shared/issue-ref.js | 15 ++++- core/test/issue-ref.test.js | 57 +++++++++++++++++++ core/test/work-dispatch.test.js | 24 +++++++- .../ds-bot/files/src/host/work-dispatch.js | 4 +- patches/ds-bot/files/src/shared/issue-ref.js | 15 ++++- patches/ds-bot/files/test/issue-ref.test.js | 57 +++++++++++++++++++ .../ds-bot/files/test/work-dispatch.test.js | 24 +++++++- 8 files changed, 186 insertions(+), 14 deletions(-) create mode 100644 core/test/issue-ref.test.js create mode 100644 patches/ds-bot/files/test/issue-ref.test.js diff --git a/core/src/host/work-dispatch.js b/core/src/host/work-dispatch.js index 0f9e75f1..6c7fe267 100644 --- a/core/src/host/work-dispatch.js +++ b/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 } } diff --git a/core/src/shared/issue-ref.js b/core/src/shared/issue-ref.js index 48316b46..40c21a55 100644 --- a/core/src/shared/issue-ref.js +++ b/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 = /(? `#${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])|(? { if (match[3]) return { repo: `${match[1]}/${match[2]}`, number: match[3] } diff --git a/core/test/issue-ref.test.js b/core/test/issue-ref.test.js new file mode 100644 index 00000000..b5fcf532 --- /dev/null +++ b/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' }], + ) +}) diff --git a/core/test/work-dispatch.test.js b/core/test/work-dispatch.test.js index 24c1683a..3ac97728 100644 --- a/core/test/work-dispatch.test.js +++ b/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?')]) diff --git a/patches/ds-bot/files/src/host/work-dispatch.js b/patches/ds-bot/files/src/host/work-dispatch.js index 0f9e75f1..6c7fe267 100644 --- a/patches/ds-bot/files/src/host/work-dispatch.js +++ b/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 } } diff --git a/patches/ds-bot/files/src/shared/issue-ref.js b/patches/ds-bot/files/src/shared/issue-ref.js index 48316b46..40c21a55 100644 --- a/patches/ds-bot/files/src/shared/issue-ref.js +++ b/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 = /(? `#${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])|(? { if (match[3]) return { repo: `${match[1]}/${match[2]}`, number: match[3] } diff --git a/patches/ds-bot/files/test/issue-ref.test.js b/patches/ds-bot/files/test/issue-ref.test.js new file mode 100644 index 00000000..b5fcf532 --- /dev/null +++ b/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' }], + ) +}) diff --git a/patches/ds-bot/files/test/work-dispatch.test.js b/patches/ds-bot/files/test/work-dispatch.test.js index 24c1683a..3ac97728 100644 --- a/patches/ds-bot/files/test/work-dispatch.test.js +++ b/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?')])