diff --git a/.github/workflows/cross-repo-issue-closer.yml b/.github/workflows/cross-repo-issue-closer.yml index d016d85f5d..8cdea80bb8 100644 --- a/.github/workflows/cross-repo-issue-closer.yml +++ b/.github/workflows/cross-repo-issue-closer.yml @@ -40,6 +40,35 @@ # as success". Isolation and outcome are now separate: the loop records the # keys it could not close, runs to the end, and the verdict is passed after it. # +# The loop's LAST silent exit was the already-closed target (#9643). It was +# skipped whole, and the skip covered two calls of which only one is redundant: +# re-closing a closed issue is a no-op worth avoiding, but the BACKLINK is the +# half this file's own first paragraph calls the defect — "no way to find the +# fix" — and a wholesale skip dropped it and reported green. +# +# So the state is no longer the question; the CAUSE is, because "already +# closed" is three situations and they want three answers: +# +# closed as not_planned/duplicate -> post NOTHING and annotate. A comment +# saying this PR fixed it contradicts the triage decision recorded on the +# issue; two people have said opposite things and only a human settles it. +# `state_reason` is the one cause the API states outright — and it is +# nullable, so a null is read as "no objection recorded", i.e. completed. +# closed by an EARLIER run of this job -> post nothing, it is already there. +# The API cannot tell this from the next case: `closed_by` is a login and +# every seat in this org shares one identity. The per-PR comment marker is +# the only evidence, so the backlink now carries one on BOTH paths — a +# close performed by run 1 must be recognisable to run 2. +# closed BY HAND before the merge -> leave the backlink, change nothing else. +# This is the ordinary sequence and the reason the file exists. +# +# The marker is per-PR and therefore STABLE across runs, which is what fixes +# the degradation direction when the comment listing cannot be read: skip and +# say so (at-most-once). Posting blind would strand a permanent second backlink +# on another repo's closed issue, which nobody here can tidy up, and a miss is +# a convenience link that a re-run recovers. merge-queue-triage.yml reaches the +# OPPOSITE conclusion from the same trade because its marker carries a run id. +# # Failing is deliberate on BOTH exits, and it is the opposite of what # docs-drift-check.yml (#9373) chose for its advisory comment. The difference is # a property of this job, measured rather than inherited — and re-measured for @@ -278,21 +307,113 @@ jobs: // already stated the requirement; only the code was missing. const failures = []; + // The backlink this PR leaves, identified so a SECOND run can see + // it. The marker is scoped to one pull request and is therefore + // STABLE across runs — one PR leaves one backlink, however many + // times the job runs. That is deliberate, and it decides the + // degradation direction below: merge-queue-triage.yml's marker + // carries a run id, so a duplicate there is self-evidently one + // comment per run and posting blind is safe; a stable marker + // strands a permanent second copy instead, which is the reason + // docs-drift-check.yml (#9423) chose the opposite. This one is in + // the second class. + const backlinkMarker = + ``; + const backlink = (lead) => + `${lead}\n\n` + + `（跨仓库的关闭关键字不会自动生效，本条由 \`cross-repo-issue-closer\` 工作流代为收口。）\n\n` + + `---\n_Generated by [Claude Code](https://claude.ai/code)_\n\n${backlinkMarker}`; + for (const [key, t] of targets) { + // What a refusal from here costs, for the verdict after the loop. + // Until the issue has been read there is no way to know it is + // already closed, so a refusal costs both halves. + let lost = 'close and backlink'; try { const { data: issue } = await github.rest.issues.get({ owner: t.owner, repo: t.repo, issue_number: t.number, }); + + // "Already closed" is NOT one situation, and the three causes + // that reach here do not deserve the same treatment (#9643). + // Until this split existed all three were skipped whole, which + // dropped the backlink — the half of the defect the header + // names — on every one of them, and reported green. if (issue.state === 'closed') { - core.info(`${key} is already closed — skipping.`); + // (a) Closed as not-planned or as a duplicate. A comment + // saying this PR fixed it would CONTRADICT the triage + // decision on the issue, so nothing is posted. Two people + // have said opposite things and only a human can settle + // it, hence an annotation rather than a silent skip. + // `state_reason` is the only cause the API states + // outright; it is nullable (a real closed issue answers + // `null` here), and null is deliberately read as "no + // objection recorded", i.e. as `completed`. + if (issue.state_reason === 'not_planned' || issue.state_reason === 'duplicate') { + core.warning( + `${key} is closed as \`${issue.state_reason}\`, but this PR's body declares it FIXED. ` + + `No backlink was left: a comment saying ${prUrl} closed it would contradict that ` + + `triage decision. Someone has to decide which of the two is right.`, + { title: 'Cross-repo close contradicts the issue triage' }, + ); + continue; + } + + // (b) Closed by an EARLIER run of this job — a re-run, which + // this workflow's own job summary invites. The marker is + // the only evidence that separates it from (c): the API + // cannot, because `closed_by` is a login and every seat + // here shares one identity, so "the workflow did it" and + // "a human did it" read the same. + let alreadyLinked = false; + try { + // One page is the bound. Overflowing it costs a DUPLICATE + // backlink, never a lost one, and the stable marker makes + // the pair self-evident. + const existing = await github.rest.issues.listComments({ + owner: t.owner, repo: t.repo, issue_number: t.number, per_page: 100, + }); + alreadyLinked = existing.data.some((c) => (c.body ?? '').includes(backlinkMarker)); + } catch (error) { + // AT-MOST-ONCE, stated: the marker is stable, so posting + // without knowing strands a second permanent backlink on + // somebody ELSE's repo, which nobody here can tidy up. A + // miss costs a convenience link on an already-closed issue + // and a re-run recovers it. Loud either way — this is a + // skip with a reason, not a silent one. + core.warning( + `${key} is already closed, but its comments could not be read ` + + `(${describe(error)}), so this run could not tell whether ${prUrl} is already ` + + `linked there. No backlink was posted — re-run this job to leave it.`, + { title: 'Cross-repo backlink skipped, not confirmed' }, + ); + continue; + } + if (alreadyLinked) { + core.info(`${key} is already closed and already carries this PR's backlink — nothing to do.`); + continue; + } + + // (c) Closed by a human before this PR merged — the ordinary + // sequence the header is about (v17 verification, #4482). + // The close is genuinely redundant; the backlink is the + // whole remaining value, so leave it and touch nothing + // else. + lost = 'backlink'; + await github.rest.issues.createComment({ + owner: t.owner, repo: t.repo, issue_number: t.number, + body: backlink( + `本 issue 在本次合并之前就已经是 closed 状态，这里只补上修复它的 PR：` + + `${thisRepo} 的 ${prUrl}。本工作流没有改动它的状态。`, + ), + }); + core.info(`${key} was already closed — left the backlink to ${prUrl}, did not re-close it.`); continue; } + await github.rest.issues.createComment({ owner: t.owner, repo: t.repo, issue_number: t.number, - body: - `已由 ${thisRepo} 的 ${prUrl} 修复并合并。\n\n` + - `（跨仓库的关闭关键字不会自动生效，本条由 \`cross-repo-issue-closer\` 工作流代为收口。）\n\n` + - `---\n_Generated by [Claude Code](https://claude.ai/code)_`, + body: backlink(`已由 ${thisRepo} 的 ${prUrl} 修复并合并。`), }); await github.rest.issues.update({ owner: t.owner, repo: t.repo, issue_number: t.number, @@ -305,15 +426,15 @@ jobs: // first: the loop continues. The failure is RECORDED instead of // dropped, and the second half is passed below. const reason = describe(error); - failures.push({ key, reason }); - core.warning(`Could not close ${key}: ${reason}`, { - title: 'Cross-repo issue left open', + failures.push({ key, reason, lost }); + core.warning(`Could not finish ${key} — ${lost} not delivered: ${reason}`, { + title: 'Cross-repo issue left unfinished', }); } } if (failures.length === 0) { - core.info(`All ${targets.size} cross-repo target(s) closed or already closed.`); + core.info(`All ${targets.size} cross-repo target(s) handled: closed, or already closed and linked.`); return; } @@ -322,17 +443,17 @@ jobs: // full list a human needs, `setFailed` carries the conclusion that // makes anyone open the run at all. const failedKeys = failures.map((f) => f.key).join(', '); - const failedList = failures.map((f) => `- \`${f.key}\` —— ${f.reason}`).join('\n'); + const failedList = failures.map((f) => `- \`${f.key}\` —— ${f.lost} 未完成：${f.reason}`).join('\n'); try { await core.summary.addRaw([ - '## ⚠️ 跨仓库 issue 没能自动关闭', + '## ⚠️ 跨仓库 issue 没能收口', '', - `本次合并声明了 ${targets.size} 个跨仓库关闭目标，其中 ${failures.length} 个被拒绝，仍是 open：`, + `本次合并声明了 ${targets.size} 个跨仓库关闭目标，其中 ${failures.length} 个没有完成：`, '', failedList, '', - `- 修复它们的是 ${prUrl} —— 需要手工关闭，并把这条链接留在目标 issue 上。`, - '- 原因排除后可以直接 re-run 本 job：已经关闭的目标会被跳过，不会重复评论。', + `- 修复它们的是 ${prUrl} —— 需要手工收口：还是 open 的要关掉，已经关掉的要把这条链接留在上面。`, + '- 原因排除后可以直接 re-run 本 job：已经关闭的目标不会被重复关闭，已经留下过本 PR 反链的目标也不会被重复评论。', '- 401/404 通常意味着 `CROSS_REPO_ISSUE_TOKEN` 对目标仓库没有 `issues: write`，而不是目标不存在 —— GitHub 对无权访问的仓库回 404。', '', ].join('\n')).write(); @@ -343,7 +464,8 @@ jobs: core.info(`Could not write the job summary: ${summaryError.message}`); } core.setFailed( - `${failures.length} of ${targets.size} cross-repo issue(s) could NOT be closed by this merge and ` - + `are still open: ${failedKeys}. Close them by hand (fixed by ${prUrl}), or re-run this job once ` - + `the cause is cleared. The full list with reasons is in this run's job summary.`, + `${failures.length} of ${targets.size} cross-repo target(s) were NOT finished by this merge: ` + + `${failedKeys}. Finish them by hand (fixed by ${prUrl}) — an open one needs closing, an ` + + `already-closed one needs this PR's link on it — or re-run this job once the cause is ` + + `cleared. The full list, with what each one is missing, is in this run's job summary.`, ); diff --git a/scripts/check-cross-repo-closer-outcome.mjs b/scripts/check-cross-repo-closer-outcome.mjs index 1278870d23..40f7674714 100644 --- a/scripts/check-cross-repo-closer-outcome.mjs +++ b/scripts/check-cross-repo-closer-outcome.mjs @@ -19,6 +19,12 @@ // this repo maintains for the occasion it exists for has, in that window, // never once had a target to close. // +// Re-measured for #9643 later the same day over the 1176 most recently UPDATED +// merged PRs (a different window of the same size, 14 qualified same-repo +// references in it): still **zero** foreign. Two independent windows agree, +// which is why the coverage in this file matters more than the behaviour it +// pins -- reading is the only thing that has ever found a defect here. +// // Two consequences, and this file answers both: // // 1. The defects in it are found by reading, one card at a time -- #9575 for @@ -54,6 +60,17 @@ // which of `core.setFailed` / `core.warning` / a job summary fires, and which // API calls were made. Those are the properties both cards are about. // +// Also asserted, since #9643: that every API the script calls is one this +// harness MODELS. That is not pedantry -- it is the failure this file walked +// into. Adding `listComments` to the shipped script moved real behaviour and +// all 52 assertions stayed green, because the unstubbed method threw a +// `TypeError` INSIDE the script's own `try` and was absorbed by the very +// degradation branch the new code had just added. A harness whose stubs lag +// the script does not under-report; it reports a pass about a path the script +// no longer takes. So the guard records the access out-of-band and `judge` +// fails the scenario on the record, whatever that scenario's own assertions +// say. +// // NOT asserted: the step's `retries:` / `retry-exempt-status-codes:` inputs. // They are consumed by the ACTION, not by the script, so no stub of `github` // can exercise them; their acceptance on the pinned action version is evidenced @@ -69,11 +86,19 @@ // one and green forever, including the day someone deletes the thing it // guards. `--self-test` mutates the extracted source -- drop the `setFailed`, // stop collecting failed keys, `break` out of the loop instead of isolating, -// disable the same-repo skip, narrow the keyword set, disable the -// already-closed skip -- and requires the battery to go RED for each, naming -// the scenario it expects. Each mutation also asserts its own anchor was -// PRESENT before substituting: a mutation that silently matched nothing would -// leave the battery green and read exactly like a passing self-test. +// disable the same-repo skip, narrow the keyword set, collapse the +// already-closed branch, strip the backlink marker, drop the triage guard, +// post blind when the comment listing is refused, stop recording which half +// was lost -- and requires the battery to go RED for each, naming the scenario +// it expects. Each mutation also asserts its own anchor was PRESENT before +// substituting: a mutation that silently matched nothing would leave the +// battery green and read exactly like a passing self-test. +// +// One scenario (L11) is driven TWICE, the second run's world built out of the +// first run's calls. Re-run idempotency is a property of the PAIR, and a +// fixture that hand-writes the marker proves each half while leaving the two +// runs free to disagree about its spelling -- which is the only way the +// property can actually break. import { execFileSync } from 'node:child_process'; import { createRequire } from 'node:module'; @@ -145,56 +170,102 @@ function httpError(status, message) { return Object.assign(new Error(message), { status }); } +/** The `github.rest.issues.*` methods this harness models. See the `guard` in `makeDoubles`. */ +const MODELLED_ISSUE_METHODS = new Set(['get', 'createComment', 'update', 'listComments']); + /** * `github` / `context` / `core` doubles plus a call log. * - * `issues` maps `owner/repo#number` to `{ state }` or to a `{ throwOn }` - * instruction, so a scenario can refuse one specific target and leave the rest - * reachable -- which is the whole point of the isolation the loop must keep. + * `issues` maps `owner/repo#number` to a fixture: `{ state, stateReason, + * comments }` describes the issue, and `{ getError, listError, commentError, + * updateError }` refuses one specific call on it -- so a scenario can break one + * target and leave the rest reachable, which is the whole point of the + * isolation the loop must keep. */ function makeDoubles({ body, token, issues = {}, prCommentError = null, summaryError = null }) { - const calls = { get: [], comment: [], update: [], prComment: [] }; + const calls = { get: [], list: [], comment: [], update: [], prComment: [] }; const log = { info: [], warning: [], failed: [], summary: [] }; + const unstubbedCalls = []; let summaryBuffer = []; const target = (o, r, n) => issues[`${o}/${r}#${n}`] ?? {}; - const github = { - rest: { - issues: { - async get({ owner, repo, issue_number: n }) { - calls.get.push(`${owner}/${repo}#${n}`); - const t = target(owner, repo, n); - if (t.getError) throw t.getError; - return { data: { state: t.state ?? 'open' } }; - }, - async createComment({ owner, repo, issue_number: n, body: text }) { - const key = `${owner}/${repo}#${n}`; - const isThisPr = owner === THIS_REPO.owner && repo === THIS_REPO.repo; - if (isThisPr && prCommentError) { - calls.prComment.push({ key, attempted: true, delivered: false }); - throw prCommentError; - } - if (isThisPr) { - calls.prComment.push({ key, attempted: true, delivered: true, body: text }); - return { data: {} }; - } - const t = target(owner, repo, n); - if (t.commentError) throw t.commentError; - calls.comment.push({ key, body: text }); - return { data: {} }; - }, - async update({ owner, repo, issue_number: n, state, state_reason: reason }) { - const key = `${owner}/${repo}#${n}`; - const t = target(owner, repo, n); - if (t.updateError) throw t.updateError; - calls.update.push({ key, state, reason }); - return { data: {} }; - }, - }, + const issuesApi = { + async get({ owner, repo, issue_number: n }) { + calls.get.push(`${owner}/${repo}#${n}`); + const t = target(owner, repo, n); + if (t.getError) throw t.getError; + // `state_reason` is nullable on a real closed issue -- objectui#4478 + // answers `null` from this very endpoint -- so the default models that + // rather than inventing a value the API does not promise. + return { data: { state: t.state ?? 'open', state_reason: t.stateReason ?? null } }; + }, + async listComments({ owner, repo, issue_number: n }) { + const key = `${owner}/${repo}#${n}`; + calls.list.push(key); + const t = target(owner, repo, n); + if (t.listError) throw t.listError; + return { data: (t.comments ?? []).map((text) => ({ body: text })) }; + }, + async createComment({ owner, repo, issue_number: n, body: text }) { + const key = `${owner}/${repo}#${n}`; + const isThisPr = owner === THIS_REPO.owner && repo === THIS_REPO.repo; + if (isThisPr && prCommentError) { + calls.prComment.push({ key, attempted: true, delivered: false }); + throw prCommentError; + } + if (isThisPr) { + calls.prComment.push({ key, attempted: true, delivered: true, body: text }); + return { data: {} }; + } + const t = target(owner, repo, n); + if (t.commentError) throw t.commentError; + calls.comment.push({ key, body: text }); + return { data: {} }; + }, + async update({ owner, repo, issue_number: n, state, state_reason: reason }) { + const key = `${owner}/${repo}#${n}`; + const t = target(owner, repo, n); + if (t.updateError) throw t.updateError; + calls.update.push({ key, state, reason }); + return { data: {} }; }, }; + // An unmodelled API call must be LOUD, and a throw alone is not loud enough + // here: every call in the loop sits inside the script's own `try`, so a + // `TypeError` from an unstubbed method is caught by the script and absorbed + // into whichever degradation that `catch` implements -- leaving the battery + // green over a behaviour it never exercised. Measured, on the #9643 change + // itself: adding `listComments` to the shipped script moved real behaviour + // and all 52 assertions stayed green, because the missing stub landed in the + // script's own "could not read the comments" branch. So the access is + // RECORDED as well as thrown, and `judge` fails the scenario on the record. + // + // The guard is applied at every level of `github`, not just to the issues + // methods: `github.paginate`, `github.request` and `github.graphql` are all + // reachable from a github-script body and all three would otherwise be + // `undefined`, i.e. the identical silent-absorption bug one level up. + const guard = (impl, path, allowed) => + new Proxy(impl, { + get(t, prop, receiver) { + if (typeof prop === 'string' && !allowed.has(prop)) { + unstubbedCalls.push(`${path}.${prop}`); + throw new Error( + `${SELF}: the shipped script now uses \`${path}.${prop}\`, which this harness does not ` + + 'stub. Model it here rather than letting a scenario absorb it.', + ); + } + return Reflect.get(t, prop, receiver); + }, + }); + + const github = guard( + { rest: guard({ issues: guard(issuesApi, 'github.rest.issues', MODELLED_ISSUE_METHODS) }, 'github.rest', new Set(['issues'])) }, + 'github', + new Set(['rest']), + ); + const summary = { addRaw(text) { summaryBuffer.push(text); @@ -226,15 +297,22 @@ function makeDoubles({ body, token, issues = {}, prCommentError = null, summaryE }, }; - return { github, context, core, calls, log, token }; + return { github, context, core, calls, log, token, unstubbedCalls }; } -/** Anything github-script hands the script that this harness does not model. */ -function unstubbed(name) { +/** + * Anything github-script hands the script that this harness does not model. + * + * Records into the same sink as the `github` guard before throwing, for the + * same reason: a throw raised inside the script's own `try` is caught by the + * script, and a scenario then reports a pass about a degradation path. + */ +function unstubbed(name, sink) { return new Proxy( {}, { - get() { + get(_t, prop) { + sink.push(typeof prop === 'string' ? `${name}.${prop}` : name); throw new Error( `${SELF}: the shipped script now uses \`${name}\`, which this harness does not stub. ` + 'Model it here rather than deleting the assertion that found it.', @@ -254,10 +332,10 @@ async function runScript(source, scenario) { github: doubles.github, context: doubles.context, core: doubles.core, - exec: unstubbed('exec'), - glob: unstubbed('glob'), - io: unstubbed('io'), - fetch: unstubbed('fetch'), + exec: unstubbed('exec', doubles.unstubbedCalls), + glob: unstubbed('glob', doubles.unstubbedCalls), + io: unstubbed('io', doubles.unstubbedCalls), + fetch: unstubbed('fetch', doubles.unstubbedCalls), require: createRequire(import.meta.url), __original_require__: createRequire(import.meta.url), }; @@ -304,6 +382,23 @@ const MIXED_BODY = [ const FOREIGN = ['objectstack-ai/objectui#456', 'my-org/some.repo#22', 'third/party#7']; +/** The target the already-closed scenarios (L2, L6-L9) work on. */ +const CLOSED_TARGET = 'objectstack-ai/objectui#456'; +const PR_URL = 'https://github.com/objectstack-ai/objectstack/pull/1234'; + +/** + * The backlink marker the shipped script is expected to write, spelled out. + * + * It is pinned as a LITERAL rather than recomputed from the script, because + * that is what makes the pair of assertions mean something: L1 asserts the + * script writes this exact string on the close path, and L6 seeds this exact + * string as an existing comment to prove the re-run path recognises it. Change + * the marker's shape in the workflow and L1 goes red -- which is the honest + * failure, because the two runs of a re-run must agree on one spelling and a + * harness that derived it from the script could never catch them disagreeing. + */ +const MARKER = ''; + /** * Each scenario names the exit path it walks and asserts the OUTCOME of it. * `check` returns an array of failure strings. @@ -387,25 +482,138 @@ export const SCENARIOS = [ 'L1 closes as `completed`, not as `not_planned`', ), t( - r.calls.comment.every((c) => c.body.includes('https://github.com/objectstack-ai/objectstack/pull/1234')), + r.calls.comment.every((c) => c.body.includes(PR_URL)), 'L1 comment carries the PR link -- the backlink is half the point of the workflow', ), + t( + r.calls.comment.every((c) => c.body.includes(MARKER)), + 'L1 comment carries the per-PR marker, so a LATER run can tell this backlink is already there (#9643)', + ), t(r.log.warning.length === 0, 'L1 warns about nothing'), ], }, { id: 'L2', - name: 'a target that is already closed is skipped, not re-commented (re-run safety)', + name: 'already closed by a human -- the backlink still lands, the close does not (#9643)', + scenario: () => ({ + body: MIXED_BODY, + token: 'pat', + // Closed, `completed`, and carrying no backlink from this PR: the + // ordinary sequence the file's header is about -- the fix shipped, + // somebody tidied the tracker, then the merge happened. + issues: { [CLOSED_TARGET]: { state: 'closed', stateReason: 'completed', comments: [] } }, + }), + check: (r, t) => { + const posted = r.calls.comment.find((c) => c.key === CLOSED_TARGET); + return [ + t(r.log.failed.length === 0, `L2 stays green, got setFailed: ${r.log.failed[0]}`), + t(r.calls.list.includes(CLOSED_TARGET), 'L2 reads the comments before posting -- the re-run guard'), + t(Boolean(posted), 'L2 LEAVES THE BACKLINK on the already-closed issue -- the defect this card is about'), + t((posted?.body ?? '').includes(PR_URL), 'L2 backlink names the PR that fixed it'), + t((posted?.body ?? '').includes(MARKER), 'L2 backlink carries the marker, so a re-run recognises it'), + t( + !r.calls.update.some((u) => u.key === CLOSED_TARGET), + 'L2 does NOT re-close it -- the close is the half that really was redundant', + ), + t(r.calls.update.length === 2, `L2 still closes the other two, got ${r.calls.update.length}`), + t(r.log.warning.length === 0, 'L2 warns about nothing -- leaving a backlink is the normal outcome, not a degradation'), + ]; + }, + }, + { + id: 'L6', + name: 'already closed AND already linked (a re-run) -- nothing is posted twice (#9643)', + scenario: () => ({ + body: MIXED_BODY, + token: 'pat', + issues: { + [CLOSED_TARGET]: { + state: 'closed', + stateReason: 'completed', + // What an earlier run of THIS job left behind. The marker is the only + // thing that separates this case from L2: `closed_by` is a login and + // every seat in this org shares one identity. + comments: ['已由 objectstack-ai/objectstack 的 ' + PR_URL + ' 修复并合并。\n\n' + MARKER], + }, + }, + }), + check: (r, t) => [ + t(r.log.failed.length === 0, 'L6 stays green'), + t(r.calls.list.includes(CLOSED_TARGET), 'L6 reads the comments'), + t( + !r.calls.comment.some((c) => c.key === CLOSED_TARGET), + 'L6 posts NO second backlink -- re-running this job must be idempotent (#9643 H2)', + ), + t(!r.calls.update.some((u) => u.key === CLOSED_TARGET), 'L6 does not re-close it either'), + t(r.calls.update.length === 2, `L6 still closes the other two, got ${r.calls.update.length}`), + ], + }, + { + id: 'L7', + name: 'already closed as not_planned -- no backlink, because it would contradict the triage (#9643)', + scenario: () => ({ + body: MIXED_BODY, + token: 'pat', + issues: { [CLOSED_TARGET]: { state: 'closed', stateReason: 'not_planned', comments: [] } }, + }), + check: (r, t) => [ + t(r.log.failed.length === 0, 'L7 stays green -- a disagreement between two humans is not a job failure'), + t( + !r.calls.comment.some((c) => c.key === CLOSED_TARGET), + 'L7 posts NO backlink -- a comment saying this PR fixed a not_planned issue would be actively wrong', + ), + t(!r.calls.update.some((u) => u.key === CLOSED_TARGET), 'L7 does not reopen or re-close it'), + t( + !r.calls.list.includes(CLOSED_TARGET), + 'L7 does not even list the comments -- the triage reason settles it before idempotency matters', + ), + t( + r.log.warning.some((w) => w.message.includes(CLOSED_TARGET) && /not_planned/.test(w.message)), + 'L7 ANNOUNCES the contradiction rather than skipping silently -- only a human can settle it', + ), + t(r.calls.update.length === 2, `L7 still closes the other two, got ${r.calls.update.length}`), + ], + }, + { + id: 'L8', + name: 'already closed but the comment listing is refused -- skip AT MOST ONCE, and say so (#9643)', + scenario: () => ({ + body: MIXED_BODY, + token: 'pat', + issues: { [CLOSED_TARGET]: { state: 'closed', stateReason: 'completed', listError: FIXED } }, + }), + check: (r, t) => [ + t(r.log.failed.length === 0, `L8 stays green, got setFailed: ${r.log.failed[0]}`), + t( + !r.calls.comment.some((c) => c.key === CLOSED_TARGET), + 'L8 does NOT post blind -- the marker is stable, so a blind post strands a permanent duplicate ' + + "on another repo's closed issue (merge-queue-triage.yml chooses the opposite, from a per-RUN marker)", + ), + t( + r.log.warning.some((w) => w.message.includes(CLOSED_TARGET) && /HTTP 503/.test(w.message)), + 'L8 names the target AND the refusal -- a skip with a stated reason, not a silent one', + ), + t(r.calls.update.length === 2, `L8 still closes the other two -- one bad listing isolates, got ${r.calls.update.length}`), + ], + }, + { + id: 'L9', + name: 'already closed with a NULL state_reason -- read as completed, so the backlink lands (#9643)', scenario: () => ({ body: MIXED_BODY, token: 'pat', - issues: { 'objectstack-ai/objectui#456': { state: 'closed' } }, + // Measured, not invented: objectstack-ai/objectui#4478 is closed and + // answers `state_reason: null` from `issues.get`. A fix that keys on the + // reason has to say what null means, and this pins the answer. + issues: { [CLOSED_TARGET]: { state: 'closed', stateReason: null, comments: [] } }, }), check: (r, t) => [ - t(r.log.failed.length === 0, 'L2 stays green'), - t(!r.calls.comment.some((c) => c.key === 'objectstack-ai/objectui#456'), 'L2 does not re-comment on a closed issue'), - t(!r.calls.update.some((u) => u.key === 'objectstack-ai/objectui#456'), 'L2 does not re-close a closed issue'), - t(r.calls.update.length === 2, `L2 still closes the other two, got ${r.calls.update.length}`), + t(r.log.failed.length === 0, 'L9 stays green'), + t( + r.calls.comment.some((c) => c.key === CLOSED_TARGET), + 'L9 treats a null reason as "no objection recorded" and leaves the backlink', + ), + t(!r.calls.update.some((u) => u.key === CLOSED_TARGET), 'L9 still does not re-close it'), ], }, { @@ -467,6 +675,61 @@ export const SCENARIOS = [ ), ], }, + { + id: 'L10', + name: 'the BACKLINK is refused on an already-closed target -- red, and the verdict says which half was lost', + scenario: () => ({ + body: MIXED_BODY, + token: 'pat', + issues: { + [CLOSED_TARGET]: { state: 'closed', stateReason: 'completed', comments: [], commentError: DENIED }, + }, + }), + check: (r, t) => [ + t(r.log.failed.length === 1, `L10 fails the job -- half the deliverable was lost; got ${r.log.failed.length}`), + t((r.log.failed[0] ?? '').includes(CLOSED_TARGET), 'L10 setFailed names the target'), + t(r.calls.update.length === 2, `L10 still closes the other two, got ${r.calls.update.length}`), + t( + /backlink/.test(r.log.summary[0] ?? ''), + 'L10 summary says the BACKLINK was lost, not the close -- reporting "still open" about a closed ' + + 'issue sends the reader to do the one thing that is already done', + ), + t( + !/close and backlink/.test(r.log.summary[0] ?? ''), + 'L10 does not claim the close was lost too -- the issue was already closed before this run', + ), + t(r.threw === null, 'L10 does not let the refusal escape the script'), + ], + }, + { + id: 'L11', + name: 'the ROUND TRIP: re-running the job after a clean close posts no second backlink (#9643 H2)', + scenario: () => ({ body: MIXED_BODY, token: 'pat' }), + // Run 2 sees exactly the world run 1 left behind: every target closed, each + // carrying the very comment run 1 posted. Nothing is hand-written -- the + // marker travels from run 1's OUTPUT into run 2's INPUT, so this is the one + // assertion that fails if the two runs ever disagree on its spelling. + rerun: (first) => ({ + body: MIXED_BODY, + token: 'pat', + issues: Object.fromEntries( + first.calls.comment.map((c) => [c.key, { state: 'closed', stateReason: 'completed', comments: [c.body] }]), + ), + }), + check: (r, t, first) => [ + t(first.calls.comment.length === 3, `L11 run 1 comments on all three, got ${first.calls.comment.length}`), + t(first.calls.update.length === 3, `L11 run 1 closes all three, got ${first.calls.update.length}`), + t(r.calls.list.length === 3, `L11 run 2 checks all three for an existing backlink, got ${r.calls.list.length}`), + t( + r.calls.comment.length === 0, + `L11 run 2 posts NOTHING -- \`pull_request_target: [closed]\` can be replayed by a re-run, and this ` + + `workflow's own job summary tells people to do exactly that; got ${r.calls.comment.length} duplicate(s)`, + ), + t(r.calls.update.length === 0, `L11 run 2 re-closes nothing, got ${r.calls.update.length}`), + t(r.log.failed.length === 0, `L11 run 2 stays green, got setFailed: ${r.log.failed[0]}`), + t(r.log.warning.length === 0, 'L11 run 2 warns about nothing -- an idempotent re-run is a normal outcome'), + ], + }, ]; // ── The battery ───────────────────────────────────────────────────────────── @@ -497,13 +760,35 @@ export async function judge(source) { return cond ? null : { id: s.id, message }; }; let result; + let first = null; try { result = await runScript(source, s.scenario()); + // A scenario with `rerun` is driven TWICE, the second run's world built + // from the first run's output. That is the only way to prove the two runs + // agree on one marker spelling -- a fixture that hand-writes the marker + // proves each half separately and the round trip not at all. + if (typeof s.rerun === 'function') { + first = result; + result = await runScript(source, s.rerun(first)); + } } catch (err) { failures.push({ id: s.id, message: `the harness itself threw -- ${err.message}` }); continue; } - for (const f of s.check(result, t)) if (f) failures.push(f); + // Checked for EVERY scenario, ahead of its own assertions: an API the + // harness does not model is swallowed by the script's own `catch` and + // reported as a degradation, so no scenario assertion can be trusted to + // notice it. See the guard in `makeDoubles`. + for (const name of new Set([...(first?.unstubbedCalls ?? []), ...result.unstubbedCalls])) { + checked++; + failures.push({ + id: s.id, + message: `the shipped script called \`${name}()\`, which this harness does not model -- ` + + "the script's own catch absorbed it, so this scenario verified a degradation path rather " + + 'than the behaviour it names. Model the call in `makeDoubles`.', + }); + } + for (const f of s.check(result, t, first)) if (f) failures.push(f); } return { failures, checked }; @@ -572,15 +857,15 @@ const MUTATIONS = [ { id: 'M2', what: 'the loop stops collecting the keys it could not close', - from: 'failures.push({ key, reason });', + from: 'failures.push({ key, reason, lost });', to: '', expect: ['L3', 'L4', 'L5'], }, { id: 'M3', what: 'the loop stops isolating and breaks out on the first refusal', - from: 'failures.push({ key, reason });', - to: 'failures.push({ key, reason }); break;', + from: 'failures.push({ key, reason, lost });', + to: 'failures.push({ key, reason, lost }); break;', expect: ['L3'], }, { @@ -599,10 +884,43 @@ const MUTATIONS = [ }, { id: 'M6', - what: 'the already-closed skip is removed, so a re-run re-comments', + what: 'the already-closed branch is removed, so a closed issue is commented on AND re-closed', from: "if (issue.state === 'closed') {", to: 'if (false) {', - expect: ['L2'], + // L2/L9 lose the "does not re-close it" half; L6 re-comments on a target + // that already carries the backlink; L7 posts a "fixed by" comment on a + // not_planned issue; L8 posts blind. Every already-closed scenario. + expect: ['L2', 'L6', 'L7', 'L8', 'L9'], + }, + { + id: 'M8', + what: 'the backlink loses its per-PR marker, so a re-run cannot tell its own comment is already there', + from: '---\\n_Generated by [Claude Code](https://claude.ai/code)_\\n\\n${backlinkMarker}', + to: '---\\n_Generated by [Claude Code](https://claude.ai/code)_', + // L1 and L2 catch it on the comments the script POSTS; L11 catches the + // consequence, by feeding run 1's output into run 2 and finding a duplicate. + expect: ['L1', 'L2', 'L11'], + }, + { + id: 'M9', + what: 'the triage guard is dropped, so a not_planned issue gets a comment claiming this PR fixed it', + from: "if (issue.state_reason === 'not_planned' || issue.state_reason === 'duplicate') {", + to: 'if (false) {', + expect: ['L7'], + }, + { + id: 'M10', + what: 'an unreadable comment listing degrades to posting BLIND instead of skipping (a stranded duplicate)', + from: '{ title: \'Cross-repo backlink skipped, not confirmed\' },\n );\n continue;', + to: "{ title: 'Cross-repo backlink skipped, not confirmed' },\n );", + expect: ['L8'], + }, + { + id: 'M11', + what: 'the loop stops recording WHICH half it lost, so the verdict cannot tell a lost close from a lost backlink', + from: 'failures.push({ key, reason, lost });', + to: 'failures.push({ key, reason });', + expect: ['L10'], }, { id: 'M7',