diff --git a/src/review/repo-skill-render.ts b/src/review/repo-skill-render.ts index 6cc14612e..6252fd77f 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-doc-pr.test.ts b/test/unit/repo-doc-pr.test.ts index 607ebb613..bfec263db 100644 --- a/test/unit/repo-doc-pr.test.ts +++ b/test/unit/repo-doc-pr.test.ts @@ -46,12 +46,15 @@ async function seedRepoDocGenerationConfig(env: ReturnType await upsertRepoFocusManifest(env, repoFullName, { repoDocGeneration: { enabled: true, ...overrides } }); } -// #3001: shouldGenerateRepoSkill needs 2 of 3 named signals. Seeds a strict linked-issue rule (settings + -// manifest) and 2 CI workflow files -- both in the SAME upsertRepoFocusManifest call as repoDocGeneration, since -// a second separate call would replace rather than merge with the first. +// #3001: shouldGenerateRepoSkill needs 2 of 3 named signals. Seeds a strict linked-issue rule (a BLOCKING +// linkedIssueGateMode -- the enforcement authority hasStrictLinkedIssueRule keys on since #9671, not merely +// requireLinkedIssue+policy) and 2 CI workflow files -- the manifest fields go in the SAME upsertRepoFocusManifest +// call as repoDocGeneration, since a second separate call would replace rather than merge with the first. async function seedSkillTriggerRepo(env: ReturnType, repoFullName: string, scope: string[] = ["agents", "skills"], overrides: { allowOverwriteExisting?: boolean } = {}): Promise { - await upsertRepositorySettings(env, { repoFullName, requireLinkedIssue: true }); - await upsertRepoFocusManifest(env, repoFullName, { linkedIssuePolicy: "required", repoDocGeneration: { enabled: true, scope, ...overrides } }); + await upsertRepositorySettings(env, { repoFullName, requireLinkedIssue: true, linkedIssueGateMode: "block" }); + // The manifest's `gate.linkedIssue` is the authority resolveEffectiveSettings maps to linkedIssueGateMode, which + // hasStrictLinkedIssueRule keys on since #9671 -- set it to "block" so the strict-linked-issue signal fires. + await upsertRepoFocusManifest(env, repoFullName, { gate: { linkedIssue: "block" }, linkedIssuePolicy: "required", repoDocGeneration: { enabled: true, scope, ...overrides } }); await seedChunk(env, ".github/workflows/ci.yml", "name: CI\non: push\n"); await seedChunk(env, ".github/workflows/lint.yml", "name: Lint\non: push\n"); } diff --git a/test/unit/repo-skill-render.test.ts b/test/unit/repo-skill-render.test.ts index c210648cd..7bf216646 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."); });