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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
195 changes: 188 additions & 7 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,7 @@ import {
detectGittensorContributor,
hasClearNoIssueRationale,
PR_PANEL_RETRIGGER_MARKER,
PR_PANEL_GENERATE_TESTS_MARKER,
type ContributorProfile,
} from "../signals/engine";
import { isDuplicateClusterWinnerByClaim, resolveDuplicateClusterWinnerNumber } from "../signals/duplicate-winner";
Expand Down Expand Up @@ -601,6 +602,15 @@ 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",
Expand Down Expand Up @@ -5853,6 +5863,22 @@ async function processGitHubWebhook(
return;
}

if (
eventName === "issue_comment" &&
(await maybeProcessPrPanelGenerateTests(env, deliveryId, payload))
) {
await recordWebhookEvent(env, {
deliveryId,
eventName,
action: payload.action,
installationId: payload.installation?.id,
repositoryFullName: payload.repository?.full_name,
payloadHash: "processed",
status: "processed",
});
return;
}

if (
eventName === "issue_comment" &&
(await maybeProcessGateOverrideCommand(env, deliveryId, payload))
Expand Down Expand Up @@ -11077,6 +11103,15 @@ async function maybePublishPrPublicSurface(
);
}
}
// #4589: the SAME finding #4583's inline CTA already reads off advisory.findings, resolved here (right
// before rendering, so it reflects the fully-populated array) rather than re-deriving it a third time.
// e2eTestGenAvailable is block-scoped to the manifestPolicyGateMode branch above (where #4583 already
// computes it once for that block's own use) and out of scope here, so it's re-resolved via the async
// convenience wrapper -- loadRepoFocusManifest is cached, so this is a cache hit, not a fresh read,
// mirroring the "reload the CACHED manifest, it's cheap" idiom this same function already documents
// a few hundred lines up for the identical reason.
const missingTestsFinding = advisory.findings.find((finding) => finding.code === "manifest_missing_tests");
const e2eTestGenAvailable = missingTestsFinding ? await convergedFeatureActive(env, repoFullName, "e2eTests") : false;
deterministicBody = buildUnifiedCommentBody({
gate: renderedGate,
...(aiReview !== undefined ? { aiReview } : {}),
Expand Down Expand Up @@ -11108,6 +11143,10 @@ async function maybePublishPrPublicSurface(
queueHealth,
...(reviewConfig !== undefined ? { review: reviewConfig } : {}),
duplicateWinnerEnabled,
// #4589: reuse the SAME finding + feature-gate the #4583 inline CTA already computed above (this
// function's own e2eTestGenAvailable const), rather than a second detection pass.
...(missingTestsFinding !== undefined ? { missingTestsFinding } : {}),
e2eTestGenAvailable,
}),
footerMarkdown: gittensoryFooter({
earnUrl: repo?.isRegistered
Expand All @@ -11118,6 +11157,11 @@ async function maybePublishPrPublicSurface(
: {}),
}),
reRunLabel: `${PR_PANEL_RETRIGGER_MARKER} Re-run Gittensory review`,
// #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` }
: {}),
...(beforeAfter.length > 0 ? { beforeAfter } : {}),
...(changedFilesSummaryEnabledForReview
? {
Expand Down Expand Up @@ -11973,12 +12017,12 @@ async function maybeProcessGenerateTestsCommand(env: Env, deliveryId: string, pa
}

/**
* The shared generation-and-delivery core behind BOTH `@gittensory generate-tests` (#4195, the explicit
* command) and the `manifest_missing_tests` auto-trigger (#4196) — one code path, so the two triggers can
* never silently drift apart. Everything the caller must have already resolved BEFORE this runs: the feature
* is enabled (#4192's `resolveConvergedFeature` gate), the repo is not paused/dry-run (`mode === "live"`), and
* (for the auto-trigger specifically) the per-head-SHA double-generation guard has already passed — this
* function itself has no opinion on any of that, it only generates, delivers, and audits.
* The shared generation-and-delivery core behind `@gittensory generate-tests` (#4195, the explicit command),
* the `manifest_missing_tests` auto-trigger (#4196), and the panel checkbox (#4589) — one code path, so the
* three triggers can never silently drift apart. Everything the caller must have already resolved BEFORE this
* runs: the feature is enabled (#4192's `resolveConvergedFeature` gate), the repo is not paused/dry-run
* (`mode === "live"`), and (for the auto-trigger specifically) the per-head-SHA double-generation guard has
* already passed — this function itself has no opinion on any of that, it only generates, delivers, and audits.
*/
async function runE2eTestGenerationAndDeliver(
env: Env,
Expand All @@ -11993,7 +12037,10 @@ async function runE2eTestGenerationAndDeliver(
mode: ReturnType<typeof resolveAgentActionMode>;
deliveryId: string;
targetKey: string;
trigger: "command" | "auto";
// #4589: "checkbox" behaves like "command" below (a real, re-authorized maintainer invoker exists, so
// delivery mode is NOT forced comment-only) -- it's kept as its own literal purely so audit/metadata can
// distinguish "typed the command" from "clicked the checkbox" without changing any behavior.
trigger: "command" | "auto" | "checkbox";
},
): Promise<void> {
const changedPaths = args.files.map((file) => file.path);
Expand Down Expand Up @@ -12566,6 +12613,128 @@ async function maybeProcessPrPanelRetrigger(
return true;
}

/**
* The generate-tests checkbox (#4589) — the interactive counterpart to #4583's text-only inline CTA, and a
* sibling of `maybeProcessPrPanelRetrigger` above: SAME `issue_comment.edited` detection shell (marker
* presence, bot's-own-comment confirmation, bot-sender guard, `payload.sender` as the real actor — a GitHub
* task-list checkbox can be toggled by anyone who can comment on the PR, so the checkbox itself proves
* nothing; only this server-side re-authorization does), but dispatches through the SAME shared
* `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.
*/
async function maybeProcessPrPanelGenerateTests(
env: Env,
deliveryId: string,
payload: GitHubWebhookPayload,
): Promise<boolean> {
const comment = payload.comment;
if (
payload.action !== "edited" ||
!comment ||
!isCheckedPrPanelGenerateTests(comment.body)
)
return false;
if (!isGittensoryPanelBotComment(env, comment.user)) return false;

const repoFullName = payload.repository?.full_name ?? null;
const issue = payload.issue;
const installationId = getInstallationId(payload);
const actor = payload.sender?.login ?? null;
const targetKey =
repoFullName && issue ? `${repoFullName}#${issue.number}` : repoFullName;
if (payload.sender?.type === "Bot" || /\[bot\]$/i.test(actor ?? "")) {
await recordGenerateTestsSkip(env, deliveryId, repoFullName, targetKey, actor, "bot_author");
return true;
}
if (!repoFullName || !issue?.pull_request || !installationId) {
await recordGenerateTestsSkip(env, deliveryId, repoFullName, targetKey, actor, "missing_repo_pr_or_installation");
return true;
}
const [pr, settings] = await Promise.all([
getPullRequest(env, repoFullName, issue.number),
resolveRepositorySettings(env, repoFullName),
]);
if (!pr) {
await recordGenerateTestsSkip(env, deliveryId, repoFullName, targetKey, actor, "cached_pr_missing");
return true;
}

const { authorization } = await authorizePrActionActor({
env,
deliveryId,
installationId,
repoFullName,
issue,
actor,
commandName: "generate-tests" as GittensoryMentionCommandName,
settings: { ...settings, commandAuthorization: PR_PANEL_GENERATE_TESTS_COMMAND_AUTHORIZATION },
pr,
needsMinerDetection: false,
});
if (!authorization.authorized) {
await recordAuditEvent(env, {
eventType: "github_app.e2e_tests_generation_denied",
actor,
targetKey: `${repoFullName}#${pr.number}`,
outcome: "denied",
detail: authorization.reason,
metadata: {
deliveryId,
repoFullName,
commentId: comment.id,
allowedRoles: commandAuthorizationAllowedRoles(PR_PANEL_GENERATE_TESTS_COMMAND_AUTHORIZATION, "generate-tests"),
},
});
await recordGithubProductUsage(env, "e2e_tests_generation_denied", {
actor,
repoFullName,
targetKey: `${repoFullName}#${pr.number}`,
outcome: "denied",
metadata: { reason: authorization.reason, actorKind: authorization.actorKind },
});
return true;
}

// Defense in depth: re-check the feature is STILL enabled -- the repo's own .gittensory.yml could have
// changed between when this comment was posted (checkbox rendered) and when it was actually clicked.
const manifest = await loadRepoFocusManifest(env, repoFullName).catch(() => null);
if (!resolveConvergedFeature(env, manifest, "e2eTests", repoFullName)) {
await recordGenerateTestsSkip(env, deliveryId, repoFullName, `${repoFullName}#${pr.number}`, actor, "feature_disabled");
return true;
}
const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isDbFrozenForRepo(env, settings.agentGlobalFreezeOverride)), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun });
if (mode !== "live") {
const skipReason = mode === "dry_run" ? "dry_run" : "agent_paused";
await recordGenerateTestsSkip(env, deliveryId, repoFullName, `${repoFullName}#${pr.number}`, actor, skipReason);
return true;
}
const files = await listPullRequestFiles(env, repoFullName, pr.number);
await runE2eTestGenerationAndDeliver(env, {
repoFullName,
installationId,
pr,
settings,
manifest,
files,
// Non-null: authorization.authorized is only ever true when actor resolved to a real login in the first
// place (evaluateCommandAuthorization can't match a maintainer/collaborator/confirmed_miner role off a
// null commenterLogin) -- guaranteed by the `authorization.authorized` check above, not re-derivable here.
actor: actor!,
mode,
deliveryId,
targetKey: `${repoFullName}#${pr.number}`,
trigger: "checkbox",
});
return true;
}

async function resolveRealRepoPermissionAssociation(
env: Env,
installationId: number,
Expand Down Expand Up @@ -12726,6 +12895,18 @@ function isCheckedPrPanelRetrigger(body: string | null | undefined): boolean {
return checkedMarkerRegex(PR_PANEL_RETRIGGER_MARKER).test(body);
}

// #4589: sibling of isCheckedPrPanelRetrigger above, same marker-presence + checkedMarkerRegex mechanism, own
// dedicated marker so the two checkboxes (re-run vs generate-tests) can independently appear/toggle in the
// same comment without either detector matching the other's line.
function isCheckedPrPanelGenerateTests(body: string | null | undefined): boolean {
if (
!body?.includes(PR_PANEL_COMMENT_MARKER) ||
!body.includes(PR_PANEL_GENERATE_TESTS_MARKER)
)
return false;
return checkedMarkerRegex(PR_PANEL_GENERATE_TESTS_MARKER).test(body);
}

function checkedMarkerRegex(marker: string): RegExp {
return new RegExp(
`(?:^|\\n)\\s*[-*]\\s*\\[[xX]\\]\\s*${escapeRegExp(marker)}`,
Expand Down
3 changes: 3 additions & 0 deletions src/review/unified-comment-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,8 @@ export type UnifiedCommentBridgeArgs = {
footerMarkdown: string;
/** The re-run checkbox label. */
reRunLabel?: string | undefined;
/** #4589: the generate-tests checkbox label. */
generateTestsLabel?: string | undefined;
/** Extra collapsed sections (e.g. signal definitions / contributor next steps). */
extraCollapsibles?: UnifiedCollapsible[] | undefined;
/** Headline brand (default "Gittensory review"). */
Expand Down Expand Up @@ -817,6 +819,7 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string
footerMarkdown: args.footerMarkdown,
reviewedAt: args.reviewedAt ?? new Date(),
...(args.reRunLabel !== undefined ? { reRunLabel: args.reRunLabel } : {}),
...(args.generateTestsLabel !== undefined ? { generateTestsLabel: args.generateTestsLabel } : {}),
...(extraCollapsibles !== undefined ? { extraCollapsibles } : {}),
...(args.heldForReview ? { heldForReview: true } : {}),
...(args.neverClosed ? { neverClosed: true } : {}),
Expand Down
20 changes: 15 additions & 5 deletions src/review/unified-comment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,10 @@ export interface UnifiedCommentContext {
extraCollapsibles?: UnifiedCollapsible[];
/** Re-run checkbox label, e.g. "Re-run Gittensory review" (omitted = no checkbox). */
reRunLabel?: string;
/** #4589: generate-tests checkbox label, e.g. "Generate an AI Playwright test for this PR" (omitted = no
* checkbox). Same top-level-outside-the-blockquote placement as reRunLabel, for the same reason (see
* renderUnifiedReviewComment's own comment on why the re-run checkbox can't render inside the alert). */
generateTestsLabel?: string;
/** Footer markdown (earning + branding), rendered under a divider. */
footerMarkdown?: string;
/** Force the status (e.g. the host knows it auto-merged). */
Expand Down Expand Up @@ -705,13 +709,19 @@ export function renderUnifiedReviewComment(input: UnifiedReviewInput, ctx: Unifi
);
if (ctx.footerMarkdown?.trim()) blocks.push(`---\n${ctx.footerMarkdown.trim()}`);

// The re-run checkbox MUST render at top level, OUTSIDE the alert blockquote. GitHub disables interactive
// Every action checkbox MUST render at top level, OUTSIDE the alert blockquote. GitHub disables interactive
// task-list checkboxes inside a blockquote (every line `> `-prefixed by asAlert), so a checkbox emitted via
// asAlert can never be ticked — no issue_comment.edited fires and maybeProcessPrPanelRetrigger never runs.
// Appending it after the alert keeps the box clickable AND keeps the checked-marker regex matching a non-
// quoted `- [x] <marker> …` line. The PR_PANEL_COMMENT_MARKER prepended by the bridge still leads the body.
// asAlert can never be ticked — no issue_comment.edited fires and neither maybeProcessPrPanelRetrigger nor
// maybeProcessPrPanelGenerateTests (#4589) ever runs. Appending them after the alert keeps each box clickable
// AND keeps its checked-marker regex matching a non-quoted `- [x] <marker> …` line. The PR_PANEL_COMMENT_MARKER
// prepended by the bridge still leads the body. Order is re-run first, generate-tests second (#4589) — stable
// and matches the order the two features shipped in.
const alerted = asAlert(meta.alert, blocks.join("\n\n"));
return ctx.reRunLabel ? `${alerted}\n\n- [ ] ${ctx.reRunLabel}` : alerted;
const checkboxLines = [
ctx.reRunLabel ? `- [ ] ${ctx.reRunLabel}` : null,
ctx.generateTestsLabel ? `- [ ] ${ctx.generateTestsLabel}` : null,
].filter((line): line is string => line !== null);
return checkboxLines.length > 0 ? `${alerted}\n\n${checkboxLines.join("\n")}` : alerted;
}

/**
Expand Down
Loading