Added ticket refinement bot #4
2
.github/workflows/ticket-refinement.yml
vendored
@@ -14,7 +14,7 @@ jobs:
|
|||||||
git checkout ${{ github.event.repository.default_branch }}
|
git checkout ${{ github.event.repository.default_branch }}
|
||||||
|
|
||||||
- name: Run AI Formatter
|
- name: Run AI Formatter
|
||||||
|
ztimson marked this conversation as resolved
|
|||||||
run: npx -y @ztimson/ai-agents@latest format .github/issue_templates/ai-refinement.md
|
run: npx -y @ztimson/ai-agents@latest format .github/issue_template/ai-refinement.md
|
||||||
env:
|
env:
|
||||||
AI_HOST: anthropic
|
AI_HOST: anthropic
|
||||||
|
ztimson marked this conversation as resolved
assistant
commented
Invalid model name: Invalid model name: `claude-sonnet-4-5` is not a valid Anthropic model name. It should likely be `claude-sonnet-4` or `claude-3-5-sonnet-20241022` (or similar valid Anthropic model identifier).
assistant
commented
The AI_MODEL value 'claude-sonnet-4-5' appears incorrect. Claude model names typically use format like 'claude-sonnet-4-20250514' or 'claude-3-5-sonnet-20241022'. Verify this is a valid model identifier. The AI_MODEL value 'claude-sonnet-4-5' appears incorrect. Claude model names typically use format like 'claude-sonnet-4-20250514' or 'claude-3-5-sonnet-20241022'. Verify this is a valid model identifier.
|
|||||||
AI_MODEL: claude-sonnet-4-5
|
AI_MODEL: claude-sonnet-4-5
|
||||||
|
ztimson marked this conversation as resolved
assistant
commented
Hardcoded AI model "claude-sonnet-4-5" appears to be incorrect. The correct model name should be "claude-sonnet-4" or "claude-3-5-sonnet-20240620" based on Anthropic's naming conventions. This will cause API errors. Hardcoded AI model "claude-sonnet-4-5" appears to be incorrect. The correct model name should be "claude-sonnet-4" or "claude-3-5-sonnet-20240620" based on Anthropic's naming conventions. This will cause API errors.
|
|||||||
|
|||||||
@@ -11,7 +11,7 @@ dotenv.config({path: '.env.local', override: true, quiet: true});
|
|||||||
|
|
||||||
(async () => {
|
(async () => {
|
||||||
|
assistant
commented
The entire script is wrapped in an async IIFE without proper error handling. If any unhandled error occurs, it will cause an unhandled promise rejection. Add a .catch() block at the end to handle errors gracefully. The entire script is wrapped in an async IIFE without proper error handling. If any unhandled error occurs, it will cause an unhandled promise rejection. Add a .catch() block at the end to handle errors gracefully.
|
|||||||
let p = process.argv[process.argv.length - 1];
|
let p = process.argv[process.argv.length - 1];
|
||||||
|
ztimson marked this conversation as resolved
assistant
commented
Wrong script name in condition: The check Wrong script name in condition: The check `if(p === 'review' || p.endsWith('review.mjs'))` should be `if(p === 'refine' || p.endsWith('refine.mjs'))` since this is the refine.mjs script, not review.mjs.
|
|||||||
if(p === 'review' || p.endsWith('review.mjs')) p = null;
|
if(p === 'refine' || p.endsWith('refine.mjs')) p = null;
|
||||||
|
ztimson marked this conversation as resolved
assistant
commented
The path detection regex '/(\/|[A-Z]:)/.test(p)' checks if the path is absolute, but the logic is inverted - if it's NOT absolute, it joins with cwd. However, this doesn't handle edge cases like './' or '../' relative paths correctly, which would be treated as absolute due to the '/' character. The path detection regex '/(\\/|[A-Z]:)/.test(p)' checks if the path is absolute, but the logic is inverted - if it's NOT absolute, it joins with cwd. However, this doesn't handle edge cases like './' or '../' relative paths correctly, which would be treated as absolute due to the '/' character.
assistant
commented
Path validation regex uses 'm' flag unnecessarily. The multiline flag isn't needed for this single-line path check. Also, the regex doesn't handle relative paths starting with './' or '../' which are valid. Path validation regex uses 'm' flag unnecessarily. The multiline flag isn't needed for this single-line path check. Also, the regex doesn't handle relative paths starting with './' or '../' which are valid.
|
|||||||
if(!/(\/|[A-Z]:)/.test(p)) p = path.join(process.cwd(), p);
|
if(!/(\/|[A-Z]:)/.test(p)) p = path.join(process.cwd(), p);
|
||||||
|
|
||||||
if(!p || !fs.existsSync(p)) throw new Error('Please provide a template');
|
if(!p || !fs.existsSync(p)) throw new Error('Please provide a template');
|
||||||
@@ -81,10 +81,14 @@ ${template.trim()}
|
|||||||
\`\`\`
|
\`\`\`
|
||||||
|
|
||||||
Output ONLY the formatted ticket, no explanation.`
|
Output ONLY the formatted ticket, no explanation.`
|
||||||
});
|
})
|
||||||
|
|
||||||
const messages = await ai.language.ask(`Title: ${issueData.title}\n\nDescription:\n${issueData.body || 'No description provided'}`);
|
const messages = await ai.language.ask(`Title: ${issueData.title}\n\nDescription:\n${issueData.body || 'No description provided'}`).catch(() => []);;
|
||||||
|
ztimson marked this conversation as resolved
Outdated
assistant
commented
No error handling for AI call: The No error handling for AI call: The `ai.language.ask()` call has no try-catch block. If the AI service fails, times out, or returns an unexpected format, the script will crash without helpful error messages.
assistant
commented
Double semicolon syntax error. Remove one semicolon from '.catch(() => []);;' Double semicolon syntax error. Remove one semicolon from '.catch(() => []);;'
assistant
commented
Error handling with Error handling with `.catch(() => [])` silently swallows all errors from the AI request. This makes debugging difficult. Consider logging the error or providing more context about what went wrong.
assistant
commented
Error handling with .catch(() => []) silently swallows all errors, making debugging difficult. At minimum, log the error before returning an empty array, or let it propagate for better error visibility. Error handling with .catch(() => []) silently swallows all errors, making debugging difficult. At minimum, log the error before returning an empty array, or let it propagate for better error visibility.
|
|||||||
const content = messages.pop().content;
|
const content = messages?.pop()?.content;
|
||||||
|
ztimson marked this conversation as resolved
Outdated
assistant
commented
Unsafe array access: Unsafe array access: `messages.pop()` assumes the array is non-empty. If the AI returns an empty response, this will throw an error when accessing `.content`. Add a check to ensure messages exist.
|
|||||||
|
if(!content) {
|
||||||
|
console.log('Invalid response from AI');
|
||||||
|
ztimson marked this conversation as resolved
Outdated
assistant
commented
Potential runtime error: Potential runtime error: `type[0].toUpperCase()` will throw if `type` is an empty string. The fallback 'Unassigned' is set earlier, but if the regex extracts an empty string, this will fail. Add a check: `type && type.length > 0` before accessing `type[0]`.
|
|||||||
|
return process.exit(1);
|
||||||
|
}
|
||||||
const title = /^# (.+)$/m.exec(content)?.[1] || issueData.title;
|
const title = /^# (.+)$/m.exec(content)?.[1] || issueData.title;
|
||||||
const typeMatch = /^## Type:\s*(.+)$/m.exec(content);
|
const typeMatch = /^## Type:\s*(.+)$/m.exec(content);
|
||||||
|
assistant
commented
Type extraction logic is fragile. The split('/')[0] assumes the format is "Type/Subtype" but the template shows "Type: [Option1/Option2/...]". This will incorrectly parse "[Bug" instead of "Bug". Use a more robust regex or trim brackets. Type extraction logic is fragile. The split('/')[0] assumes the format is "Type/Subtype" but the template shows "Type: [Option1/Option2/...]". This will incorrectly parse "[Bug" instead of "Bug". Use a more robust regex or trim brackets.
|
|||||||
const type = typeMatch?.[1]?.split('/')[0]?.trim() || 'Unassigned';
|
const type = typeMatch?.[1]?.split('/')[0]?.trim() || 'Unassigned';
|
||||||
|
ztimson marked this conversation as resolved
assistant
commented
The type extraction logic 'type[0].toUpperCase() + type.slice(1).toLowerCase()' operates on a string but 'type' is already a string (not an array), so 'type[0]' gets the first character. While this works, the variable name is misleading. Also, this doesn't handle the 'Unassigned' case - it would create a 'Kind/Unassigned' label which may not be desired. The type extraction logic 'type[0].toUpperCase() + type.slice(1).toLowerCase()' operates on a string but 'type' is already a string (not an array), so 'type[0]' gets the first character. While this works, the variable name is misleading. Also, this doesn't handle the 'Unassigned' case - it would create a 'Kind/Unassigned' label which may not be desired.
assistant
commented
The type parsing logic The type parsing logic `type?.split('/')[0]?.trim()` assumes a specific format but then constructs a label with `Kind/${type}`. This creates a mismatch - if the AI returns "Bug", the label becomes "Kind/Bug", but the check on line 36 looks for "Review/AI". Consider standardizing the label format.
|
|||||||
@@ -98,7 +102,7 @@ Output ONLY the formatted ticket, no explanation.`
|
|||||||
body: JSON.stringify({
|
body: JSON.stringify({
|
||||||
title,
|
title,
|
||||||
body,
|
body,
|
||||||
|
assistant
commented
The PATCH request replaces all labels with just the type label, which will remove the "Review/AI" label that triggered the workflow. This could cause issues if the workflow is re-triggered or if other labels are needed. Consider appending to existing labels instead of replacing them. The PATCH request replaces all labels with just the type label, which will remove the "Review/AI" label that triggered the workflow. This could cause issues if the workflow is re-triggered or if other labels are needed. Consider appending to existing labels instead of replacing them.
|
|||||||
labels: [`Type/${type[0].toUpperCase() + type.slice(1).toLowerCase()}`]
|
labels: type?.length ? [`Kind/${type[0].toUpperCase() + type.slice(1).toLowerCase()}`] : []
|
||||||
|
ztimson marked this conversation as resolved
assistant
commented
The label update logic replaces ALL labels with just the Kind label. This will remove the 'Review/AI' label and any other existing labels. Consider appending to existing labels or filtering more carefully: The label update logic replaces ALL labels with just the Kind label. This will remove the 'Review/AI' label and any other existing labels. Consider appending to existing labels or filtering more carefully: `labels: [...issueData.labels.map(l => l.name).filter(n => !n.startsWith('Kind/')), ...]`
assistant
commented
Label assignment logic has a bug: checking 'type?.length' on a string will always be truthy (even empty string has length property). This should check if type is truthy or has non-zero length. Also, the label format assumes "Kind/" prefix which may not match existing label conventions. Label assignment logic has a bug: checking 'type?.length' on a string will always be truthy (even empty string has length property). This should check if type is truthy or has non-zero length. Also, the label format assumes "Kind/" prefix which may not match existing label conventions.
assistant
commented
The labels array assignment completely replaces existing labels. This will remove the "Review/AI" label that triggered the workflow, and any other labels the user added. Consider appending to existing labels instead of replacing them. The labels array assignment completely replaces existing labels. This will remove the "Review/AI" label that triggered the workflow, and any other labels the user added. Consider appending to existing labels instead of replacing them.
|
|||||||
})
|
})
|
||||||
});
|
});
|
||||||
if(!updateRes.ok) throw new Error(`${updateRes.status} ${await updateRes.text()}`);
|
if(!updateRes.ok) throw new Error(`${updateRes.status} ${await updateRes.text()}`);
|
||||||
|
|||||||
@@ -37,7 +37,7 @@ dotenv.config({path: '.env.local', override: true, quiet: true, debug: false});
|
|||||||
...options,
|
...options,
|
||||||
model: [host, model],
|
model: [host, model],
|
||||||
path: process.env['path'] || os.tmpdir(),
|
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 one brief bullet-point summary.`,
|
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.`,
|
||||||
tools: [{
|
tools: [{
|
||||||
name: 'read_file',
|
name: 'read_file',
|
||||||
description: 'Read contents of a file',
|
description: 'Read contents of a file',
|
||||||
|
|||||||
Path mismatch: The workflow references
.github/issue_templates/ai-refinement.mdbut the actual file is located at.github/issue_template/ai-refinement.md(singular "template" not "templates"). This will cause the workflow to fail when trying to read the template file.Command name mismatch: The workflow runs
npx -y @ztimson/ai-agents@latest formatbut according to package.json, the binary is named "refine", not "format". This should benpx -y @ztimson/ai-agents@latest refine.The workflow calls 'format' subcommand but package.json defines 'refine' as the binary name. This mismatch will cause the workflow to fail. Either change the command to 'npx -y @ztimson/ai-agents@latest refine' or update package.json to use 'format' as the binary name.