Skip to content

Commit f051cd2

Browse files
authored
fix(review): key the generated skill's linked-issue trigger off gate mode (#9924)
hasStrictLinkedIssueRule fired on requireLinkedIssue + a non-optional linkedIssuePolicy, which is advisory-only and never blocks a PR on its own. That let the generated SKILL.md's trigger sentence say "a linked issue is required" right above a Linked-issues section reporting "Required: no" for the same repo. linkedIssueGateMode is the actual enforcement authority (matches the identical assertion already used in repo-doc-render.ts), so key off that instead. Co-authored-by: bitfathers94 <237535319+bitfathers94@users.noreply.github.com>
1 parent 45471e7 commit f051cd2

3 files changed

Lines changed: 51 additions & 11 deletions

File tree

src/review/repo-skill-render.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,12 @@ function hasBlockingGate(contributionWorkflow: RepoProfileContributionWorkflow):
2121
return contributionWorkflow.gatePublishesCheck;
2222
}
2323

24-
/** A repo that both requires a linked issue AND has a policy stricter than "optional" has a real, non-obvious
25-
* admission rule worth writing down -- mirrors the mismatch repo-policy-readiness.ts already treats as notable. */
24+
/** A repo whose linked-issue rule actually BLOCKS a PR has a real, non-obvious admission rule worth writing
25+
* down. `linkedIssueGateMode` is the enforcement authority (repo-profile.ts's doc comment on that field) --
26+
* `requireLinkedIssue`/`linkedIssuePolicy` alone are advisory-only and must not decide this, matching the
27+
* identical assertion in repo-doc-render.ts's "Requires a linked issue" line. */
2628
function hasStrictLinkedIssueRule(contributionWorkflow: RepoProfileContributionWorkflow): boolean {
27-
return contributionWorkflow.requireLinkedIssue && contributionWorkflow.linkedIssuePolicy !== "optional";
29+
return contributionWorkflow.linkedIssueGateMode === "block";
2830
}
2931

3032
function hasMultiStageCi(contributionWorkflow: RepoProfileContributionWorkflow): boolean {

test/unit/repo-doc-pr.test.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,13 @@ async function seedRepoDocGenerationConfig(env: ReturnType<typeof createTestEnv>
4848

4949
// #3001: shouldGenerateRepoSkill needs 2 of 3 named signals. Seeds a strict linked-issue rule (settings +
5050
// manifest) and 2 CI workflow files -- both in the SAME upsertRepoFocusManifest call as repoDocGeneration, since
51-
// a second separate call would replace rather than merge with the first.
51+
// a second separate call would replace rather than merge with the first. `gate.linkedIssue: "block"` is what
52+
// actually makes the rule strict (#9671) -- requireLinkedIssue/linkedIssuePolicy alone are advisory-only;
53+
// linkedIssueGateMode itself is config-as-code only, so it can only be set via the manifest's `gate` block, not
54+
// upsertRepositorySettings (that field is a silent no-op there per repositories.ts's Batch A comment).
5255
async function seedSkillTriggerRepo(env: ReturnType<typeof createTestEnv>, repoFullName: string, scope: string[] = ["agents", "skills"], overrides: { allowOverwriteExisting?: boolean } = {}): Promise<void> {
5356
await upsertRepositorySettings(env, { repoFullName, requireLinkedIssue: true });
54-
await upsertRepoFocusManifest(env, repoFullName, { linkedIssuePolicy: "required", repoDocGeneration: { enabled: true, scope, ...overrides } });
57+
await upsertRepoFocusManifest(env, repoFullName, { linkedIssuePolicy: "required", gate: { linkedIssue: "block" }, repoDocGeneration: { enabled: true, scope, ...overrides } });
5558
await seedChunk(env, ".github/workflows/ci.yml", "name: CI\non: push\n");
5659
await seedChunk(env, ".github/workflows/lint.yml", "name: Lint\non: push\n");
5760
}

test/unit/repo-skill-render.test.ts

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,12 @@ describe("shouldGenerateRepoSkill (#3001)", () => {
2525
it.each([
2626
["no signals", contributionWorkflow(), false],
2727
["gate only", contributionWorkflow({ gatePublishesCheck: true }), false],
28-
["strict linked issue only", contributionWorkflow({ requireLinkedIssue: true, linkedIssuePolicy: "required" }), false],
28+
["strict linked issue only", contributionWorkflow({ requireLinkedIssue: true, linkedIssuePolicy: "required", linkedIssueGateMode: "block" }), false],
2929
["multi-stage CI only", contributionWorkflow({ ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }), false],
30-
["gate + strict linked issue (2 of 3)", contributionWorkflow({ gatePublishesCheck: true, requireLinkedIssue: true, linkedIssuePolicy: "required" }), true],
30+
["gate + strict linked issue (2 of 3)", contributionWorkflow({ gatePublishesCheck: true, requireLinkedIssue: true, linkedIssuePolicy: "required", linkedIssueGateMode: "block" }), true],
3131
["gate + multi-stage CI (2 of 3)", contributionWorkflow({ gatePublishesCheck: true, ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }), true],
32-
["strict linked issue + multi-stage CI (2 of 3)", contributionWorkflow({ requireLinkedIssue: true, linkedIssuePolicy: "preferred", ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }), true],
33-
["all three signals (3 of 3)", contributionWorkflow({ gatePublishesCheck: true, requireLinkedIssue: true, linkedIssuePolicy: "required", ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }), true],
32+
["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],
33+
["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],
3434
])("%s -> %s", (_label, workflow, expected) => {
3535
expect(shouldGenerateRepoSkill(presentProfile({ contributionWorkflow: workflow }) as Extract<RepoProfile, { present: true }>)).toBe(expected);
3636
});
@@ -42,6 +42,24 @@ describe("shouldGenerateRepoSkill (#3001)", () => {
4242
expect(shouldGenerateRepoSkill(presentProfile({ contributionWorkflow: workflow }) as Extract<RepoProfile, { present: true }>)).toBe(false);
4343
});
4444

45+
it("only linkedIssueGateMode \"block\" counts as a strict linked-issue rule, not requireLinkedIssue/linkedIssuePolicy alone (#9671)", () => {
46+
// requireLinkedIssue: true + linkedIssuePolicy: "required" used to be sufficient on their own; the actual
47+
// enforcement authority is linkedIssueGateMode (repo-profile.ts's doc comment on that field), matching
48+
// repo-doc-render.ts's identical "Requires a linked issue" assertion.
49+
const gateOnlyAdvisoryLinkedIssue = contributionWorkflow({ gatePublishesCheck: true, requireLinkedIssue: true, linkedIssuePolicy: "required", linkedIssueGateMode: "advisory" });
50+
expect(shouldGenerateRepoSkill(presentProfile({ contributionWorkflow: gateOnlyAdvisoryLinkedIssue }) as Extract<RepoProfile, { present: true }>)).toBe(false);
51+
52+
const gateOnlyBlockLinkedIssue = contributionWorkflow({ gatePublishesCheck: true, requireLinkedIssue: true, linkedIssuePolicy: "required", linkedIssueGateMode: "block" });
53+
expect(shouldGenerateRepoSkill(presentProfile({ contributionWorkflow: gateOnlyBlockLinkedIssue }) as Extract<RepoProfile, { present: true }>)).toBe(true);
54+
});
55+
56+
it("a repo with only advisory linked-issue + multi-stage CI does not reach the 2-of-3 threshold", () => {
57+
// linkedIssueGateMode "advisory" means the linked-issue signal must not count, even though
58+
// requireLinkedIssue/linkedIssuePolicy look strict -- leaving only 1 real signal (multi-stage CI).
59+
const workflow = contributionWorkflow({ requireLinkedIssue: true, linkedIssuePolicy: "required", linkedIssueGateMode: "advisory", ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] });
60+
expect(shouldGenerateRepoSkill(presentProfile({ contributionWorkflow: workflow }) as Extract<RepoProfile, { present: true }>)).toBe(false);
61+
});
62+
4563
it("exactly one CI workflow file does not count as multi-stage", () => {
4664
const workflow = contributionWorkflow({ gatePublishesCheck: true, ciWorkflowFiles: [".github/workflows/ci.yml"] });
4765
expect(shouldGenerateRepoSkill(presentProfile({ contributionWorkflow: workflow }) as Extract<RepoProfile, { present: true }>)).toBe(false);
@@ -116,13 +134,30 @@ describe("renderRepoSkillContent (#3001)", () => {
116134
});
117135
const content = renderRepoSkillContent(profile);
118136
expect(content).not.toBeNull();
137+
expect(content).not.toContain("A linked issue is required");
119138
expect(content).toContain("Required: no");
120139
expect(content).not.toContain("Required: yes");
121140
});
122141

142+
it("the trigger sentence and the Linked-issues section agree in block mode: both state a linked issue is required (#9671)", () => {
143+
const profile = presentProfile({
144+
contributionWorkflow: contributionWorkflow({
145+
gatePublishesCheck: true,
146+
requireLinkedIssue: true,
147+
linkedIssuePolicy: "required",
148+
linkedIssueGateMode: "block",
149+
ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"],
150+
}),
151+
});
152+
const content = renderRepoSkillContent(profile);
153+
expect(content).not.toBeNull();
154+
expect(content).toContain('A linked issue is required, with a "required" policy.');
155+
expect(content).toContain("Required: yes");
156+
});
157+
123158
it("omits the gate-check reason line when the trigger fires via linked-issue + multi-stage CI alone (no blocking gate)", () => {
124159
const profile = presentProfile({
125-
contributionWorkflow: contributionWorkflow({ gatePublishesCheck: false, requireLinkedIssue: true, linkedIssuePolicy: "preferred", ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }),
160+
contributionWorkflow: contributionWorkflow({ gatePublishesCheck: false, requireLinkedIssue: true, linkedIssuePolicy: "preferred", linkedIssueGateMode: "block", ciWorkflowFiles: [".github/workflows/a.yml", ".github/workflows/b.yml"] }),
126161
});
127162
const content = renderRepoSkillContent(profile);
128163
expect(content).not.toBeNull();
@@ -133,7 +168,7 @@ describe("renderRepoSkillContent (#3001)", () => {
133168

134169
it("omits the multi-stage-CI reason line when the trigger fires via gate + strict linked issue alone (single CI file)", () => {
135170
const profile = presentProfile({
136-
contributionWorkflow: contributionWorkflow({ gatePublishesCheck: true, requireLinkedIssue: true, linkedIssuePolicy: "required", ciWorkflowFiles: [".github/workflows/ci.yml"] }),
171+
contributionWorkflow: contributionWorkflow({ gatePublishesCheck: true, requireLinkedIssue: true, linkedIssuePolicy: "required", linkedIssueGateMode: "block", ciWorkflowFiles: [".github/workflows/ci.yml"] }),
137172
});
138173
const content = renderRepoSkillContent(profile);
139174
expect(content).not.toBeNull();

0 commit comments

Comments
 (0)