diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 606825a657..8e25c88d93 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -9201,6 +9201,128 @@ export function reviewDurationMsSince(startedAt: string | null, nowMs: number): return Number.isFinite(ms) && ms >= 0 ? ms : undefined; } +/** + * Focus-manifest policy gate (#555, opt-in via `manifestPolicyGateMode`). Reloads the CACHED manifest (the + * settings resolver discards the raw manifest, but loadRepoFocusManifest is cached so this is cheap), + * recomputes the guidance over the PR's changed files, and pushes ONLY the three enforceable policy findings + * onto the advisory so isConfiguredGateBlocker can block under `manifestPolicy: block`. Also runs the E2E + * test-generation auto-trigger (#4196, part of the #4189 epic) — see the inline comments below for the full + * rationale of each step. `manifestPolicyGateMode: "off"` (the default) is a no-op, so the advisory/gate + * stays byte-identical to today. Extracted from maybePublishPrPublicSurface (#4607) — pure code motion; + * every branch, condition, and comment is preserved verbatim and in the same order. + */ +async function maybeApplyManifestPolicyGate( + env: Env, + args: { + repoFullName: string; + installationId: number; + pr: Awaited>; + repo: Awaited>; + settings: RepositorySettings; + advisory: Awaited>; + webhook: { liveFacts: LiveGithubFacts; deliveryId: string }; + gateFiles: Awaited> | null; + author: string | null; + }, +): Promise { + // Focus-manifest policy (#555, opt-in via manifestPolicyGateMode). Reload the CACHED manifest (the + // settings resolver discards the raw manifest, but loadRepoFocusManifest is cached so this is cheap), + // recompute the guidance over the PR's changed files, and push ONLY the three enforceable policy + // findings into the advisory so isConfiguredGateBlocker can block under manifestPolicy: block. + if (args.settings.manifestPolicyGateMode !== "off") { + // `gateFiles` is threaded in by the ONLY caller (maybePublishPrPublicSurface) already resolved via + // getReviewFiles() whenever manifestPolicyGateMode is not "off" -- the same condition gating this whole + // block -- so it is never actually null here; the `| null` on the parameter type exists only because the + // caller's own local starts as `let gateFiles: ... | null = null` for TypeScript soundness before that + // conditional assignment runs. The `?? []` fallback is unreachable on this webhook-integration path + // (pre-existing on origin/main before this function was extracted from maybePublishPrPublicSurface, #4607). + /* v8 ignore next -- see the comment above */ + const manifestFiles = args.gateFiles ?? []; + const manifest = await loadRepoFocusManifest(env, args.repoFullName); + const testFileCount = manifestFiles.filter((file) => isTestPath(file.path)).length; + const passedValidationCount = await resolveManifestPassedValidationCount(env, { + repoFullName: args.repoFullName, + installationId: args.installationId, + prNumber: args.pr.number, + headSha: args.pr.headSha, + baseRef: args.pr.baseRef ?? args.repo?.defaultBranch, + body: args.pr.body, + expectedCiContexts: args.settings.expectedCiContexts, + liveFacts: args.webhook.liveFacts, + testExpectationsConfigured: manifest.testExpectations.length > 0, + testFileCount, + }); + const guidance = buildFocusManifestGuidance({ + manifest, + changedPaths: manifestFiles.map((file) => file.path), + labels: args.pr.labels, + linkedIssueCount: args.pr.linkedIssues.length, + testFileCount, + passedValidationCount, + hasNoIssueRationale: hasClearNoIssueRationale(args.pr), + }); + const policyCodes = new Set([ + "manifest_blocked_path", + "manifest_linked_issue_required", + "manifest_missing_tests", + ]); + // Keep deterministic manifest policy findings independent from AI-review eligibility: ignored authors + // suppress review/public output only, never maintainer-configured gate blockers or their downstream triggers. + const policyFindings = guidance.findings; + // Computed once and reused below for the #4196 auto-trigger check -- same feature gate, one call. Also + // feeds #4583's inline CTA so the missing-tests finding surfaces `@gittensory generate-tests` right in + // ORB's own comment (mirrors CodeRabbit's inline walkthrough checkbox) only when the command would + // actually work for this repo, never as noise on a repo that hasn't opted in. + const e2eTestGenAvailable = resolveConvergedFeature(env, manifest, "e2eTests", args.repoFullName); + for (const finding of policyFindings) { + if (!policyCodes.has(finding.code)) continue; + args.advisory.findings.push(publicSafeManifestPolicyFinding(finding, { e2eTestGenAvailable })); + } + // E2E test-generation auto-trigger (#4196, part of the #4189 epic): promotes the deterministic + // manifest_missing_tests finding above from advisory-only text into an actual trigger for #4192/#4194's + // generation-and-render path -- additive to, never a replacement for, the explicit `@gittensory + // generate-tests` command (#4195), which stays available regardless of whether this signal fired. + // Filters the SAME policyFindings just computed above rather than re-deriving "PR probably needs + // tests" from scratch, per the issue's own requirement -- this is why the auto-trigger lives inside this + // exact manifestPolicyGateMode-gated block instead of a parallel code path: that is the only place this + // finding is computed at all today. + if (args.pr.headSha && policyFindings.some((finding) => finding.code === "manifest_missing_tests") && e2eTestGenAvailable) { + const e2eTargetKey = `${args.repoFullName}#${args.pr.number}`; + // Double-generation guard: an unchanged head SHA re-entering this pass (a re-review/sweep tick, not a + // new push) must never re-spend an LLM call or repost a duplicate suggestion. A genuinely NEW push + // (a new head SHA) is always a fresh miss here regardless of how many prior SHAs already fired. The + // explicit command deliberately does NOT consult this guard -- a maintainer typing the command always + // gets a fresh generation, even on a SHA the auto-trigger already covered (simplicity over a cache that + // would need its own invalidation rules; the daily neuron budget shared by both paths already bounds + // the cost of a maintainer choosing to ask twice). + const alreadyTriggered = await hasAuditEventForHeadSha(env, "github_app.e2e_tests_generation", e2eTargetKey, args.pr.headSha); + if (!alreadyTriggered) { + const e2eMode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isDbFrozenForRepo(env, args.settings.agentGlobalFreezeOverride)), agentPaused: args.settings.agentPaused, agentDryRun: args.settings.agentDryRun }); + if (e2eMode === "live") { + await runE2eTestGenerationAndDeliver(env, { + repoFullName: args.repoFullName, + installationId: args.installationId, + pr: args.pr, + settings: args.settings, + manifest, + files: manifestFiles, + // No comment-invoker exists for an automated trigger -- the PR's own author is the closest + // analogue to "who this generated test is for" (unlike the explicit command, where `actor` is + // whoever typed the command). + actor: args.author ?? "the PR author", + mode: e2eMode, + deliveryId: args.webhook.deliveryId, + targetKey: e2eTargetKey, + trigger: "auto", + }); + } else { + await recordGenerateTestsSkip(env, args.webhook.deliveryId, args.repoFullName, e2eTargetKey, args.author, e2eMode === "dry_run" ? "dry_run" : "agent_paused"); + } + } + } + } +} + async function maybePublishPrPublicSurface( env: Env, installationId: number, @@ -10080,95 +10202,18 @@ async function maybePublishPrPublicSurface( }); } } - // Focus-manifest policy (#555, opt-in via manifestPolicyGateMode). Reload the CACHED manifest (the - // settings resolver discards the raw manifest, but loadRepoFocusManifest is cached so this is cheap), - // recompute the guidance over the PR's changed files, and push ONLY the three enforceable policy - // findings into the advisory so isConfiguredGateBlocker can block under manifestPolicy: block. - if (settings.manifestPolicyGateMode !== "off") { - const manifestFiles = gateFiles ?? []; - const manifest = await loadRepoFocusManifest(env, repoFullName); - const testFileCount = manifestFiles.filter((file) => isTestPath(file.path)).length; - const passedValidationCount = await resolveManifestPassedValidationCount(env, { - repoFullName, - installationId, - prNumber: pr.number, - headSha: pr.headSha, - baseRef: pr.baseRef ?? repo?.defaultBranch, - body: pr.body, - expectedCiContexts: settings.expectedCiContexts, - liveFacts: webhook.liveFacts, - testExpectationsConfigured: manifest.testExpectations.length > 0, - testFileCount, - }); - const guidance = buildFocusManifestGuidance({ - manifest, - changedPaths: manifestFiles.map((file) => file.path), - labels: pr.labels, - linkedIssueCount: pr.linkedIssues.length, - testFileCount, - passedValidationCount, - hasNoIssueRationale: hasClearNoIssueRationale(pr), - }); - const policyCodes = new Set([ - "manifest_blocked_path", - "manifest_linked_issue_required", - "manifest_missing_tests", - ]); - // Keep deterministic manifest policy findings independent from AI-review eligibility: ignored authors - // suppress review/public output only, never maintainer-configured gate blockers or their downstream triggers. - const policyFindings = guidance.findings; - // Computed once and reused below for the #4196 auto-trigger check -- same feature gate, one call. Also - // feeds #4583's inline CTA so the missing-tests finding surfaces `@gittensory generate-tests` right in - // ORB's own comment (mirrors CodeRabbit's inline walkthrough checkbox) only when the command would - // actually work for this repo, never as noise on a repo that hasn't opted in. - const e2eTestGenAvailable = resolveConvergedFeature(env, manifest, "e2eTests", repoFullName); - for (const finding of policyFindings) { - if (!policyCodes.has(finding.code)) continue; - advisory.findings.push(publicSafeManifestPolicyFinding(finding, { e2eTestGenAvailable })); - } - // E2E test-generation auto-trigger (#4196, part of the #4189 epic): promotes the deterministic - // manifest_missing_tests finding above from advisory-only text into an actual trigger for #4192/#4194's - // generation-and-render path -- additive to, never a replacement for, the explicit `@gittensory - // generate-tests` command (#4195), which stays available regardless of whether this signal fired. - // Filters the SAME policyFindings just computed above rather than re-deriving "PR probably needs - // tests" from scratch, per the issue's own requirement -- this is why the auto-trigger lives inside this - // exact manifestPolicyGateMode-gated block instead of a parallel code path: that is the only place this - // finding is computed at all today. - if (pr.headSha && policyFindings.some((finding) => finding.code === "manifest_missing_tests") && e2eTestGenAvailable) { - const e2eTargetKey = `${repoFullName}#${pr.number}`; - // Double-generation guard: an unchanged head SHA re-entering this pass (a re-review/sweep tick, not a - // new push) must never re-spend an LLM call or repost a duplicate suggestion. A genuinely NEW push - // (a new head SHA) is always a fresh miss here regardless of how many prior SHAs already fired. The - // explicit command deliberately does NOT consult this guard -- a maintainer typing the command always - // gets a fresh generation, even on a SHA the auto-trigger already covered (simplicity over a cache that - // would need its own invalidation rules; the daily neuron budget shared by both paths already bounds - // the cost of a maintainer choosing to ask twice). - const alreadyTriggered = await hasAuditEventForHeadSha(env, "github_app.e2e_tests_generation", e2eTargetKey, pr.headSha); - if (!alreadyTriggered) { - const e2eMode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isDbFrozenForRepo(env, settings.agentGlobalFreezeOverride)), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun }); - if (e2eMode === "live") { - await runE2eTestGenerationAndDeliver(env, { - repoFullName, - installationId, - pr, - settings, - manifest, - files: manifestFiles, - // No comment-invoker exists for an automated trigger -- the PR's own author is the closest - // analogue to "who this generated test is for" (unlike the explicit command, where `actor` is - // whoever typed the command). - actor: author ?? "the PR author", - mode: e2eMode, - deliveryId: webhook.deliveryId, - targetKey: e2eTargetKey, - trigger: "auto", - }); - } else { - await recordGenerateTestsSkip(env, webhook.deliveryId, repoFullName, e2eTargetKey, author, e2eMode === "dry_run" ? "dry_run" : "agent_paused"); - } - } - } - } + // Focus-manifest policy gate (#555) -- see maybeApplyManifestPolicyGate's own doc comment. + await maybeApplyManifestPolicyGate(env, { + repoFullName, + installationId, + pr, + repo, + settings, + advisory, + webhook, + gateFiles, + author, + }); // Pre-merge checks (#review-pre-merge-checks, opt-in via .gittensory.yml review.pre_merge_checks). DETERMINISTIC // content assertions (title/description must contain a phrase, a label must be present), optionally path-gated. // Each FAILED check appends an advisory `pre_merge_check_failed` finding — or a blocking `pre_merge_check_required` diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 947b54751a..d8e0087de1 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -10015,6 +10015,105 @@ describe("queue processors", () => { expect(JSON.stringify(gatePatches[0])).toContain("Configured validation evidence missing"); }); + // #4607 (maybeApplyManifestPolicyGate extraction): buildFocusManifestGuidance can produce findings whose + // code is NOT one of the three enforceable manifest-policy codes (manifest_blocked_path / + // manifest_linked_issue_required / manifest_missing_tests) -- e.g. manifest_off_focus, when wantedPaths is + // configured and no changed path matches it. Those non-enforceable findings must be filtered out before + // ever reaching the advisory/gate, never published alongside an enforceable one from the same pass. + it("filters out a non-enforceable manifest finding (manifest_off_focus) while still surfacing an enforceable one", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertInstallation(env, { + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", pull_requests: "write", issues: "write" }, + events: ["pull_request"], + }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "off", + publicSurface: "off", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "enabled", + linkedIssueGateMode: "off", + manifestPolicyGateMode: "block", + requireLinkedIssue: false, + typeLabelsEnabled: false, + }); + // wantedPaths configured + a changed file outside it produces manifest_off_focus (NOT one of the three + // enforceable codes); testExpectations configured + no evidence produces manifest_missing_tests (IS + // enforceable) -- so this single pass yields one filtered finding and one published finding. + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { + wantedPaths: ["docs/"], + testExpectations: ["Run npm run test:ci."], + }); + await upsertPullRequestFile(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 45, + path: "src/feature.ts", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }); + + const gatePatches: Array> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/commits/gate-off-focus/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/check-runs") && method === "POST") return Response.json({ id: 903 }, { status: 201 }); + if (url.includes("/check-runs/903") && method === "PATCH") { + gatePatches.push(JSON.parse(String(init?.body ?? "{}")) as Record); + return Response.json({ id: 903, html_url: "https://github.com/checks/903" }); + } + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "gate-off-focus-filtered", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 45, + title: "Out of focus change", + state: "open", + user: { login: "contributor" }, + head: { sha: "gate-off-focus" }, + labels: [], + body: "No tests run.", + }, + }, + }); + + expect(gatePatches).toHaveLength(1); + // The enforceable finding (manifest_missing_tests) is published... + expect(JSON.stringify(gatePatches[0])).toContain("Configured validation evidence missing"); + // ...but the non-enforceable finding (manifest_off_focus) is filtered out before it ever reaches the advisory. + expect(JSON.stringify(gatePatches[0])).not.toContain("Change is outside maintainer-wanted areas"); + expect(JSON.stringify(gatePatches[0])).not.toContain("manifest_off_focus"); + }); + // REGRESSION (#3304): a PR with no body at all (GitHub sends `body: null` for an empty description) must // fall back to treating validation evidence as absent, not throw or silently pass the manifest gate. it("still flags manifest_missing_tests for a PR with a null body", async () => {