Check for duplicates before adding tickets #12
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"name": "@ztimson/ai-agents",
|
||||
"version": "0.0.8",
|
||||
"version": "0.1.0",
|
||||
"description": "AI agents",
|
||||
"keywords": ["ai", "review"],
|
||||
"author": "ztimson",
|
||||
|
||||
@@ -32,7 +32,7 @@ dotenv.config({path: '.env.local', override: true, quiet: true});
|
||||
if(resp.ok) return resp.json();
|
||||
else throw new Error(`${resp.status} ${await resp.text()}`);
|
||||
|
ztimson marked this conversation as resolved
Outdated
|
||||
});
|
||||
if(issueData.labels.length !== 1 || issueData.labels?.[0] !== 'Review/AI') {
|
||||
if(issueData.labels?.[0]?.name !== 1 || issueData.labels?.[0] !== 'Review/AI') {
|
||||
|
ztimson marked this conversation as resolved
Outdated
assistant
commented
Logic error: The check 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.
assistant
commented
Logic error: The condition checks if 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.
assistant
commented
Critical logic error: The condition 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();
|
||||
}
|
||||
@@ -150,11 +150,13 @@ Output ONLY markdown. No explanations, labels, or extra formatting.`});
|
||||
if(!body) throw new Error('Invalid response from AI');
|
||||
|
ztimson marked this conversation as resolved
assistant
commented
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
assistant
commented
Potential bug: The search API endpoint 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.
assistant
commented
Critical bug: The search API endpoint is missing the repository path segment. This should be 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({
|
||||
priority_repo_id: repo,
|
||||
owner,
|
||||
|
ztimson marked this conversation as resolved
Outdated
assistant
commented
Bug: The 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.
assistant
commented
Bug: The 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
assistant
commented
Error handling issue: If the search API returns an error, the code defaults to an empty array 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.
assistant
commented
Type safety issue: The fallback 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.
assistant
commented
Race condition: The duplicate search uses the 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');`
|
||||
@@ -167,7 +169,7 @@ Output ONLY markdown. No explanations, labels, or extra formatting.`});
|
||||
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
assistant
commented
Logic issue: The duplicate detection uses 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.
assistant
commented
Logic issue: The regex pattern 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 => hasDuplicates.includes(id.toString()))) != null) {
|
||||
if(!!hasDuplicates && (dupeId = dupeIds.find(id => new RegExp(`(^| )${id}( |$)`, 'm').test(hasDuplicates)))) {
|
||||
|
ztimson marked this conversation as resolved
Outdated
assistant
commented
Same critical bug as line 31: Same critical bug as line 31: `updateRes` is referenced before it's defined. Should reference `resp.status` and `resp.text()` instead.
assistant
commented
Same syntax error as line 31: Cannot use 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
assistant
commented
Bug: JSON string is manually constructed instead of using 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.
assistant
commented
JSON injection vulnerability: The JSON body is manually constructed using a template literal instead of JSON.stringify(). If 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'},
|
||||
@@ -188,7 +190,7 @@ Output ONLY markdown. No explanations, labels, or extra formatting.`});
|
||||
}
|
||||
|
||||
// Update ticket
|
||||
const updateRes = await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}`, {
|
||||
await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}`, {
|
||||
|
ztimson marked this conversation as resolved
Outdated
assistant
commented
The label name changed from 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
assistant
commented
Same bugs as above: 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
assistant
commented
Missing error handling: If 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})
|
||||
@@ -202,4 +204,7 @@ Output ONLY markdown. No explanations, labels, or extra formatting.`});
|
||||
}
|
||||
|
||||
console.log(`Title: ${title}\nType: ${type}\nBody:\n${body}`);
|
||||
})();
|
||||
})().catch(err => {
|
||||
console.error(`Error: ${err.message || err.toString()}`);
|
||||
process.exit(1);
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
|
||||
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.