generated from ztimson/template
Check for duplicates before adding tickets #12
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#12
Reference in New Issue
Block a user
Looks good!
Test
Review Summary
This code change introduces multiple critical bugs that will prevent the code from running:
Critical Issues:
Undefined Variable References: Throughout the new code,
updateResis referenced in error messages before it's defined (lines 31, 172, 176, 181, 189, 194). This will causeReferenceErrorexceptions.Syntax Errors: The ternary expression
!resp.ok ? throw new Error(...) : nullis invalid JavaScript syntax. Thethrowstatement cannot be used directly in a ternary expression without wrapping.Logic Error on Line 31-32: The fetch promise handler returns
nullwhen not ok, but then the code immediately tries to callissueRes.json(), which will fail with "Cannot read property 'json' of null".Breaking Label Check (Line 33): The label validation was changed from checking objects with a
nameproperty to direct string comparison, which will always fail since labels are objects, not strings.Other Concerns:
Kind/toReviewed/without clear justificationRecommendation: This PR should not be merged in its current state. The code needs significant fixes to even run without throwing exceptions. The original error handling approach was more reliable and should be restored or properly refactored.
@@ -30,3 +30,2 @@headers: {'Authorization': `token ${auth}`}});if(!issueRes.ok) throw new Error(`${issueRes.status} ${await issueRes.text()}`);}).then(async resp => !resp.ok ? throw new Error(`${updateRes.status} ${await updateRes.text()}`) : null);Critical bug:
updateResis referenced before it's defined. This variable is only defined later on line 189. The error message should referenceresp.statusandresp.text()instead ofupdateRes.statusandupdateRes.text().Syntax error: Cannot use
throwin a ternary expression without wrapping it in a function or using proper syntax. Should bethrow new Error(...)wrapped in parentheses or use a different approach like.then(async resp => { if (!resp.ok) throw new Error(...); return resp; }).Logic error: After throwing an error or returning null, the code tries to call
issueRes.json()on line 32. If the response is not ok,issueReswill be null, causing a runtime error when trying to call.json()on it.@@ -33,2 +31,3 @@}).then(async resp => !resp.ok ? throw new Error(`${updateRes.status} ${await updateRes.text()}`) : null);const issueData = await issueRes.json();if(!issueData.labels?.some(l => l.name === 'Review/AI')) {if(issueData.labels[0] !== 'Review/AI' || issueData.labels.length !== 1) {Bug: Unsafe array access. The code assumes
issueData.labels[0]exists and is a string, but labels is an array of objects (with properties likename). The original code correctly checkedissueData.labels?.some(l => l.name === 'Review/AI'). This will always fail sincelabels[0]is an object, not a string.@@ -149,0 +169,4 @@await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}/comments`, {method: 'POST',headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},body: `{"body": "Duplicate of #${dupeId}"}`Same critical bug as line 31:
updateResis referenced before it's defined. Should referenceresp.statusandresp.text()instead.Same syntax error as line 31: Cannot use
throwin a ternary expression without proper syntax.@@ -149,0 +173,4 @@}).then(async resp => !resp.ok ? throw new Error(`${updateRes.status} ${await updateRes.text()}`) : null);await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}/labels`, {method: 'POST',headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},Same bugs as line 172:
updateResreferenced before definition and invalid throw syntax in ternary expression.@@ -149,0 +178,4 @@}).then(async resp => !resp.ok ? throw new Error(`${updateRes.status} ${await updateRes.text()}`) : null);await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}`, {method: 'PATCH',headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},Same bugs as line 172:
updateResreferenced before definition and invalid throw syntax in ternary expression.@@ -149,2 +186,4 @@}// Update ticketconst updateRes = await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}`, {Same bugs:
updateResis used in its own error handler before being assigned, and invalid throw syntax in ternary expression. This creates a circular reference issue.@@ -163,1 +191,3 @@const resp = await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}/labels`, {headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},body: JSON.stringify({title, body})}).then(async resp => !resp.ok ? throw new Error(`${updateRes.status} ${await updateRes.text()}`) : null);The label name changed from
Kind/${type}toReviewed/${type}without explanation. This could break existing label filtering logic. Verify this is intentional and consistent with the system's labeling scheme.@@ -164,0 +191,4 @@headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},body: JSON.stringify({title, body})}).then(async resp => !resp.ok ? throw new Error(`${updateRes.status} ${await updateRes.text()}`) : null);if(type) { // LabelSame bugs as above:
updateResreferenced in error handler instead ofresp, and invalid throw syntax.Summary
This code review identified 13 additional issues beyond those already flagged in the existing comments:
Critical Bugs:
.namepropertytype[0]Logic Issues:
titlebefore AI may have set itupdateResassigned but not usedError Handling Concerns:
The most critical issues are the label comparison bug (line 35) which will cause the script to never process any issues, the API endpoint error (line 153), and the manual JSON string construction which poses security risks. These should be addressed immediately before deployment.
@@ -32,3 +35,1 @@if(!issueRes.ok) throw new Error(`${issueRes.status} ${await issueRes.text()}`);const issueData = await issueRes.json();if(!issueData.labels?.some(l => l.name === 'Review/AI')) {if(issueData.labels.length !== 1 || issueData.labels?.[0] !== 'Review/AI') {Logic error: The check
issueData.labels?.[0] !== 'Review/AI'compares a label object to a string. Labels are objects with properties likename, not strings. This should beissueData.labels?.[0]?.name !== 'Review/AI'to correctly access the label's name property.Logic error: The condition checks if
labels.length !== 1which will skip processing for issues with 0 or 2+ labels. However, the original code used.some()which would process issues that have the 'Review/AI' label among other labels. This change makes the filter more restrictive and may break existing workflows where issues have multiple labels.@@ -146,3 +150,1 @@if(!body) {console.log('Invalid response from AI');return process.exit(1);if(!body) throw new Error('Invalid response from AI');Error handling change: Changed from graceful exit with exit code 1 to throwing an error. This is a behavioral change that could affect how the script is used in CI/CD pipelines. Ensure this is intentional and that calling code can handle the exception.
@@ -149,0 +150,4 @@if(!body) throw new Error('Invalid response from AI');// Check for duplicatesconst search = await fetch(`${git}/api/v1/repos/issues/search`, {Potential bug: The search API endpoint
${git}/api/v1/repos/issues/searchis missing the${owner}/${repo}path segment. This should likely be${git}/api/v1/repos/${owner}/${repo}/issues/searchto search within the specific repository, otherwise it may search globally or fail.@@ -149,0 +155,4 @@headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},body: JSON.stringify({priority_repo_id: repo,type: 'issues',Bug: The
priority_repo_idparameter expects a repository ID (numeric), butrepovariable contains the repository name (string). This will likely cause the search to fail or return incorrect results.@@ -149,0 +159,4 @@limit: 3,q: title})}).then(resp => resp.ok ? resp.json() : []);Error handling issue: If the search API returns an error, the code defaults to an empty array
[]. This silently fails and skips duplicate detection. Consider logging a warning or handling the error more explicitly to avoid missing duplicates due to API failures.Type safety issue: The fallback
[]whenresp.okis false meanssearchcould be an empty array or the actual response data. This inconsistency could cause issues if the API response structure is unexpected. Consider explicitly handling the error case.@@ -149,0 +161,4 @@})}).then(resp => resp.ok ? resp.json() : []);let dupeId = null;const dupeIds = search.map(t => t.id);Potential null reference: If
searchis an empty array (from error or no results),search.map()will work butdupeswill be empty. The AI duplicate check on line 166 will then have no context to compare against, making the duplicate detection ineffective when it fails silently.@@ -149,0 +163,4 @@let dupeId = null;const dupeIds = search.map(t => t.id);const dupes = search.map(t => `ID: ${t.id}\nTitle: ${t.title}\n\`\`\`markdown\n${t.body}\n\`\`\``).join('\n\n');const hasDuplicates = (await ai.language.ask(`${title}\n\`\`\`markdown\n${body}\n\`\`\``, {Race condition risk: The duplicate check uses
titlewhich is set by the AI tool call. If the AI hasn't called thetitletool yet or it's still empty, the search query will be empty or invalid, potentially returning incorrect results or all issues.@@ -149,0 +167,4 @@system: `Your job is to identify duplicates. Respond with the ID number of the duplicate or nothing if there are no matches \n\n${dupes}`}))?.pop()?.content;// Handle duplicatesif(!!hasDuplicates && (dupeId = dupeIds.find(id => hasDuplicates.includes(id.toString()))) != null) {Logic issue: The duplicate detection uses
hasDuplicates.includes(id.toString())which is a substring match. This could cause false positives (e.g., ID 123 would match content containing "1234"). Consider using a more precise matching strategy or parsing the AI response more carefully.@@ -149,0 +171,4 @@await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}/comments`, {method: 'POST',headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},body: `{"body": "Duplicate of #${dupeId}"}`Bug: JSON string is manually constructed instead of using
JSON.stringify(). The body should bebody: JSON.stringify({body: \Duplicate of #${dupeId}`})` to properly escape special characters and prevent JSON injection vulnerabilities.@@ -149,1 +186,4 @@console.log('Duplicate');return process.exit();}Unused variable:
updateResis assigned but never used. The.then()chain doesn't return the response, soupdateReswill be a Promise that resolves to undefined. If you need the response, remove the.then()or return the response from it.@@ -163,1 +193,3 @@const resp = await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}/labels`, {headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},body: JSON.stringify({title, body})}).then(async resp => { if(!resp.ok) throw new Error(`${resp.status} ${await resp.text()}`); });Missing error handling: If
typeis an empty string or undefined, thetype[0].toUpperCase()will throw a runtime error. Add a check to ensuretypeis valid before attempting to transform it.@@ -169,3 +199,1 @@body: JSON.stringify({labels: [`Kind/${type[0].toUpperCase() + type.slice(1).toLowerCase()}`]})});if(!resp.ok) throw new Error(`${resp.status} ${await resp.text()}`);headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},Bug: JSON string is manually constructed with template literal instead of using
JSON.stringify(). This is error-prone and could break iftypecontains special characters. Should bebody: JSON.stringify({labels: [\Reviewed/${type[0].toUpperCase() + type.slice(1).toLowerCase()}`]})`.Review Summary
This code review identified 10 critical issues in the refactoring of
src/refine.mjs:Critical Bugs (Must Fix):
1instead of checking length, and comparing label object to string - this will cause the label filtering to fail completelyrepoInfois null from failed fetchtitlemay be empty when used in search querySecurity Issues:
Runtime Safety:
typeis undefined when accessingtype[0]Breaking Changes:
Kind/toReviewed/- verify intentional and update dependent systemsLogic Issues:
Positive Changes:
.then()chains for error handlingRecommendation: Address the critical bugs (especially lines 35, 153, 158) before deployment as they will cause runtime failures. The security issues should also be fixed to prevent potential JSON injection attacks.
@@ -32,3 +35,1 @@if(!issueRes.ok) throw new Error(`${issueRes.status} ${await issueRes.text()}`);const issueData = await issueRes.json();if(!issueData.labels?.some(l => l.name === 'Review/AI')) {if(issueData.labels?.[0]?.name !== 1 || issueData.labels?.[0] !== 'Review/AI') {Critical logic error: The condition
issueData.labels?.[0]?.name !== 1compares a label name (string) to the number 1, which will always be true. This appears to be a typo and should likely be checking the length:issueData.labels?.length !== 1. Additionally, the second partissueData.labels?.[0] !== 'Review/AI'compares a label object to a string, which will always be true. The correct check should be:issueData.labels?.length !== 1 || issueData.labels?.[0]?.name !== 'Review/AI'@@ -156,0 +150,4 @@if(!body) throw new Error('Invalid response from AI');// Check for duplicatesconst repoInfo = await fetch(`${git}/api/v1/repos/${owner}/${repo}`, {headers: {'Authorization': `token ${auth}`},}).then(resp => resp.ok ? resp.json() : null);Critical bug: The search API endpoint is missing the repository path segment. This should be
${git}/api/v1/repos/${owner}/${repo}/issues/searchinstead of${git}/api/v1/repos/issues/search. Without the proper path, the API call will likely fail or search globally instead of within the specific repository.@@ -156,3 +157,2 @@body: JSON.stringify({title,body,owner,Bug: The
priority_repo_idparameter expects a numeric repository ID, butrepoInfo.idcould be null if the fetch on line 152 failed (returns null when !resp.ok). This will cause the search to fail. Add proper error handling:if(!repoInfo) throw new Error('Failed to fetch repository info');before using repoInfo.id.@@ -159,0 +159,4 @@priority_repo_id: repoInfo.id,type: 'issues',limit: 3,q: titleRace condition: The duplicate search uses the
titlevariable which is set by the AI tool call. If the AI hasn't called thetitletool yet at this point in execution,titlewill be an empty string, causing the search query to be empty or invalid. Consider moving the duplicate check after ensuring the AI has completed and set the title, or add validation:if(!title) throw new Error('AI failed to set title');@@ -161,3 +164,1 @@if(!updateRes.ok) throw new Error(`${updateRes.status} ${await updateRes.text()}`);if(type) {const resp = await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}/labels`, {}).then(resp => resp.ok ? resp.json() : []);Silent failure risk: When the search API fails (line 153 returns []), the
dupesvariable becomes an empty string, and the AI duplicate check has no context to compare against. The duplicate detection becomes ineffective but continues silently. Consider logging a warning or throwing an error when search fails to ensure duplicate detection is working properly.@@ -164,0 +167,4 @@const dupes = search.map(t => `ID: ${t.id}\nTitle: ${t.title}\n\`\`\`markdown\n${t.body}\n\`\`\``).join('\n\n');const hasDuplicates = (await ai.language.ask(`${title}\n\`\`\`markdown\n${body}\n\`\`\``, {system: `Your job is to identify duplicates. Respond with the ID number of the duplicate or nothing if there are no matches \n\n${dupes}`}))?.pop()?.content;Logic issue: The regex pattern
(^| )${id}( |$)performs substring matching which could cause false positives. For example, if the AI response contains "123" it would match issue ID 12 or 23. Consider using word boundaries or more precise parsing of the AI response to extract the exact ID number.@@ -164,1 +171,4 @@// Handle duplicatesif(!!hasDuplicates && (dupeId = dupeIds.find(id => new RegExp(`(^| )${id}( |$)`, 'm').test(hasDuplicates)))) {await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}/comments`, {method: 'POST',JSON injection vulnerability: The JSON body is manually constructed using a template literal instead of JSON.stringify(). If
dupeIdcontains special characters like quotes or backslashes, this will produce invalid JSON or allow injection. Change to:body: JSON.stringify({body: \Duplicate of #${dupeId}`})`@@ -172,0 +196,4 @@body: JSON.stringify({title, body})}).then(async resp => { if(!resp.ok) throw new Error(`${resp.status} ${await resp.text()}`); });if(type) { // Labelawait fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}/labels`, {JSON injection vulnerability: The JSON body is manually constructed using a template literal. If
typecontains special characters, this could produce invalid JSON. Whiletypeis constrained by the enum in the tool definition, it's still better practice to use JSON.stringify():body: JSON.stringify({labels: [\Reviewed/${type[0].toUpperCase() + type.slice(1).toLowerCase()}`]})`Potential runtime error: If
typeis an empty string or undefined (e.g., if the AI fails to call the type tool), thentype[0].toUpperCase()will throw a TypeError. Add validation before this line:if(!type) throw new Error('AI failed to set type');Breaking change: Label prefix changed from
Kind/${type}toReviewed/${type}without explanation. This could break existing workflows, filters, or automation that rely on theKind/label prefix. Verify this is intentional and update any dependent systems accordingly.