Skip to content

feat: Add line-specific comments functionality #21

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
May 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 56 additions & 25 deletions __tests__/private-repo-support.test.ts
Original file line number Diff line number Diff line change
@@ -1,35 +1,66 @@
import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'
import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest'
import { ChildProcess } from 'child_process'
import type { ExecOptions, ExecException } from 'child_process'

// Définir les mocks avant d'importer quoi que ce soit
vi.mock('child_process', () => ({
exec: vi.fn()
}))

vi.mock('util', () => ({
promisify: vi.fn().mockImplementation((fn) => fn)
}))

// Importer après avoir configuré les mocks
import { cloneRepository } from '../src/repo-utils.ts'
import * as childProcess from 'child_process'

// Mock the execAsync function to test URL transformation with tokens
vi.mock('child_process', () => {
return {
exec: vi.fn((command, options, callback) => {
if (callback) {
callback(null, { stdout: '', stderr: '' })
}
return {
stdout: '',
stderr: ''
}
})
}
})

vi.mock('util', () => {
return {
promisify: vi.fn().mockImplementation((fn) => {
return fn
})
}
})
/**
* Type exact pour la fonction callback d'exec
*/
type ExecCallback = (
error: ExecException | null,
stdout: string,
stderr: string
) => void

describe('Private Repository Support', () => {
const mockExec = vi.spyOn(childProcess, 'exec')
const mockExec = vi.mocked(childProcess.exec)

beforeEach(() => {
mockExec.mockClear()
// Reset the mock implementation for each test
mockExec.mockReset()

/**
* Implémentation du mock qui respecte le format de l'API Node.js
* en utilisant un cast nécessaire mais explicite
*/
mockExec.mockImplementation(
(
command: string,
optionsOrCallback?: ExecOptions | ExecCallback,
callback?: ExecCallback
) => {
// Gérer le cas où options est en fait le callback
if (typeof optionsOrCallback === 'function') {
callback = optionsOrCallback
}

// Appeler le callback si fourni
if (callback && typeof callback === 'function') {
callback(null, '', '')
}

// Retourner un objet minimal qui simule un ChildProcess
// Le cast est nécessaire ici car nous ne pouvons pas implémenter
// toutes les propriétés de ChildProcess dans ce contexte de test
return {
stdout: { on: vi.fn(), pipe: vi.fn() },
stderr: { on: vi.fn(), pipe: vi.fn() },
stdin: { on: vi.fn(), pipe: vi.fn() }
} as unknown as ChildProcess
}
)
})

afterEach(() => {
Expand Down
2 changes: 1 addition & 1 deletion config.json
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"promptStrategy":"modified-files"}
{"promptStrategy":"line-comments"}
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
"@anthropic-ai/sdk": "^0.40.0",
"dotenv": "^16.4.7",
"handlebars": "^4.7.8",
"probot": "^13.4.3"
"probot": "^13.4.3",
"zod": "^3.24.3"
},
"devDependencies": {
"@eslint/eslintrc": "^3.3.1",
Expand Down
35 changes: 35 additions & 0 deletions src/anthropic-senders/default-sender.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import Anthropic from '@anthropic-ai/sdk'

/**
* Default Anthropic sender.
* This sender is used by default and sends the prompt to Anthropic
* expecting a regular text response.
*
* @param prompt - The prompt to send to Anthropic
* @returns The text response from Anthropic
*/
export async function defaultSender(prompt: string): Promise<string> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'ai fait un commentaire plus bas qui permettrait d'éliminer ce defaultSender et de le remplacer par un "anthropicSender" avec les "tools" en paramètre optionnel.

// Initialize Anthropic client
const anthropic = new Anthropic({
apiKey: process.env.ANTHROPIC_API_KEY
})

// Send to Anthropic API
const message = await anthropic.messages.create({
model: 'claude-3-7-sonnet-latest',
max_tokens: 4096,
temperature: 0, // Using 0 for consistent, deterministic code review feedback
messages: [
{
role: 'user',
content: prompt
}
]
})

// Extract text from the content block
if (message.content[0].type !== 'text') {
throw new Error('Unexpected response type from Anthropic')
}
return message.content[0].text
}
27 changes: 27 additions & 0 deletions src/anthropic-senders/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { defaultSender } from './default-sender.ts'
import { lineCommentsSender } from './line-comments-sender.ts'

/**
* Type definition for all Anthropic senders
*/
export type AnthropicSender = (prompt: string) => Promise<string>

/**
* Gets the appropriate sender based on the strategy name.
* This selects how to send and process the response to/from Anthropic API.
*
* @param strategyName - The name of the prompt strategy used
* @returns The appropriate sender function
*/
export function getSender(strategyName?: string): AnthropicSender {
switch (strategyName?.toLowerCase()) {
case 'line-comments':
return lineCommentsSender
case 'default':
case 'modified-files':
default:
return defaultSender
}
}

export { defaultSender, lineCommentsSender }
121 changes: 121 additions & 0 deletions src/anthropic-senders/line-comments-sender.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
import Anthropic from '@anthropic-ai/sdk'

// Type for code review response
interface CodeReviewResponse {
summary: string
comments: Array<{
path: string
line: number
body: string
suggestion?: string
}>
}
Comment on lines +4 to +12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je pense que j'aurais plutôt mis ça dans le même fichier que le tool "provide_code_review" vu que c'est lié à ça plutôt qu'au lineCommentsSender, non ?


/**
* Line comments Anthropic sender.
* This sender uses Anthropic's Tool Use / Function Calling capability
* to enforce a structured JSON response with specific line-based comments.
*
* @param prompt - The prompt to send to Anthropic
* @returns A stringified JSON response containing structured review comments
*/
export async function lineCommentsSender(prompt: string): Promise<string> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dans l'idée c'est un "client" anthropic pour envoyer un prompt avec utilisation de tools.
En théorie c'est pas lié à la stratégie "line-comments" puisqu'on pourrait l'utiliser avec d'autres stratégies qui pourraient avoir besoin de tools, non ?

Dans ce cas, je l'appellerais simplement "anthropicClient", "anthropicSender", "anthropicClientWithTools" ou un truc du genre.

J'irais même plus loin :
On devrait avoir un seul "anthropicSender" avec en paramètre optionnel une liste de tools qui sera déterminée par la stratégie. Donc "modified-files" laisserait ce paramètre vide et "line-comments" enverrait une liste avec un seul tool : "provide_code_review"

Ça te paraît cohérent ?

Dispo pour en parler si besoin

// Initialize Anthropic client
const anthropic = new Anthropic({
apiKey: process.env.ANTHROPIC_API_KEY
})

// Send to Anthropic API with tool use configuration
const message = await anthropic.messages.create({
model: 'claude-3-7-sonnet-latest',
max_tokens: 4096,
temperature: 0,
messages: [
{
role: 'user',
content: prompt
}
],
tools: [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je me dis que ça pourrait être bien d'avoir un tool utils capable de construire un objet JSON "anthropicToolDefinition" (ou openAI, Mistral, LLaMa plus tard) automatiquement. Sinon on va devoir réécrire et maintenir cet objet dans chaque sender qui communique avec anthropic et dès qu'un tool va changer un peu ça risque de devenir embêtant à maintenir et source d'erreurs.

On peut commencer simple avec une fonction qui renvoie simplement :

{
        name: 'provide_code_review',
        description:
          'Provide structured code review with line-specific comments',
        [Reste de l'objet...]
}

{
name: 'provide_code_review',
description:
'Provide structured code review with line-specific comments',
input_schema: {
type: 'object',
properties: {
summary: {
type: 'string',
description: 'Overall summary of the PR'
},
comments: {
type: 'array',
items: {
type: 'object',
properties: {
path: {
type: 'string',
description: 'File path relative to repository root'
},
line: {
type: 'integer',
description: 'Line number for the comment'
},
body: {
type: 'string',
description: 'Detailed comment about the issue'
},
suggestion: {
type: 'string',
description: 'Suggested code to fix the issue (optional)'
}
},
required: ['path', 'line', 'body']
}
}
},
required: ['summary', 'comments']
}
}
]
})

// Extract response from tool use
// Find content blocks that are tool_use type
for (const content of message.content) {
if (content.type === 'tool_use') {
if (content.name === 'provide_code_review' && content.input) {
// Return the structured response as a JSON string
return JSON.stringify(content.input as CodeReviewResponse)
} else {
console.log('Input:', content.input)
console.log('Tool name:', content.name)
throw new Error('Tool name or input incorect')
}
} else {
// Fallback if tool use failed or returned unexpected format
if (content.type === 'text') {
// Try to parse any JSON that might be in the response
try {
const text = content.text
// const jsonMatch = text.match(/```json\s*([\s\S]*?)\s*```/)
const jsonMatch = text.match(/```json\n([\s\S]{1,10000}?)\n```/)
if (jsonMatch && jsonMatch[1]) {
return jsonMatch[1].trim()
}
// If the whole response is potentially JSON
if (text.trim().startsWith('{') && text.trim().endsWith('}')) {
return text
}

// Just return the text as is
return text
} catch {
// Silent catch - continue to next content block or error
}
}
}
}

throw new Error('Unexpected response format from Anthropic')
}
71 changes: 71 additions & 0 deletions src/comment-handlers/global-comment-handler.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import { type Context, ProbotOctokit } from 'probot'

type ListCommentsResponse = Awaited<
ReturnType<ProbotOctokit['rest']['issues']['listComments']>
>

type SingleComment = ListCommentsResponse['data'][number]

// Marker to identify our AI analysis comments
const COMMENT_MARKER = '<!-- REVU-AI-ANALYSIS -->'

/**
* Find existing AI analysis comment by looking for the unique marker
*/
async function findExistingAnalysisComment(context: Context, prNumber: number) {
const repo = context.repo()

// Get all comments on the PR
const { data: comments } = await context.octokit.issues.listComments({
...repo,
issue_number: prNumber
})

// Find the comment with our marker
return comments.find((comment) => comment.body.includes(COMMENT_MARKER))
}

/**
* Handles the creation or update of a global comment containing the analysis.
* This is the original behavior of the application.
*/
export async function globalCommentHandler(
context: Context,
prNumber: number,
analysis: string
) {
// Format the analysis with our marker
const formattedAnalysis = `${COMMENT_MARKER}\n\n${analysis}`

// Check if we already have an analysis comment
const existingComment = await findExistingAnalysisComment(context, prNumber)

await upsertComment(context, existingComment, formattedAnalysis, prNumber)
}

export async function upsertComment(
context: Context,
existingComment: SingleComment,
formattedAnalysis: string,
prNumber: number
) {
const repo = context.repo()

if (existingComment) {
// Update the existing comment
await context.octokit.issues.updateComment({
...repo,
comment_id: existingComment.id,
body: formattedAnalysis
})
return `Updated existing analysis comment on PR #${prNumber}`
} else {
// Post a new comment
await context.octokit.issues.createComment({
...repo,
issue_number: prNumber,
body: formattedAnalysis
})
return `Created new analysis comment on PR #${prNumber}`
}
}
32 changes: 32 additions & 0 deletions src/comment-handlers/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { type Context } from 'probot'
import { globalCommentHandler } from './global-comment-handler.ts'
import { lineCommentsHandler } from './line-comments-handler.ts'

/**
* Callback type for comment handlers
*/
export type CommentHandler = (
context: Context,
prNumber: number,
analysis: string
) => Promise<string>

/**
* Gets the appropriate comment handler based on the strategy name.
* This allows for different comment handling strategies based on the prompt strategy.
*
* @param strategyName - The name of the prompt strategy used
* @returns The appropriate comment handler function
*/
export function getCommentHandler(strategyName: string): CommentHandler {
switch (strategyName.toLowerCase()) {
case 'line-comments':
return lineCommentsHandler
case 'default':
case 'modified-files':
default:
return globalCommentHandler
}
}

export { globalCommentHandler, lineCommentsHandler }
Loading