generated from ztimson/template
review-labels #14
Assignees
ztimson (Zakary Timson)
Clear assignees
ztimson (Zakary Timson)
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: ztimson/ai-agents#14
Reference in New Issue
Block a user
Description
Checklist
]Summary
The changes introduce label-based triggering for code reviews and improve duplicate detection logic. However, there are some critical issues that need attention:
Critical Issues:
src/review.mjsline 30: The boolean condition will cause the review to skip incorrectly when labels are present. The||operator should be&&or the condition should be restructured.Code Quality Issues:
3. Type coercion: The duplicate detection uses loose equality (
==) which may cause bugs if IDs are inconsistently typed.4. Complex condition: The duplicate detection logic is difficult to read and maintain.
Minor Issues:
5. Informal language: The word "sitrep" in the system prompt is military jargon that may be unclear.
The version bump and workflow changes look appropriate. The PR template simplification is good. Overall, the changes add useful functionality but need fixes to the conditional logic before merging.
@@ -184,2 +184,3 @@// Handle duplicatesif(hasDuplicates && hasDuplicates !== 'NONE' && (dupeId = dupeIds.find(id => id == hasDuplicates.trim()))) {if(hasDuplicates && !hasDuplicates.toUpperCase().includes('NONE') && (dupeId = dupeIds.find(id => id == hasDuplicates.trim())) != null && dupeId != issueData.id) {Improved duplicate detection logic, but the condition is overly complex. Consider simplifying:
dupeId = dupeIds.find(id => id == hasDuplicates.trim()); if(hasDuplicates && !hasDuplicates.toUpperCase().includes('NONE') && dupeId != null && dupeId != issueData.id)could be clearer with early returns or better variable naming.Potential type coercion bug: Using
==instead of===indupeIds.find(id => id == hasDuplicates.trim())anddupeId != issueData.idmay cause unexpected behavior if IDs are sometimes strings and sometimes numbers. Use strict equality (===and!==) for more predictable comparisons.@@ -25,3 +26,4 @@token = process.env['AI_TOKEN'];console.log(`Reviewing: ${root}\n`);const info = await fetch(`${git}/api/v1/repos/${owner}/${repo}/pulls/${pr}`)Missing error handling: The fetch call should handle errors. If the API call fails,
infowill be undefined and line 30 will throw an error when trying to accessinfo.labels.@@ -26,2 +27,4 @@console.log(`Reviewing: ${root}\n`);const info = await fetch(`${git}/api/v1/repos/${owner}/${repo}/pulls/${pr}`).then(async resp => { return resp.ok ? resp.json() : throw new Error(`${resp.status} ${await resp.text()}`); });Logic error: The condition
info.labels?.length > 0 || !info.labels.some(l => l.name === labelEnabled)will always be true when labels exist. This should use&&instead of||. Currently, if labels.length > 0, it skips even if the enabled label is present. The correct logic should be:!info.labels?.some(l => l.name === labelEnabled)orinfo.labels?.length === 0 || !info.labels.some(l => l.name === labelEnabled)@@ -54,3 +62,3 @@model: [host, model],path: process.env['path'] || os.tmpdir(),system: `You are a code reviewer. Analyze the git diff and use the \`recommend\` tool for EACH issue you find. You must call \`recommend\` exactly once for every bug or improvement opportunity directly related to changes. Ignore formatting recommendations. After making all recommendations, provide some concluding remarks about the overall state of the changes.${existingComments}`,system: `You are a code reviewer. Analyze the git diff and use the \`recommend\` tool for EACH issue you find. You must call \`recommend\` exactly once for every bug or improvement opportunity directly related to changes. Ignore formatting recommendations. After making all recommendations, provide a quick 75 words or less sitrep.${existingComments}`,Typo in system prompt: 'sitrep' is informal military jargon. Consider using more professional language like 'summary' or 'overview' for better clarity in a code review context.
Review Summary
This PR introduces label-based triggering for AI code reviews and refines the duplicate detection logic. However, there are critical bugs that will prevent the code from running:
Critical Issues:
throwstatement cannot be used directly in a ternary expression, causing a parse errorModerate Issues:
3. Missing error handling for network failures on the fetch call
4. Type coercion bugs in refine.mjs using
==instead of===5. Potential null/undefined errors when PR title/body are missing
Minor Issues:
6. Informal language ("sitrep") in system prompt
7. Redundant check in duplicate detection
The workflow configuration changes and version bump look good. The PR template simplification is appropriate. Please address the critical syntax and logic errors before merging, as they will cause runtime failures.
@@ -184,2 +184,3 @@// Handle duplicatesif(hasDuplicates && hasDuplicates !== 'NONE' && (dupeId = dupeIds.find(id => id == hasDuplicates.trim()))) {if(hasDuplicates && !hasDuplicates.toUpperCase().includes('NONE') && (dupeId = dupeIds.find(id => id == hasDuplicates.trim())) != null && dupeId != issueData.id) {Potential type coercion bug: Using
==instead of===indupeIds.find(id => id == hasDuplicates.trim())may cause unexpected behavior if IDs are sometimes strings and sometimes numbers. Use strict equality (===) for more predictable comparisons.Redundant check: The condition
dupeId != issueData.idis unnecessary since the search is limited to 3 results and unlikely to return the current issue. However, if kept, use strict inequality (!==) instead of!=for consistency.@@ -25,3 +26,4 @@token = process.env['AI_TOKEN'];console.log(`Reviewing: ${root}\n`);const info = await fetch(`${git}/api/v1/repos/${owner}/${repo}/pulls/${pr}`)Missing error handling: The fetch call should handle network errors with a .catch() handler. If the API call fails due to network issues, the promise will reject and crash the script before reaching line 30.
@@ -26,2 +27,4 @@console.log(`Reviewing: ${root}\n`);const info = await fetch(`${git}/api/v1/repos/${owner}/${repo}/pulls/${pr}`).then(async resp => { return resp.ok ? resp.json() : throw new Error(`${resp.status} ${await resp.text()}`); });Syntax error:
throw new Error(...)cannot be used directly in a ternary expression. This should be wrapped in a block:resp.ok ? resp.json() : (() => { throw new Error(...); })()or use an if-else statement instead.@@ -27,1 +28,4 @@console.log(`Reviewing: ${root}\n`);const info = await fetch(`${git}/api/v1/repos/${owner}/${repo}/pulls/${pr}`).then(async resp => { return resp.ok ? resp.json() : throw new Error(`${resp.status} ${await resp.text()}`); });if(info.labels?.length > 0 && !info.labels.some(l => l.name === labelEnabled)) {Logic error: The condition
info.labels?.length > 0 && !info.labels.some(l => l.name === labelEnabled)will skip the review when labels exist but the enabled label is NOT present. This appears backwards - it should run when the label IS present. Consider:if(!info.labels?.some(l => l.name === labelEnabled))to skip when the label is missing.@@ -54,3 +62,3 @@model: [host, model],path: process.env['path'] || os.tmpdir(),system: `You are a code reviewer. Analyze the git diff and use the \`recommend\` tool for EACH issue you find. You must call \`recommend\` exactly once for every bug or improvement opportunity directly related to changes. Ignore formatting recommendations. After making all recommendations, provide some concluding remarks about the overall state of the changes.${existingComments}`,system: `You are a code reviewer. Analyze the git diff and use the \`recommend\` tool for EACH issue you find. You must call \`recommend\` exactly once for every bug or improvement opportunity directly related to changes. Ignore formatting recommendations. After making all recommendations, provide a quick 75 words or less sitrep.${existingComments}`,Typo in system prompt: 'sitrep' is informal military jargon. Consider using more professional language like 'summary' or 'overview' for better clarity in a code review context.
@@ -88,3 +96,3 @@});const messages = await ai.language.ask(gitDiff);const messages = await ai.language.ask(`Title: ${info.title}Potential null/undefined error: If
info.titleorinfo.bodyis null/undefined, this will insert "null" or "undefined" as text in the prompt. Add fallback values:Title: ${info.title || 'No title'}and${info.body || 'No description'}.