ztimson/ai-agents
1
0
Fork 0
generated from ztimson/template
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

review-labels #14

Merged
ztimson merged 5 commits from review-labels into master 2026-01-14 14:06:21 -05:00
Conversation 2 Commits 5 Files Changed 6 +28 -8
6 changed files with 28 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
.github/pull_request_template.md vendored
View File

@@ -9,7 +9,5 @@
## Checklist ## Checklist
<!-- Complete after creating PR --> <!-- Complete after creating PR -->
- [ ] Linked issues - [ ] Reviewed changes (or use `Review/AI` label)
- [ ] Reviewed changes
- [ ] Updated comments/documentation - [ ] Updated comments/documentation

2
.github/workflows/code-review.yml vendored
View File

@@ -2,7 +2,7 @@ name: Code review
on: on:
pull_request: pull_request:
types: [opened, synchronize, reopened] types: [opened, synchronize, reopened, labeled]
jobs: jobs:
review: review:

1
.github/workflows/ticket-refinement.yml vendored
View File

@@ -3,6 +3,7 @@ name: Ticket refinement
on: on:
issues: issues:
types: [labeled] types: [labeled]
jobs: jobs:
format: format:
runs-on: ubuntu-latest runs-on: ubuntu-latest

2
package.json
View File

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

3
src/refine.mjs
View File

@@ -181,8 +181,9 @@ Output ONLY markdown. No explanations, labels, or extra formatting.`});
const hasDuplicates = (await ai.language.ask(`ID: ${issueData.id}\nTitle: ${title}\n\`\`\`markdown\n${body}\n\`\`\``, { const hasDuplicates = (await ai.language.ask(`ID: ${issueData.id}\nTitle: ${title}\n\`\`\`markdown\n${body}\n\`\`\``, {
system: `Your job is to identify duplicates. Respond ONLY with the duplicate's ID number or "NONE" if no match exists\n\n${dupes}` system: `Your job is to identify duplicates. Respond ONLY with the duplicate's ID number or "NONE" if no match exists\n\n${dupes}`
}))?.pop()?.content; }))?.pop()?.content;
// Handle duplicates // Handle duplicates
if(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) {
ztimson marked this conversation as resolved
assistant commented 2026-01-14 13:47:40 -05:00
Review
Copy Link

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.

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.
assistant commented 2026-01-14 13:47:40 -05:00
Review
Copy Link

Potential type coercion bug: Using == instead of === in dupeIds.find(id => id == hasDuplicates.trim()) and dupeId != issueData.id may cause unexpected behavior if IDs are sometimes strings and sometimes numbers. Use strict equality (=== and !==) for more predictable comparisons.

Potential type coercion bug: Using `==` instead of `===` in `dupeIds.find(id => id == hasDuplicates.trim())` and `dupeId != issueData.id` may cause unexpected behavior if IDs are sometimes strings and sometimes numbers. Use strict equality (`===` and `!==`) for more predictable comparisons.
assistant commented 2026-01-14 13:52:35 -05:00
Review
Copy Link

Potential type coercion bug: Using == instead of === in dupeIds.find(id => id == hasDuplicates.trim()) may cause unexpected behavior if IDs are sometimes strings and sometimes numbers. Use strict equality (===) for more predictable comparisons.

Potential type coercion bug: Using `==` instead of `===` in `dupeIds.find(id => id == hasDuplicates.trim())` may cause unexpected behavior if IDs are sometimes strings and sometimes numbers. Use strict equality (`===`) for more predictable comparisons.
assistant commented 2026-01-14 13:52:35 -05:00
Review
Copy Link

Redundant check: The condition dupeId != issueData.id is 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.

Redundant check: The condition `dupeId != issueData.id` is 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.
await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}/comments`, { await fetch(`${git}/api/v1/repos/${owner}/${repo}/issues/${ticket}/comments`, {
method: 'POST', method: 'POST',
headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'}, headers: {'Authorization': `token ${auth}`, 'Content-Type': 'application/json'},

24
src/review.mjs
View File

@@ -19,12 +19,23 @@ dotenv.config({path: '.env.local', override: true, quiet: true, debug: false});
owner = process.env['GIT_OWNER'], owner = process.env['GIT_OWNER'],
repo = process.env['GIT_REPO'], repo = process.env['GIT_REPO'],
auth = process.env['GIT_TOKEN'], auth = process.env['GIT_TOKEN'],
labelEnabled = process.env['LABEL_ENABLED'] || 'Review/AI',
pr = process.env['PULL_REQUEST'], pr = process.env['PULL_REQUEST'],
host = process.env['AI_HOST'], host = process.env['AI_HOST'],
model = process.env['AI_MODEL'], model = process.env['AI_MODEL'],
token = process.env['AI_TOKEN']; token = process.env['AI_TOKEN'];
console.log(`Reviewing: ${root}\n`); console.log(`Reviewing: ${root}\n`);
const info = await fetch(`${git}/api/v1/repos/${owner}/${repo}/pulls/${pr}`)
ztimson marked this conversation as resolved
assistant commented 2026-01-14 13:47:40 -05:00
Review
Copy Link

Missing error handling: The fetch call should handle errors. If the API call fails, info will be undefined and line 30 will throw an error when trying to access info.labels.

Missing error handling: The fetch call should handle errors. If the API call fails, `info` will be undefined and line 30 will throw an error when trying to access `info.labels`.
assistant commented 2026-01-14 13:52:35 -05:00
Review
Copy Link

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.

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.
.then(async resp => {
ztimson marked this conversation as resolved Outdated
assistant commented 2026-01-14 13:47:40 -05:00
Outdated
Review
Copy Link

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) or 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 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)` or `info.labels?.length === 0 || !info.labels.some(l => l.name === labelEnabled)`
assistant commented 2026-01-14 13:52:35 -05:00
Outdated
Review
Copy Link

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.

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.
if(resp.ok) return resp.json();
ztimson marked this conversation as resolved Outdated
assistant commented 2026-01-14 13:52:35 -05:00
Outdated
Review
Copy Link

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.

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.
throw new Error(`${resp.status} ${await resp.text()}`);
});
if(!info.labels.some(l => l.name === labelEnabled)) {
console.log('Skipping');
return process.exit();
}
const branch = process.env['GIT_BRANCH'] || await $`cd ${root} && git symbolic-ref refs/remotes/origin/HEAD`; const branch = process.env['GIT_BRANCH'] || await $`cd ${root} && git symbolic-ref refs/remotes/origin/HEAD`;
const comments = []; const comments = [];
const commit = await $`cd ${root} && git log -1 --pretty=format:%H`; const commit = await $`cd ${root} && git log -1 --pretty=format:%H`;
@@ -53,7 +64,7 @@ dotenv.config({path: '.env.local', override: true, quiet: true, debug: false});
...options, ...options,
ztimson marked this conversation as resolved Outdated
assistant commented 2026-01-14 13:47:40 -05:00
Outdated
Review
Copy Link

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.

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.
assistant commented 2026-01-14 13:52:35 -05:00
Outdated
Review
Copy Link

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.

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.
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 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}`,
tools: [{ tools: [{
name: 'read_file', name: 'read_file',
description: 'Read contents of a file', description: 'Read contents of a file',
@@ -87,7 +98,16 @@ dotenv.config({path: '.env.local', override: true, quiet: true, debug: false});
}] }]
ztimson marked this conversation as resolved Outdated
assistant commented 2026-01-14 13:52:35 -05:00
Outdated
Review
Copy Link

Potential null/undefined error: If info.title or info.body is 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'}.

Potential null/undefined error: If `info.title` or `info.body` is 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'}`.
}); });
const messages = await ai.language.ask(gitDiff); const messages = await ai.language.ask(`Title: ${info.title || 'None'}
Description:
\`\`\`md
${info.body || 'None'}
\`\`\`
Git Diff:
\`\`\`
${gitDiff}
\`\`\``);
const summary = messages.pop().content; const summary = messages.pop().content;
if(git) { if(git) {
const res = await fetch(`${git}/api/v1/repos/${owner}/${repo}/pulls/${pr}/reviews`, { const res = await fetch(`${git}/api/v1/repos/${owner}/${repo}/pulls/${pr}/reviews`, {