diff --git a/.github/workflows/cross-repo-issue-closer.yml b/.github/workflows/cross-repo-issue-closer.yml index 8cdea80bb8..fd162a10a5 100644 --- a/.github/workflows/cross-repo-issue-closer.yml +++ b/.github/workflows/cross-repo-issue-closer.yml @@ -307,6 +307,13 @@ jobs: // already stated the requirement; only the code was missing. const failures = []; + // Targets that are not issues AT ALL. Kept apart from `failures` + // above because the two need opposite remedies: a failure is an API + // refusal a re-run can clear, this one is a defect in the merged + // PR's own body that no re-run will ever change. Fusing them would + // send the reader off to re-run a job that is going to refuse again. + const malformed = []; + // 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 @@ -334,6 +341,47 @@ jobs: owner: t.owner, repo: t.repo, issue_number: t.number, }); + // A pull request is ALSO an issue to this endpoint. Every PR + // answers `GET /repos/{owner}/{repo}/issues/{N}` with `state`, + // `state_reason` and the rest, plus a `pull_request` key that + // nothing here used to read (#9711) — the key is ABSENT on a + // real issue, present as an object on a PR, so its truthiness + // is the whole test. The target regex takes any `owner/repo#N` + // and PR and issue numbers share one sequence, so a body saying + // `Fixes owner/repo#4500` where 4500 is a pull request would + // have made this loop comment on that PR and CLOSE it. + // + // GitHub's own closing-keyword parser — the behaviour this + // workflow exists to carry across repository boundaries — never + // closes a pull request; `Fixes #N` aimed at a PR leaves a + // reference and nothing else. So the keyword is a MALFORMED + // instruction, and it is refused OUT LOUD rather than skipped: + // this file's ten exit paths were arrived at by fixing one + // quiet one after another, and a quiet `continue` would have + // been the eleventh. The refusal is also the conservative half + // of an asymmetry — closing a pull request drops its + // merge-queue membership and any armed auto-merge in the same + // step, and neither comes back by itself. + // + // Placed BEFORE the `state === 'closed'` branch on purpose: a + // merged pull request reads `state: 'closed'` with + // `state_reason: null` from this very endpoint (measured on + // objectstack-ai/objectstack#9143), which that branch reads as + // "no objection recorded" and answers with a backlink comment. + // A guard sitting after it would still write on somebody else's + // pull request. + if (issue.pull_request) { + malformed.push({ key, url: issue.pull_request.html_url }); + core.warning( + `${key} is a PULL REQUEST, not an issue — ${prUrl} declares it FIXED, but GitHub's own ` + + `closing-keyword parser never closes a pull request, so this workflow does not either. ` + + `Nothing was commented on and nothing was closed. Re-running cannot fix a keyword that ` + + `can never fire: point the reference at the issue it meant.`, + { title: 'Cross-repo closing keyword names a pull request' }, + ); + continue; + } + // "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 @@ -433,7 +481,7 @@ jobs: } } - if (failures.length === 0) { + if (failures.length === 0 && malformed.length === 0) { core.info(`All ${targets.size} cross-repo target(s) handled: closed, or already closed and linked.`); return; } @@ -442,10 +490,23 @@ jobs: // notice branch uses, for the same reasons: the summary carries the // full list a human needs, `setFailed` carries the conclusion that // makes anyone open the run at all. + // + // Both classes are red, and each says what to do about ITSELF: a + // refusal is retried, a malformed reference is rewritten. Green + // with an annotation was the alternative for the malformed class, + // and this file's own history rejects it — 2334 runs, every one of + // them `run_attempt` 1 and 99 of the last 100 green, so a green run + // of this job has never been opened by anybody. An annotation on a + // green post-merge run is the silent path with extra steps, and a + // declared close that can never happen is exactly what #9595 made + // red for. const failedKeys = failures.map((f) => f.key).join(', '); const failedList = failures.map((f) => `- \`${f.key}\` —— ${f.lost} 未完成：${f.reason}`).join('\n'); - try { - await core.summary.addRaw([ + const malformedKeys = malformed.map((m) => m.key).join(', '); + const malformedList = malformed.map((m) => `- \`${m.key}\` —— 这个编号是一个 pull request：${m.url}`).join('\n'); + const summaryLines = []; + if (failures.length > 0) { + summaryLines.push( '## ⚠️ 跨仓库 issue 没能收口', '', `本次合并声明了 ${targets.size} 个跨仓库关闭目标，其中 ${failures.length} 个没有完成：`, @@ -456,16 +517,45 @@ jobs: '- 原因排除后可以直接 re-run 本 job：已经关闭的目标不会被重复关闭，已经留下过本 PR 反链的目标也不会被重复评论。', '- 401/404 通常意味着 `CROSS_REPO_ISSUE_TOKEN` 对目标仓库没有 `issues: write`，而不是目标不存在 —— GitHub 对无权访问的仓库回 404。', '', - ].join('\n')).write(); + ); + } + if (malformed.length > 0) { + summaryLines.push( + '## ⛔ 关闭关键字指向的是 pull request，不是 issue', + '', + `本次合并声明的 ${targets.size} 个跨仓库关闭目标里，有 ${malformed.length} 个的编号是 pull request：`, + '', + malformedList, + '', + '- 它们没有被评论，也没有被关闭。GitHub 自己的关闭关键字解析器从不关闭 pull request，本工作流也不会 —— 关掉一个 PR 会在同一步里丢掉它的 merge queue 成员资格和已经武装的 auto-merge，两者都不会自己回来。', + `- 这不是 re-run 能解决的故障：写错的是 ${prUrl} 的正文。把引用改成它真正想关的那个 issue 编号，或者手工关掉那个 issue。`, + '', + ); + } + try { + await core.summary.addRaw(summaryLines.join('\n')).write(); } catch (summaryError) { // Same asymmetry as the notice branch: the summary is the richer // channel, the conclusion the reliable one. Losing the richer one // must not restore the silence. core.info(`Could not write the job summary: ${summaryError.message}`); } - core.setFailed( - `${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.`, - ); + const verdict = []; + if (failures.length > 0) { + verdict.push( + `${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.`, + ); + } + if (malformed.length > 0) { + verdict.push( + `${malformed.length} of ${targets.size} cross-repo closing keyword(s) in this merged pull ` + + `request name a PULL REQUEST rather than an issue: ${malformedKeys}. GitHub's own keyword ` + + `parser never closes a pull request and this workflow does not either, so nothing was ` + + `commented on and nothing was closed. Re-running this job will refuse them again — what ` + + `has to change is the reference in ${prUrl}'s body.`, + ); + } + core.setFailed(verdict.join(' ')); diff --git a/scripts/check-cross-repo-closer-outcome.mjs b/scripts/check-cross-repo-closer-outcome.mjs index 40f7674714..f1a50cdf6d 100644 --- a/scripts/check-cross-repo-closer-outcome.mjs +++ b/scripts/check-cross-repo-closer-outcome.mjs @@ -56,7 +56,12 @@ // ## What is asserted, and what is deliberately not // // Asserted: the target parse (which keyword spellings qualify, which forms do -// not, that a same-repo reference is skipped), and the OUTCOME of every exit -- +// not, that a same-repo reference is skipped), the target KIND (#9711: a pull +// request is also an issue to `issues.get`, so `owner/repo#N` naming a PR +// reaches the loop like anything else -- the scenarios pin that it is REFUSED +// rather than closed, and that the refusal happens before the already-closed +// branch can leave a comment on somebody else's pull request), and the OUTCOME +// of every exit -- // which of `core.setFailed` / `core.warning` / a job summary fires, and which // API calls were made. Those are the properties both cards are about. // @@ -198,7 +203,21 @@ function makeDoubles({ body, token, issues = {}, prCommentError = null, summaryE // `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 } }; + // + // `pull_request` is how the same endpoint says the number is a PULL + // REQUEST, and it is modelled as ABSENCE rather than as `undefined` + // because that is what was measured: objectstack-ai/objectstack#9716 (a + // PR) answers `{ url, html_url, diff_url, patch_url, merged_at }`, and + // #9711 (an issue) carries no such key at all. A fixture that always + // spelled the key would let a truthiness test pass here that the real + // API would fail. (#9711) + return { + data: { + state: t.state ?? 'open', + state_reason: t.stateReason ?? null, + ...(t.pullRequest ? { pull_request: t.pullRequest } : {}), + }, + }; }, async listComments({ owner, repo, issue_number: n }) { const key = `${owner}/${repo}#${n}`; @@ -384,6 +403,27 @@ const FOREIGN = ['objectstack-ai/objectui#456', 'my-org/some.repo#22', 'third/pa /** The target the already-closed scenarios (L2, L6-L9) work on. */ const CLOSED_TARGET = 'objectstack-ai/objectui#456'; + +/** The target the PULL REQUEST scenarios (L12, L13) work on (#9711). */ +const PR_TARGET = 'my-org/some.repo#22'; + +/** + * The `pull_request` key GitHub returns when an issue number is a pull request. + * + * Copied from a real response rather than invented: `GET /repos/ + * objectstack-ai/objectstack/issues/9716` answers exactly these five fields, + * and `.../issues/9711` -- an issue -- answers with the key absent. The loop + * reads only its truthiness, but the shape is pinned so that a change in what + * the script reads out of it (`html_url` reaches the job summary) fails here + * instead of printing `undefined` into somebody's run. + */ +const PR_KEY = { + url: 'https://api.github.com/repos/my-org/some.repo/pulls/22', + html_url: 'https://github.com/my-org/some.repo/pull/22', + diff_url: 'https://github.com/my-org/some.repo/pull/22.diff', + patch_url: 'https://github.com/my-org/some.repo/pull/22.patch', + merged_at: null, +}; const PR_URL = 'https://github.com/objectstack-ai/objectstack/pull/1234'; /** @@ -730,6 +770,84 @@ export const SCENARIOS = [ t(r.log.warning.length === 0, 'L11 run 2 warns about nothing -- an idempotent re-run is a normal outcome'), ], }, + { + id: 'L12', + name: 'the target is an OPEN pull request -- refused out loud, never closed (#9711)', + scenario: () => ({ + body: MIXED_BODY, + token: 'pat', + issues: { [PR_TARGET]: { state: 'open', pullRequest: PR_KEY } }, + }), + check: (r, t) => [ + t(r.calls.get.includes(PR_TARGET), 'L12 reads the target -- the kind is only knowable from the response'), + t( + !r.calls.update.some((u) => u.key === PR_TARGET), + "L12 does NOT close the pull request -- GitHub's own keyword parser never closes one, and this " + + 'workflow exists to carry that behaviour across repos, not to exceed it. A closed PR also loses ' + + 'its merge-queue membership and any armed auto-merge in the same step, and neither returns by itself', + ), + t(!r.calls.comment.some((c) => c.key === PR_TARGET), 'L12 posts nothing on the foreign pull request either'), + t( + !r.calls.list.includes(PR_TARGET), + 'L12 does not even list its comments -- the target KIND settles it before idempotency can matter', + ), + t( + r.log.warning.some((w) => w.message.includes(PR_TARGET) && /PULL REQUEST/.test(w.message)), + 'L12 ANNOUNCES the refusal and names the target -- a quiet `continue` here would have been the ' + + "eleventh silent exit in a file whose ten known ones were each found by somebody reading it", + ), + t( + r.log.failed.length === 1, + `L12 fails the job: the merged body declares a close that can never fire, and this run is the only ` + + `thing that will ever know; got ${r.log.failed.length}`, + ), + t((r.log.failed[0] ?? '').includes(PR_TARGET), 'L12 setFailed names the malformed target'), + t( + !/re-run this job once the cause is cleared/.test(r.log.failed[0] ?? ''), + 'L12 verdict does not send the reader to re-run -- unlike every other red path here the refusal is ' + + 'deterministic, and only the PR body can change it', + ), + t((r.log.summary[0] ?? '').includes(PR_TARGET), 'L12 summary carries it too, with the PR url'), + t(r.calls.update.length === 2, `L12 still closes the other two targets -- isolation holds; got ${r.calls.update.length}`), + t(r.threw === null, 'L12 does not let anything escape the script'), + ], + }, + { + id: 'L13', + name: 'the target is a MERGED pull request -- refused BEFORE the already-closed branch comments on it (#9711)', + scenario: () => ({ + body: MIXED_BODY, + token: 'pat', + // A merged pull request answers `state: 'closed'` with + // `state_reason: null` from the issues endpoint -- measured on + // objectstack-ai/objectstack#9143. That is the very fixture L9 uses to + // prove a closed ISSUE gets its backlink, which makes this scenario the + // ORDERING test: a `pull_request` guard placed after the state branch + // would sail past it and comment on somebody else's pull request. + issues: { + [PR_TARGET]: { + state: 'closed', + stateReason: null, + comments: [], + pullRequest: { ...PR_KEY, merged_at: '2026-08-16T14:56:58Z' }, + }, + }, + }), + check: (r, t) => [ + t( + !r.calls.comment.some((c) => c.key === PR_TARGET), + 'L13 leaves NO backlink on a merged pull request -- the path L9 pins for a closed issue must not be reached here', + ), + t( + !r.calls.list.includes(PR_TARGET), + 'L13 never reaches the idempotency check, which is what proves the kind guard runs FIRST', + ), + t(!r.calls.update.some((u) => u.key === PR_TARGET), 'L13 does not touch its state'), + t(r.log.failed.length === 1, `L13 fails the job for the same reason L12 does, got ${r.log.failed.length}`), + t((r.log.failed[0] ?? '').includes(PR_TARGET), 'L13 setFailed names it'), + t(r.calls.update.length === 2, `L13 still closes the other two, got ${r.calls.update.length}`), + ], + }, ]; // ── The battery ───────────────────────────────────────────────────────────── @@ -850,9 +968,11 @@ const MUTATIONS = [ { id: 'M1', what: 'the post-loop verdict is downgraded back to a warning (the #9595 defect, restored)', - from: 'core.setFailed(\n `${failures.length} of ${targets.size}', - to: 'core.warning(\n `${failures.length} of ${targets.size}', - expect: ['L3', 'L4', 'L5'], + from: "core.setFailed(verdict.join(' '));", + to: "core.warning(verdict.join(' '));", + // Since #9711 the verdict carries both red classes, so this one mutation + // now has to be caught by an API refusal AND by a malformed target. + expect: ['L3', 'L4', 'L5', 'L12', 'L13'], }, { id: 'M2', @@ -922,6 +1042,27 @@ const MUTATIONS = [ to: 'failures.push({ key, reason });', expect: ['L10'], }, + { + id: 'M12', + what: 'the pull-request guard is dropped, so a keyword aimed at a foreign PR comments on it and CLOSES it (#9711)', + from: 'if (issue.pull_request) {', + to: 'if (false) {', + expect: ['L12', 'L13'], + }, + { + id: 'M13', + what: 'a refused pull-request target stops being recorded, so the run refuses it and still reports green', + from: 'malformed.push({ key, url: issue.pull_request.html_url });', + to: '', + expect: ['L12', 'L13'], + }, + { + id: 'M14', + what: 'the refusal is downgraded to an info line, so nothing annotates the run it happened in', + from: 'core.warning(\n `${key} is a PULL REQUEST', + to: 'core.info(\n `${key} is a PULL REQUEST', + expect: ['L12'], + }, { id: 'M7', what: 'the notice path loses its verdict (the #9594 half, restored to its defect)',