Check for duplicates before adding tickets #12

Merged
ztimson merged 6 commits from ticket-duplicates into master 2025-12-31 00:02:18 -05:00
3 changed files with 68 additions and 30 deletions

View File

@@ -1,6 +1,6 @@
{
"name": "@ztimson/ai-agents",
"version": "0.0.8",
"version": "0.1.0",
"description": "AI agents",
"keywords": ["ai", "review"],
"author": "ztimson",

View File

@@ -26,16 +26,18 @@ dotenv.config({path: '.env.local', override: true, quiet: true});
console.log(`Processing issue #${ticket}`);
// Fetch issue
const issueRes = await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}`, {
const issueData = await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}`, {
headers: {'Authorization': `token ${auth}`}
}).then(async resp => {
ztimson marked this conversation as resolved Outdated

Critical bug: updateRes is referenced before it's defined. This variable is only defined later on line 189. The error message should reference resp.status and resp.text() instead of updateRes.status and updateRes.text().

Critical bug: `updateRes` is referenced before it's defined. This variable is only defined later on line 189. The error message should reference `resp.status` and `resp.text()` instead of `updateRes.status` and `updateRes.text()`.

Syntax error: Cannot use throw in a ternary expression without wrapping it in a function or using proper syntax. Should be throw new Error(...) wrapped in parentheses or use a different approach like .then(async resp => { if (!resp.ok) throw new Error(...); return resp; }).

Syntax error: Cannot use `throw` in a ternary expression without wrapping it in a function or using proper syntax. Should be `throw 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, issueRes will be null, causing a runtime error when trying to call .json() on it.

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, `issueRes` will be null, causing a runtime error when trying to call `.json()` on it.
if(resp.ok) return resp.json();
else throw new Error(`${resp.status} ${await resp.text()}`);
ztimson marked this conversation as resolved Outdated

Bug: Unsafe array access. The code assumes issueData.labels[0] exists and is a string, but labels is an array of objects (with properties like name). The original code correctly checked issueData.labels?.some(l => l.name === 'Review/AI'). This will always fail since labels[0] is an object, not a string.

Bug: Unsafe array access. The code assumes `issueData.labels[0]` exists and is a string, but labels is an array of objects (with properties like `name`). The original code correctly checked `issueData.labels?.some(l => l.name === 'Review/AI')`. This will always fail since `labels[0]` is an object, not a string.
});
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] !== 1 || issueData.labels?.[0]?.name !== 'Review/AI') {
ztimson marked this conversation as resolved Outdated

Logic error: The check issueData.labels?.[0] !== 'Review/AI' compares a label object to a string. Labels are objects with properties like name, not strings. This should be issueData.labels?.[0]?.name !== 'Review/AI' to correctly access the label's name property.

Logic error: The check `issueData.labels?.[0] !== 'Review/AI'` compares a label object to a string. Labels are objects with properties like `name`, not strings. This should be `issueData.labels?.[0]?.name !== 'Review/AI'` to correctly access the label's name property.

Logic error: The condition checks if labels.length !== 1 which 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.

Logic error: The condition checks if `labels.length !== 1` which 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.

Critical logic error: The condition issueData.labels?.[0]?.name !== 1 compares 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 part issueData.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'

Critical logic error: The condition `issueData.labels?.[0]?.name !== 1` compares 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 part `issueData.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'`
console.log('Skipping');
return process.exit();
}
// Gather readme & template
let title = '', type = '', readme = '', readmeP = path.join(process.cwd(), 'README.md');
if(fs.existsSync(readmeP)) readme = fs.readFileSync(readmeP, 'utf-8');
const template = p ? fs.readFileSync(p, 'utf-8') : `## Description
@@ -80,6 +82,7 @@ Implementation details, constraints, dependencies, design decisions
| **Total** | **0-15** |
`;
// Create AI
let options = {ollama: {model, host}};
if(host === 'anthropic') options = {anthropic: {model, token}};
else if(host === 'openai') options = {openAi: {model, token}};
@@ -141,35 +144,67 @@ ${template.trim()}
Output ONLY markdown. No explanations, labels, or extra formatting.`});
// Format ticket with AI
const messages = await ai.language.ask(`Title: ${issueData.title}\n\nDescription:\n${issueData.body || 'No description provided'}`).catch(() => []);
const body = messages?.pop()?.content;
if(!body) {
console.log('Invalid response from AI');
return process.exit(1);
}
const updateRes = await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}`, {
method: 'PATCH',
headers: {
'Authorization': `token ${auth}`,
'Content-Type': 'application/json'
},
body: JSON.stringify({
title,
body,
})
});
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`, {
if(!body) throw new Error('Invalid response from AI');
ztimson marked this conversation as resolved
Review

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.

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.
// Check for duplicates
const repoInfo = await fetch(`${git}/api/v1/repos/${owner}/${repo}`, {headers: {'Authorization': `token ${auth}`},}).then(resp => resp.ok ? resp.json() : null);
ztimson marked this conversation as resolved Outdated

Potential bug: The search API endpoint ${git}/api/v1/repos/issues/search is missing the ${owner}/${repo} path segment. This should likely be ${git}/api/v1/repos/${owner}/${repo}/issues/search to search within the specific repository, otherwise it may search globally or fail.

Potential bug: The search API endpoint `${git}/api/v1/repos/issues/search` is missing the `${owner}/${repo}` path segment. This should likely be `${git}/api/v1/repos/${owner}/${repo}/issues/search` to search within the specific repository, otherwise it may search globally or fail.
Review

Critical bug: The search API endpoint is missing the repository path segment. This should be ${git}/api/v1/repos/${owner}/${repo}/issues/search instead 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.

Critical bug: The search API endpoint is missing the repository path segment. This should be `${git}/api/v1/repos/${owner}/${repo}/issues/search` instead 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.
const search = await fetch(`${git}/api/v1/repos/issues/search`, {
method: 'POST',
headers: {
'Authorization': `token ${auth}`,
'Content-Type': 'application/json'
},
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'},
body: JSON.stringify({
owner,
ztimson marked this conversation as resolved Outdated

Bug: The priority_repo_id parameter expects a repository ID (numeric), but repo variable contains the repository name (string). This will likely cause the search to fail or return incorrect results.

Bug: The `priority_repo_id` parameter expects a repository ID (numeric), but `repo` variable contains the repository name (string). This will likely cause the search to fail or return incorrect results.
Review

Bug: The priority_repo_id parameter expects a numeric repository ID, but repoInfo.id could 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.

Bug: The `priority_repo_id` parameter expects a numeric repository ID, but `repoInfo.id` could 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.
priority_repo_id: repoInfo.id,
type: 'issues',
limit: 3,
q: title
ztimson marked this conversation as resolved
Review

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.

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.
Review

Type safety issue: The fallback [] when resp.ok is false means search could 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.

Type safety issue: The fallback `[]` when `resp.ok` is false means `search` could 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.
Review

Race condition: The duplicate search uses the title variable which is set by the AI tool call. If the AI hasn't called the title tool yet at this point in execution, title will 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');

Race condition: The duplicate search uses the `title` variable which is set by the AI tool call. If the AI hasn't called the `title` tool yet at this point in execution, `title` will 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');`
})
}).then(resp => resp.ok ? resp.json() : []);
ztimson marked this conversation as resolved
Review

Potential null reference: If search is an empty array (from error or no results), search.map() will work but dupes will 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.

Potential null reference: If `search` is an empty array (from error or no results), `search.map()` will work but `dupes` will 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.
Review

Silent failure risk: When the search API fails (line 153 returns []), the dupes variable 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.

Silent failure risk: When the search API fails (line 153 returns []), the `dupes` variable 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.
let dupeId = null;
const dupeIds = search.map(t => t.id);
ztimson marked this conversation as resolved
Review

Race condition risk: The duplicate check uses title which is set by the AI tool call. If the AI hasn't called the title tool yet or it's still empty, the search query will be empty or invalid, potentially returning incorrect results or all issues.

Race condition risk: The duplicate check uses `title` which is set by the AI tool call. If the AI hasn't called the `title` tool yet or it's still empty, the search query will be empty or invalid, potentially returning incorrect results or all issues.
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;
ztimson marked this conversation as resolved
Review

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.

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.
Review

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.

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.
// Handle duplicates
if(!!hasDuplicates && (dupeId = dupeIds.find(id => new RegExp(`\\b${id}\\b`, 'm').test(hasDuplicates)))) {
ztimson marked this conversation as resolved Outdated

Same critical bug as line 31: updateRes is referenced before it's defined. Should reference resp.status and resp.text() instead.

Same critical bug as line 31: `updateRes` is referenced before it's defined. Should reference `resp.status` and `resp.text()` instead.

Same syntax error as line 31: Cannot use throw in a ternary expression without proper syntax.

Same syntax error as line 31: Cannot use `throw` in a ternary expression without proper syntax.
await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}/comments`, {
method: 'POST',
ztimson marked this conversation as resolved
Review

Bug: JSON string is manually constructed instead of using JSON.stringify(). The body should be body: JSON.stringify({body: \Duplicate of #${dupeId}`})` to properly escape special characters and prevent JSON injection vulnerabilities.

Bug: JSON string is manually constructed instead of using `JSON.stringify()`. The body should be `body: JSON.stringify({body: \`Duplicate of #${dupeId}\`})` to properly escape special characters and prevent JSON injection vulnerabilities.
Review

JSON injection vulnerability: The JSON body is manually constructed using a template literal instead of JSON.stringify(). If dupeId contains special characters like quotes or backslashes, this will produce invalid JSON or allow injection. Change to: body: JSON.stringify({body: \Duplicate of #${dupeId}`})`

JSON injection vulnerability: The JSON body is manually constructed using a template literal instead of JSON.stringify(). If `dupeId` contains special characters like quotes or backslashes, this will produce invalid JSON or allow injection. Change to: `body: JSON.stringify({body: \`Duplicate of #${dupeId}\`})`
headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},
body: `{"body": "Duplicate of #${dupeId}"}`
ztimson marked this conversation as resolved
Review

Same bugs as line 172: updateRes referenced before definition and invalid throw syntax in ternary expression.

Same bugs as line 172: `updateRes` referenced before definition and invalid throw syntax in ternary expression.
}).then(async resp => { if(!resp.ok) throw new Error(`${resp.status} ${await resp.text()}`); });
await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}/labels`, {
method: 'POST',
headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},
body: '{"labels":["Reviewed/Duplicate"]}'
ztimson marked this conversation as resolved
Review

Same bugs as line 172: updateRes referenced before definition and invalid throw syntax in ternary expression.

Same bugs as line 172: `updateRes` referenced before definition and invalid throw syntax in ternary expression.
}).then(async resp => { if(!resp.ok) throw new Error(`${resp.status} ${await resp.text()}`); });
await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}`, {
method: 'PATCH',
headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},
body: '{"state": "closed"}'
}).then(async resp => { if(!resp.ok) throw new Error(`${resp.status} ${await resp.text()}`); });
console.log('Duplicate');
return process.exit();
ztimson marked this conversation as resolved Outdated

Same bugs: updateRes is used in its own error handler before being assigned, and invalid throw syntax in ternary expression. This creates a circular reference issue.

Same bugs: `updateRes` is used in its own error handler before being assigned, and invalid throw syntax in ternary expression. This creates a circular reference issue.
Review

Unused variable: updateRes is assigned but never used. The .then() chain doesn't return the response, so updateRes will be a Promise that resolves to undefined. If you need the response, remove the .then() or return the response from it.

Unused variable: `updateRes` is assigned but never used. The `.then()` chain doesn't return the response, so `updateRes` will be a Promise that resolves to undefined. If you need the response, remove the `.then()` or return the response from it.
}
// Update ticket
await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}`, {
ztimson marked this conversation as resolved Outdated

The label name changed from Kind/${type} to Reviewed/${type} without explanation. This could break existing label filtering logic. Verify this is intentional and consistent with the system's labeling scheme.

The label name changed from `Kind/${type}` to `Reviewed/${type}` without explanation. This could break existing label filtering logic. Verify this is intentional and consistent with the system's labeling scheme.
method: 'PATCH',
ztimson marked this conversation as resolved Outdated

Same bugs as above: updateRes referenced in error handler instead of resp, and invalid throw syntax.

Same bugs as above: `updateRes` referenced in error handler instead of `resp`, and invalid throw syntax.
headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},
ztimson marked this conversation as resolved Outdated

Missing error handling: If type is an empty string or undefined, the type[0].toUpperCase() will throw a runtime error. Add a check to ensure type is valid before attempting to transform it.

Missing error handling: If `type` is an empty string or undefined, the `type[0].toUpperCase()` will throw a runtime error. Add a check to ensure `type` is valid before attempting to transform it.
body: JSON.stringify({title, body})
}).then(async resp => { if(!resp.ok) throw new Error(`${resp.status} ${await resp.text()}`); });
if(type) { // Label
await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}/labels`, {
ztimson marked this conversation as resolved
Review

Bug: JSON string is manually constructed with template literal instead of using JSON.stringify(). This is error-prone and could break if type contains special characters. Should be body: JSON.stringify({labels: [\Reviewed/${type[0].toUpperCase() + type.slice(1).toLowerCase()}`]})`.

Bug: JSON string is manually constructed with template literal instead of using `JSON.stringify()`. This is error-prone and could break if `type` contains special characters. Should be `body: JSON.stringify({labels: [\`Reviewed/${type[0].toUpperCase() + type.slice(1).toLowerCase()}\`]})`.
Review

JSON injection vulnerability: The JSON body is manually constructed using a template literal. If type contains special characters, this could produce invalid JSON. While type is 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()}`]})`

JSON injection vulnerability: The JSON body is manually constructed using a template literal. If `type` contains special characters, this could produce invalid JSON. While `type` is 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()}\`]})`
Review

Potential runtime error: If type is an empty string or undefined (e.g., if the AI fails to call the type tool), then type[0].toUpperCase() will throw a TypeError. Add validation before this line: if(!type) throw new Error('AI failed to set type');

Potential runtime error: If `type` is an empty string or undefined (e.g., if the AI fails to call the type tool), then `type[0].toUpperCase()` will throw a TypeError. Add validation before this line: `if(!type) throw new Error('AI failed to set type');`
Review

Breaking change: Label prefix changed from Kind/${type} to Reviewed/${type} without explanation. This could break existing workflows, filters, or automation that rely on the Kind/ label prefix. Verify this is intentional and update any dependent systems accordingly.

Breaking change: Label prefix changed from `Kind/${type}` to `Reviewed/${type}` without explanation. This could break existing workflows, filters, or automation that rely on the `Kind/` label prefix. Verify this is intentional and update any dependent systems accordingly.
method: 'POST',
headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},
body: `{"labels":["Reviewed/${type[0].toUpperCase() + type.slice(1).toLowerCase()}"]}`
}).then(async resp => { if(!resp.ok) throw new Error(`${resp.status} ${await resp.text()}`); });
}
console.log(`Title: ${title}\nType: ${type}\nBody:\n${body}`);
})();
})().catch(err => {
console.error(`Error: ${err.message || err.toString()}`);
process.exit(1);
});

View File

@@ -106,4 +106,7 @@ dotenv.config({path: '.env.local', override: true, quiet: true, debug: false});
if(!res.ok) throw new Error(`${res.status} ${await res.text()}`);
}
console.log(comments.map(c => `${c.path}${c.new_position ? `:${c.new_position}` : ''}\n${c.body}`).join('\n\n') + '\n\n' + summary);
})();
})().catch(err => {
console.error(`Error: ${err.message || err.toString()}`);
process.exit(1);
});