From 075fcc09e92970dbf8e303050e8d93dacc43f841 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 16:58:34 -0700 Subject: [PATCH] feat(review): decouple e2e-test-gen auto-trigger and widen checkbox auth (#4199) The manifest_missing_tests auto-trigger and the maintainer-initiated command/checkbox paths shared one flag (features.e2eTests), so enabling on-demand test generation for a repo also silently turned on unprompted auto-firing. Add review.e2e_test_auto_trigger (default off, per-repo) as a separate, explicit opt-in required in addition to features.e2eTests. The generate-tests checkbox also hardcoded a maintainer-only commandAuthorization override, ignoring whatever a repo's own .gittensory.yml configured -- unlike the identical text command, which already respects settings.commandAuthorization. Remove the override so the checkbox resolves the same, already-safely-clamped policy (still maintainer-only by default; widenable per repo to collaborator/ confirmed_miner, never to the spoofable raw pr_author role). Add a [BETA] badge to the checkbox label. --- .gittensory.yml.example | 7 ++ config/examples/gittensory.full.yml | 7 ++ .../gittensory-engine/src/focus-manifest.ts | 21 +++- src/queue/processors.ts | 38 +++--- src/signals/focus-manifest.ts | 10 ++ test/unit/focus-manifest.test.ts | 33 +++++- test/unit/queue.test.ts | 110 +++++++++++++++++- test/unit/signals-coverage.test.ts | 2 +- 8 files changed, 200 insertions(+), 28 deletions(-) diff --git a/.gittensory.yml.example b/.gittensory.yml.example index c1e54b9192..1495f00742 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -517,6 +517,13 @@ review: # comment | commit. Default: null/comment (byte-identical -- no write access to any PR branch). # e2e_test_delivery: comment + # Opts THIS repo into the manifest_missing_tests AUTO-TRIGGER (#4196, part of the #4189 epic) -- an + # unprompted generation run whenever a PR looks like it needs tests. Separate from features.e2eTests, which + # only unlocks the maintainer-initiated paths (the `@gittensory generate-tests` command and the PR-panel + # checkbox). Bool | null. Default: null/false -- the auto-trigger never fires, even with e2eTests already + # on; a maintainer who wants unprompted generation opts in here explicitly, per repo. + # e2e_test_auto_trigger: false + # Inline-comment layer toggles (#1956 / #1958). Bool | null. Default: null/false — byte-identical. # Requires operator flag GITTENSORY_REVIEW_INLINE_COMMENTS + cutover allowlist + review.inline_comments: true. # inline_comments: false diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index 06fadd51d2..6fefc1abc6 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -530,6 +530,13 @@ review: # comment | commit. Default: null/comment (byte-identical -- no write access to any PR branch). # e2e_test_delivery: comment + # Opts THIS repo into the manifest_missing_tests AUTO-TRIGGER (#4196, part of the #4189 epic) -- an + # unprompted generation run whenever a PR looks like it needs tests. Separate from features.e2eTests, which + # only unlocks the maintainer-initiated paths (the `@gittensory generate-tests` command and the PR-panel + # checkbox). Bool | null. Default: null/false -- the auto-trigger never fires, even with e2eTests already + # on; a maintainer who wants unprompted generation opts in here explicitly, per repo. + # e2e_test_auto_trigger: false + # Inline-comment layer toggles (#1956 / #1958). Bool | null. Default: null/false — byte-identical. # Requires operator flag GITTENSORY_REVIEW_INLINE_COMMENTS + cutover allowlist + review.inline_comments: true. # inline_comments: false diff --git a/packages/gittensory-engine/src/focus-manifest.ts b/packages/gittensory-engine/src/focus-manifest.ts index 3eb5dbce57..924210b14c 100644 --- a/packages/gittensory-engine/src/focus-manifest.ts +++ b/packages/gittensory-engine/src/focus-manifest.ts @@ -582,6 +582,15 @@ export type FocusManifestReviewConfig = { * is a confirmed Gittensor miner, to protect the external, upstream-computed score from ever including a * maintainer-authored line the miner didn't write themselves — see `src/github/e2e-test-commit.ts`. */ e2eTestDelivery: E2eTestDeliveryMode | null; + /** `review.e2e_test_auto_trigger` (#4196, part of the #4189 epic): opts THIS repo into the `manifest_missing_tests` + * auto-trigger, which promotes that advisory finding into an actual unprompted generation run whenever a PR looks + * like it needs tests -- separate from `features.e2eTests`, which only unlocks the maintainer-initiated paths + * (the `@gittensory generate-tests` command and the PR-panel checkbox). Deliberately independent and OFF by + * default: enabling `e2eTests` for on-demand use must never, by itself, start firing generation unprompted on + * every under-tested PR (the exact loophole this field closes) -- a maintainer who *wants* the auto-trigger opts + * in explicitly per repo. null/false (default, absent) ⇒ the auto-trigger never fires, even with e2eTests on; + * true additionally requires e2eTests to already be enabled (this field alone does nothing). */ + e2eTestAutoTrigger: boolean | null; /** `review.path_instructions`: per-path natural-language guidance handed to the AI reviewer when the PR's * changed files match the glob. Empty (default) ⇒ byte-identical reviewer prompt. Also consumed by * AI-generated E2E test coverage (`resolveE2eTestGenInstructions` in `ai-e2e-test-gen.ts`, #4200) when @@ -1034,7 +1043,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, e2eTestDelivery: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -1065,7 +1074,7 @@ function emptyManifest(source: FocusManifestSource, warnings: string[] = []): Fo warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, e2eTestDelivery: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -2170,7 +2179,7 @@ function parsePublicSafeText(value: JsonValue | undefined, field: string, warnin * throws; invalid/unsafe values are dropped with warnings. */ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): FocusManifestReviewConfig { - const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, e2eTestDelivery: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }; if (value === undefined || value === null) return empty; if (typeof value !== "object" || Array.isArray(value)) { warnings.push(`Manifest field "review" must be a mapping; ignoring it.`); @@ -2231,6 +2240,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const maxFindings = parseMaxFindingsConfig(r.max_findings, warnings); const commentVerbosity = normalizeOptionalEnum(r.comment_verbosity, "review.comment_verbosity", COMMENT_VERBOSITY_LEVELS, warnings); const e2eTestDelivery = normalizeOptionalEnum(r.e2e_test_delivery, "review.e2e_test_delivery", E2E_TEST_DELIVERY_MODES, warnings); + const e2eTestAutoTrigger = normalizeOptionalBoolean(r.e2e_test_auto_trigger, "review.e2e_test_auto_trigger", warnings); const pathInstructions = parseReviewPathInstructions(r.path_instructions, warnings); const instructions = parsePublicSafeText(r.instructions, "review.instructions", warnings); const excludePaths = parseReviewExcludePaths(r.exclude_paths, warnings); @@ -2264,6 +2274,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo maxFindingsPresent(maxFindings) || commentVerbosity !== null || e2eTestDelivery !== null || + e2eTestAutoTrigger !== null || pathInstructions.length > 0 || instructions !== null || excludePaths.length > 0 || @@ -2303,6 +2314,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo maxFindings, commentVerbosity, e2eTestDelivery, + e2eTestAutoTrigger, pathInstructions, instructions, excludePaths, @@ -2402,6 +2414,7 @@ function computeReviewConfigPresent(review: Omit 0 || review.instructions !== null || review.excludePaths.length > 0 || @@ -2447,6 +2460,7 @@ export function overlayReviewConfig( maxFindings: overlayMaxFindingsConfig(base.maxFindings, override.maxFindings), commentVerbosity: pickOverlayNullable(override.commentVerbosity, base.commentVerbosity), e2eTestDelivery: pickOverlayNullable(override.e2eTestDelivery, base.e2eTestDelivery), + e2eTestAutoTrigger: pickOverlayNullable(override.e2eTestAutoTrigger, base.e2eTestAutoTrigger), pathInstructions: override.pathInstructions.length > 0 ? [...override.pathInstructions] : [...base.pathInstructions], instructions: pickOverlayNullable(override.instructions, base.instructions), excludePaths: pickOverlayStringList(override.excludePaths, base.excludePaths), @@ -2972,6 +2986,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue } if (review.commentVerbosity !== null) out.comment_verbosity = review.commentVerbosity; if (review.e2eTestDelivery !== null) out.e2e_test_delivery = review.e2eTestDelivery; + if (review.e2eTestAutoTrigger !== null) out.e2e_test_auto_trigger = review.e2eTestAutoTrigger; if (review.instructions !== null) out.instructions = review.instructions; if (review.pathInstructions.length > 0) out.path_instructions = review.pathInstructions.map((entry) => ({ path: entry.path, instructions: entry.instructions })); if (review.excludePaths.length > 0) out.exclude_paths = [...review.excludePaths]; diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9253c5fca6..22447ab0f3 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -433,6 +433,7 @@ import { resolveReviewPreMergeChecks, resolveReviewPromptOverrides, resolveReviewMemoryManifestToggle, + resolveE2eTestAutoTriggerManifestToggle, resolveReviewVisualConfig, type AiReviewCadence, type FocusManifestFinding, @@ -624,15 +625,6 @@ const PR_PANEL_RETRIGGER_COMMAND_AUTHORIZATION: RepositoryCommandAuthorizationPo default: ["maintainer", "collaborator"], commands: { "review-now": ["maintainer", "collaborator"] }, }; -// #4589: the generate-tests checkbox is hardcoded to maintainer-only regardless of what a repo's own -// .gittensory.yml commandAuthorization might configure for the text-command version of generate-tests (which -// CAN be widened to collaborator/confirmed_miner) -- a one-click checkbox is meaningfully lower-friction than -// typing a command, so it gets a hard floor that can't be misconfigured away. Mirrors -// PR_PANEL_RETRIGGER_COMMAND_AUTHORIZATION's exact same override pattern, one tier narrower (no collaborator). -const PR_PANEL_GENERATE_TESTS_COMMAND_AUTHORIZATION: RepositoryCommandAuthorizationPolicy = { - default: ["maintainer"], - commands: { "generate-tests": ["maintainer"] }, -}; const PR_PUBLIC_SURFACE_ACTIONS = new Set([ "opened", "reopened", @@ -9285,7 +9277,12 @@ async function maybeApplyManifestPolicyGate( // 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) { + // Deliberately gated by its OWN separate manifest toggle (review.e2e_test_auto_trigger) on top of + // e2eTestGenAvailable -- enabling features.e2eTests only unlocks the maintainer-initiated command/checkbox + // paths below; it must never, by itself, start firing generation unprompted on every under-tested PR. A + // repo opts into the auto-trigger explicitly, in addition to the base feature. + const e2eAutoTriggerOptedIn = resolveE2eTestAutoTriggerManifestToggle(manifest); + if (args.pr.headSha && policyFindings.some((finding) => finding.code === "manifest_missing_tests") && e2eTestGenAvailable && e2eAutoTriggerOptedIn) { 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 @@ -11874,7 +11871,7 @@ async function maybePublishPrPublicSurface( // #4589: only rendered when there's an actual gap AND the checkbox would work for this repo -- same // condition testCoverageBody gates its own (informational) collapsible on, so the two always agree. ...(missingTestsFinding && e2eTestGenAvailable - ? { generateTestsLabel: `${PR_PANEL_GENERATE_TESTS_MARKER} Generate an AI Playwright test for this PR` } + ? { generateTestsLabel: `${PR_PANEL_GENERATE_TESTS_MARKER} **[BETA]** Generate an AI Playwright test for this PR` } : {}), ...(beforeAfter.length > 0 ? { beforeAfter } : {}), ...(changedFilesSummaryEnabledForReview @@ -13337,12 +13334,15 @@ async function maybeProcessPrPanelRetrigger( * `runE2eTestGenerationAndDeliver` core `@gittensory generate-tests` (#4195) and the `manifest_missing_tests` * auto-trigger (#4196) already use, rather than a full panel re-render. * - * Hardcoded to `commandAuthorization: PR_PANEL_GENERATE_TESTS_COMMAND_AUTHORIZATION` (maintainer-only, one - * tier narrower than the retrigger's own maintainer+collaborator floor) regardless of what a repo's own - * `.gittensory.yml` might configure for the text-command version of `generate-tests` — a one-click checkbox - * must never be wider than the deliberately narrow default the text command itself already has. An - * unauthorized click is a SILENT no-op (no comment fetch, no patch, no revert, no explanation) — audit-logged - * only, exactly mirroring `maybeProcessPrPanelRetrigger`'s own denial behavior above. + * Authorization uses the repo's OWN `settings.commandAuthorization` — same as the text-command version of + * `generate-tests` above, and configurable like every other command (#4589 follow-up: this used to hardcode a + * maintainer-only override here, overriding whatever `.gittensory.yml` configured; a self-hoster who wants + * contributors/confirmed miners to trigger test generation can now widen it there instead). Out of the box — + * no override configured — this still resolves to maintainer-only, since `DEFAULT_COMMAND_AUTHORIZATION_POLICY` + * already restricts `generate-tests` to `["maintainer"]` and `normalizeCommandRoleList` clamps any configured + * widening to `maintainer`/`collaborator`/`confirmed_miner` (the spoofable raw `pr_author` role is always + * dropped for this command). An unauthorized click is a SILENT no-op (no comment fetch, no patch, no revert, no + * explanation) — audit-logged only, exactly mirroring `maybeProcessPrPanelRetrigger`'s own denial behavior above. */ async function maybeProcessPrPanelGenerateTests( env: Env, @@ -13389,7 +13389,7 @@ async function maybeProcessPrPanelGenerateTests( issue, actor, commandName: "generate-tests" as GittensoryMentionCommandName, - settings: { ...settings, commandAuthorization: PR_PANEL_GENERATE_TESTS_COMMAND_AUTHORIZATION }, + settings, pr, needsMinerDetection: false, }); @@ -13404,7 +13404,7 @@ async function maybeProcessPrPanelGenerateTests( deliveryId, repoFullName, commentId: comment.id, - allowedRoles: commandAuthorizationAllowedRoles(PR_PANEL_GENERATE_TESTS_COMMAND_AUTHORIZATION, "generate-tests"), + allowedRoles: commandAuthorizationAllowedRoles(settings.commandAuthorization, "generate-tests"), }, }); await recordGithubProductUsage(env, "e2e_tests_generation_denied", { diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index be7118e998..006f45ded8 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -298,6 +298,16 @@ export function resolveReviewMemoryManifestToggle(manifest: FocusManifest | null return manifest?.review.reviewMemory === true; } +/** Resolve `review.e2e_test_auto_trigger` (#4196, part of the #4189 epic) from a possibly-null manifest (null = + * load failure ⇒ reads as unset/false). Mirrors `resolveReviewMemoryManifestToggle` exactly — true ONLY when the + * manifest explicitly set `review.e2e_test_auto_trigger: true`; null/false/absent ⇒ false. The caller ADDITIONALLY + * requires `features.e2eTests` to already be enabled for this repo (via `resolveConvergedFeature`) — this toggle + * alone never activates generation, it only decides whether an already-enabled repo also gets the unprompted + * `manifest_missing_tests` auto-trigger on top of the maintainer-initiated command/checkbox paths. */ +export function resolveE2eTestAutoTriggerManifestToggle(manifest: FocusManifest | null): boolean { + return manifest?.review.e2eTestAutoTrigger === true; +} + /** Resolve `review.pre_merge_checks` from a possibly-null manifest (null = load failure ⇒ no checks). Centralized * so the gate caller resolves them in one place with the null-manifest branch covered here (unit-tested) rather * than inline in the processor. (#review-pre-merge-checks) */ diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index e5fd6fbc96..037d6c5159 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -35,6 +35,7 @@ import { resolveReviewVisualConfig, repoDocGenerationConfigToJson, resolveReviewMemoryManifestToggle, + resolveE2eTestAutoTriggerManifestToggle, reviewConfigToJson, overlayReviewConfig, parseReviewConfigMapping, @@ -392,6 +393,7 @@ describe(".gittensory.yml.example field-exhaustiveness (#1670)", () => { maxFindings: "max_findings:", commentVerbosity: "comment_verbosity:", e2eTestDelivery: "e2e_test_delivery:", + e2eTestAutoTrigger: "e2e_test_auto_trigger:", pathInstructions: "path_instructions:", instructions: "instructions:", excludePaths: "exclude_paths:", @@ -830,7 +832,7 @@ describe("compileFocusManifestPolicy", () => { publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], gate: { present: false, enabled: null, checkMode: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, e2eTestDelivery: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null, grounding: null, e2eTests: null, screenshots: null, improvementSignal: null }, contentLane: { present: false, entryFileGlob: null, providerFileGlob: null, artifactGlob: null, collectionField: null, maxAppendedEntries: null, duplicateKeyFields: [], validatorId: null }, repoDocGeneration: { present: false, enabled: false, scope: ["agents"], allowOverwriteExisting: false, refreshIntervalDays: 7 }, @@ -3570,6 +3572,35 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { expect(resolveReviewMemoryManifestToggle(parseFocusManifest({ review: { memory: true } }))).toBe(true); }); + it("parses review.e2e_test_auto_trigger (default OFF), marks present, round-trips, and warns on a non-boolean (#4196)", () => { + expect(parseFocusManifest({ review: { e2e_test_auto_trigger: true } }).review.e2eTestAutoTrigger).toBe(true); + const on = parseFocusManifest({ review: { e2e_test_auto_trigger: true } }); + expect(on.review.present).toBe(true); // an auto-trigger-only manifest IS present + expect(parseFocusManifest({ review: reviewConfigToJson(on.review) }).review).toEqual(on.review); // survives round-trip + // Explicit false is retained (and marks present, since the maintainer set it). + const off = parseFocusManifest({ review: { e2e_test_auto_trigger: false } }); + expect(off.review.e2eTestAutoTrigger).toBe(false); + expect(off.review.present).toBe(true); + // Absent ⇒ null (the byte-identical default), config not present. + expect(parseFocusManifest({ review: {} }).review.e2eTestAutoTrigger).toBeNull(); + // A non-boolean is ignored with a warning. + const bad = parseFocusManifest({ review: { e2e_test_auto_trigger: "yes" } }); + expect(bad.review.e2eTestAutoTrigger).toBeNull(); + expect(bad.warnings.some((w) => /review\.e2e_test_auto_trigger.*must be a boolean/.test(w))).toBe(true); + }); + + it("resolves review.e2e_test_auto_trigger's manifest toggle to a strict boolean, independent of features.e2eTests (#4196)", () => { + expect(resolveE2eTestAutoTriggerManifestToggle(null)).toBe(false); // null manifest (load failure) ⇒ false + expect(resolveE2eTestAutoTriggerManifestToggle(parseFocusManifest({}))).toBe(false); // absent ⇒ false + expect(resolveE2eTestAutoTriggerManifestToggle(parseFocusManifest({ review: { e2e_test_auto_trigger: false } }))).toBe(false); + expect(resolveE2eTestAutoTriggerManifestToggle(parseFocusManifest({ review: { e2e_test_auto_trigger: true } }))).toBe(true); + // The toggle reads purely off review.e2e_test_auto_trigger -- features.e2eTests being on has no bearing on it + // (the two are ANDed together by the caller, not merged here). + expect( + resolveE2eTestAutoTriggerManifestToggle(parseFocusManifest({ features: { e2eTests: true }, review: {} })), + ).toBe(false); + }); + it("parses review.min_finding_severity, round-trips, and warns on invalid values (#2048)", () => { const major = parseFocusManifest({ review: { min_finding_severity: "major" } }); expect(major.review.minFindingSeverity).toBe("major"); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index f5654cae01..eb8777c042 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -27135,7 +27135,7 @@ describe("queue processors", () => { repoFullName: string, prNumber: number, headSha: string, - opts: { e2eTests?: boolean; hasTestFile?: boolean; validationNote?: boolean; manifestPolicyGateMode?: "advisory" | "block"; e2eTestDelivery?: "comment" | "commit" } = {}, + opts: { e2eTests?: boolean; hasTestFile?: boolean; validationNote?: boolean; manifestPolicyGateMode?: "advisory" | "block"; e2eTestDelivery?: "comment" | "commit"; autoTrigger?: boolean } = {}, ) { const slash = repoFullName.indexOf("/"); const owner = repoFullName.slice(0, slash); @@ -27181,10 +27181,14 @@ describe("queue processors", () => { // testExpectations is a TOP-LEVEL manifest field (unlike review.e2e_test_delivery's nested snake_case) -- // both it and features.e2eTests must land in the SAME upsertRepoFocusManifest call, since a second // separate call replaces rather than merges with the first. + // autoTrigger defaults to true here (NOT the production default) since this whole describe block exists + // to exercise the auto-trigger's own behavior -- the one test that cares about the real production + // default (OFF) passes `autoTrigger: false` explicitly, mirroring how the `e2eTests: false` case above + // already tests ITS OWN negative default the same way. await upsertRepoFocusManifest(env, repoFullName, { testExpectations: ["Run npm run test:ci."], features: { e2eTests: opts.e2eTests ?? true }, - ...(opts.e2eTestDelivery ? { review: { e2e_test_delivery: opts.e2eTestDelivery } } : {}), + review: { e2e_test_auto_trigger: opts.autoTrigger ?? true, ...(opts.e2eTestDelivery ? { e2e_test_delivery: opts.e2eTestDelivery } : {}) }, }); } @@ -27312,6 +27316,25 @@ describe("queue processors", () => { expect(audited?.n).toBe(0); }); + it("does not auto-trigger when features.e2eTests is enabled but review.e2e_test_auto_trigger is not set (safe default, #4196 separation)", async () => { + const repoFullName = "JSONbored/auto-e2e-4196-no-opt-in"; + const run = vi.fn(); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), AI: { run } as unknown as Ai, GITTENSORY_REVIEW_E2E_TESTS: "true", AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true" }); + // e2eTests stays enabled (the master feature, which unlocks the command/checkbox) but autoTrigger is + // explicitly withheld -- the exact "enabled for maintainer-initiated use, but never fires unprompted" + // shape the feature must default to. + await seedAutoTriggerPr(env, repoFullName, 5012, "auto-4196-no-opt-in-sha", { autoTrigger: false }); + const posted = { count: 0, body: "" }; + stubAutoTriggerFetch(5012, posted); + + await processJob(env, autoTriggerWebhook(repoFullName, 5012, "auto-4196-no-opt-in-sha")); + + expect(run).not.toHaveBeenCalled(); + expect(posted.count).toBe(0); + const audited = await env.DB.prepare("select count(*) as n from audit_events where event_type like 'github_app.e2e_tests_generation%'").first<{ n: number }>(); + expect(audited?.n).toBe(0); + }); + it("does not auto-trigger when the PR already carries a test file (the manifest_missing_tests signal never fires)", async () => { const repoFullName = "JSONbored/auto-e2e-4196-has-test"; const run = vi.fn(); @@ -27488,7 +27511,7 @@ describe("queue processors", () => { // branch is reachable only from the auto-trigger, which has no comment-invoker to fall back on). await upsertPullRequestFromGitHub(env, repoFullName, { number: 5010, title: "Add retry to checkout", state: "open", author_association: "CONTRIBUTOR", head: { sha: "auto-4196-no-author-sha", ref: "feature/checkout-retry" }, labels: [], body: "No validation evidence mentioned here." }); await upsertPullRequestFile(env, { repoFullName, pullNumber: 5010, path: "src/checkout.ts", status: "modified", additions: 3, deletions: 0, changes: 3, payload: { patch: "+function retryPayment() {\n+ return true;\n+}" } }); - await upsertRepoFocusManifest(env, repoFullName, { testExpectations: ["Run npm run test:ci."], features: { e2eTests: true } }); + await upsertRepoFocusManifest(env, repoFullName, { testExpectations: ["Run npm run test:ci."], features: { e2eTests: true }, review: { e2e_test_auto_trigger: true } }); const posted = { count: 0, body: "" }; stubAutoTriggerFetch(5010, posted); @@ -27662,6 +27685,85 @@ describe("queue processors", () => { expect(denied).toMatchObject({ actor: "drive-by-user", outcome: "denied" }); }); + // Authorization used to be hardcoded to maintainer-only here, ignoring whatever a repo's own + // .gittensory.yml commandAuthorization configured -- a self-hoster who wants their contributors to be + // able to trigger test generation had no way to widen it. It now respects settings.commandAuthorization, + // the exact same resolved (and safely clamped) policy the text-command version already uses. + it("dispatches generation for a COLLABORATOR (not just a maintainer) once the repo widens commandAuthorization for generate-tests", async () => { + const repoFullName = "JSONbored/checkbox-4589-widened"; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => ({ response: "```typescript\n" + CHECKBOX_TEST_SOURCE + "\n```" }) } as unknown as Ai, + GITTENSORY_REVIEW_E2E_TESTS: "true", + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + }); + await seedCheckboxPr(env, repoFullName, 6013, "checkbox-4589-widened-sha"); + await upsertRepositorySettings(env, { + repoFullName, + commentMode: "off", + publicSurface: "off", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "off", + requireLinkedIssue: false, + linkedIssueGateMode: "off", + manifestPolicyGateMode: "advisory", + aiReviewMode: "off", + commandAuthorization: { default: ["maintainer"], commands: { "generate-tests": ["maintainer", "collaborator"] } }, + }); + const posted = { count: 0, body: "" }; + stubCheckboxFetch(6013, "collab-user", "write", posted); // "write" permission resolves to the COLLABORATOR association + + await processJob(env, checkboxWebhook(repoFullName, 6013, 911, { login: "collab-user" })); + + expect(posted.count).toBe(1); + expect(posted.body).toContain("test('checkbox-generated coverage'"); + const audited = await env.DB.prepare("select outcome, actor from audit_events where event_type = ?") + .bind("github_app.e2e_tests_generation") + .first<{ outcome: string; actor: string }>(); + expect(audited).toMatchObject({ outcome: "completed", actor: "collab-user" }); + }); + + it("still denies the PR's own author even if the repo tries to configure the raw pr_author role for generate-tests (safety clamp holds)", async () => { + const repoFullName = "JSONbored/checkbox-4589-clamped"; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: vi.fn() } as unknown as Ai, + GITTENSORY_REVIEW_E2E_TESTS: "true", + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + }); + // seedCheckboxPr's own PR fixture is authored by "contributor" -- the SAME login checks the box below. + await seedCheckboxPr(env, repoFullName, 6014, "checkbox-4589-clamped-sha"); + await upsertRepositorySettings(env, { + repoFullName, + commentMode: "off", + publicSurface: "off", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "off", + requireLinkedIssue: false, + linkedIssueGateMode: "off", + manifestPolicyGateMode: "advisory", + aiReviewMode: "off", + // A repo attempting to grant its own PR authors unconditional access -- normalizeCommandRoleList drops + // the spoofable raw pr_author role for any MAINTAINER_ONLY_DEFAULT_COMMANDS entry (generate-tests is + // one), re-clamped at the point of use regardless of what's stored here. + commandAuthorization: { default: ["maintainer"], commands: { "generate-tests": ["pr_author"] } }, + }); + const posted = { count: 0, body: "" }; + stubCheckboxFetch(6014, "contributor", "read", posted); + + await processJob(env, checkboxWebhook(repoFullName, 6014, 912, { login: "contributor" })); + + expect(posted.count).toBe(0); + const denied = await env.DB.prepare("select actor, outcome from audit_events where event_type = ?") + .bind("github_app.e2e_tests_generation_denied") + .first<{ actor: string; outcome: string }>(); + expect(denied).toMatchObject({ actor: "contributor", outcome: "denied" }); + }); + it("skips a bot-initiated edit (the bot's own comment re-render) without dispatching generation", async () => { const repoFullName = "JSONbored/checkbox-4589-bot"; const env = createTestEnv({ @@ -27975,7 +28077,7 @@ describe("queue processors", () => { expect(posted.count).toBeGreaterThan(0); expect(posted.body).toContain("
Test coverage"); expect(posted.body).toContain("No changed test files or passing validation evidence were detected for this PR."); - expect(posted.body).toContain("- [ ] Generate an AI Playwright test for this PR"); + expect(posted.body).toContain("- [ ] **[BETA]** Generate an AI Playwright test for this PR"); }); it("handles a sparse payload with no repository, sender, or issue without throwing", async () => { diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 8623f0e32a..a55c9708d9 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1141,7 +1141,7 @@ describe("signal coverage edge cases", () => { collisions: buildCollisionReport(directRepo.fullName, [], [currentPr]), preflight: buildPreflightResult({ repoFullName: directRepo.fullName, title: "Fix isolated issue", body: "Fixes #99", linkedIssues: [99] }, directRepo, [], [currentPr]), settings: gateSettings, - review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, e2eTestDelivery: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, cadence: null, ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], skipDocsOnly: null, maxAddedLines: 0, maxFiles: 0, baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null, ollamaModel: null, openaiModel: null, openaiCompatibleModel: null, anthropicModel: null }, visual: { productionUrl: null, preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], gif: false, enabled: null, themeStorageKey: null, actionsFallback: false }, linkedIssueSatisfaction: null, sharedConfigSource: null }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, cadence: null, ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], skipDocsOnly: null, maxAddedLines: 0, maxFiles: 0, baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null, ollamaModel: null, openaiModel: null, openaiCompatibleModel: null, anthropicModel: null }, visual: { productionUrl: null, preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], gif: false, enabled: null, themeStorageKey: null, actionsFallback: false }, linkedIssueSatisfaction: null, sharedConfigSource: null }, aiReview: { notes: "The change is focused.\n\n**Nits (2)**\n- Add a test for the
edge case.\n- Keep the validator helper scoped." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead