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
6 changes: 5 additions & 1 deletion src/services/ai-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,10 @@
"- blockers: each ONE sentence naming a defect that WILL break the code as written — a missing import/symbol (ReferenceError), a logic error that produces wrong output, a security hole, data loss, a build/test breakage, or an API/contract break. Reference the file (and function/line). Empty [] if there are genuinely none.",
"- nits: each ONE sentence — a NON-blocking point: style, naming, a missing doc, or DEFENSIVE hardening ('should handle the empty case', 'consider catching errors', 'add validation'). File-reference where you can.",
"- suggestions: a few concrete, file-referenced improvements (may overlap nits).",
"BE SELECTIVE — report only the findings that genuinely matter. List at MOST ~3 blockers and ~5 nits, keeping only the most important; prefer signal over volume and do NOT pad the lists.",

Check notice on line 48 in src/services/ai-review.ts

View check run for this annotation

Deleted GitHub App / Gittensory Context

Issue discovery is disabled for this repo

This repo is configured for direct contribution review rather than issue-discovery flow.

Check notice on line 48 in src/services/ai-review.ts

View check run for this annotation

Deleted GitHub App / Gittensory Context

PR author has maintainer association

This PR appears to come from a maintainer-associated account.
"DEDUPLICATE — if the same kind of issue recurs across several functions or lines, report it ONCE and note it applies broadly; never repeat a near-identical finding per occurrence.",
"SEVERITY DISCIPLINE — defensive or speculative hardening ('should handle X', 'consider validating', 'add error handling') is a NIT, not a blocker, UNLESS a real input WILL actually trigger the failure. CI or check status itself (failing, pending, unverified) is NOT a code defect — never list it (the gate evaluates CI separately).",
"DIFF SCOPE — the diff shows only CHANGED lines, NOT whole files. A function, variable, import, type, or symbol you do not SEE may already be defined or imported elsewhere in the same file/module. NEVER report a 'missing import', 'undefined/not-imported symbol', or 'X is not defined -> ReferenceError' as a blocker unless the diff ITSELF removes the definition or introduces the symbol without defining it anywhere shown. When you cannot confirm a symbol is missing from the visible diff, it is NOT a blocker — at most a nit ('verify X is imported/defined').",
"Do NOT rubber-stamp: if the diff is genuinely clean, the assessment states specifically why and blockers is [].",
"Never mention rewards, rankings, payouts, wallets, hotkeys, coldkeys, trust scores, scoreability, reviewability, or farming.",
].join(" ");
Expand Down Expand Up @@ -352,8 +353,11 @@
lines.push("");
}
if (safeNits.length > 0) {
lines.push("**Nits**");
// Nits go inside a collapsed <details> toggle so the body stays focused on the assessment + blockers;
// the blank line after </summary> lets GitHub render the markdown list inside the dropdown. (#focused-reviews)
lines.push("<details>", `<summary>Nits (${safeNits.length})</summary>`, "");
lines.push(...safeNits.map((s) => `- ${s}`));
lines.push("</details>");
}
// Reaching here means at least one section was pushed (the all-empty case returned null above).
return lines.join("\n").trim();
Expand Down
6 changes: 3 additions & 3 deletions test/unit/ai-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -329,14 +329,14 @@

it("composeAdvisoryNotes renders only the sections that have public-safe content", () => {
const review = (over: Partial<{ assessment: string; suggestions: string[]; nits: string[]; blockers: string[] }>) => ({ assessment: over.assessment ?? "", suggestions: over.suggestions ?? [], nits: over.nits ?? [], blockers: over.blockers ?? [] });
const assessmentOnly = composeAdvisoryNotes([review({ assessment: "Looks good." })]);

Check notice on line 332 in test/unit/ai-review.test.ts

View check run for this annotation

Deleted GitHub App / Gittensory Context

Issue discovery is disabled for this repo

This repo is configured for direct contribution review rather than issue-discovery flow.

Check notice on line 332 in test/unit/ai-review.test.ts

View check run for this annotation

Deleted GitHub App / Gittensory Context

PR author has maintainer association

This PR appears to come from a maintainer-associated account.
expect(assessmentOnly).toBe("Looks good.");
const nitsOnly = composeAdvisoryNotes([review({ nits: ["Add a test."] })]);
expect(nitsOnly).toContain("**Nits**");
expect(nitsOnly).toContain("<summary>Nits");
expect(nitsOnly).not.toContain("**Blockers**");
const blockersOnly = composeAdvisoryNotes([review({ blockers: ["Null deref in src/a.ts."] })]);
expect(blockersOnly).toContain("**Blockers**");
expect(blockersOnly).not.toContain("**Nits**");
expect(blockersOnly).not.toContain("<summary>Nits");
});

it("composeAdvisoryNotes merges + dedupes blockers/nits across two reviewers and renders both sections", () => {
Expand All @@ -346,7 +346,7 @@
expect(out).toContain("Solid change."); // first reviewer's assessment wins
expect(out).toContain("**Blockers**");
expect(out).toContain("Off-by-one in the loop bound.");
expect(out).toContain("**Nits**");
expect(out).toContain("<summary>Nits");
expect(out).toContain("Tighten the type."); // nits + suggestions merged
// the shared blocker + the shared nit/suggestion each appear exactly once (dedupe across reviewers)
expect(out.match(/Null deref in src\/a\.ts\./g)?.length).toBe(1);
Expand Down
Loading