From 8892f95d2145fc13317142a89571267392ac769f Mon Sep 17 00:00:00 2001 From: kai392 Date: Thu, 30 Jul 2026 07:26:32 +0800 Subject: [PATCH] fix(orb): SKILL.md linked-issue trigger must key on the gate-mode enforcement authority (#9671) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit repo-skill-render.ts asserted "a linked issue is required" from two different fields in the same generated file: hasStrictLinkedIssueRule keyed on `requireLinkedIssue && linkedIssuePolicy !== "optional"`, while the Linked-issues section keyed on `linkedIssueGateMode === "block"`. For a repo with requireLinkedIssue:true, policy:"required", gateMode:"advisory" (a real, common combo — advisory is the default), the generated SKILL.md said "A linked issue is required, with a 'required' policy." a few lines above "Required: no" — directly contradictory misinformation, since only "block" mode actually blocks (repo-profile.ts:68-70). hasStrictLinkedIssueRule is also one of shouldGenerateRepoSkill's three signals, so a skill file could be generated off a signal that enforces nothing. - hasStrictLinkedIssueRule now keys on `linkedIssueGateMode === "block"`, the documented enforcement authority, matching repo-doc-render.ts:106. - renderTriggerReasons' sentence states the blocking mode ("...gate is in 'block' mode.") and only fires when the gate actually blocks. - The Linked-issues section (Policy/Required) is unchanged. Tests: the signal counts only under block mode; a regression rendering the full advisory-mode SKILL.md asserts it never claims a linked issue is required while saying "Required: no"; the block-mode doc agrees in both directions; and advisory linked-issue + multi-stage CI no longer reaches the 2-of-3 threshold. Existing fixtures that meant a genuine strict rule now set block mode. All fail against the current field choice. Closes #9671 Co-Authored-By: Claude Opus 4.8 --- src/review/repo-skill-render.ts | 10 +++-- test/unit/repo-skill-render.test.ts | 60 ++++++++++++++++++++++++----- 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/src/review/repo-skill-render.ts b/src/review/repo-skill-render.ts index 6cc14612e1..6252fd77f5 100644 --- a/src/review/repo-skill-render.ts +++ b/src/review/repo-skill-render.ts @@ -21,10 +21,12 @@ function hasBlockingGate(contributionWorkflow: RepoProfileContributionWorkflow): return contributionWorkflow.gatePublishesCheck; } -/** A repo that both requires a linked issue AND has a policy stricter than "optional" has a real, non-obvious - * admission rule worth writing down -- mirrors the mismatch repo-policy-readiness.ts already treats as notable. */ +/** A repo whose linked-issue gate actually BLOCKS a PR has a real, non-obvious admission rule worth writing down. + * #9671: key on `linkedIssueGateMode === "block"` -- the documented enforcement authority (repo-profile.ts:68-70) + * -- not on `requireLinkedIssue`/`linkedIssuePolicy`, which alone do NOT block (advisory is the default mode), so + * a repo requiring a linked issue only advisorily is not a strict rule. Matches repo-doc-render.ts:106. */ function hasStrictLinkedIssueRule(contributionWorkflow: RepoProfileContributionWorkflow): boolean { - return contributionWorkflow.requireLinkedIssue && contributionWorkflow.linkedIssuePolicy !== "optional"; + return contributionWorkflow.linkedIssueGateMode === "block"; } function hasMultiStageCi(contributionWorkflow: RepoProfileContributionWorkflow): boolean { @@ -70,7 +72,7 @@ export function repoSkillFilePath(repoFullName: string): string { function renderTriggerReasons(contributionWorkflow: RepoProfileContributionWorkflow): string { const reasons: string[] = []; if (hasBlockingGate(contributionWorkflow)) reasons.push("- CI publishes a required check before a pull request can merge."); - if (hasStrictLinkedIssueRule(contributionWorkflow)) reasons.push(`- A linked issue is required, with a "${contributionWorkflow.linkedIssuePolicy}" policy.`); + if (hasStrictLinkedIssueRule(contributionWorkflow)) reasons.push(`- A linked issue is required to merge — the linked-issue gate is in "${contributionWorkflow.linkedIssueGateMode}" mode.`); if (hasMultiStageCi(contributionWorkflow)) reasons.push(`- ${contributionWorkflow.ciWorkflowFiles.length} CI workflow files run on a pull request.`); return reasons.join("\n"); } diff --git a/test/unit/repo-skill-render.test.ts b/test/unit/repo-skill-render.test.ts index c210648cd6..7bf2166462 100644 --- a/test/unit/repo-skill-render.test.ts +++ b/test/unit/repo-skill-render.test.ts @@ -25,12 +25,14 @@ describe("shouldGenerateRepoSkill (#3001)", () => { it.each([ ["no signals", contributionWorkflow(), false], ["gate only", contributionWorkflow({ gatePublishesCheck: true }), false], - ["strict linked issue only", contributionWorkflow({ requireLinkedIssue: true, linkedIssuePolicy: "required" }), false], + // #9671: a "strict linked-issue rule" now means the gate actually blocks (linkedIssueGateMode: "block"), + // not merely requireLinkedIssue+non-optional-policy — so these fixtures set block mode to represent it. + ["strict linked issue only", contributionWorkflow({ requireLinkedIssue: true, linkedIssuePolicy: "required", linkedIssueGateMode: "block" }), false], ["multi-stage CI only", contributionWorkflow({ ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }), false], - ["gate + strict linked issue (2 of 3)", contributionWorkflow({ gatePublishesCheck: true, requireLinkedIssue: true, linkedIssuePolicy: "required" }), true], + ["gate + strict linked issue (2 of 3)", contributionWorkflow({ gatePublishesCheck: true, requireLinkedIssue: true, linkedIssuePolicy: "required", linkedIssueGateMode: "block" }), true], ["gate + multi-stage CI (2 of 3)", contributionWorkflow({ gatePublishesCheck: true, ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }), true], - ["strict linked issue + multi-stage CI (2 of 3)", contributionWorkflow({ requireLinkedIssue: true, linkedIssuePolicy: "preferred", ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }), true], - ["all three signals (3 of 3)", contributionWorkflow({ gatePublishesCheck: true, requireLinkedIssue: true, linkedIssuePolicy: "required", ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }), true], + ["strict linked issue + multi-stage CI (2 of 3)", contributionWorkflow({ requireLinkedIssue: true, linkedIssuePolicy: "preferred", linkedIssueGateMode: "block", ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }), true], + ["all three signals (3 of 3)", contributionWorkflow({ gatePublishesCheck: true, requireLinkedIssue: true, linkedIssuePolicy: "required", linkedIssueGateMode: "block", ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }), true], ])("%s -> %s", (_label, workflow, expected) => { expect(shouldGenerateRepoSkill(presentProfile({ contributionWorkflow: workflow }) as Extract)).toBe(expected); }); @@ -48,6 +50,44 @@ describe("shouldGenerateRepoSkill (#3001)", () => { }); }); +describe("#9671: the linked-issue trigger keys on the gate-mode enforcement authority", () => { + // A "requireLinkedIssue: true, policy: required" repo whose gate is only ADVISORY (the default) does NOT block a + // PR — the old code treated it as a strict rule anyway, contradicting the SKILL.md's own "Required: no" line. + const advisoryLinked = { requireLinkedIssue: true, linkedIssuePolicy: "required" as const, linkedIssueGateMode: "advisory" as const }; + const blockLinked = { requireLinkedIssue: true, linkedIssuePolicy: "required" as const, linkedIssueGateMode: "block" as const }; + const twoCi = [".github/workflows/a.yml", ".github/workflows/b.yml"]; + + it("counts the linked-issue signal only when linkedIssueGateMode is 'block' (Deliverable 1)", () => { + // advisory linked-issue + a single CI file => 0 real signals (the linked-issue signal must not fire). + expect( + shouldGenerateRepoSkill(presentProfile({ contributionWorkflow: contributionWorkflow({ ...advisoryLinked, ciWorkflowFiles: [".github/workflows/a.yml"] }) }) as Extract), + ).toBe(false); + // block linked-issue + a blocking gate => 2 signals fire. + expect( + shouldGenerateRepoSkill(presentProfile({ contributionWorkflow: contributionWorkflow({ ...blockLinked, gatePublishesCheck: true }) }) as Extract), + ).toBe(true); + }); + + it("REGRESSION (#9671): advisory-mode SKILL.md never claims a linked issue is required, and says Required: no", () => { + // gate + multi-stage CI reach the threshold so a body is rendered regardless of the (advisory) linked-issue signal. + const body = renderRepoSkillContent(presentProfile({ contributionWorkflow: contributionWorkflow({ ...advisoryLinked, gatePublishesCheck: true, ciWorkflowFiles: twoCi }) })) ?? ""; + expect(body).not.toContain("A linked issue is required"); + expect(body).toContain("Required: no"); + }); + + it("block-mode SKILL.md states a linked issue is required AND says Required: yes (the two agree)", () => { + const body = renderRepoSkillContent(presentProfile({ contributionWorkflow: contributionWorkflow({ ...blockLinked, gatePublishesCheck: true, ciWorkflowFiles: twoCi }) })) ?? ""; + expect(body).toContain("A linked issue is required"); + expect(body).toContain("Required: yes"); + }); + + it("does not reach the 2-of-3 threshold on advisory linked-issue + multi-stage CI alone (Deliverable 4)", () => { + expect( + shouldGenerateRepoSkill(presentProfile({ contributionWorkflow: contributionWorkflow({ ...advisoryLinked, ciWorkflowFiles: twoCi }) }) as Extract), + ).toBe(false); + }); +}); + describe("repoSkillName / repoSkillFilePath (#3001)", () => { it("derives a lowercase, dash-joined name from the repo segment", () => { expect(repoSkillName("owner/widgets")).toBe("contributing-to-widgets"); @@ -93,7 +133,8 @@ describe("renderRepoSkillContent (#3001)", () => { expect(content).toContain("name: contributing-to-widgets"); expect(content).toContain("# Contributing to widgets"); expect(content).toContain("CI publishes a required check before a pull request can merge."); - expect(content).toContain('A linked issue is required, with a "required" policy.'); + expect(content).toContain("A linked issue is required to merge"); + expect(content).toContain('gate is in "block" mode'); expect(content).toContain("2 CI workflow files run on a pull request."); expect(content).toContain("Build: `npm run build`"); expect(content).toContain("Test: `npm run test`"); @@ -122,23 +163,24 @@ describe("renderRepoSkillContent (#3001)", () => { it("omits the gate-check reason line when the trigger fires via linked-issue + multi-stage CI alone (no blocking gate)", () => { const profile = presentProfile({ - contributionWorkflow: contributionWorkflow({ gatePublishesCheck: false, requireLinkedIssue: true, linkedIssuePolicy: "preferred", ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }), + contributionWorkflow: contributionWorkflow({ gatePublishesCheck: false, requireLinkedIssue: true, linkedIssuePolicy: "preferred", linkedIssueGateMode: "block", ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }), }); const content = renderRepoSkillContent(profile); expect(content).not.toBeNull(); expect(content).not.toContain("CI publishes a required check"); - expect(content).toContain('A linked issue is required, with a "preferred" policy.'); + expect(content).toContain("A linked issue is required to merge"); + expect(content).toContain('gate is in "block" mode'); expect(content).toContain("2 CI workflow files run on a pull request."); }); it("omits the multi-stage-CI reason line when the trigger fires via gate + strict linked issue alone (single CI file)", () => { const profile = presentProfile({ - contributionWorkflow: contributionWorkflow({ gatePublishesCheck: true, requireLinkedIssue: true, linkedIssuePolicy: "required", ciWorkflowFiles: [".github/workflows/ci.yml"] }), + contributionWorkflow: contributionWorkflow({ gatePublishesCheck: true, requireLinkedIssue: true, linkedIssuePolicy: "required", linkedIssueGateMode: "block", ciWorkflowFiles: [".github/workflows/ci.yml"] }), }); const content = renderRepoSkillContent(profile); expect(content).not.toBeNull(); expect(content).toContain("CI publishes a required check before a pull request can merge."); - expect(content).toContain('A linked issue is required, with a "required" policy.'); + expect(content).toContain("A linked issue is required to merge"); expect(content).not.toContain("CI workflow files run on a pull request."); });