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

Conversation

gary-van-woerkens
Copy link
Contributor

No description provided.

@gary-van-woerkens gary-van-woerkens marked this pull request as draft April 25, 2025 22:29
Copy link

socket-security bot commented Apr 25, 2025

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedzod@​3.24.410010010094100

View full report

Copy link
Contributor

@RealVidy RealVidy left a comment

Choose a reason for hiding this comment

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

Très cool si ça fonctionne ça !

J'ai fait pas mal de commentaires mais dans l'ensemble je suis aligné. Merci !

- A comprehensive overall summary of the PR in the "summary" field
- Multiple specific comments targeting different issues with accurate file paths and line numbers
- Detailed explanations in each comment's body
- Code suggestions where appropriate
Copy link
Contributor

Choose a reason for hiding this comment

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

Il faut peut-être lui fournir un exemple de "code suggestion" parce que c'est dépendant de la plateforme où est faite la PR. Pas sûr que gitlab et github utilisent le même format en gros.

Là comme on lui dit pas que c'est du github en plus, il va pas forcément savoir quoi faire.

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...]
}

Comment on lines +4 to +12
interface CodeReviewResponse {
summary: string
comments: Array<{
path: string
line: number
body: string
suggestion?: string
}>
}
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 ?

* @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.

// Schémas de validation pour garantir le bon format de la réponse
const CommentSchema = z.object({
path: z.string(),
line: z.number().int().positive(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Je flag ça juste une fois maintenant : Là on bosse sur une seule ligne mais est-ce qu'on voudra mettre le temps nécessaire à la gestion d'un interval de lignes plutôt qu'une seule ?

Si oui, ça peut valoir le coup de prévoir des maintenant une "startLine" et une "endLine" peut-être ?

@gary-van-woerkens gary-van-woerkens linked an issue Apr 28, 2025 that may be closed by this pull request
@revu-bot
Copy link

revu-bot bot commented May 6, 2025

Pull Request Analysis

Overview

This PR implements a new "line-comments" strategy for the Revu code review tool. The main goal is to enhance the review experience by allowing the AI to provide specific comments on individual lines of code rather than just a single global comment on the entire PR. The implementation includes:

  1. A new prompt strategy for generating line-specific comments
  2. A specialized Anthropic sender that uses Claude's tool use/function calling capability
  3. A comment handler that posts individual review comments on specific lines in GitHub PRs
  4. Integration with Zod for schema validation of the structured response

The changes effectively solve the intended problem by creating a complete pipeline from prompt generation to structured response handling, with appropriate fallbacks when things go wrong.

Code Quality Review

Strengths

  • Well-structured architecture: The code follows a clear separation of concerns with distinct modules for prompt strategies, Anthropic senders, and comment handlers.
  • Strategy pattern implementation: The use of strategy patterns for both prompt generation and comment handling makes the system extensible for future strategies.
  • Robust error handling: The implementation includes fallback mechanisms when the structured response format fails, defaulting to the original global comment approach.
  • Type safety: The code uses TypeScript interfaces and Zod schemas to ensure type safety and validate the structured response from Claude.
  • Modular design: The changes maintain the existing functionality while adding new capabilities, making it easy to switch between strategies.
  • Comprehensive documentation: Each new module and function includes clear JSDoc comments explaining its purpose and behavior.

Areas for Improvement

  • Error handling in lineCommentsSender.ts: The silent catch block in the fallback logic could hide important errors:
try {
  // ...parsing logic...
} catch {
  // Silent catch - continue to next content block or error
}

Consider logging the error or adding more specific error handling.

  • Hardcoded values: The Claude model name (claude-3-7-sonnet-latest) is hardcoded in multiple places. Consider extracting this to a configuration constant.

  • French comments in English codebase: There are some French comments in line-comments-handler.ts:

// Schémas de validation pour garantir le bon format de la réponse

These should be translated to English for consistency.

  • Potential race condition: When updating existing comments, there's no locking mechanism to prevent conflicts if multiple updates happen simultaneously.

  • Duplicate code: The Anthropic client initialization is duplicated across different sender implementations. Consider extracting this to a shared utility function.

Security Assessment

  • API key handling: The code properly uses environment variables for the Anthropic API key, which is a good practice.

  • Input validation: The implementation uses Zod for validating the structured response from Claude, which helps prevent injection attacks or malformed data.

  • GitHub token handling: The GitHub token is properly passed through the system without being exposed.

  • Comment marker IDs: The code sanitizes file paths when creating comment marker IDs:

return `${path}:${line}`.replace(/[^a-zA-Z0-9-_:.]/g, '_');

This prevents potential injection issues in comment markers.

  • Error messages: Some error messages in line-comments-sender.ts include potentially sensitive data:
console.log('Input:', content.input)
console.log('Tool name:', content.name)

Consider limiting the information exposed in logs.

Best Practices Evaluation

  • Configuration-driven behavior: The system reads from a configuration file to determine which strategy to use, making it easy to switch behaviors without code changes.

  • Graceful degradation: The implementation includes fallback mechanisms when the structured response format fails.

  • Consistent naming conventions: The code follows consistent naming patterns across new modules.

  • Comprehensive type definitions: The code uses TypeScript interfaces and type aliases to ensure type safety.

  • Missing tests: There are no tests included for the new functionality, which would be important for ensuring reliability.

  • Template organization: The new Handlebars template is well-structured and provides clear instructions to the AI.

  • Dependency management: The PR properly adds the new Zod dependency to package.json.

Recommendations

  1. Add unit tests: Create tests for the new functionality, especially for the response parsing and comment handling logic.

  2. Extract configuration constants: Move hardcoded values like the Claude model name to a central configuration file:

// config/ai-models.ts
export const CLAUDE_MODEL = 'claude-3-7-sonnet-latest';
  1. Improve error logging: Add more detailed error logging in the catch blocks:
try {
  // ...parsing logic...
} catch (error) {
  console.error('Failed to parse JSON response:', error);
  // Continue to next content block or error
}
  1. Translate French comments: Ensure all comments are in English for consistency.

  2. Add retry logic: Consider adding retry logic for GitHub API calls that might fail due to rate limiting or network issues.

  3. Extract shared initialization code: Create a utility function for initializing the Anthropic client:

// utils/anthropic-client.ts
export function createAnthropicClient() {
  return new Anthropic({
    apiKey: process.env.ANTHROPIC_API_KEY
  });
}
  1. Add more detailed logging: Add structured logging throughout the process to make debugging easier.

Additional Notes

  • The PR changes the default strategy in config.json to "line-comments", which means all PRs will now use the new strategy. Consider whether this should be a gradual rollout.

  • The implementation relies on Claude's tool use capability, which is a relatively new feature. It would be good to monitor its reliability in production.

  • The PR adds a significant amount of new code, which increases the maintenance burden. Ensure the team is familiar with the new architecture.

  • Consider adding a feature flag system to enable/disable the line comments feature independently of the configuration file, which would allow for easier rollbacks if issues are discovered.

@gary-van-woerkens gary-van-woerkens marked this pull request as ready for review May 13, 2025 12:10
Copy link

@RealVidy RealVidy merged commit 5a75a6c into main May 13, 2025
6 checks passed
@tokenbureau
Copy link

tokenbureau bot commented May 13, 2025

🎉 This PR is included in version 1.5.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@tokenbureau tokenbureau bot added the released label May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inline comments
2 participants