chore: sync core lib and CLAUDE.md from agent-core#10
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the core library by synchronizing it with the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request syncs files from the agent-core repository, notably adding a new Git history collector. My review of the new lib/collectors/git.js file has identified a few areas for improvement. The new collectGitData function includes an adjustForAi option that is not used, its return object contains redundant data which could be confusing, and it relies on synchronous, blocking calls which could impact performance in an asynchronous environment. While I understand this is an automated sync, addressing these points would improve the quality of the code being integrated.
| * @param {Object} [options={}] - Collection options | ||
| * @param {string} [options.cwd] - Repository path (default: process.cwd()) | ||
| * @param {number} [options.top=20] - Number of hotspots to return | ||
| * @param {boolean} [options.adjustForAi=false] - Adjust bus factor for AI commits |
There was a problem hiding this comment.
The adjustForAi option is defined in DEFAULT_OPTIONS and documented in the JSDoc for collectGitData, but it is never used within the function. This is misleading for consumers of this function who might expect it to have an effect. Please either implement the logic to adjust the bus factor for AI commits or remove this unused option.
| function collectGitData(options = {}) { | ||
| const opts = { ...DEFAULT_OPTIONS, ...options }; | ||
| const cwd = opts.cwd || process.cwd(); | ||
|
|
||
| try { | ||
| binary.ensureBinarySync(); | ||
| } catch (err) { | ||
| return { | ||
| available: false, | ||
| error: `Binary not available: ${err.message}` | ||
| }; | ||
| } | ||
|
|
||
| let map; | ||
| try { | ||
| const json = binary.runAnalyzer(['git-map', 'init', cwd]); | ||
| map = JSON.parse(json); | ||
| } catch (err) { | ||
| return { | ||
| available: false, | ||
| error: `Git analysis failed: ${err.message}` | ||
| }; | ||
| } |
There was a problem hiding this comment.
The collectGitData function uses synchronous, blocking calls like binary.ensureBinarySync() and binary.runAnalyzer(). These can block the Node.js event loop, especially ensureBinarySync which might download a binary from the network. If this code is used in a server or other non-blocking context, this could cause performance issues.
Consider converting collectGitData to an async function and using the asynchronous alternatives ensureBinary() and runAnalyzerAsync() which are available in lib/binary/index.js. This would require the calling function collect in lib/collectors/index.js to also become async and await the result.
| return { | ||
| available: true, | ||
| health: { | ||
| active: humanList.length > 0, | ||
| busFactor, | ||
| aiRatio: Math.round(aiRatio * 100) / 100, | ||
| totalCommits: allCommits, | ||
| totalContributors: humanList.length | ||
| }, | ||
| hotspots, | ||
| contributors: humanList.slice(0, 10), | ||
| aiAttribution: { | ||
| ratio: Math.round(aiRatio * 100) / 100, | ||
| attributed: aiAttribution.attributed || 0, | ||
| heuristic: aiAttribution.heuristic || 0, | ||
| none: aiAttribution.none || 0, | ||
| tools: aiAttribution.tools || {} | ||
| }, | ||
| busFactor, | ||
| conventions: { | ||
| style: conventions.style || null, | ||
| prefixes: conventions.prefixes || {}, | ||
| scopes: conventions.scopes || {} | ||
| }, | ||
| releaseInfo: { | ||
| tagCount: releases.tags ? releases.tags.length : 0, | ||
| lastRelease: releases.tags && releases.tags.length > 0 | ||
| ? releases.tags[releases.tags.length - 1] | ||
| : null, | ||
| cadence: releases.cadence || null | ||
| }, | ||
| commitShape: { | ||
| typicalSize: commitShape.typicalSize || null, | ||
| filesPerCommit: commitShape.filesPerCommit || null, | ||
| mergeCount: commitShape.mergeCount || 0 | ||
| } | ||
| }; |
There was a problem hiding this comment.
The returned object has some redundant properties. For example:
busFactoris present at the top level and also within thehealthobject.- A rounded
aiRatiois present inhealth.aiRatioand also inaiAttribution.ratio.
This duplication can be confusing for API consumers and might lead to inconsistencies. It would be better to have a single source of truth for each metric. Consider removing the duplicate properties to create a cleaner API.
1 participant
Automated sync of lib/ and CLAUDE.md from agent-core.