Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/services/contributor-issue-draft.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,15 @@ const GENERIC_TESTING_REQUIREMENTS = [
"Public GitHub output must stay advisory and must not imply guaranteed participation outcomes.",
];

/** The manifest's testExpectations, filtered to public-safe entries and formatted for a contributor-facing
* draft. The single source shared by both testingRequirements and implementationRequirements so a public-unsafe
* expectation can never leak through one path while the other filters it, and neither re-diverges (#9704). */
function publicSafeTestExpectations(manifest: FocusManifest): string[] {
return manifest.testExpectations.filter(isFocusManifestPublicSafe).map(formatContributorIssueDraftTestExpectation);
}

export function buildContributorIssueDraftTestingRequirements(manifest: FocusManifest): string[] {
const policyExpectations = manifest.testExpectations.filter(isFocusManifestPublicSafe).map(formatContributorIssueDraftTestExpectation);
const policyExpectations = publicSafeTestExpectations(manifest);
if (policyExpectations.length === 0) return [...GENERIC_TESTING_REQUIREMENTS];
return [
...policyExpectations,
Expand Down Expand Up @@ -483,7 +490,9 @@ function wantedPathCandidate(repoFullName: string, wantedPath: string, openIssue
implementationRequirements: [
`Stay within ${wantedPath} unless safety or release readiness requires adjacent files.`,
"Avoid blocked manifest paths and keep PRs narrowly scoped.",
...(manifest.testExpectations.length > 0 ? manifest.testExpectations.map((entry) => `Run ${entry} before requesting review.`) : []),
// Same public-safe-filtered, formatted expectations as testingRequirements -- an unfiltered raw
// `Run ${entry}` here would both double-format and leak a public-unsafe expectation the sibling drops (#9704).
...publicSafeTestExpectations(manifest),
],
publicPrivateBoundaries: [
"Public issues must not promise compensation, sort contributors, or expose private maintainer-only claims.",
Expand Down
39 changes: 37 additions & 2 deletions test/unit/contributor-issue-draft.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,36 @@ describe("contributor issue drafts", () => {
expect(candidates.some((entry) => entry.sections.implementationRequirements.some((line) => line.includes("npm run test:ci")))).toBe(true);
});

it("#9704: a wanted-path candidate's implementationRequirements carry the SAME public-safe-filtered expectations as testingRequirements", () => {
const manifest = parseFocusManifestContent(
// A safe expectation that survives the filter and an unsafe one that must be dropped from BOTH lists.
'{"wantedPaths":["src/"],"testExpectations":["npm run test:ci","wallet seed phrase"],"issueDiscoveryPolicy":"discouraged"}',
"repo_file",
);
const testing = buildContributorIssueDraftTestingRequirements(manifest);
const candidates = buildContributorIssueDraftCandidates({
repoFullName: "owner/repo",
repo: { fullName: "owner/repo", isRegistered: true } as never,
settings: { requireLinkedIssue: false } as never,
lane: buildLaneAdvice({ fullName: "owner/repo", isRegistered: true } as never, "owner/repo"),
configQuality: buildConfigQuality({ fullName: "owner/repo" } as never, [], [], "owner/repo"),
labelAudit: buildLabelAudit({ fullName: "owner/repo" } as never, [], [], [], "owner/repo"),
queueHealth: buildQueueHealth({ fullName: "owner/repo" } as never, [], [], buildCollisionReport("owner/repo", [], [])),
contributorIntakeHealth: buildContributorIntakeHealth({ fullName: "owner/repo" } as never, [], [], "owner/repo", buildCollisionReport("owner/repo", [], [])),
openIssues: [],
upstreamDriftWarnings: [],
focusManifest: manifest,
});
const wantedPathCandidate = candidates.find((entry) => entry.topic?.startsWith("focus:wanted_path:"));
expect(wantedPathCandidate).toBeDefined();
const implExpectations = wantedPathCandidate!.sections.implementationRequirements.filter((line) => line.includes("test:ci") || /wallet|seed phrase/i.test(line));
const testingExpectations = testing.filter((line) => line.includes("test:ci") || /wallet|seed phrase/i.test(line));
// Both derive from the shared publicSafeTestExpectations helper: identical list, unsafe entry gone from each.
expect(implExpectations).toEqual(testingExpectations);
expect(implExpectations).not.toEqual([]);
expect(JSON.stringify(implExpectations)).not.toMatch(/wallet|seed phrase/i);
});

it("ignores closed issues and empty title keys when checking duplicates", () => {
const fingerprint = "fp";
const title = "feat(issues): address validation policy readiness for repo";
Expand Down Expand Up @@ -490,7 +520,7 @@ describe("contributor issue drafts", () => {
expect(new Set(candidates.map((entry) => entry.topic)).size).toBe(candidates.length);
});

it("skips unsafe drafts when wanted-path validation text fails public hygiene", async () => {
it("#9704: filters a public-unsafe testExpectation out of BOTH requirement lists (never leaks into the draft)", async () => {
const env = createTestEnv();
await upsertRepoFocusManifest(env, "owner/unsafe-path", {
wantedPaths: ["src/unsafe-path-only/"],
Expand All @@ -500,7 +530,12 @@ describe("contributor issue drafts", () => {
});
vi.spyOn(repositories, "listOpenIssues").mockResolvedValue([]);
const result = await generateContributorIssueDrafts(env, "owner/unsafe-path", { dryRun: true, limit: 10 });
expect(result.drafts.some((draft) => draft.status === "skipped_unsafe")).toBe(true);
// Before #9704, implementationRequirements emitted a raw `Run wallet seed phrase ...` that only got caught
// downstream as skipped_unsafe. Now BOTH lists filter through isFocusManifestPublicSafe, so the unsafe
// expectation never reaches the draft at all -- the wanted-path draft is produced and carries no such text.
expect(result.drafts.some((draft) => draft.status === "skipped_unsafe")).toBe(false);
expect(JSON.stringify(result.drafts)).not.toMatch(/wallet|seed phrase/i);
expect(result.drafts.some((draft) => draft.topic?.startsWith("focus:wanted_path:"))).toBe(true);
});

it("returns null for invalid repo names when creating GitHub issues", async () => {
Expand Down