From 1a816458ddd96247b5c3492faba7bec55cbc1150 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 16:09:32 +0000 Subject: [PATCH 1/2] fix(ci): an already-closed cross-repo issue gets the PR backlink instead of being skipped whole The per-target loop skipped an already-closed issue entirely, and the skip covered two calls of which only one is redundant. Re-closing a closed issue is a no-op worth avoiding; dropping the BACKLINK is the half this workflow's own header calls the defect it exists to fix -- "no reference to the PR on the issue's own page either, so the next reader has no way to find the fix". "Already closed" is three situations and the API can separate the ones that matter: not_planned / duplicate -> post nothing, annotate. A comment claiming this PR fixed it contradicts the triage decision recorded on the issue. Measured: 26 of 144 recently closed issues in this repo carry one of those reasons, so this is not a theoretical branch. `state_reason` is nullable in practice (objectui#4478 answers null from `issues.get`), and null is read as "no objection recorded". closed by an earlier run of this job -> post nothing, it is already there. The API cannot tell this from a human close -- `closed_by` is a login and every seat here shares one identity -- so the backlink now carries a per-PR marker on both paths and the loop looks for it. closed by hand before the merge -> leave the backlink, change nothing else. Idempotency was a prerequisite rather than a nicety: there was none on the comment path, and the skip WAS the re-run guard -- the job summary told people "already-closed targets are skipped, so they are not re-commented". That sentence is now true by construction instead of by side effect. The marker is per-PR and therefore stable across runs, which 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. merge-queue-triage.yml reaches the opposite conclusion from the same trade because its marker carries a run id. The post-loop verdict no longer reports every failure as "could NOT be closed and is still open" -- a lost backlink on an already-closed issue sends the reader to do the one thing already done. Each failure records which half it lost. Harness (#9645): L2 is rewritten rather than deleted, plus L6-L11 for the re-run, the triage contradiction, the unreadable listing, the null reason, the backlink-refused verdict, and a two-run ROUND TRIP that feeds run 1's output into run 2. 52 assertions over 10 scenarios -> 88 over 16; 7 mutations -> 11. The harness also grew a guard it needed: adding `listComments` moved real behaviour and all 52 assertions stayed GREEN, because the unstubbed method threw inside the script's own try and was absorbed by the degradation branch the new code had just added. Unmodelled API calls are now recorded out-of-band and fail the scenario regardless of its own assertions. Refs #9595, #9575, #4482 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja --- .github/workflows/cross-repo-issue-closer.yml | 156 ++++++- scripts/check-cross-repo-closer-outcome.mjs | 413 +++++++++++++++--- 2 files changed, 496 insertions(+), 73 deletions(-) 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..ed1f500e81 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,92 @@ function httpError(status, message) { return Object.assign(new Error(message), { status }); } +/** The `github.rest.issues.*` methods this harness models. See `ISSUES_GUARD`. */ +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. + const issuesGuard = new Proxy(issuesApi, { + get(t, prop, receiver) { + if (typeof prop === 'string' && !MODELLED_ISSUE_METHODS.has(prop)) { + unstubbedCalls.push(`github.rest.issues.${prop}`); + throw new Error( + `${SELF}: the shipped script now calls \`github.rest.issues.${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 = { rest: { issues: issuesGuard } }; + const summary = { addRaw(text) { summaryBuffer.push(text); @@ -226,7 +287,7 @@ 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. */ @@ -304,6 +365,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 +465,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: { 'objectstack-ai/objectui#456': { state: 'closed' } }, + issues: { [CLOSED_TARGET]: { state: 'closed', stateReason: 'not_planned', 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, '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', + // 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, '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 +658,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 +743,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 +840,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 +867,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', From f804ec290009b7261e68b3455f621f3614512608 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 16:12:50 +0000 Subject: [PATCH 2/2] test(ci): close the unmodelled-API blind spot at every level of the harness doubles The guard added with the backlink fix covered `github.rest.issues.*` only, which leaves the identical bug one level up: `github.paginate`, `github.request` and `github.graphql` are all reachable from a github-script body and were plain `undefined`, so a call to one throws a TypeError inside the script's own try and is absorbed by whatever degradation that catch implements. The top-level `exec`/`glob`/`io`/`fetch` stubs threw but recorded nothing, so the same absorption applied to them. Every level now records into one sink before throwing, and `judge` already fails the scenario on the record. Verified by mutating the shipped script to call `github.paginate(...)` and `github.rest.issues.addLabels(...)`: both are named by the failure text instead of vanishing into a degradation branch. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja --- scripts/check-cross-repo-closer-outcome.mjs | 59 +++++++++++++-------- 1 file changed, 38 insertions(+), 21 deletions(-) diff --git a/scripts/check-cross-repo-closer-outcome.mjs b/scripts/check-cross-repo-closer-outcome.mjs index ed1f500e81..40f7674714 100644 --- a/scripts/check-cross-repo-closer-outcome.mjs +++ b/scripts/check-cross-repo-closer-outcome.mjs @@ -170,7 +170,7 @@ function httpError(status, message) { return Object.assign(new Error(message), { status }); } -/** The `github.rest.issues.*` methods this harness models. See `ISSUES_GUARD`. */ +/** The `github.rest.issues.*` methods this harness models. See the `guard` in `makeDoubles`. */ const MODELLED_ISSUE_METHODS = new Set(['get', 'createComment', 'update', 'listComments']); /** @@ -241,20 +241,30 @@ function makeDoubles({ body, token, issues = {}, prCommentError = null, summaryE // 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. - const issuesGuard = new Proxy(issuesApi, { - get(t, prop, receiver) { - if (typeof prop === 'string' && !MODELLED_ISSUE_METHODS.has(prop)) { - unstubbedCalls.push(`github.rest.issues.${prop}`); - throw new Error( - `${SELF}: the shipped script now calls \`github.rest.issues.${prop}()\`, which this harness ` + - 'does not stub. Model it here rather than letting a scenario absorb it.', - ); - } - return Reflect.get(t, prop, receiver); - }, - }); + // + // 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 = { rest: { issues: issuesGuard } }; + 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) { @@ -290,12 +300,19 @@ function makeDoubles({ body, token, issues = {}, prCommentError = null, summaryE 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.', @@ -315,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), };