From 4fc6b748b5c0321cf4e2a10b9ad8ceb639204941 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Wed, 29 Jul 2026 15:30:46 +0900 Subject: [PATCH] fix(orb): filter contributor-draft implementation requirements through the public-safe expectation path wantedPathCandidate built two sections from the same manifest.testExpectations array but handled them differently. testingRequirements ran the array through isFocusManifestPublicSafe + formatContributorIssueDraftTestExpectation (via buildContributorIssueDraftTestingRequirements), while implementationRequirements four lines away mapped the raw entries into `Run ${entry} before requesting review.` -- no public-safe filter and a different format. A public-unsafe expectation was dropped from one contributor-facing list but rendered verbatim in the other, and the two "Run" spellings diverged. Extract the shared computation into one private publicSafeTestExpectations(manifest) helper and use it from both buildContributorIssueDraftTestingRequirements and wantedPathCandidate, so the filter+format is applied identically and a third consumer cannot repeat the divergence. The GENERIC_TESTING_REQUIREMENTS fallback stays specific to testingRequirements, and implementationRequirements keeps its contribute-nothing-when-empty behaviour. No change to isFocusManifestPublicSafe, formatContributorIssueDraftTestExpectation, or isContributorIssueDraftPublicSafe. Adds tests: a public-unsafe expectation is filtered out of BOTH lists (never reaches the draft), and a wanted-path candidate's implementationRequirements carry the identical public-safe-filtered expectations as testingRequirements. Closes #9704 --- src/services/contributor-issue-draft.ts | 13 ++++++-- test/unit/contributor-issue-draft.test.ts | 39 +++++++++++++++++++++-- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/services/contributor-issue-draft.ts b/src/services/contributor-issue-draft.ts index e72eef309e..b3d8bac82c 100644 --- a/src/services/contributor-issue-draft.ts +++ b/src/services/contributor-issue-draft.ts @@ -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, @@ -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.", diff --git a/test/unit/contributor-issue-draft.test.ts b/test/unit/contributor-issue-draft.test.ts index 7a923e52f8..b9e0d37b72 100644 --- a/test/unit/contributor-issue-draft.test.ts +++ b/test/unit/contributor-issue-draft.test.ts @@ -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"; @@ -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/"], @@ -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 () => {