Skip to content

Commit cb9ff23

Browse files
os-steveclaude
andauthored
fix(ci): a cross-repo closing keyword aimed at a foreign PULL REQUEST is refused, not honoured (#9759)
Every pull request is also an issue to `issues.get`, and the per-target loop never read the `pull_request` key the response carries. A merged body saying `Fixes owner/repo#N` where N is a pull request would have made this workflow comment on that PR and close it — a power GitHub's own closing-keyword parser does not have, exercised with a credential scoped for issue hygiene, and asymmetric: a closed PR loses its merge-queue membership and any armed auto-merge in the same step. The guard sits before the already-closed branch on purpose: a merged PR reads `state: 'closed'` with `state_reason: null` from that endpoint, so a guard placed after it would still leave a comment on somebody else's pull request. The refusal is loud. A malformed target is recorded apart from API failures — the two need opposite remedies — and the post-loop verdict now carries both classes, each stating its own: retry a refusal, rewrite a bad reference. Harness: L12 (open PR) and L13 (merged PR, the ordering test) added, doubles model the `pull_request` key as absence-or-object, M12/M13/M14 added and M1 follows the verdict's new shape. 88 assertions / 16 scenarios -> 105 / 18; 11 mutations -> 14. Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 1a3336a commit cb9ff23

2 files changed

Lines changed: 246 additions & 15 deletions

File tree

‎.github/workflows/cross-repo-issue-closer.yml‎

Lines changed: 100 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,13 @@ jobs:
307307
// already stated the requirement; only the code was missing.
308308
const failures = [];
309309
310+
// Targets that are not issues AT ALL. Kept apart from `failures`
311+
// above because the two need opposite remedies: a failure is an API
312+
// refusal a re-run can clear, this one is a defect in the merged
313+
// PR's own body that no re-run will ever change. Fusing them would
314+
// send the reader off to re-run a job that is going to refuse again.
315+
const malformed = [];
316+
310317
// The backlink this PR leaves, identified so a SECOND run can see
311318
// it. The marker is scoped to one pull request and is therefore
312319
// STABLE across runs — one PR leaves one backlink, however many
@@ -334,6 +341,47 @@ jobs:
334341
owner: t.owner, repo: t.repo, issue_number: t.number,
335342
});
336343
344+
// A pull request is ALSO an issue to this endpoint. Every PR
345+
// answers `GET /repos/{owner}/{repo}/issues/{N}` with `state`,
346+
// `state_reason` and the rest, plus a `pull_request` key that
347+
// nothing here used to read (#9711) — the key is ABSENT on a
348+
// real issue, present as an object on a PR, so its truthiness
349+
// is the whole test. The target regex takes any `owner/repo#N`
350+
// and PR and issue numbers share one sequence, so a body saying
351+
// `Fixes owner/repo#4500` where 4500 is a pull request would
352+
// have made this loop comment on that PR and CLOSE it.
353+
//
354+
// GitHub's own closing-keyword parser — the behaviour this
355+
// workflow exists to carry across repository boundaries — never
356+
// closes a pull request; `Fixes #N` aimed at a PR leaves a
357+
// reference and nothing else. So the keyword is a MALFORMED
358+
// instruction, and it is refused OUT LOUD rather than skipped:
359+
// this file's ten exit paths were arrived at by fixing one
360+
// quiet one after another, and a quiet `continue` would have
361+
// been the eleventh. The refusal is also the conservative half
362+
// of an asymmetry — closing a pull request drops its
363+
// merge-queue membership and any armed auto-merge in the same
364+
// step, and neither comes back by itself.
365+
//
366+
// Placed BEFORE the `state === 'closed'` branch on purpose: a
367+
// merged pull request reads `state: 'closed'` with
368+
// `state_reason: null` from this very endpoint (measured on
369+
// objectstack-ai/objectstack#9143), which that branch reads as
370+
// "no objection recorded" and answers with a backlink comment.
371+
// A guard sitting after it would still write on somebody else's
372+
// pull request.
373+
if (issue.pull_request) {
374+
malformed.push({ key, url: issue.pull_request.html_url });
375+
core.warning(
376+
`${key} is a PULL REQUEST, not an issue — ${prUrl} declares it FIXED, but GitHub's own `
377+
+ `closing-keyword parser never closes a pull request, so this workflow does not either. `
378+
+ `Nothing was commented on and nothing was closed. Re-running cannot fix a keyword that `
379+
+ `can never fire: point the reference at the issue it meant.`,
380+
{ title: 'Cross-repo closing keyword names a pull request' },
381+
);
382+
continue;
383+
}
384+
337385
// "Already closed" is NOT one situation, and the three causes
338386
// that reach here do not deserve the same treatment (#9643).
339387
// Until this split existed all three were skipped whole, which
@@ -433,7 +481,7 @@ jobs:
433481
}
434482
}
435483
436-
if (failures.length === 0) {
484+
if (failures.length === 0 && malformed.length === 0) {
437485
core.info(`All ${targets.size} cross-repo target(s) handled: closed, or already closed and linked.`);
438486
return;
439487
}
@@ -442,10 +490,23 @@ jobs:
442490
// notice branch uses, for the same reasons: the summary carries the
443491
// full list a human needs, `setFailed` carries the conclusion that
444492
// makes anyone open the run at all.
493+
//
494+
// Both classes are red, and each says what to do about ITSELF: a
495+
// refusal is retried, a malformed reference is rewritten. Green
496+
// with an annotation was the alternative for the malformed class,
497+
// and this file's own history rejects it — 2334 runs, every one of
498+
// them `run_attempt` 1 and 99 of the last 100 green, so a green run
499+
// of this job has never been opened by anybody. An annotation on a
500+
// green post-merge run is the silent path with extra steps, and a
501+
// declared close that can never happen is exactly what #9595 made
502+
// red for.
445503
const failedKeys = failures.map((f) => f.key).join(', ');
446504
const failedList = failures.map((f) => `- \`${f.key}\` —— ${f.lost} 未完成:${f.reason}`).join('\n');
447-
try {
448-
await core.summary.addRaw([
505+
const malformedKeys = malformed.map((m) => m.key).join(', ');
506+
const malformedList = malformed.map((m) => `- \`${m.key}\` —— 这个编号是一个 pull request:${m.url}`).join('\n');
507+
const summaryLines = [];
508+
if (failures.length > 0) {
509+
summaryLines.push(
449510
'## ⚠️ 跨仓库 issue 没能收口',
450511
'',
451512
`本次合并声明了 ${targets.size} 个跨仓库关闭目标,其中 ${failures.length} 个没有完成:`,
@@ -456,16 +517,45 @@ jobs:
456517
'- 原因排除后可以直接 re-run 本 job:已经关闭的目标不会被重复关闭,已经留下过本 PR 反链的目标也不会被重复评论。',
457518
'- 401/404 通常意味着 `CROSS_REPO_ISSUE_TOKEN` 对目标仓库没有 `issues: write`,而不是目标不存在 —— GitHub 对无权访问的仓库回 404。',
458519
'',
459-
].join('\n')).write();
520+
);
521+
}
522+
if (malformed.length > 0) {
523+
summaryLines.push(
524+
'## ⛔ 关闭关键字指向的是 pull request,不是 issue',
525+
'',
526+
`本次合并声明的 ${targets.size} 个跨仓库关闭目标里,有 ${malformed.length} 个的编号是 pull request:`,
527+
'',
528+
malformedList,
529+
'',
530+
'- 它们没有被评论,也没有被关闭。GitHub 自己的关闭关键字解析器从不关闭 pull request,本工作流也不会 —— 关掉一个 PR 会在同一步里丢掉它的 merge queue 成员资格和已经武装的 auto-merge,两者都不会自己回来。',
531+
`- 这不是 re-run 能解决的故障:写错的是 ${prUrl} 的正文。把引用改成它真正想关的那个 issue 编号,或者手工关掉那个 issue。`,
532+
'',
533+
);
534+
}
535+
try {
536+
await core.summary.addRaw(summaryLines.join('\n')).write();
460537
} catch (summaryError) {
461538
// Same asymmetry as the notice branch: the summary is the richer
462539
// channel, the conclusion the reliable one. Losing the richer one
463540
// must not restore the silence.
464541
core.info(`Could not write the job summary: ${summaryError.message}`);
465542
}
466-
core.setFailed(
467-
`${failures.length} of ${targets.size} cross-repo target(s) were NOT finished by this merge: `
468-
+ `${failedKeys}. Finish them by hand (fixed by ${prUrl}) — an open one needs closing, an `
469-
+ `already-closed one needs this PR's link on it — or re-run this job once the cause is `
470-
+ `cleared. The full list, with what each one is missing, is in this run's job summary.`,
471-
);
543+
const verdict = [];
544+
if (failures.length > 0) {
545+
verdict.push(
546+
`${failures.length} of ${targets.size} cross-repo target(s) were NOT finished by this merge: `
547+
+ `${failedKeys}. Finish them by hand (fixed by ${prUrl}) — an open one needs closing, an `
548+
+ `already-closed one needs this PR's link on it — or re-run this job once the cause is `
549+
+ `cleared. The full list, with what each one is missing, is in this run's job summary.`,
550+
);
551+
}
552+
if (malformed.length > 0) {
553+
verdict.push(
554+
`${malformed.length} of ${targets.size} cross-repo closing keyword(s) in this merged pull `
555+
+ `request name a PULL REQUEST rather than an issue: ${malformedKeys}. GitHub's own keyword `
556+
+ `parser never closes a pull request and this workflow does not either, so nothing was `
557+
+ `commented on and nothing was closed. Re-running this job will refuse them again — what `
558+
+ `has to change is the reference in ${prUrl}'s body.`,
559+
);
560+
}
561+
core.setFailed(verdict.join(' '));

‎scripts/check-cross-repo-closer-outcome.mjs‎

Lines changed: 146 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,12 @@
5656
// ## What is asserted, and what is deliberately not
5757
//
5858
// Asserted: the target parse (which keyword spellings qualify, which forms do
59-
// not, that a same-repo reference is skipped), and the OUTCOME of every exit --
59+
// not, that a same-repo reference is skipped), the target KIND (#9711: a pull
60+
// request is also an issue to `issues.get`, so `owner/repo#N` naming a PR
61+
// reaches the loop like anything else -- the scenarios pin that it is REFUSED
62+
// rather than closed, and that the refusal happens before the already-closed
63+
// branch can leave a comment on somebody else's pull request), and the OUTCOME
64+
// of every exit --
6065
// which of `core.setFailed` / `core.warning` / a job summary fires, and which
6166
// API calls were made. Those are the properties both cards are about.
6267
//
@@ -198,7 +203,21 @@ function makeDoubles({ body, token, issues = {}, prCommentError = null, summaryE
198203
// `state_reason` is nullable on a real closed issue -- objectui#4478
199204
// answers `null` from this very endpoint -- so the default models that
200205
// rather than inventing a value the API does not promise.
201-
return { data: { state: t.state ?? 'open', state_reason: t.stateReason ?? null } };
206+
//
207+
// `pull_request` is how the same endpoint says the number is a PULL
208+
// REQUEST, and it is modelled as ABSENCE rather than as `undefined`
209+
// because that is what was measured: objectstack-ai/objectstack#9716 (a
210+
// PR) answers `{ url, html_url, diff_url, patch_url, merged_at }`, and
211+
// #9711 (an issue) carries no such key at all. A fixture that always
212+
// spelled the key would let a truthiness test pass here that the real
213+
// API would fail. (#9711)
214+
return {
215+
data: {
216+
state: t.state ?? 'open',
217+
state_reason: t.stateReason ?? null,
218+
...(t.pullRequest ? { pull_request: t.pullRequest } : {}),
219+
},
220+
};
202221
},
203222
async listComments({ owner, repo, issue_number: n }) {
204223
const key = `${owner}/${repo}#${n}`;
@@ -384,6 +403,27 @@ const FOREIGN = ['objectstack-ai/objectui#456', 'my-org/some.repo#22', 'third/pa
384403

385404
/** The target the already-closed scenarios (L2, L6-L9) work on. */
386405
const CLOSED_TARGET = 'objectstack-ai/objectui#456';
406+
407+
/** The target the PULL REQUEST scenarios (L12, L13) work on (#9711). */
408+
const PR_TARGET = 'my-org/some.repo#22';
409+
410+
/**
411+
* The `pull_request` key GitHub returns when an issue number is a pull request.
412+
*
413+
* Copied from a real response rather than invented: `GET /repos/
414+
* objectstack-ai/objectstack/issues/9716` answers exactly these five fields,
415+
* and `.../issues/9711` -- an issue -- answers with the key absent. The loop
416+
* reads only its truthiness, but the shape is pinned so that a change in what
417+
* the script reads out of it (`html_url` reaches the job summary) fails here
418+
* instead of printing `undefined` into somebody's run.
419+
*/
420+
const PR_KEY = {
421+
url: 'https://api.github.com/repos/my-org/some.repo/pulls/22',
422+
html_url: 'https://github.com/my-org/some.repo/pull/22',
423+
diff_url: 'https://github.com/my-org/some.repo/pull/22.diff',
424+
patch_url: 'https://github.com/my-org/some.repo/pull/22.patch',
425+
merged_at: null,
426+
};
387427
const PR_URL = 'https://github.com/objectstack-ai/objectstack/pull/1234';
388428

389429
/**
@@ -730,6 +770,84 @@ export const SCENARIOS = [
730770
t(r.log.warning.length === 0, 'L11 run 2 warns about nothing -- an idempotent re-run is a normal outcome'),
731771
],
732772
},
773+
{
774+
id: 'L12',
775+
name: 'the target is an OPEN pull request -- refused out loud, never closed (#9711)',
776+
scenario: () => ({
777+
body: MIXED_BODY,
778+
token: 'pat',
779+
issues: { [PR_TARGET]: { state: 'open', pullRequest: PR_KEY } },
780+
}),
781+
check: (r, t) => [
782+
t(r.calls.get.includes(PR_TARGET), 'L12 reads the target -- the kind is only knowable from the response'),
783+
t(
784+
!r.calls.update.some((u) => u.key === PR_TARGET),
785+
"L12 does NOT close the pull request -- GitHub's own keyword parser never closes one, and this "
786+
+ 'workflow exists to carry that behaviour across repos, not to exceed it. A closed PR also loses '
787+
+ 'its merge-queue membership and any armed auto-merge in the same step, and neither returns by itself',
788+
),
789+
t(!r.calls.comment.some((c) => c.key === PR_TARGET), 'L12 posts nothing on the foreign pull request either'),
790+
t(
791+
!r.calls.list.includes(PR_TARGET),
792+
'L12 does not even list its comments -- the target KIND settles it before idempotency can matter',
793+
),
794+
t(
795+
r.log.warning.some((w) => w.message.includes(PR_TARGET) && /PULL REQUEST/.test(w.message)),
796+
'L12 ANNOUNCES the refusal and names the target -- a quiet `continue` here would have been the '
797+
+ "eleventh silent exit in a file whose ten known ones were each found by somebody reading it",
798+
),
799+
t(
800+
r.log.failed.length === 1,
801+
`L12 fails the job: the merged body declares a close that can never fire, and this run is the only `
802+
+ `thing that will ever know; got ${r.log.failed.length}`,
803+
),
804+
t((r.log.failed[0] ?? '').includes(PR_TARGET), 'L12 setFailed names the malformed target'),
805+
t(
806+
!/re-run this job once the cause is cleared/.test(r.log.failed[0] ?? ''),
807+
'L12 verdict does not send the reader to re-run -- unlike every other red path here the refusal is '
808+
+ 'deterministic, and only the PR body can change it',
809+
),
810+
t((r.log.summary[0] ?? '').includes(PR_TARGET), 'L12 summary carries it too, with the PR url'),
811+
t(r.calls.update.length === 2, `L12 still closes the other two targets -- isolation holds; got ${r.calls.update.length}`),
812+
t(r.threw === null, 'L12 does not let anything escape the script'),
813+
],
814+
},
815+
{
816+
id: 'L13',
817+
name: 'the target is a MERGED pull request -- refused BEFORE the already-closed branch comments on it (#9711)',
818+
scenario: () => ({
819+
body: MIXED_BODY,
820+
token: 'pat',
821+
// A merged pull request answers `state: 'closed'` with
822+
// `state_reason: null` from the issues endpoint -- measured on
823+
// objectstack-ai/objectstack#9143. That is the very fixture L9 uses to
824+
// prove a closed ISSUE gets its backlink, which makes this scenario the
825+
// ORDERING test: a `pull_request` guard placed after the state branch
826+
// would sail past it and comment on somebody else's pull request.
827+
issues: {
828+
[PR_TARGET]: {
829+
state: 'closed',
830+
stateReason: null,
831+
comments: [],
832+
pullRequest: { ...PR_KEY, merged_at: '2026-08-16T14:56:58Z' },
833+
},
834+
},
835+
}),
836+
check: (r, t) => [
837+
t(
838+
!r.calls.comment.some((c) => c.key === PR_TARGET),
839+
'L13 leaves NO backlink on a merged pull request -- the path L9 pins for a closed issue must not be reached here',
840+
),
841+
t(
842+
!r.calls.list.includes(PR_TARGET),
843+
'L13 never reaches the idempotency check, which is what proves the kind guard runs FIRST',
844+
),
845+
t(!r.calls.update.some((u) => u.key === PR_TARGET), 'L13 does not touch its state'),
846+
t(r.log.failed.length === 1, `L13 fails the job for the same reason L12 does, got ${r.log.failed.length}`),
847+
t((r.log.failed[0] ?? '').includes(PR_TARGET), 'L13 setFailed names it'),
848+
t(r.calls.update.length === 2, `L13 still closes the other two, got ${r.calls.update.length}`),
849+
],
850+
},
733851
];
734852

735853
// ── The battery ─────────────────────────────────────────────────────────────
@@ -850,9 +968,11 @@ const MUTATIONS = [
850968
{
851969
id: 'M1',
852970
what: 'the post-loop verdict is downgraded back to a warning (the #9595 defect, restored)',
853-
from: 'core.setFailed(\n `${failures.length} of ${targets.size}',
854-
to: 'core.warning(\n `${failures.length} of ${targets.size}',
855-
expect: ['L3', 'L4', 'L5'],
971+
from: "core.setFailed(verdict.join(' '));",
972+
to: "core.warning(verdict.join(' '));",
973+
// Since #9711 the verdict carries both red classes, so this one mutation
974+
// now has to be caught by an API refusal AND by a malformed target.
975+
expect: ['L3', 'L4', 'L5', 'L12', 'L13'],
856976
},
857977
{
858978
id: 'M2',
@@ -922,6 +1042,27 @@ const MUTATIONS = [
9221042
to: 'failures.push({ key, reason });',
9231043
expect: ['L10'],
9241044
},
1045+
{
1046+
id: 'M12',
1047+
what: 'the pull-request guard is dropped, so a keyword aimed at a foreign PR comments on it and CLOSES it (#9711)',
1048+
from: 'if (issue.pull_request) {',
1049+
to: 'if (false) {',
1050+
expect: ['L12', 'L13'],
1051+
},
1052+
{
1053+
id: 'M13',
1054+
what: 'a refused pull-request target stops being recorded, so the run refuses it and still reports green',
1055+
from: 'malformed.push({ key, url: issue.pull_request.html_url });',
1056+
to: '',
1057+
expect: ['L12', 'L13'],
1058+
},
1059+
{
1060+
id: 'M14',
1061+
what: 'the refusal is downgraded to an info line, so nothing annotates the run it happened in',
1062+
from: 'core.warning(\n `${key} is a PULL REQUEST',
1063+
to: 'core.info(\n `${key} is a PULL REQUEST',
1064+
expect: ['L12'],
1065+
},
9251066
{
9261067
id: 'M7',
9271068
what: 'the notice path loses its verdict (the #9594 half, restored to its defect)',

0 commit comments

Comments
 (0)