From bf4ffb45c78f0b0f9e1deb034be622480dc95e0c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 16:08:19 -0700 Subject: [PATCH 1/3] feat(agent): auto-match PRs to open GitHub Milestones in suggest-mode (#3183) Adds a ProjectTrackerAdapter interface (src/integrations/project-tracker-adapter.ts) with a GitHubMilestonesAdapter implementation, so #3184 (Projects v2) and #3186 (Linear) slot in without reshaping the matching/suggestion logic. Matching reuses the existing tokenize/termOverlap primitives from the duplicate-PR collision detector (now exported), at a stricter threshold than that gate since misattaching tracked progress is worse than an advisory-only duplicate note. New tri-state autoProjectMilestoneMatch setting (off/suggest/auto, default off), wired through the full config-as-code chain: migration, Drizzle schema, DB resolver, OpenAPI, and .gittensory.yml parity, mirroring the reviewCheckMode template. "auto" behaves like "suggest" until #3185 wires real milestone attachment. Hooked into the main PR-webhook path right after the existing linked-issue evidence gathering, entirely independent of the gate/disposition -- a missed or wrong match never affects CI or merge. --- apps/gittensory-ui/public/openapi.json | 32 ++ .../0110_project_milestone_match_mode.sql | 4 + src/db/repositories.ts | 9 + src/db/schema.ts | 1 + src/integrations/project-tracker-adapter.ts | 201 ++++++++++++ src/openapi/schemas.ts | 4 + src/queue/processors.ts | 27 ++ src/signals/engine.ts | 9 +- src/signals/focus-manifest.ts | 3 + src/types.ts | 11 + test/unit/focus-manifest.test.ts | 20 ++ test/unit/project-tracker-adapter.test.ts | 309 ++++++++++++++++++ ...y-settings-project-milestone-match.test.ts | 48 +++ 13 files changed, 675 insertions(+), 3 deletions(-) create mode 100644 migrations/0110_project_milestone_match_mode.sql create mode 100644 src/integrations/project-tracker-adapter.ts create mode 100644 test/unit/project-tracker-adapter.test.ts create mode 100644 test/unit/repository-settings-project-milestone-match.test.ts diff --git a/apps/gittensory-ui/public/openapi.json b/apps/gittensory-ui/public/openapi.json index cb8563f55d..092bec053f 100644 --- a/apps/gittensory-ui/public/openapi.json +++ b/apps/gittensory-ui/public/openapi.json @@ -3638,6 +3638,14 @@ "visible", "disabled" ] + }, + "autoProjectMilestoneMatch": { + "type": "string", + "enum": [ + "off", + "suggest", + "auto" + ] } }, "required": [ @@ -9175,6 +9183,14 @@ "verifyBeforeClose", "closeDelaySeconds" ] + }, + "autoProjectMilestoneMatch": { + "type": "string", + "enum": [ + "off", + "suggest", + "auto" + ] } }, "required": [ @@ -9284,6 +9300,14 @@ "visible", "disabled" ] + }, + "autoProjectMilestoneMatch": { + "type": "string", + "enum": [ + "off", + "suggest", + "auto" + ] } }, "required": [ @@ -9863,6 +9887,14 @@ "visible", "disabled" ] + }, + "autoProjectMilestoneMatch": { + "type": "string", + "enum": [ + "off", + "suggest", + "auto" + ] } }, "required": [ diff --git a/migrations/0110_project_milestone_match_mode.sql b/migrations/0110_project_milestone_match_mode.sql new file mode 100644 index 0000000000..041b4a2043 --- /dev/null +++ b/migrations/0110_project_milestone_match_mode.sql @@ -0,0 +1,4 @@ +-- Auto-project/milestone matching (#3183): detects when a PR is likely part of an open GitHub Milestone even +-- with no closing-keyword issue link, and posts a bot-comment suggestion in "suggest" mode. Defaults to 'off' +-- (opt-in) -- no existing repo should start getting suggestion comments without an explicit choice. +ALTER TABLE repository_settings ADD COLUMN project_milestone_match_mode TEXT NOT NULL DEFAULT 'off'; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index d82d073e78..ee1fb7ab1f 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -480,6 +480,7 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise checkRunDetailLevel: "minimal", gateCheckMode: "off", reviewCheckMode: "disabled", + autoProjectMilestoneMatch: "off", gatePack: "gittensor", linkedIssueGateMode: "advisory", duplicatePrGateMode: "block", @@ -550,6 +551,7 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise checkRunDetailLevel: parseCheckRunDetailLevel(row.checkRunDetailLevel), gateCheckMode: parseGateCheckMode(row.gateCheckMode), reviewCheckMode: parseReviewCheckMode(row.reviewCheckMode), + autoProjectMilestoneMatch: parseProjectMilestoneMatchMode(row.projectMilestoneMatchMode), gatePack: parseGatePack(row.gatePack), linkedIssueGateMode: parseGateRuleMode(row.linkedIssueGateMode), duplicatePrGateMode: parseGateRuleMode(row.duplicatePrGateMode), @@ -663,6 +665,7 @@ export async function upsertRepositorySettings(env: Env, settings: Partial; + listOpenMilestones(ctx: ProjectTrackerContext): Promise; + attachToProject(ctx: ProjectTrackerContext, pullNumber: number, projectId: string): Promise; + attachToMilestone(ctx: ProjectTrackerContext, pullNumber: number, milestoneId: string): Promise; +} + +function parseRepoFullName(repoFullName: string): { owner: string; repo: string } { + const parts = repoFullName.split("/"); + const owner = parts[0]; + const repo = parts[1]; + if (parts.length !== 2 || !owner || !repo || /\s/.test(repoFullName)) { + throw new Error(`Invalid repository full name: ${repoFullName}`); + } + return { owner, repo }; +} + +type GitHubMilestone = { + number: number; + title: string; +}; + +/** GitHub REST implementation of {@link ProjectTrackerAdapter}. Only the Milestone half is real (#3183) -- + * Projects v2 is GraphQL-only and needs a separate `organization_projects` App permission not yet granted, so + * those two methods are inert placeholders until #3184. */ +export class GitHubMilestonesAdapter implements ProjectTrackerAdapter { + // Inert placeholder until #3184 (Projects v2 needs GraphQL + a separate App permission). + async listOpenProjects(): Promise { + return []; + } + + async listOpenMilestones(ctx: ProjectTrackerContext): Promise { + const { owner, repo } = parseRepoFullName(ctx.repoFullName); + const token = await createInstallationToken(ctx.env, ctx.installationId); + const octokit = makeInstallationOctokit(ctx.env, token, "live", githubRateLimitAdmissionKeyForInstallation(ctx.installationId)); + const response = await octokit.request("GET /repos/{owner}/{repo}/milestones", { + owner, + repo, + state: "open", + per_page: 100, + }); + const milestones = response.data as GitHubMilestone[]; + return milestones.map((milestone) => ({ id: String(milestone.number), title: milestone.title })); + } + + // Inert placeholder until #3184 (Projects v2 needs GraphQL + a separate App permission). + async attachToProject(): Promise { + return { attached: false }; + } + + async attachToMilestone(ctx: ProjectTrackerContext, pullNumber: number, milestoneId: string): Promise { + const { owner, repo } = parseRepoFullName(ctx.repoFullName); + const token = await createInstallationToken(ctx.env, ctx.installationId); + const octokit = makeInstallationOctokit(ctx.env, token, "live", githubRateLimitAdmissionKeyForInstallation(ctx.installationId)); + await octokit.request("PATCH /repos/{owner}/{repo}/issues/{issue_number}", { + owner, + repo, + issue_number: pullNumber, + milestone: Number(milestoneId), + }); + return { attached: true }; + } +} + +// Stricter than the duplicate-PR collision gate's 0.58/2 (src/signals/engine.ts) -- misattaching a PR to the +// wrong milestone corrupts tracked progress, whereas a missed duplicate just skips an advisory note. +const MILESTONE_MATCH_MIN_SCORE = 0.65; +const MILESTONE_MATCH_MIN_SHARED = 3; + +export type ProjectTrackerMatch = { + milestone: ProjectTrackerRef; + score: number; + shared: number; +}; + +function termsFor(value: string): CollisionTerms { + const terms = new Set(tokenize(value)); + return { terms, size: terms.size }; +} + +/** + * Match PR title+body text against a list of open milestones (#3183), reusing the same tokenize/termOverlap + * heuristic as duplicate-PR collision detection. Returns null on no match -- AND on an ambiguous multi-match + * (more than one milestone clears the threshold): guessing between two plausible milestones is worse than + * suggesting neither, since a maintainer can always link one manually. + */ +export function matchOpenMilestones(prTitle: string, prBody: string | null | undefined, milestones: ProjectTrackerRef[]): ProjectTrackerMatch | null { + if (milestones.length === 0) return null; + const prTerms = termsFor([prTitle, prBody ?? ""].join(" ")); + const candidates = milestones + .map((milestone) => ({ milestone, ...termOverlap(prTerms, termsFor(milestone.title)) })) + .filter((candidate) => candidate.score >= MILESTONE_MATCH_MIN_SCORE && candidate.shared >= MILESTONE_MATCH_MIN_SHARED); + if (candidates.length !== 1) return null; + const best = candidates[0]; + /* v8 ignore next -- defensive: candidates.length === 1 above guarantees index 0 exists. */ + if (!best) return null; + return { milestone: best.milestone, score: best.score, shared: best.shared }; +} + +export const MILESTONE_SUGGEST_COMMENT_MARKER = ""; + +function renderSuggestionComment(match: ProjectTrackerMatch): string { + const confidencePercent = Math.round(match.score * 100); + return [ + MILESTONE_SUGGEST_COMMENT_MARKER, + `This PR looks like it's part of the **${match.milestone.title}** milestone (${confidencePercent}% title/body term overlap).`, + "", + "This is an advisory suggestion only — nothing has been attached automatically.", + ].join("\n"); +} + +type IssueComment = { + body?: string | null; + user?: { type?: string; login?: string } | null; +}; + +/** + * Best-effort, idempotent suggest-mode comment (#3183): posts ONCE per PR (never updates or reposts), so a + * repeated sweep/webhook pass never spams the thread. Never calls attachToMilestone -- suggest mode only ever + * comments; #3185 wires the real attach path behind the "auto" config value. + */ +export async function maybeSuggestMilestoneMatch(ctx: ProjectTrackerContext, pullNumber: number, prTitle: string, prBody: string | null | undefined): Promise<{ suggested: boolean }> { + const adapter = new GitHubMilestonesAdapter(); + const milestones = await adapter.listOpenMilestones(ctx); + const match = matchOpenMilestones(prTitle, prBody, milestones); + if (!match) return { suggested: false }; + + const { owner, repo } = parseRepoFullName(ctx.repoFullName); + const token = await createInstallationToken(ctx.env, ctx.installationId); + const octokit = makeInstallationOctokit(ctx.env, token, "live", githubRateLimitAdmissionKeyForInstallation(ctx.installationId)); + const botLogin = `${ctx.env.GITHUB_APP_SLUG}[bot]`; + const existing = await octokit.request("GET /repos/{owner}/{repo}/issues/{issue_number}/comments", { + owner, + repo, + issue_number: pullNumber, + per_page: 100, + }); + const alreadyPosted = (existing.data as IssueComment[]).some( + (comment) => comment.user?.type === "Bot" && comment.user.login?.toLowerCase() === botLogin.toLowerCase() && comment.body?.includes(MILESTONE_SUGGEST_COMMENT_MARKER), + ); + if (alreadyPosted) return { suggested: false }; + + await createIssueComment(ctx.env, ctx.installationId, ctx.repoFullName, pullNumber, renderSuggestionComment(match)); + return { suggested: true }; +} + +/** + * Webhook-level entry point (#3183): folds the "should this even run" gating (installed app, PR still open, + * feature opted in) AND the best-effort error swallowing into one call, so the PR-webhook handler in + * processors.ts has a single, unconditional call site instead of an inline compound branch that's awkward to + * exercise through the full webhook pipeline. `onError` lets the caller log/audit without this module needing + * to know about the caller's logging shape. + */ +export async function maybeSuggestMilestoneMatchForPr(args: { + env: Env; + installationId: number | null | undefined; + repoFullName: string; + pullNumber: number; + prState: string; + prTitle: string; + prBody: string | null | undefined; + mode: ProjectMilestoneMatchModeInput; + onError: (error: unknown) => void; +}): Promise { + if (!args.installationId) return; + if (args.prState !== "open") return; + if (!args.mode || args.mode === "off") return; + await maybeSuggestMilestoneMatch({ env: args.env, installationId: args.installationId, repoFullName: args.repoFullName }, args.pullNumber, args.prTitle, args.prBody).catch(args.onError); +} + +// Kept as a standalone alias (rather than importing RepositorySettings from ../types) so this integrations +// module has no dependency on the settings type -- it only needs to know "off" vs. anything else. +type ProjectMilestoneMatchModeInput = "off" | "suggest" | "auto" | null | undefined; diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index 50683d517a..b88f663f18 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -591,6 +591,7 @@ export const RepositorySettingsSchema = z checkRunDetailLevel: z.enum(["minimal", "standard", "deep"]), gateCheckMode: z.enum(["off", "enabled"]), reviewCheckMode: z.enum(["required", "visible", "disabled"]), + autoProjectMilestoneMatch: z.enum(["off", "suggest", "auto"]).optional(), gatePack: z.enum(["gittensor", "oss-anti-slop"]), linkedIssueGateMode: z.enum(["off", "advisory", "block"]), duplicatePrGateMode: z.enum(["off", "advisory", "block"]), @@ -724,6 +725,7 @@ export const RepoSettingsPreviewSchema = z checkRunDetailLevel: z.enum(["minimal", "standard", "deep"]), gateCheckMode: z.enum(["off", "enabled"]), reviewCheckMode: z.enum(["required", "visible", "disabled"]), + autoProjectMilestoneMatch: z.enum(["off", "suggest", "auto"]).optional(), gatePack: z.enum(["gittensor", "oss-anti-slop"]), linkedIssueGateMode: z.enum(["off", "advisory", "block"]), duplicatePrGateMode: z.enum(["off", "advisory", "block"]), @@ -1102,6 +1104,7 @@ export const InstallationRepairSchema = z checkRunMode: z.enum(["off", "enabled"]), gateCheckMode: z.enum(["off", "enabled"]), reviewCheckMode: z.enum(["required", "visible", "disabled"]), + autoProjectMilestoneMatch: z.enum(["off", "suggest", "auto"]).optional(), autoLabelEnabled: z.boolean(), }), }), @@ -2015,6 +2018,7 @@ export const RegistrationReadinessSchema = z checkRunMode: z.enum(["off", "enabled"]), gateCheckMode: z.enum(["off", "enabled"]), reviewCheckMode: z.enum(["required", "visible", "disabled"]), + autoProjectMilestoneMatch: z.enum(["off", "suggest", "auto"]).optional(), quietByDefault: z.boolean(), behavior: z.string(), warnings: z.array(z.string()), diff --git a/src/queue/processors.ts b/src/queue/processors.ts index a1b2296445..35c3314c90 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -465,6 +465,7 @@ import type { import { retryFailedRelays } from "../orb/relay"; import { sha256Hex } from "../utils/crypto"; import { errorMessage, nowIso } from "../utils/json"; +import { maybeSuggestMilestoneMatchForPr } from "../integrations/project-tracker-adapter"; const OFFICIAL_MINER_DETECTION_TTL_MS = 5 * 60 * 1000; const OFFICIAL_MINER_DETECTION_UNAVAILABLE_TTL_MS = 60 * 1000; @@ -5301,6 +5302,32 @@ async function processGitHubWebhook( linkedIssueAuthorLogins, }); await persistAdvisory(env, advisory); + // Auto-project/milestone matching (#3183): independent of the gate/disposition entirely -- a missed or + // wrong match must never affect CI/merge, so this is a best-effort side comment, never a blocker. All the + // "should this run at all" gating + error swallowing lives in maybeSuggestMilestoneMatchForPr itself, so + // this call site stays a single unconditional call. + await maybeSuggestMilestoneMatchForPr({ + env, + installationId, + repoFullName, + pullNumber: pr.number, + prState: pr.state, + prTitle: pr.title, + prBody: pr.body, + mode: settings.autoProjectMilestoneMatch, + onError: (error) => { + console.error( + JSON.stringify({ + level: "warn", + event: "milestone_suggest_failed", + deliveryId, + repoFullName, + pullNumber: pr.number, + error: errorMessage(error), + }), + ); + }, + }); // Draft-dodge guard (#converted-to-draft): a contributor converting an OPEN PR to draft cannot use // draft state to keep a gate-rejected PR alive. When a prior gate failure exists for the PR's current // headSha (and the block has not been maintainer-overridden), close the PR immediately — the gate diff --git a/src/signals/engine.ts b/src/signals/engine.ts index 8d6704639c..fb8346461f 100644 --- a/src/signals/engine.ts +++ b/src/signals/engine.ts @@ -5239,7 +5239,7 @@ function itemKey(item: CollisionItem): string { return `${item.type}-${item.number}`; } -type CollisionTerms = { +export type CollisionTerms = { terms: Set; size: number; }; @@ -5270,7 +5270,7 @@ function plannedContributionTerms(input: PreflightInput): CollisionTerms { return { terms, size: terms.size }; } -function termOverlap(left: CollisionTerms, right: CollisionTerms): { score: number; shared: number } { +export function termOverlap(left: CollisionTerms, right: CollisionTerms): { score: number; shared: number } { if (left.size === 0 || right.size === 0) return { score: 0, shared: 0 }; let shared = 0; const [smaller, larger] = left.size <= right.size ? [left.terms, right.terms] : [right.terms, left.terms]; @@ -5298,7 +5298,10 @@ function truncateText(value: string, maxChars: number): string { return value.length > maxChars ? value.slice(0, maxChars) : value; } -function tokenize(value: string): string[] { +// Exported (#3183) so the project/milestone text matcher (src/integrations/project-tracker-adapter.ts) can +// reuse the exact same term-overlap heuristic already proven here for duplicate-PR collision detection, rather +// than re-implementing a second, subtly different tokenizer. +export function tokenize(value: string): string[] { return value .toLowerCase() .split(/[^a-z0-9]+/g) diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index a814ee33e0..1eabce2dbc 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -193,6 +193,7 @@ export type FocusManifestSettings = Partial< | "checkRunDetailLevel" | "gateCheckMode" | "reviewCheckMode" + | "autoProjectMilestoneMatch" | "linkedIssueGateMode" | "duplicatePrGateMode" | "selfAuthoredLinkedIssueGateMode" @@ -1070,6 +1071,8 @@ function parseSettingsOverride(value: JsonValue | undefined, warnings: string[]) // resolveEffectiveSettings, and wins when both are set). const reviewCheckMode = normalizeOptionalEnum(r.reviewCheckMode, "settings.reviewCheckMode", ["required", "visible", "disabled"] as const, warnings); if (reviewCheckMode !== null) out.reviewCheckMode = reviewCheckMode; + const autoProjectMilestoneMatch = normalizeOptionalEnum(r.autoProjectMilestoneMatch, "settings.autoProjectMilestoneMatch", ["off", "suggest", "auto"] as const, warnings); + if (autoProjectMilestoneMatch !== null) out.autoProjectMilestoneMatch = autoProjectMilestoneMatch; const linkedIssueGateMode = normalizeOptionalGateMode(r.linkedIssueGateMode, "settings.linkedIssueGateMode", warnings); if (linkedIssueGateMode !== null) out.linkedIssueGateMode = linkedIssueGateMode; const duplicatePrGateMode = normalizeOptionalGateMode(r.duplicatePrGateMode, "settings.duplicatePrGateMode", warnings); diff --git a/src/types.ts b/src/types.ts index bbe0f95bc2..9334b4858e 100644 --- a/src/types.ts +++ b/src/types.ts @@ -550,6 +550,14 @@ export type GateRuleMode = "off" | "advisory" | "block"; * cannot do this on the operator's behalf -- it is a GitHub branch-protection setting). */ export type ReviewCheckMode = "required" | "visible" | "disabled"; +/** Auto-project/milestone matching (#3183): detects when a PR is likely part of an open GitHub Milestone even + * with no closing-keyword issue link, and posts a bot-comment suggestion. `"off"` (default) runs no matching + * at all; `"suggest"` matches and posts a single advisory comment, never mutating the PR; `"auto"` is accepted + * by config today but behaves identically to `"suggest"` until #3185 wires real milestone attachment -- no + * attach/auto-apply code exists yet, so treating it as inert-but-silent would be a worse failure mode than + * degrading to the safe, visible suggest behavior. */ +export type ProjectMilestoneMatchMode = "off" | "suggest" | "auto"; + /** Which policy pack the gate runs under (#692). `gittensor` = the full Gittensor policy: registry/emissions- * aware, and it threads the author's confirmed status for on-chain scoring (the gate verdict itself blocks * every author the same — confirmed status no longer changes it, #gate-nonconfirmed). `oss-anti-slop` = a @@ -587,6 +595,9 @@ export type RepositorySettings = { * See {@link ReviewCheckMode}. `gateCheckMode` above stays wired for API/back-compat display but no longer * drives the publish decision on its own. */ reviewCheckMode: ReviewCheckMode; + /** Auto-project/milestone matching (#3183). See {@link ProjectMilestoneMatchMode}. Always populated by the DB + * layer (default `"off"`); optional so existing settings fixtures/callers need not be touched. */ + autoProjectMilestoneMatch?: ProjectMilestoneMatchMode | undefined; /** Policy pack the gate evaluates under (#692). Default `gittensor` (registry-aware; threads confirmed * status for scoring only). `oss-anti-slop` runs the deterministic rules against any author on any repo. */ gatePack: GatePolicyPack; diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index b789272181..4eae47e25a 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -1835,6 +1835,26 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = }); }); + describe("autoProjectMilestoneMatch precedence (#3183)", () => { + it("parses settings.autoProjectMilestoneMatch and drops an invalid value with a warning", () => { + const m = parseFocusManifest({ settings: { autoProjectMilestoneMatch: "suggest" } }); + expect(m.settings.autoProjectMilestoneMatch).toBe("suggest"); + const invalid = parseFocusManifest({ settings: { autoProjectMilestoneMatch: "sometimes" as never } }); + expect(invalid.settings.autoProjectMilestoneMatch).toBeUndefined(); + expect(invalid.warnings.some((w) => /settings\.autoProjectMilestoneMatch/.test(w))).toBe(true); + }); + + it("settings.autoProjectMilestoneMatch overlays (replaces) the DB value when set, and is preserved when omitted", () => { + const overridden = resolveEffectiveSettings( + { autoProjectMilestoneMatch: "off" } as unknown as RepositorySettings, + parseFocusManifest({ settings: { autoProjectMilestoneMatch: "auto" } }), + ); + expect(overridden.autoProjectMilestoneMatch).toBe("auto"); + const noOverride = resolveEffectiveSettings({ autoProjectMilestoneMatch: "suggest" } as unknown as RepositorySettings, parseFocusManifest({})); + expect(noOverride.autoProjectMilestoneMatch).toBe("suggest"); + }); + }); + it("an EXPLICIT yml null force-clears a DB-configured cap, distinct from an omitted key (regression, gate finding on #2467)", () => { // Omitted key preserves the DB value (already covered above); an explicit `null` must ALSO be able to // override a DB-configured cap back to "no cap" — the documented `yml > DB > null` precedence otherwise diff --git a/test/unit/project-tracker-adapter.test.ts b/test/unit/project-tracker-adapter.test.ts new file mode 100644 index 0000000000..e49f4299b4 --- /dev/null +++ b/test/unit/project-tracker-adapter.test.ts @@ -0,0 +1,309 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { generateKeyPairSync } from "node:crypto"; +import { GitHubMilestonesAdapter, MILESTONE_SUGGEST_COMMENT_MARKER, maybeSuggestMilestoneMatch, maybeSuggestMilestoneMatchForPr, matchOpenMilestones, type ProjectTrackerRef } from "../../src/integrations/project-tracker-adapter"; +import { createTestEnv } from "../helpers/d1"; + +function generateRsaPrivateKeyPem(): string { + const { privateKey } = generateKeyPairSync("rsa", { modulusLength: 2048 }); + return privateKey.export({ type: "pkcs1", format: "pem" }).toString(); +} + +describe("matchOpenMilestones (#3183)", () => { + const milestones: ProjectTrackerRef[] = [{ id: "14", title: "Self-host reliability roadmap" }, { id: "9", title: "Bounty Wave 2" }]; + + it("returns null when there are no open milestones", () => { + expect(matchOpenMilestones("Fix self-host reliability roadmap flakiness", null, [])).toBeNull(); + }); + + it("returns null when no milestone clears the match threshold", () => { + expect(matchOpenMilestones("Fix a typo in the readme", "no relation to any tracked work", milestones)).toBeNull(); + }); + + it("matches a PR whose title/body clearly overlaps one open milestone's title", () => { + const match = matchOpenMilestones("Improve self-host reliability roadmap convergence", "Follow-up on the self-host reliability roadmap work", milestones); + expect(match?.milestone.id).toBe("14"); + expect(match?.score).toBeGreaterThanOrEqual(0.65); + expect(match?.shared).toBeGreaterThanOrEqual(3); + }); + + it("returns null on an ambiguous multi-match (more than one milestone clears the threshold) rather than guessing", () => { + const tied: ProjectTrackerRef[] = [ + { id: "1", title: "self host reliability roadmap convergence work" }, + { id: "2", title: "self host reliability roadmap convergence effort" }, + ]; + expect(matchOpenMilestones("self host reliability roadmap convergence", null, tied)).toBeNull(); + }); + + it("treats a missing PR body as empty text without throwing", () => { + expect(() => matchOpenMilestones("just a title", undefined, milestones)).not.toThrow(); + expect(() => matchOpenMilestones("just a title", null, milestones)).not.toThrow(); + }); +}); + +describe("GitHubMilestonesAdapter (#3183)", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("listOpenProjects and attachToProject are inert placeholders until #3184", async () => { + const adapter = new GitHubMilestonesAdapter(); + await expect(adapter.listOpenProjects()).resolves.toEqual([]); + await expect(adapter.attachToProject()).resolves.toEqual({ attached: false }); + }); + + it("rejects an invalid repository full name before making any GitHub call", async () => { + const adapter = new GitHubMilestonesAdapter(); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }); + await expect(adapter.listOpenMilestones({ env, installationId: 123, repoFullName: "invalid" })).rejects.toThrow(/Invalid repository full name/); + await expect(adapter.attachToMilestone({ env, installationId: 123, repoFullName: "owner/repo/extra" }, 4, "14")).rejects.toThrow(/Invalid repository full name/); + }); + + it("listOpenMilestones fetches and maps open milestones from the REST API", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/milestones")) return Response.json([{ number: 14, title: "Self-host reliability roadmap" }]); + return new Response("unexpected", { status: 500 }); + }); + const adapter = new GitHubMilestonesAdapter(); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }); + const result = await adapter.listOpenMilestones({ env, installationId: 123, repoFullName: "JSONbored/gittensory" }); + expect(result).toEqual([{ id: "14", title: "Self-host reliability roadmap" }]); + }); + + it("attachToMilestone PATCHes the issue with the milestone number", async () => { + let patchedBody: unknown; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/issues/4") && method === "PATCH") { + patchedBody = JSON.parse(String(init?.body ?? "{}")); + return Response.json({ number: 4, milestone: { number: 14 } }); + } + return new Response("unexpected", { status: 500 }); + }); + const adapter = new GitHubMilestonesAdapter(); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }); + const result = await adapter.attachToMilestone({ env, installationId: 123, repoFullName: "JSONbored/gittensory" }, 4, "14"); + expect(result).toEqual({ attached: true }); + expect(patchedBody).toMatchObject({ milestone: 14 }); + }); +}); + +describe("maybeSuggestMilestoneMatch (#3183)", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("posts a suggestion comment when a milestone matches and none has been posted yet", async () => { + const posted: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/milestones")) return Response.json([{ number: 14, title: "Self-host reliability roadmap" }]); + if (url.includes("/issues/4/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/4/comments") && method === "POST") { + const body = JSON.parse(String(init?.body ?? "{}")) as { body?: string }; + posted.push(body.body ?? ""); + return Response.json({ id: 1 }); + } + return new Response("unexpected", { status: 500 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + const result = await maybeSuggestMilestoneMatch( + { env, installationId: 123, repoFullName: "JSONbored/gittensory" }, + 4, + "Improve self-host reliability roadmap convergence", + "Follow-up on the self-host reliability roadmap work", + ); + expect(result).toEqual({ suggested: true }); + expect(posted).toHaveLength(1); + expect(posted[0]).toContain(MILESTONE_SUGGEST_COMMENT_MARKER); + expect(posted[0]).toContain("Self-host reliability roadmap"); + }); + + it("does nothing when no milestone matches (never calls the comment POST endpoint)", async () => { + let posted = false; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/milestones")) return Response.json([{ number: 14, title: "Self-host reliability roadmap" }]); + if (url.includes("/comments") && method === "POST") { + posted = true; + return Response.json({ id: 1 }); + } + return new Response("unexpected", { status: 500 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + const result = await maybeSuggestMilestoneMatch({ env, installationId: 123, repoFullName: "JSONbored/gittensory" }, 4, "unrelated typo fix", null); + expect(result).toEqual({ suggested: false }); + expect(posted).toBe(false); + }); + + it("is idempotent — skips posting when the marker comment already exists from this bot", async () => { + let posted = false; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/milestones")) return Response.json([{ number: 14, title: "Self-host reliability roadmap" }]); + if (url.includes("/issues/4/comments") && method === "GET") { + return Response.json([{ body: MILESTONE_SUGGEST_COMMENT_MARKER, user: { type: "Bot", login: "gittensory[bot]" } }]); + } + if (url.includes("/comments") && method === "POST") { + posted = true; + return Response.json({ id: 1 }); + } + return new Response("unexpected", { status: 500 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + const result = await maybeSuggestMilestoneMatch( + { env, installationId: 123, repoFullName: "JSONbored/gittensory" }, + 4, + "Improve self-host reliability roadmap convergence", + "Follow-up on the self-host reliability roadmap work", + ); + expect(result).toEqual({ suggested: false }); + expect(posted).toBe(false); + }); + + it("ignores a marker-matching comment from a non-bot user (a human quoting the marker text)", async () => { + let posted = false; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/milestones")) return Response.json([{ number: 14, title: "Self-host reliability roadmap" }]); + if (url.includes("/issues/4/comments") && method === "GET") { + return Response.json([{ body: MILESTONE_SUGGEST_COMMENT_MARKER, user: { type: "User", login: "alice" } }]); + } + if (url.includes("/issues/4/comments") && method === "POST") { + posted = true; + return Response.json({ id: 1 }); + } + return new Response("unexpected", { status: 500 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + const result = await maybeSuggestMilestoneMatch( + { env, installationId: 123, repoFullName: "JSONbored/gittensory" }, + 4, + "Improve self-host reliability roadmap convergence", + "Follow-up on the self-host reliability roadmap work", + ); + expect(result).toEqual({ suggested: true }); + expect(posted).toBe(true); + }); +}); + +describe("maybeSuggestMilestoneMatchForPr (#3183 webhook-level gating)", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + function baseArgs(overrides: Partial[0]> = {}) { + return { + env: createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }), + installationId: 123, + repoFullName: "JSONbored/gittensory", + pullNumber: 4, + prState: "open", + prTitle: "Improve self-host reliability roadmap convergence", + prBody: "Follow-up on the self-host reliability roadmap work", + mode: "suggest" as const, + onError: vi.fn(), + ...overrides, + }; + } + + it("does nothing when installationId is falsy (never touches the network)", async () => { + let called = false; + vi.stubGlobal("fetch", async () => { + called = true; + return new Response("unexpected", { status: 500 }); + }); + const onError = vi.fn(); + await maybeSuggestMilestoneMatchForPr(baseArgs({ installationId: null, onError })); + expect(called).toBe(false); + expect(onError).not.toHaveBeenCalled(); + }); + + it("does nothing when the PR is not open", async () => { + let called = false; + vi.stubGlobal("fetch", async () => { + called = true; + return new Response("unexpected", { status: 500 }); + }); + await maybeSuggestMilestoneMatchForPr(baseArgs({ prState: "closed" })); + expect(called).toBe(false); + }); + + it("does nothing when mode is off", async () => { + let called = false; + vi.stubGlobal("fetch", async () => { + called = true; + return new Response("unexpected", { status: 500 }); + }); + await maybeSuggestMilestoneMatchForPr(baseArgs({ mode: "off" })); + expect(called).toBe(false); + }); + + it("does nothing when mode is null/undefined (unconfigured repo, always populated by the DB layer in practice)", async () => { + let called = false; + vi.stubGlobal("fetch", async () => { + called = true; + return new Response("unexpected", { status: 500 }); + }); + await maybeSuggestMilestoneMatchForPr(baseArgs({ mode: null })); + expect(called).toBe(false); + called = false; + await maybeSuggestMilestoneMatchForPr(baseArgs({ mode: undefined })); + expect(called).toBe(false); + }); + + it("runs the match when mode is suggest and every gate passes", async () => { + let milestonesFetched = false; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/milestones")) { + milestonesFetched = true; + return Response.json([]); + } + if (url.includes("/comments") && method === "GET") return Response.json([]); + return new Response("unexpected", { status: 500 }); + }); + await maybeSuggestMilestoneMatchForPr(baseArgs()); + expect(milestonesFetched).toBe(true); + }); + + it("runs the match when mode is auto (identical to suggest until #3185)", async () => { + let milestonesFetched = false; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/milestones")) { + milestonesFetched = true; + return Response.json([]); + } + return new Response("unexpected", { status: 500 }); + }); + await maybeSuggestMilestoneMatchForPr(baseArgs({ mode: "auto" })); + expect(milestonesFetched).toBe(true); + }); + + it("routes a failure to onError instead of throwing", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/milestones")) return new Response("boom", { status: 500 }); + return new Response("unexpected", { status: 500 }); + }); + const onError = vi.fn(); + await expect(maybeSuggestMilestoneMatchForPr(baseArgs({ onError }))).resolves.toBeUndefined(); + expect(onError).toHaveBeenCalledTimes(1); + }); +}); diff --git a/test/unit/repository-settings-project-milestone-match.test.ts b/test/unit/repository-settings-project-milestone-match.test.ts new file mode 100644 index 0000000000..737bd5e995 --- /dev/null +++ b/test/unit/repository-settings-project-milestone-match.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from "vitest"; +import { getRepositorySettings, upsertRepositorySettings } from "../../src/db/repositories"; +import { createTestEnv } from "../helpers/d1"; + +// #3183: autoProjectMilestoneMatch is the tri-state config for auto-project/milestone matching, mirroring the +// reviewCheckMode template (#2852). "off" is the conservative, opt-in default; "suggest" and "auto" currently +// behave identically (post a comment) since real milestone attachment isn't wired until #3185. +describe("repository_settings: autoProjectMilestoneMatch default + round-trip (#3183)", () => { + it("getRepositorySettings returns off for a repo with no DB row at all (conservative, opt-in default)", async () => { + const env = createTestEnv(); + const settings = await getRepositorySettings(env, "acme/brand-new-repo"); + expect(settings.autoProjectMilestoneMatch).toBe("off"); + }); + + it("upsertRepositorySettings persists off when the caller omits the field entirely", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/omits-field" }); + const settings = await getRepositorySettings(env, "acme/omits-field"); + expect(settings.autoProjectMilestoneMatch).toBe("off"); + }); + + it("an explicit suggest/auto opt-in round-trips through a re-upsert that carries it forward explicitly", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/round-trip", autoProjectMilestoneMatch: "suggest" }); + const settings = await getRepositorySettings(env, "acme/round-trip"); + expect(settings.autoProjectMilestoneMatch).toBe("suggest"); + // A true read-modify-write caller (the route-handler pattern: spread current settings, then override) must + // carry the persisted value forward explicitly -- upsertRepositorySettings never merges against the DB row. + await upsertRepositorySettings(env, { ...settings, repoFullName: "acme/round-trip" }); + const after = await getRepositorySettings(env, "acme/round-trip"); + expect(after.autoProjectMilestoneMatch).toBe("suggest"); + }); + + it("auto round-trips distinctly from suggest", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/auto-mode", autoProjectMilestoneMatch: "auto" }); + const settings = await getRepositorySettings(env, "acme/auto-mode"); + expect(settings.autoProjectMilestoneMatch).toBe("auto"); + }); + + it("an invalid persisted DB value fails closed to off on read", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/malformed" }); + await env.DB.prepare("UPDATE repository_settings SET project_milestone_match_mode = ? WHERE repo_full_name = ?").bind("sometimes", "acme/malformed").run(); + const settings = await getRepositorySettings(env, "acme/malformed"); + expect(settings.autoProjectMilestoneMatch).toBe("off"); + }); +}); From 1335a70f0f46d45bcdaaa95f7db061402312c400 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 16:21:19 -0700 Subject: [PATCH 2/3] fix(agent): move milestone-suggest failure logging out of the webhook handler The processors.ts call site passed an inline onError closure whose console.error body lived in the huge webhook file and was only reachable through a full pipeline test, leaving it uncovered (codecov/patch: 50% on that file). maybeSuggestMilestoneMatchForPr now takes deliveryId directly and does its own logging internally, where it already has dedicated, isolated test coverage -- the processors.ts call site is now a plain data object literal with no logic of its own. --- src/integrations/project-tracker-adapter.ts | 24 +++++++++++++++------ src/queue/processors.ts | 17 +++------------ test/unit/project-tracker-adapter.test.ts | 17 ++++++++------- 3 files changed, 30 insertions(+), 28 deletions(-) diff --git a/src/integrations/project-tracker-adapter.ts b/src/integrations/project-tracker-adapter.ts index 8c3c040baf..77e9c7b5ee 100644 --- a/src/integrations/project-tracker-adapter.ts +++ b/src/integrations/project-tracker-adapter.ts @@ -2,6 +2,7 @@ import { createInstallationToken } from "../github/app"; import { githubRateLimitAdmissionKeyForInstallation, makeInstallationOctokit } from "../github/client"; import { createIssueComment } from "../github/pr-actions"; import { termOverlap, tokenize, type CollisionTerms } from "../signals/engine"; +import { errorMessage } from "../utils/json"; /** Repo-scoped context shared by every ProjectTrackerAdapter call (#3183). */ export type ProjectTrackerContext = { @@ -174,10 +175,10 @@ export async function maybeSuggestMilestoneMatch(ctx: ProjectTrackerContext, pul /** * Webhook-level entry point (#3183): folds the "should this even run" gating (installed app, PR still open, - * feature opted in) AND the best-effort error swallowing into one call, so the PR-webhook handler in - * processors.ts has a single, unconditional call site instead of an inline compound branch that's awkward to - * exercise through the full webhook pipeline. `onError` lets the caller log/audit without this module needing - * to know about the caller's logging shape. + * feature opted in) AND the best-effort error logging into one call, so the PR-webhook handler in + * processors.ts has a single, unconditional call site with no logic/logging body of its own -- everything + * testable lives here, where it already has dedicated, isolated coverage, rather than in an inline closure + * inside the huge webhook file that only a full pipeline test could exercise. */ export async function maybeSuggestMilestoneMatchForPr(args: { env: Env; @@ -188,12 +189,23 @@ export async function maybeSuggestMilestoneMatchForPr(args: { prTitle: string; prBody: string | null | undefined; mode: ProjectMilestoneMatchModeInput; - onError: (error: unknown) => void; + deliveryId: string; }): Promise { if (!args.installationId) return; if (args.prState !== "open") return; if (!args.mode || args.mode === "off") return; - await maybeSuggestMilestoneMatch({ env: args.env, installationId: args.installationId, repoFullName: args.repoFullName }, args.pullNumber, args.prTitle, args.prBody).catch(args.onError); + await maybeSuggestMilestoneMatch({ env: args.env, installationId: args.installationId, repoFullName: args.repoFullName }, args.pullNumber, args.prTitle, args.prBody).catch((error) => { + console.error( + JSON.stringify({ + level: "warn", + event: "milestone_suggest_failed", + deliveryId: args.deliveryId, + repoFullName: args.repoFullName, + pullNumber: args.pullNumber, + error: errorMessage(error), + }), + ); + }); } // Kept as a standalone alias (rather than importing RepositorySettings from ../types) so this integrations diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 35c3314c90..6ce3a302a1 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5304,8 +5304,8 @@ async function processGitHubWebhook( await persistAdvisory(env, advisory); // Auto-project/milestone matching (#3183): independent of the gate/disposition entirely -- a missed or // wrong match must never affect CI/merge, so this is a best-effort side comment, never a blocker. All the - // "should this run at all" gating + error swallowing lives in maybeSuggestMilestoneMatchForPr itself, so - // this call site stays a single unconditional call. + // "should this run at all" gating + error logging lives in maybeSuggestMilestoneMatchForPr itself, so + // this call site stays a single unconditional call with no logic of its own. await maybeSuggestMilestoneMatchForPr({ env, installationId, @@ -5315,18 +5315,7 @@ async function processGitHubWebhook( prTitle: pr.title, prBody: pr.body, mode: settings.autoProjectMilestoneMatch, - onError: (error) => { - console.error( - JSON.stringify({ - level: "warn", - event: "milestone_suggest_failed", - deliveryId, - repoFullName, - pullNumber: pr.number, - error: errorMessage(error), - }), - ); - }, + deliveryId, }); // Draft-dodge guard (#converted-to-draft): a contributor converting an OPEN PR to draft cannot use // draft state to keep a gate-rejected PR alive. When a prior gate failure exists for the PR's current diff --git a/test/unit/project-tracker-adapter.test.ts b/test/unit/project-tracker-adapter.test.ts index e49f4299b4..a657458c67 100644 --- a/test/unit/project-tracker-adapter.test.ts +++ b/test/unit/project-tracker-adapter.test.ts @@ -213,7 +213,7 @@ describe("maybeSuggestMilestoneMatchForPr (#3183 webhook-level gating)", () => { prTitle: "Improve self-host reliability roadmap convergence", prBody: "Follow-up on the self-host reliability roadmap work", mode: "suggest" as const, - onError: vi.fn(), + deliveryId: "test-delivery", ...overrides, }; } @@ -224,10 +224,8 @@ describe("maybeSuggestMilestoneMatchForPr (#3183 webhook-level gating)", () => { called = true; return new Response("unexpected", { status: 500 }); }); - const onError = vi.fn(); - await maybeSuggestMilestoneMatchForPr(baseArgs({ installationId: null, onError })); + await maybeSuggestMilestoneMatchForPr(baseArgs({ installationId: null })); expect(called).toBe(false); - expect(onError).not.toHaveBeenCalled(); }); it("does nothing when the PR is not open", async () => { @@ -295,15 +293,18 @@ describe("maybeSuggestMilestoneMatchForPr (#3183 webhook-level gating)", () => { expect(milestonesFetched).toBe(true); }); - it("routes a failure to onError instead of throwing", async () => { + it("logs a failure instead of throwing", async () => { vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { const url = input.toString(); if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); if (url.includes("/milestones")) return new Response("boom", { status: 500 }); return new Response("unexpected", { status: 500 }); }); - const onError = vi.fn(); - await expect(maybeSuggestMilestoneMatchForPr(baseArgs({ onError }))).resolves.toBeUndefined(); - expect(onError).toHaveBeenCalledTimes(1); + const consoleError = vi.spyOn(console, "error").mockImplementation(() => undefined); + await expect(maybeSuggestMilestoneMatchForPr(baseArgs({ deliveryId: "delivery-42" }))).resolves.toBeUndefined(); + expect(consoleError).toHaveBeenCalledTimes(1); + const logged = JSON.parse(String(consoleError.mock.calls[0]?.[0])); + expect(logged).toMatchObject({ event: "milestone_suggest_failed", deliveryId: "delivery-42", repoFullName: "JSONbored/gittensory", pullNumber: 4 }); + consoleError.mockRestore(); }); }); From f20f1be7ac9b6b019349cf31d30e5da02bf2a66a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 16:38:22 -0700 Subject: [PATCH 3/3] fix(agent): paginate milestone/comment listing and harden the suggest comment Gate review found a real gap: GitHub milestones and issue comments both paginate at 100 per page, but listOpenMilestones and the suggest comment's marker search only ever read page 1 -- a repo with >100 open milestones, or a busy PR with >100 comments, could silently miss a match or double-post. Both now page up to a bounded limit (mirroring the same pattern already used in src/github/comments.ts). Also: attachToMilestone now rejects a non-positive-integer milestoneId instead of sending NaN to GitHub, and the suggestion comment code-formats the milestone title (stripping literal backticks) so a maintainer-authored title containing @mentions or markdown emphasis can't leak into the rendered comment. --- src/integrations/project-tracker-adapter.ts | 67 ++++++++--- test/unit/project-tracker-adapter.test.ts | 126 ++++++++++++++++++++ 2 files changed, 175 insertions(+), 18 deletions(-) diff --git a/src/integrations/project-tracker-adapter.ts b/src/integrations/project-tracker-adapter.ts index 77e9c7b5ee..9530bc6794 100644 --- a/src/integrations/project-tracker-adapter.ts +++ b/src/integrations/project-tracker-adapter.ts @@ -49,6 +49,18 @@ type GitHubMilestone = { title: string; }; +// Bounded pagination for both the milestone list and the comment-marker search below (mirrors +// src/github/comments.ts's COMMENT_SEARCH_PAGE_LIMIT): 3 pages * 100 = 300 items is generously above any +// realistic open-milestone or PR-comment count, while still bounding worst-case GitHub API calls per PR event. +const GITHUB_LIST_PAGE_LIMIT = 3; + +/** A positive-integer milestone/issue number as a string, or null if `value` isn't one. Guards against a + * malformed/forged `milestoneId` reaching GitHub's PATCH as `NaN` or a negative/zero number. */ +function parsePositiveIntegerId(value: string): number | null { + const parsed = Number(value); + return Number.isInteger(parsed) && parsed > 0 ? parsed : null; +} + /** GitHub REST implementation of {@link ProjectTrackerAdapter}. Only the Milestone half is real (#3183) -- * Projects v2 is GraphQL-only and needs a separate `organization_projects` App permission not yet granted, so * those two methods are inert placeholders until #3184. */ @@ -62,13 +74,19 @@ export class GitHubMilestonesAdapter implements ProjectTrackerAdapter { const { owner, repo } = parseRepoFullName(ctx.repoFullName); const token = await createInstallationToken(ctx.env, ctx.installationId); const octokit = makeInstallationOctokit(ctx.env, token, "live", githubRateLimitAdmissionKeyForInstallation(ctx.installationId)); - const response = await octokit.request("GET /repos/{owner}/{repo}/milestones", { - owner, - repo, - state: "open", - per_page: 100, - }); - const milestones = response.data as GitHubMilestone[]; + const milestones: GitHubMilestone[] = []; + for (let page = 1; page <= GITHUB_LIST_PAGE_LIMIT; page += 1) { + const response = await octokit.request("GET /repos/{owner}/{repo}/milestones", { + owner, + repo, + state: "open", + per_page: 100, + page, + }); + const batch = response.data as GitHubMilestone[]; + milestones.push(...batch); + if (batch.length < 100) break; + } return milestones.map((milestone) => ({ id: String(milestone.number), title: milestone.title })); } @@ -78,6 +96,8 @@ export class GitHubMilestonesAdapter implements ProjectTrackerAdapter { } async attachToMilestone(ctx: ProjectTrackerContext, pullNumber: number, milestoneId: string): Promise { + const milestoneNumber = parsePositiveIntegerId(milestoneId); + if (milestoneNumber === null) return { attached: false }; const { owner, repo } = parseRepoFullName(ctx.repoFullName); const token = await createInstallationToken(ctx.env, ctx.installationId); const octokit = makeInstallationOctokit(ctx.env, token, "live", githubRateLimitAdmissionKeyForInstallation(ctx.installationId)); @@ -85,7 +105,7 @@ export class GitHubMilestonesAdapter implements ProjectTrackerAdapter { owner, repo, issue_number: pullNumber, - milestone: Number(milestoneId), + milestone: milestoneNumber, }); return { attached: true }; } @@ -128,11 +148,18 @@ export function matchOpenMilestones(prTitle: string, prBody: string | null | und export const MILESTONE_SUGGEST_COMMENT_MARKER = ""; +/** Code-formats a maintainer-authored title for safe Markdown embedding: backticks strip any literal backtick + * from the title (so it can't break out of the code span) rather than escaping them, since a broken-out title + * could otherwise re-enable `@mentions` or `**`/`_` emphasis the code span exists to neutralize. */ +function codeFormat(title: string): string { + return `\`${title.replace(/`/g, "")}\``; +} + function renderSuggestionComment(match: ProjectTrackerMatch): string { const confidencePercent = Math.round(match.score * 100); return [ MILESTONE_SUGGEST_COMMENT_MARKER, - `This PR looks like it's part of the **${match.milestone.title}** milestone (${confidencePercent}% title/body term overlap).`, + `This PR looks like it's part of the ${codeFormat(match.milestone.title)} milestone (${confidencePercent}% title/body term overlap).`, "", "This is an advisory suggestion only — nothing has been attached automatically.", ].join("\n"); @@ -158,15 +185,19 @@ export async function maybeSuggestMilestoneMatch(ctx: ProjectTrackerContext, pul const token = await createInstallationToken(ctx.env, ctx.installationId); const octokit = makeInstallationOctokit(ctx.env, token, "live", githubRateLimitAdmissionKeyForInstallation(ctx.installationId)); const botLogin = `${ctx.env.GITHUB_APP_SLUG}[bot]`; - const existing = await octokit.request("GET /repos/{owner}/{repo}/issues/{issue_number}/comments", { - owner, - repo, - issue_number: pullNumber, - per_page: 100, - }); - const alreadyPosted = (existing.data as IssueComment[]).some( - (comment) => comment.user?.type === "Bot" && comment.user.login?.toLowerCase() === botLogin.toLowerCase() && comment.body?.includes(MILESTONE_SUGGEST_COMMENT_MARKER), - ); + let alreadyPosted = false; + for (let page = 1; page <= GITHUB_LIST_PAGE_LIMIT && !alreadyPosted; page += 1) { + const existing = await octokit.request("GET /repos/{owner}/{repo}/issues/{issue_number}/comments", { + owner, + repo, + issue_number: pullNumber, + per_page: 100, + page, + }); + const batch = existing.data as IssueComment[]; + alreadyPosted = batch.some((comment) => comment.user?.type === "Bot" && comment.user.login?.toLowerCase() === botLogin.toLowerCase() && comment.body?.includes(MILESTONE_SUGGEST_COMMENT_MARKER)); + if (batch.length < 100) break; + } if (alreadyPosted) return { suggested: false }; await createIssueComment(ctx.env, ctx.installationId, ctx.repoFullName, pullNumber, renderSuggestionComment(match)); diff --git a/test/unit/project-tracker-adapter.test.ts b/test/unit/project-tracker-adapter.test.ts index a657458c67..2464ea1b4b 100644 --- a/test/unit/project-tracker-adapter.test.ts +++ b/test/unit/project-tracker-adapter.test.ts @@ -89,6 +89,69 @@ describe("GitHubMilestonesAdapter (#3183)", () => { expect(result).toEqual({ attached: true }); expect(patchedBody).toMatchObject({ milestone: 14 }); }); + + it("attachToMilestone rejects a non-positive-integer milestoneId without calling GitHub", async () => { + let patched = false; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if ((init?.method ?? "GET") === "PATCH") { + patched = true; + return Response.json({}); + } + return new Response("unexpected", { status: 500 }); + }); + const adapter = new GitHubMilestonesAdapter(); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }); + for (const invalidId of ["not-a-number", "0", "-5", "3.5", ""]) { + const result = await adapter.attachToMilestone({ env, installationId: 123, repoFullName: "JSONbored/gittensory" }, 4, invalidId); + expect(result).toEqual({ attached: false }); + } + expect(patched).toBe(false); + }); + + it("listOpenMilestones paginates past the first 100 results (regression: gate-flagged pagination gap)", async () => { + const pageOne = Array.from({ length: 100 }, (_, i) => ({ number: i + 1, title: `Milestone ${i + 1}` })); + const pageTwoMatch = { number: 101, title: "Self-host reliability roadmap" }; + let requestedPages: number[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/milestones")) { + const page = Number(new URL(url).searchParams.get("page") ?? "1"); + requestedPages.push(page); + if (page === 1) return Response.json(pageOne); + if (page === 2) return Response.json([pageTwoMatch]); + return Response.json([]); + } + return new Response("unexpected", { status: 500 }); + }); + const adapter = new GitHubMilestonesAdapter(); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }); + const result = await adapter.listOpenMilestones({ env, installationId: 123, repoFullName: "JSONbored/gittensory" }); + expect(requestedPages).toEqual([1, 2]); + expect(result).toHaveLength(101); + expect(result).toContainEqual({ id: "101", title: "Self-host reliability roadmap" }); + }); + + it("listOpenMilestones stops paginating at the configured page limit even if GitHub reports more", async () => { + let requestedPages: number[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/milestones")) { + const page = Number(new URL(url).searchParams.get("page") ?? "1"); + requestedPages.push(page); + // Always a full page, so the loop would run forever without the hard page-limit cap. + return Response.json(Array.from({ length: 100 }, (_, i) => ({ number: page * 1000 + i, title: "filler" }))); + } + return new Response("unexpected", { status: 500 }); + }); + const adapter = new GitHubMilestonesAdapter(); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }); + await adapter.listOpenMilestones({ env, installationId: 123, repoFullName: "JSONbored/gittensory" }); + expect(requestedPages).toEqual([1, 2, 3]); + }); }); describe("maybeSuggestMilestoneMatch (#3183)", () => { @@ -124,6 +187,69 @@ describe("maybeSuggestMilestoneMatch (#3183)", () => { expect(posted[0]).toContain("Self-host reliability roadmap"); }); + it("code-formats the milestone title and strips literal backticks, neutralizing markdown/mention injection", async () => { + const posted: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/milestones")) return Response.json([{ number: 14, title: "self host reliability roadmap `@everyone` **pwned**" }]); + if (url.includes("/issues/4/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/4/comments") && method === "POST") { + const body = JSON.parse(String(init?.body ?? "{}")) as { body?: string }; + posted.push(body.body ?? ""); + return Response.json({ id: 1 }); + } + return new Response("unexpected", { status: 500 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await maybeSuggestMilestoneMatch( + { env, installationId: 123, repoFullName: "JSONbored/gittensory" }, + 4, + "self host reliability roadmap convergence", + "self host reliability roadmap convergence work", + ); + expect(posted).toHaveLength(1); + // The rendered title is wrapped in a single code span with every literal backtick stripped -- no unescaped + // backtick can break out of the span and re-enable the mention/emphasis markup it carries. + expect(posted[0]).toContain("`self host reliability roadmap @everyone **pwned**`"); + expect(posted[0]).not.toMatch(/`[^`]*`[^`]*`/); + }); + + it("paginates the comment-marker search past the first 100 comments before deciding to post", async () => { + const pageOneComments = Array.from({ length: 100 }, (_, i) => ({ body: `unrelated comment ${i}`, user: { type: "User", login: "someone" } })); + let posted = false; + let requestedPages: number[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/milestones")) return Response.json([{ number: 14, title: "Self-host reliability roadmap" }]); + if (url.includes("/issues/4/comments") && method === "GET") { + const page = Number(new URL(url).searchParams.get("page") ?? "1"); + requestedPages.push(page); + if (page === 1) return Response.json(pageOneComments); + if (page === 2) return Response.json([{ body: MILESTONE_SUGGEST_COMMENT_MARKER, user: { type: "Bot", login: "gittensory[bot]" } }]); + return Response.json([]); + } + if (url.includes("/issues/4/comments") && method === "POST") { + posted = true; + return Response.json({ id: 1 }); + } + return new Response("unexpected", { status: 500 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + const result = await maybeSuggestMilestoneMatch( + { env, installationId: 123, repoFullName: "JSONbored/gittensory" }, + 4, + "Improve self-host reliability roadmap convergence", + "Follow-up on the self-host reliability roadmap work", + ); + expect(requestedPages).toEqual([1, 2]); + expect(result).toEqual({ suggested: false }); + expect(posted).toBe(false); + }); + it("does nothing when no milestone matches (never calls the comment POST endpoint)", async () => { let posted = false; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {