Skip to content

Commit 1bfac9a

Browse files
committed
feat(review): surface generate-tests command inline in the review comment (#4583)
manifest_missing_tests already renders in ORB's own comment, but nothing there told a maintainer the @gittensory generate-tests command exists -- mirrors CodeRabbit's inline "Generate unit tests" walkthrough checkbox instead of a separate dashboard toggle, gated on the same e2eTests feature check the #4196 auto-trigger already uses so an unconfigured repo never sees a command that would just bounce. Fixes #4583
1 parent 6e1493a commit 1bfac9a

2 files changed

Lines changed: 53 additions & 3 deletions

File tree

src/queue/processors.ts

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1029,10 +1029,16 @@ const PUBLIC_MANIFEST_POLICY_FINDING_OVERRIDES: Partial<
10291029
},
10301030
};
10311031

1032+
// #4583: surfaces the AI test-generation command right where a maintainer already sees the missing-coverage
1033+
// finding, mirroring CodeRabbit's inline "Generate unit tests" walkthrough checkbox instead of requiring the
1034+
// maintainer to already know the `@gittensory generate-tests` command exists from documentation alone.
1035+
const E2E_TEST_GEN_CTA = "Maintainers can also comment `@gittensory generate-tests` for an AI-generated Playwright test.";
1036+
10321037
export function publicSafeManifestPolicyFinding(
10331038
finding: FocusManifestFinding,
1039+
options: { e2eTestGenAvailable?: boolean } = {},
10341040
): AdvisoryFinding {
1035-
return {
1041+
const base: AdvisoryFinding = {
10361042
code: finding.code,
10371043
severity: finding.severity,
10381044
title: finding.title,
@@ -1043,6 +1049,16 @@ export function publicSafeManifestPolicyFinding(
10431049
// private blocked-path globs / test expectations; codes absent from the table keep their already-generic text.
10441050
...PUBLIC_MANIFEST_POLICY_FINDING_OVERRIDES[finding.code],
10451051
};
1052+
// Only appended when e2eTests is actually enabled for this repo (the SAME resolveConvergedFeature check the
1053+
// #4196 auto-trigger already gates on), so an unconfigured repo is never told about a command that would just
1054+
// bounce with "not enabled" -- and only for the missing-tests finding, the one case where the command is a
1055+
// directly relevant next step rather than noise on an unrelated finding. base.action is always defined here
1056+
// (PUBLIC_MANIFEST_POLICY_FINDING_OVERRIDES.manifest_missing_tests always sets one), the same always-populated
1057+
// guarantee the v8-ignore above already documents for this code path.
1058+
if (finding.code === "manifest_missing_tests" && options.e2eTestGenAvailable) {
1059+
return { ...base, action: `${base.action} ${E2E_TEST_GEN_CTA}` };
1060+
}
1061+
return base;
10461062
}
10471063

10481064
export async function processJob(env: Env, message: JobMessage): Promise<void> {
@@ -9442,9 +9458,14 @@ async function maybePublishPrPublicSurface(
94429458
// Keep deterministic manifest policy findings independent from AI-review eligibility: ignored authors
94439459
// suppress review/public output only, never maintainer-configured gate blockers or their downstream triggers.
94449460
const policyFindings = guidance.findings;
9461+
// Computed once and reused below for the #4196 auto-trigger check -- same feature gate, one call. Also
9462+
// feeds #4583's inline CTA so the missing-tests finding surfaces `@gittensory generate-tests` right in
9463+
// ORB's own comment (mirrors CodeRabbit's inline walkthrough checkbox) only when the command would
9464+
// actually work for this repo, never as noise on a repo that hasn't opted in.
9465+
const e2eTestGenAvailable = resolveConvergedFeature(env, manifest, "e2eTests", repoFullName);
94459466
for (const finding of policyFindings) {
94469467
if (!policyCodes.has(finding.code)) continue;
9447-
advisory.findings.push(publicSafeManifestPolicyFinding(finding));
9468+
advisory.findings.push(publicSafeManifestPolicyFinding(finding, { e2eTestGenAvailable }));
94489469
}
94499470
// E2E test-generation auto-trigger (#4196, part of the #4189 epic): promotes the deterministic
94509471
// manifest_missing_tests finding above from advisory-only text into an actual trigger for #4192/#4194's
@@ -9454,7 +9475,7 @@ async function maybePublishPrPublicSurface(
94549475
// tests" from scratch, per the issue's own requirement -- this is why the auto-trigger lives inside this
94559476
// exact manifestPolicyGateMode-gated block instead of a parallel code path: that is the only place this
94569477
// finding is computed at all today.
9457-
if (pr.headSha && policyFindings.some((finding) => finding.code === "manifest_missing_tests") && resolveConvergedFeature(env, manifest, "e2eTests", repoFullName)) {
9478+
if (pr.headSha && policyFindings.some((finding) => finding.code === "manifest_missing_tests") && e2eTestGenAvailable) {
94589479
const e2eTargetKey = `${repoFullName}#${pr.number}`;
94599480
// Double-generation guard: an unchanged head SHA re-entering this pass (a re-review/sweep tick, not a
94609481
// new push) must never re-spend an LLM call or repost a duplicate suggestion. A genuinely NEW push

test/unit/public-safe-manifest-finding.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,4 +33,33 @@ describe("publicSafeManifestPolicyFinding", () => {
3333
expect(safe.detail).toBe(finding.detail);
3434
expect(safe.action).toBe(finding.action);
3535
});
36+
37+
// #4583: surfaces `@gittensory generate-tests` inline in the SAME comment as the missing-coverage finding
38+
// (mirrors CodeRabbit's inline walkthrough checkbox), gated on the caller-resolved e2eTests feature state.
39+
it("appends the generate-tests CTA to the missing-tests finding when e2e test generation is available", () => {
40+
const finding: FocusManifestFinding = {
41+
code: "manifest_missing_tests",
42+
severity: "warning",
43+
title: "Maintainer test expectations unmet",
44+
detail: "Maintainer expects test evidence: run the private fuzz suite.",
45+
action: "Add or update tests for the private fuzz suite.",
46+
};
47+
const safe = publicSafeManifestPolicyFinding(finding, { e2eTestGenAvailable: true });
48+
expect(safe.action).toBe(
49+
"Add regression/invariant coverage, update relevant tests, or attach passing validation output that satisfies the repo's configured expectations. Maintainers can also comment `@gittensory generate-tests` for an AI-generated Playwright test.",
50+
);
51+
});
52+
53+
it("never appends the generate-tests CTA to a finding other than manifest_missing_tests, even when e2e test generation is available", () => {
54+
const finding: FocusManifestFinding = {
55+
code: "manifest_linked_issue_required",
56+
severity: "warning",
57+
title: "Maintainer requires a linked issue",
58+
detail: "This repo's maintainer focus manifest requires every PR to reference a tracked issue.",
59+
action: "Link the relevant issue (for example `Closes #123`) before opening the PR.",
60+
};
61+
const safe = publicSafeManifestPolicyFinding(finding, { e2eTestGenAvailable: true });
62+
expect(safe.action).toBe(finding.action);
63+
expect(safe.action).not.toContain("generate-tests");
64+
});
3665
});

0 commit comments

Comments
 (0)