diff --git a/.github/scripts/intake-triage/admit.mjs b/.github/scripts/intake-triage/admit.mjs index dbb571a83f..bfe2ebee54 100644 --- a/.github/scripts/intake-triage/admit.mjs +++ b/.github/scripts/intake-triage/admit.mjs @@ -655,7 +655,7 @@ export async function sayWhy(github, repository, base, comments, text) { // The body, the comment count and the first 100 comments, each with its // author, last editor and edit time: the first 100 comments are what pr-verify // reads, and it stops on an issue with more. -const SPEC_COMMENTS = 100; +export const SPEC_COMMENTS = 100; const SPEC_QUERY = `query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { issue(number: $number) { body lastEditedAt author { ...actor } editor { ...actor } diff --git a/.github/scripts/intake-triage/refine.mjs b/.github/scripts/intake-triage/refine.mjs index 782283a0e0..39052e5cad 100755 --- a/.github/scripts/intake-triage/refine.mjs +++ b/.github/scripts/intake-triage/refine.mjs @@ -27,8 +27,9 @@ // criteria as a task list. On an issue a listed human filed, admit.mjs and // pr-verify count the newest refinement as spec while it is unedited (owner // decision Q78); on any other issue its criteria count only once a listed -// human posts them as their own. Only when that text already holds a task list -// (admit.mjs hasTrustedCriteria) does it ask for one more triage, which runs +// human posts them as their own. Only when that text, with this refinement +// counted as posted, holds a task list (admit.mjs hasTrustedCriteria) does it +// ask for one more triage, which runs // admission again: it takes the lanes App's `status: needs-decision` off, or, // for a `status: ready` issue admission has not taken, keeps the labels, adds // `needs-triage`, and sets the job's `redispatch` output. Otherwise the labels @@ -83,7 +84,7 @@ import { appendFileSync } from "node:fs"; import { writeOutputs } from "../../actions/check-kill-switch/check-kill-switch.mjs"; import { createGitHub, GitHubError, loadTrustedList, paginate } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { selectTrustedIssueComments } from "../../actions/select-trusted-text/select-trusted-text.mjs"; -import { ADMIT_MARKER, decisionStays, hasTrustedCriteria, loadRules, REFINE_FOOTERS, REFINE_MARKER, refusalText, specOf, taskItems, unrunnableCriteria, unrunnableReason } from "./admit.mjs"; +import { ADMIT_MARKER, decisionStays, hasTrustedCriteria, loadRules, REFINE_FOOTERS, REFINE_MARKER, refusalText, SPEC_COMMENTS, specOf, taskItems, unrunnableCriteria, unrunnableReason } from "./admit.mjs"; import { LANES_APP_LOGIN, MARKER } from "./apply-verdict.mjs"; import { linesDigest, renderSplitRecord, SIZE_SPLIT_MARKER, SIZE_SPLIT_PENDING, SPLIT_CHILD_MARKER, splitChildHeader, splitRecord, splitSourceLine, splitTaskLines } from "./admit.mjs"; @@ -577,8 +578,8 @@ export function splitBody(body, taskList, moved) { // edit leaves the held items in place for the next triage to hold and split // again, reusing that comment. // `humans` and `rules` decide whether an exact refinement asks for a -// re-triage (admit.mjs hasTrustedCriteria); a failed read of the spec throws -// before any label write. +// re-triage (admit.mjs hasTrustedCriteria, over the spec with this refinement +// posted); a failed read of the spec throws before any label write. export async function applyRefinement({ github, repository, number, appId, humans, rules, output, context, log = console.log }) { const base = `/repos/${repository}/issues/${number}`; const issue = await github("GET", base); @@ -622,14 +623,20 @@ export async function applyRefinement({ github, repository, number, appId, human return { outcome: "split", redispatch: true }; } const post = (retriage = false) => github("POST", `${base}/comments`, { body: render(output, { body: issue.body, retriage }) }); - // Admission reads criteria only from a listed human's text, so a re-triage - // without them would refuse again: one read, before any label write. + // Admission reads criteria only from a listed human's text and, on an issue + // a listed human filed, the newest unedited refinement, so a re-triage + // without them would refuse again. It judges the spec as it will read once + // this refinement is posted, as the newest comment: one read, before any + // label write. Past admission's comment window the refinement is unread + // and admission refuses, so it asks for no triage. const untrusted = async () => { - if (hasTrustedCriteria(await specOf(github, repository, number), humans, rules["task-list"], appId)) { + const spec = await specOf(github, repository, number); + const refinement = { body: render(output, { body: issue.body, retriage: true }), lastEditedAt: null, author: { __typename: "Bot", databaseId: appId } }; + if (spec.commentCount < SPEC_COMMENTS && hasTrustedCriteria({ ...spec, comments: [...spec.comments, refinement] }, humans, rules["task-list"], appId)) { return false; } await post(); - log(`refine: #${number} is exact; no listed human's text holds a task list yet, so the labels stay and no triage is asked`); + log(`refine: #${number} is exact; no text admission counts holds a task list yet, so the labels stay and no triage is asked`); return true; }; if (!output.exact) { @@ -644,7 +651,8 @@ export async function applyRefinement({ github, repository, number, appId, human return { outcome: "needs-decision", redispatch: false }; } // A `status: ready` issue admission has not taken (no needs-decision, no - // agent-ready) is triaged again when a listed human's text holds criteria. + // agent-ready) is triaged again when the text admission counts, with this + // refinement posted, holds criteria. if (!labels.includes(NEEDS_DECISION)) { if (!labels.includes(READY) || labels.includes(AGENT_READY)) { await post(); diff --git a/.github/scripts/intake-triage/refine.test.mjs b/.github/scripts/intake-triage/refine.test.mjs index bc4388d5fa..49ac1d7e73 100644 --- a/.github/scripts/intake-triage/refine.test.mjs +++ b/.github/scripts/intake-triage/refine.test.mjs @@ -418,8 +418,10 @@ describe("applyRefinement", () => { ); }); - // Owner decision Q9: admission reads criteria only from a listed human's - // text, so a re-triage on the App's text alone would only refuse again. + // Owner decisions Q9 and Q78: admission reads criteria from a listed + // human's text and, on an issue a listed human filed, the newest unedited + // refinement, this one once posted; a re-triage on other text would only + // refuse again. for (const [branch, setup] of [ ["the App's needs-decision", { issue: issueOf(), events: labeledBy(APP) }], ["a status: ready issue awaiting admission", { issue: issueOf({ labels: [{ name: "status: ready" }] }), events: [] }], @@ -440,6 +442,45 @@ describe("applyRefinement", () => { assert.match(writes(fake.calls).at(-1).body.body, /triages the issue again/); }); + // A listed human filed it with no task list, so the refinement this + // run posts is the spec admission reads. + test(`an exact refinement of ${branch} re-triages when a listed human filed it and only this refinement will hold the task list`, async () => { + const fake = fakeGitHub({ ...setup, comments: [laneComment()], spec: specOf() }); + assert.deepEqual(await apply(fake, EXACT), { outcome: "exact", redispatch: true }); + const done = writes(fake.calls).map(({ method, path }) => `${method} ${path}`); + const lifted = setup.events.length > 0 ? ["DELETE /repos/o/r/issues/7/labels/status%3A%20needs-decision"] : []; + assert.deepEqual(done, ["POST /repos/o/r/issues/7/labels", ...lifted, "POST /repos/o/r/issues/7/comments"]); + assert.deepEqual(writes(fake.calls)[0].body, { labels: ["needs-triage"] }); + assert.match(writes(fake.calls).at(-1).body.body, /triages the issue again/); + }); + + test(`an exact refinement of ${branch} re-triages over an older unedited refinement with no task list: the newest counts`, async () => { + const older = { body: `${REFINE_MARKER}\nAn earlier refinement with no criteria.`, lastEditedAt: null, author: actor(APP), editor: null }; + const spec = { ...specOf(), comments: { totalCount: 1, nodes: [older] } }; + const fake = fakeGitHub({ ...setup, comments: [laneComment()], spec }); + assert.deepEqual(await apply(fake, EXACT), { outcome: "exact", redispatch: true }); + }); + + test(`an exact refinement of ${branch} past admission's comment window keeps the labels: admission would not read it`, async () => { + const spec = { ...specOf(), comments: { totalCount: 100, nodes: [] } }; + const fake = fakeGitHub({ ...setup, comments: [laneComment()], spec }); + assert.deepEqual(await apply(fake, EXACT), { outcome: "exact", redispatch: false }); + assert.deepEqual( + writes(fake.calls).map(({ method, path }) => `${method} ${path}`), + ["POST /repos/o/r/issues/7/comments"], + ); + }); + + test(`an exact refinement of ${branch} the lanes App filed with no task list keeps the labels: its refinement is no one's spec`, async () => { + const fake = fakeGitHub({ ...setup, comments: [laneComment()], spec: { ...specOf(), author: actor(APP) } }); + assert.deepEqual(await apply(fake, EXACT), { outcome: "exact", redispatch: false }); + assert.deepEqual( + writes(fake.calls).map(({ method, path }) => `${method} ${path}`), + ["POST /repos/o/r/issues/7/comments"], + ); + assert.match(writes(fake.calls)[0].body.body, /post them as your own comment/); + }); + test(`an exact refinement of ${branch} reads the spec before its first label write and re-triages on a listed human's task list`, async () => { const fake = fakeGitHub({ ...setup, comments: [laneComment()], spec: specOf("The README is wrong.\n\n- [ ] The README count matches the plugin count") }); const result = await apply(fake, EXACT);