From d937c496cf2f8dea54cd9610e062a77b02d7bc2e Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Tue, 27 Oct 2026 10:01:28 +0000 Subject: [PATCH] chore(issue-ref): drop the dead delivery-board nesting helper (#1376) issueCardForTask and the private half only it used (cardMentions, mentionOf, MENTION, asNumber) went with the delivery board in #1363/#1370. cardNamesIssue keeps its callers in work-dispatch.js and tasks.js. core-host.md no longer lists the dead export, and the #1363 guard now pins the removal. --- changelog.d/1376.removed.md | 1 + core/src/shared/issue-ref.js | 60 ------------------- core/test/issue-ref.test.js | 10 +--- core/test/no-delivery-board.test.js | 18 +++++- docs/subsystems/core-host.md | 2 +- patches/ds-bot/files/src/shared/issue-ref.js | 60 ------------------- patches/ds-bot/files/test/issue-ref.test.js | 10 +--- .../files/test/no-delivery-board.test.js | 18 +++++- 8 files changed, 38 insertions(+), 141 deletions(-) create mode 100644 changelog.d/1376.removed.md diff --git a/changelog.d/1376.removed.md b/changelog.d/1376.removed.md new file mode 100644 index 00000000..867d8ac1 --- /dev/null +++ b/changelog.d/1376.removed.md @@ -0,0 +1 @@ +The leftover delivery-board helper is gone ([#1376](https://github.com/ThinkFlowLab/FlyBot/issues/1376)). `issue-ref.js` exports only the matcher the dispatch and the task store call (`issueRefs`, `namesIssue`, `cardNamesIssue`): `issueCardForTask` and the private half it alone used (`cardMentions`, `mentionOf`, `MENTION`, `asNumber`) were deleted with the board that read them, and `core-host.md` no longer lists them. diff --git a/core/src/shared/issue-ref.js b/core/src/shared/issue-ref.js index 40c21a55..015f8559 100644 --- a/core/src/shared/issue-ref.js +++ b/core/src/shared/issue-ref.js @@ -38,63 +38,3 @@ export function cardNamesIssue(task, wanted) { } 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})(?![\w])|github\.com\/([\w.-]+)\/([\w.-]+)\/(?:issues|pull)\/(\d{1,7})(?![\w])|(? { - if (match[3]) return { repo: `${match[1]}/${match[2]}`, number: match[3] } - if (match[6]) return { repo: `${match[4]}/${match[5]}`, number: match[6] } - if (match[7]) return { repo: null, number: match[7] } - return null -} - -// Mentions in the text `cardNamesIssue` consults: title and `target.href`, and the note only -// when those name no issue. A note cannot attach a repository to a number the title already named. -export function cardMentions(task) { - const own = `${task?.title ?? ''} ${task?.target?.href ?? ''}` - const text = issueRefs(own).length > 0 ? own : `${own} ${task?.note ?? ''}` - const mentions = [] - const seen = new Set() - for (const match of text.matchAll(new RegExp(MENTION.source, 'gi'))) { - const mention = mentionOf(match) - if (!mention) continue - const key = `${mention.repo ?? ''}#${mention.number}` - if (seen.has(key)) continue - seen.add(key) - mentions.push(mention) - } - return mentions -} - -const asNumber = (value) => (value == null || value === '' ? null : String(value)) - -// The one issue card this task hangs under, or null when `cardNamesIssue` names none or names -// more than one. `issues` are `{ repo, number, linked }` or `{ repo, card: { number, linked } }`. -// A repository written next to the number selects that card; a bare number selects the only -// card with that number (or the only card whose linked pull request has it). -export function issueCardForTask(task, issues) { - const mentions = cardMentions(task) - if (mentions.length === 0) return null - const list = (issues ?? []).filter((issue) => asNumber(issue?.card?.number ?? issue?.number) != null) - const numberOf = (issue) => asNumber(issue?.card?.number ?? issue?.number) - const linkedOf = (issue) => asNumber(issue?.card?.linked ?? issue?.linked) - const repoOf = (issue) => (typeof issue?.repo === 'string' ? issue.repo : '') - const qualified = list.filter((issue) => mentions.some((mention) => - mention.repo != null && mention.repo === repoOf(issue) - && (mention.number === numberOf(issue) || mention.number === linkedOf(issue)))) - if (qualified.length === 1) return qualified[0] - if (qualified.length > 1) { - const byNumber = qualified.filter((issue) => mentions.some((mention) => - mention.repo === repoOf(issue) && mention.number === numberOf(issue))) - return byNumber.length === 1 ? byNumber[0] : null - } - const bare = new Set(mentions.filter((mention) => mention.repo == null).map((mention) => mention.number)) - if (bare.size === 0) return null - const bareHits = list.filter((issue) => bare.has(numberOf(issue)) || (linkedOf(issue) != null && bare.has(linkedOf(issue)))) - if (bareHits.length === 1) return bareHits[0] - const byNumber = bareHits.filter((issue) => bare.has(numberOf(issue))) - return byNumber.length === 1 ? byNumber[0] : null -} diff --git a/core/test/issue-ref.test.js b/core/test/issue-ref.test.js index b5fcf532..2fd62473 100644 --- a/core/test/issue-ref.test.js +++ b/core/test/issue-ref.test.js @@ -4,7 +4,7 @@ // 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' +import { issueRefs, namesIssue } from '../src/shared/issue-ref.js' const rejected = [ ['a hex colour', '@Dev-2 set the accent colour to #3366ff'], @@ -47,11 +47,3 @@ test('namesIssue answers the same question, so a colour cannot name an issue', ( 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/no-delivery-board.test.js b/core/test/no-delivery-board.test.js index 42a8fab9..7f0823e4 100644 --- a/core/test/no-delivery-board.test.js +++ b/core/test/no-delivery-board.test.js @@ -2,7 +2,7 @@ // probe script. These assertions are red while that surface is still in the tree, and red // again if a later change puts the panel or either endpoint back. import assert from 'node:assert/strict' -import { existsSync, readFileSync } from 'node:fs' +import { existsSync, readdirSync, readFileSync } from 'node:fs' import { test } from 'node:test' const url = (path) => new URL(`../${path}`, import.meta.url) @@ -39,3 +39,19 @@ test('#1363: the task board does not mount the delivery view', () => { assert.doesNotMatch(client, /delivery-board|delivery-probe|deliveryBoard|deliveryProbe/) assert.match(kanban, /h\(BoardView/) }) + +// #1376: the delivery board's nesting helper went with it. The matcher keeps `cardNamesIssue`, +// which the dispatch and the task store call; `issueCardForTask` and its private half have no +// reader, so no file may name them again. This file is the only place the two names may appear. +test('#1376: the dead nesting helper is gone and nothing names it', () => { + assert.doesNotMatch(read('src/shared/issue-ref.js'), /issueCardForTask|cardMentions/) + const self = 'test/no-delivery-board.test.js' + for (const root of ['src', 'test']) { + for (const entry of readdirSync(url(root), { recursive: true })) { + const path = `${root}/${String(entry)}` + if (path === self) continue + if (!/\.(?:js|mjs|cjs|json)$/.test(path)) continue + assert.doesNotMatch(read(path), /issueCardForTask|cardMentions/, path) + } + } +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index ce4409c8..57308f71 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -53,7 +53,7 @@ The server side of the patched `ds-bot` core (`core/`): the part of [ds-bot-core | `core/src/shared/preview.js` | What a stored line may show a person (#130, #1347): `visibleText(text)` unwraps a complete `` wrap, then removes leftover marker fragments until neither changes the text, then folds to one line. `stripFence` is the marker half alone. The sidebar rows read it (`previewOf` in `src/client/text.js`), and so does the `preview` this channel puts on a room in the `state` payload, which used to be a plain `unwrapUntrusted` and so leaked `from="User"> … >` on a line whose opener had already been cut. Tests: `core/test/ui-clickthrough.test.js`. | | `core/src/shared/failure-copy.js` | A failed turn's sentence a person can act on (#1347): `failureSentence(raw, translate)` keys on the provider's own `code` field in the stored text — not on the message, which is not stable — and returns the mapped sentence (naming the feature in an `UnsupportedModel` refusal, and where to change the model), or the text unchanged when the code is not mapped, because a failure nobody has mapped reads better as it came than as a guess. `src/shared/turn-failures.js` and `src/client/inbox.js` both read it, so the chat's dead-turn lines and the Inbox card say the same thing. Tests: `core/test/ui-clickthrough.test.js`. | | `core/src/shared/board-card.js` | Task-card identity for the per-Bot board (#1363): `taskCardKey(id)` is `task:` and `taskCard(task)` builds the model the client's `BoardCard` draws (`source: 'task'`, `data-key`). Tests: `core/test/board-card.test.js`. | -| `core/src/shared/issue-ref.js` | The one issue/PR matcher (#1254, #1331, #1360): `issueRefs`, `namesIssue`, `cardNamesIssue` and `issueCardForTask`. Own fields (`title`, `target.href`) name the card; a note counts only when those name no issue, and a repository written in that note does not count either. `work-dispatch.js` re-exports the first three and `tasks.js` `assignFromDispatch` uses `cardNamesIssue` to pick the newest open record. `issueCardForTask` answers which GitHub issue card a task would hang under; that hang was the delivery board and is not drawn (#1363). Tests: `core/test/work-dispatch.test.js`. | +| `core/src/shared/issue-ref.js` | The one issue/PR matcher (#1254, #1331, #1360): `issueRefs`, `namesIssue` and `cardNamesIssue`. Own fields (`title`, `target.href`) name the card; a note counts only when those name no issue, and a repository written in that note does not count either. `work-dispatch.js` re-exports the first three and `tasks.js` `assignFromDispatch` uses `cardNamesIssue` to pick the newest open record. Tests: `core/test/work-dispatch.test.js`. | | `core/src/shared/workflow.js` | The workflow engine's pure half (#1272): a workflow is data — `nodes` (each with a required `role` and typed `inputs` / `outputs`), `edges` (`. -> .`) and declared `loops` — and every function below is a function of that file. `validateWorkflow` refuses each way a definition cannot run, with its own sentence (an input nothing produces, an output nothing consumes and that is not a `deliverable`, two producers of one fact, an undeclared cycle, a node with no `role`, a role not on the team, an unknown artifact kind, a misaligned edge, a guard whose `is` is not one of `present` / `fresh` / `equals`, a `column` the board does not have); `workflowColumns` is the board's columns in dependency order, so the node ids are the state and no built-in list exists, and a node carrying `column: false` draws no column at all while a definition that states none keeps the node id as its column (#1265); `layoutWorkflow` is the flowchart (one box per node carrying its role and rails, one edge per `edges[]` row, ranks left to right); `enterNode` / `inputStates` / `renderRefusal` answer whether a card may start and, when it may not, say which input is missing or stale with both revisions, in the node's own `refusal` wording when it has one and in the guard's own wording when that guard carries one; both read a fact by the same parsed key the rest of the engine uses, so the spelling on the card and the spelling on the producer cannot disagree (#1298); `holderOf` is the node's role, which is the card's assignee. #1291's four debts are closed here: one word for one thing (`role`, never `bot`), an unknown guard `is` refuses instead of being read as `present`, `producersOf` / `consumersOf` key on the parsed spec so `text: draft` and `text:draft` are one fact, and `resolvedEdges` reports the artifact it carries once. Tests: `core/test/workflow.test.js`. | | `core/src/shared/workflow-blank.js` | The `blank` preset (#1272): three nodes (`draft` / `review` / `publish`, roles `Writer` / `Editor` / `Publisher`), no guards, artifacts `text:topic -> text:draft -> text:edited -> link:published`. Data, not logic — the same JSON is what a workspace `workflows/blank.json` holds — and the proof that the engine special-cases no node id. Tests: `core/test/workflow.test.js`. | | `core/src/shared/workflow-repo-dev.js` | The `repo-dev` preset (#1272 step 2): one repository change as data — `triage` → `build` → `verify` → `review` → `gate` → `merge` → `release`, roles `Triage` / `Dev` / `SelfTest` / `Reviewer` / `system` / `Release`, and the artifacts `issue` → `issue.ready` → `pr` → `check:tests-ok` → `verdict` → `risk:tier` → `merge-commit` → `release`. Two roles say what they are rather than who: `verify` is `SelfTest` **and** CI, the run being the `check:tests-ok` fact it hands out, and `gate` is held by the Reviewer that judged the pull request, read from the verdict's own `Written-by` through a guard. The gate's guards refuse in the merge gate's own words against `.github/scripts/fixtures/workflow-cases.json`, which no file imports and both sides read. `column` per node: `not-started` / `in-progress` / `done`, with `review` on `false` — a review is the pull request's state, not a third lane (#1265). Tests: `core/test/workflow-repo-dev.test.js`. | diff --git a/patches/ds-bot/files/src/shared/issue-ref.js b/patches/ds-bot/files/src/shared/issue-ref.js index 40c21a55..015f8559 100644 --- a/patches/ds-bot/files/src/shared/issue-ref.js +++ b/patches/ds-bot/files/src/shared/issue-ref.js @@ -38,63 +38,3 @@ export function cardNamesIssue(task, wanted) { } 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})(?![\w])|github\.com\/([\w.-]+)\/([\w.-]+)\/(?:issues|pull)\/(\d{1,7})(?![\w])|(? { - if (match[3]) return { repo: `${match[1]}/${match[2]}`, number: match[3] } - if (match[6]) return { repo: `${match[4]}/${match[5]}`, number: match[6] } - if (match[7]) return { repo: null, number: match[7] } - return null -} - -// Mentions in the text `cardNamesIssue` consults: title and `target.href`, and the note only -// when those name no issue. A note cannot attach a repository to a number the title already named. -export function cardMentions(task) { - const own = `${task?.title ?? ''} ${task?.target?.href ?? ''}` - const text = issueRefs(own).length > 0 ? own : `${own} ${task?.note ?? ''}` - const mentions = [] - const seen = new Set() - for (const match of text.matchAll(new RegExp(MENTION.source, 'gi'))) { - const mention = mentionOf(match) - if (!mention) continue - const key = `${mention.repo ?? ''}#${mention.number}` - if (seen.has(key)) continue - seen.add(key) - mentions.push(mention) - } - return mentions -} - -const asNumber = (value) => (value == null || value === '' ? null : String(value)) - -// The one issue card this task hangs under, or null when `cardNamesIssue` names none or names -// more than one. `issues` are `{ repo, number, linked }` or `{ repo, card: { number, linked } }`. -// A repository written next to the number selects that card; a bare number selects the only -// card with that number (or the only card whose linked pull request has it). -export function issueCardForTask(task, issues) { - const mentions = cardMentions(task) - if (mentions.length === 0) return null - const list = (issues ?? []).filter((issue) => asNumber(issue?.card?.number ?? issue?.number) != null) - const numberOf = (issue) => asNumber(issue?.card?.number ?? issue?.number) - const linkedOf = (issue) => asNumber(issue?.card?.linked ?? issue?.linked) - const repoOf = (issue) => (typeof issue?.repo === 'string' ? issue.repo : '') - const qualified = list.filter((issue) => mentions.some((mention) => - mention.repo != null && mention.repo === repoOf(issue) - && (mention.number === numberOf(issue) || mention.number === linkedOf(issue)))) - if (qualified.length === 1) return qualified[0] - if (qualified.length > 1) { - const byNumber = qualified.filter((issue) => mentions.some((mention) => - mention.repo === repoOf(issue) && mention.number === numberOf(issue))) - return byNumber.length === 1 ? byNumber[0] : null - } - const bare = new Set(mentions.filter((mention) => mention.repo == null).map((mention) => mention.number)) - if (bare.size === 0) return null - const bareHits = list.filter((issue) => bare.has(numberOf(issue)) || (linkedOf(issue) != null && bare.has(linkedOf(issue)))) - if (bareHits.length === 1) return bareHits[0] - const byNumber = bareHits.filter((issue) => bare.has(numberOf(issue))) - return byNumber.length === 1 ? byNumber[0] : null -} diff --git a/patches/ds-bot/files/test/issue-ref.test.js b/patches/ds-bot/files/test/issue-ref.test.js index b5fcf532..2fd62473 100644 --- a/patches/ds-bot/files/test/issue-ref.test.js +++ b/patches/ds-bot/files/test/issue-ref.test.js @@ -4,7 +4,7 @@ // 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' +import { issueRefs, namesIssue } from '../src/shared/issue-ref.js' const rejected = [ ['a hex colour', '@Dev-2 set the accent colour to #3366ff'], @@ -47,11 +47,3 @@ test('namesIssue answers the same question, so a colour cannot name an issue', ( 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/no-delivery-board.test.js b/patches/ds-bot/files/test/no-delivery-board.test.js index 42a8fab9..7f0823e4 100644 --- a/patches/ds-bot/files/test/no-delivery-board.test.js +++ b/patches/ds-bot/files/test/no-delivery-board.test.js @@ -2,7 +2,7 @@ // probe script. These assertions are red while that surface is still in the tree, and red // again if a later change puts the panel or either endpoint back. import assert from 'node:assert/strict' -import { existsSync, readFileSync } from 'node:fs' +import { existsSync, readdirSync, readFileSync } from 'node:fs' import { test } from 'node:test' const url = (path) => new URL(`../${path}`, import.meta.url) @@ -39,3 +39,19 @@ test('#1363: the task board does not mount the delivery view', () => { assert.doesNotMatch(client, /delivery-board|delivery-probe|deliveryBoard|deliveryProbe/) assert.match(kanban, /h\(BoardView/) }) + +// #1376: the delivery board's nesting helper went with it. The matcher keeps `cardNamesIssue`, +// which the dispatch and the task store call; `issueCardForTask` and its private half have no +// reader, so no file may name them again. This file is the only place the two names may appear. +test('#1376: the dead nesting helper is gone and nothing names it', () => { + assert.doesNotMatch(read('src/shared/issue-ref.js'), /issueCardForTask|cardMentions/) + const self = 'test/no-delivery-board.test.js' + for (const root of ['src', 'test']) { + for (const entry of readdirSync(url(root), { recursive: true })) { + const path = `${root}/${String(entry)}` + if (path === self) continue + if (!/\.(?:js|mjs|cjs|json)$/.test(path)) continue + assert.doesNotMatch(read(path), /issueCardForTask|cardMentions/, path) + } + } +})