chore: sync core lib and CLAUDE.md from agent-core - #366
Closed
avifenesh wants to merge 1 commit into
Closed
Conversation
![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 unifies repository intelligence by folding lib/repo-map into lib/repo-intel, introducing an opt-in embedding submodule (lib/repo-intel/embed/), and adding various query, cache, and updater helpers. It also extracts shared binary download and extraction helpers, and increases the analyzer's maxBuffer to 256 MB to handle large repositories. Review feedback highlights several critical robustness and performance improvements: preventing unhandled EPIPE crashes on stdin when running the analyzer, avoiding temporary directory leaks if PowerShell fails to spawn during zip extraction, addressing a potential promise hang when child processes exit with non-zero codes during embedding streams, and optimizing file reading in topHotspots to load only the first 500 bytes instead of reading entire files into memory.
Comment on lines
+254
to
+278
| async function runAnalyzerWithStdin(args, stdinJson) { | ||
| const binPath = await binary.ensureBinary(); | ||
| return new Promise((resolve, reject) => { | ||
| const proc = cp.spawn(binPath, args, { | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| windowsHide: true | ||
| }); | ||
| let stdout = ''; | ||
| let stderr = ''; | ||
| proc.stdout.on('data', (chunk) => { stdout += chunk.toString('utf8'); }); | ||
| proc.stderr.on('data', (chunk) => { stderr += chunk.toString('utf8'); }); | ||
| proc.on('error', reject); | ||
| proc.on('close', (code) => { | ||
| if (code === 0) { | ||
| resolve({ stdout, stderr }); | ||
| } else { | ||
| reject(new Error( | ||
| `agent-analyzer ${args.join(' ')} exited ${code}: ${stderr.trim() || stdout.trim()}` | ||
| )); | ||
| } | ||
| }); | ||
| proc.stdin.write(stdinJson); | ||
| proc.stdin.end(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
If the child process exits or crashes before or during the write to proc.stdin, an EPIPE error will be emitted on proc.stdin. Since there is no error listener on proc.stdin, this will crash the entire Node.js process with an unhandled exception. Adding a no-op error listener on proc.stdin prevents this crash.
async function runAnalyzerWithStdin(args, stdinJson) {
const binPath = await binary.ensureBinary();
return new Promise((resolve, reject) => {
const proc = cp.spawn(binPath, args, {
stdio: ['pipe', 'pipe', 'pipe'],
windowsHide: true
});
let stdout = '';
let stderr = '';
proc.stdout.on('data', (chunk) => { stdout += chunk.toString('utf8'); });
proc.stderr.on('data', (chunk) => { stderr += chunk.toString('utf8'); });
proc.on('error', reject);
proc.on('close', (code) => {
if (code === 0) {
resolve({ stdout, stderr });
} else {
reject(new Error(
`agent-analyzer ${args.join(' ')} exited ${code}: ${stderr.trim() || stdout.trim()}`
));
}
});
proc.stdin.on('error', () => {}); // Prevent unhandled EPIPE crashes
proc.stdin.write(stdinJson);
proc.stdin.end();
});
}
Comment on lines
+130
to
+153
| function extractZip(buf, destDir, binaryName) { | ||
| return new Promise(function (resolve, reject) { | ||
| var tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), binaryName + '-')); | ||
| var tmpZip = path.join(tmpDir, 'archive.zip'); | ||
| fs.writeFileSync(tmpZip, buf); | ||
| var ps = cp.spawn( | ||
| 'powershell', | ||
| ['-NoProfile', '-NonInteractive', '-Command', | ||
| 'Expand-Archive', '-Path', tmpZip, '-DestinationPath', destDir, '-Force'], | ||
| { stdio: ['ignore', 'pipe', 'pipe'] } | ||
| ); | ||
| var stderr = ''; | ||
| ps.stderr.on('data', function (d) { stderr += d; }); | ||
| ps.on('close', function (code) { | ||
| try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch (e) { /* ignore */ } | ||
| if (code !== 0) { | ||
| reject(new Error('zip extraction failed (code ' + code + '): ' + stderr)); | ||
| } else { | ||
| resolve(); | ||
| } | ||
| }); | ||
| ps.on('error', reject); | ||
| }); | ||
| } |
There was a problem hiding this comment.
If cp.spawn fails to start (e.g., if powershell is not found on the system), the 'error' event is emitted and the 'close' event is never triggered. This causes the temporary directory created by fs.mkdtempSync to be leaked. Adding an error handler that cleans up the directory before rejecting prevents this leak.
function extractZip(buf, destDir, binaryName) {
return new Promise(function (resolve, reject) {
var tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), binaryName + '-'));
var tmpZip = path.join(tmpDir, 'archive.zip');
fs.writeFileSync(tmpZip, buf);
var ps = cp.spawn(
'powershell',
['-NoProfile', '-NonInteractive', '-Command',
'Expand-Archive', '-Path', tmpZip, '-DestinationPath', destDir, '-Force'],
{ stdio: ['ignore', 'pipe', 'pipe'] }
);
var stderr = '';
ps.stderr.on('data', function (d) { stderr += d; });
ps.on('close', function (code) {
try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch (e) { /* ignore */ }
if (code !== 0) {
reject(new Error('zip extraction failed (code ' + code + '): ' + stderr));
} else {
resolve();
}
});
ps.on('error', function (err) {
try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch (e) { /* ignore */ }
reject(err);
});
});
}
Comment on lines
+185
to
+222
| function maybeFinish() { | ||
| if (settled || embedExit === null || setExit === null) return; | ||
| if (embedExit !== 0) { | ||
| return done(new Error( | ||
| embedBinary.EMBED_BINARY_NAME + ' exited ' + embedExit + | ||
| (embedStderr.trim() ? ': ' + embedStderr.trim().slice(0, 500) : '') | ||
| )); | ||
| } | ||
| if (setExit !== 0) { | ||
| return done(new Error( | ||
| 'agent-analyzer set-embeddings exited ' + setExit + | ||
| (setStderr.trim() ? ': ' + setStderr.trim().slice(0, 500) : '') | ||
| )); | ||
| } | ||
| const m = setStdout.match(/(\d+)\s+files?/); | ||
| done(null, { files: m ? parseInt(m[1], 10) : undefined }); | ||
| } | ||
|
|
||
| // stderr piped (not inherited) so failures carry a message. | ||
| embedChild.stderr.on('data', function (d) { embedStderr += d.toString('utf8'); }); | ||
| setChild.stderr.on('data', function (d) { setStderr += d.toString('utf8'); }); | ||
| setChild.stdout.on('data', function (d) { setStdout += d.toString('utf8'); }); | ||
|
|
||
| // Stream wiring with error handling on BOTH ends of the pipe — a bare | ||
| // .pipe() swallows these and hangs. | ||
| embedChild.stdout.on('error', function (e) { done(e); }); | ||
| setChild.stdin.on('error', function (e) { | ||
| // EPIPE when set-embeddings has already exited is benign — its close | ||
| // handler reports the real cause. Only surface other stdin errors. | ||
| if (e && e.code !== 'EPIPE') done(e); | ||
| }); | ||
| embedChild.stdout.pipe(setChild.stdin); | ||
|
|
||
| embedChild.on('error', function (e) { done(e); }); | ||
| setChild.on('error', function (e) { done(e); }); | ||
| embedChild.on('close', function (code) { embedExit = code; maybeFinish(); }); | ||
| setChild.on('close', function (code) { setExit = code; maybeFinish(); }); | ||
| }); |
There was a problem hiding this comment.
If either embedChild or setChild exits with a non-zero code, we should fail immediately and kill the sibling process. Currently, if embedChild exits with a non-zero code, maybeFinish returns immediately because setExit is still null. If setChild does not exit on EOF (or hangs), the promise will hang indefinitely. Handling non-zero exits immediately in the 'close' handlers makes the orchestration much more robust.
function maybeFinish() {
if (settled || embedExit === null || setExit === null) return;
const m = setStdout.match(/(\d+)\s+files?/);
done(null, { files: m ? parseInt(m[1], 10) : undefined });
}
// stderr piped (not inherited) so failures carry a message.
embedChild.stderr.on('data', function (d) { embedStderr += d.toString('utf8'); });
setChild.stderr.on('data', function (d) { setStderr += d.toString('utf8'); });
setChild.stdout.on('data', function (d) { setStdout += d.toString('utf8'); });
// Stream wiring with error handling on BOTH ends of the pipe — a bare
// .pipe() swallows these and hangs.
embedChild.stdout.on('error', function (e) { done(e); });
setChild.stdin.on('error', function (e) {
// EPIPE when set-embeddings has already exited is benign — its close
// handler reports the real cause. Only surface other stdin errors.
if (e && e.code !== 'EPIPE') done(e);
});
embedChild.stdout.pipe(setChild.stdin);
embedChild.on('error', function (e) { done(e); });
setChild.on('error', function (e) { done(e); });
embedChild.on('close', function (code) {
embedExit = code;
if (code !== 0) {
done(new Error(
embedBinary.EMBED_BINARY_NAME + ' exited ' + code +
(embedStderr.trim() ? ': ' + embedStderr.trim().slice(0, 500) : '')
));
} else {
maybeFinish();
}
});
setChild.on('close', function (code) {
setExit = code;
if (code !== 0) {
done(new Error(
'agent-analyzer set-embeddings exited ' + code +
(setStderr.trim() ? ': ' + setStderr.trim().slice(0, 500) : '')
));
} else {
maybeFinish();
}
});
Comment on lines
+75
to
+86
| function topHotspots(repoPath, repoIntelData, n = 10) { | ||
| const paths = topPaths(repoIntelData, n); | ||
| return paths.map((p) => { | ||
| const abs = path.join(repoPath, p); | ||
| let head = ''; | ||
| try { | ||
| const buf = fs.readFileSync(abs); | ||
| head = buf.subarray(0, Math.min(buf.length, 500)).toString('utf8'); | ||
| } catch { /* file missing on disk, skip */ } | ||
| return { path: p, head }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Reading the entire file into memory using fs.readFileSync is highly inefficient for large hotspot files when only the first 500 bytes are needed. Using fs.openSync and fs.readSync to read only the required chunk avoids unnecessary memory allocation and disk I/O.
Suggested change
| function topHotspots(repoPath, repoIntelData, n = 10) { | |
| const paths = topPaths(repoIntelData, n); | |
| return paths.map((p) => { | |
| const abs = path.join(repoPath, p); | |
| let head = ''; | |
| try { | |
| const buf = fs.readFileSync(abs); | |
| head = buf.subarray(0, Math.min(buf.length, 500)).toString('utf8'); | |
| } catch { /* file missing on disk, skip */ } | |
| return { path: p, head }; | |
| }); | |
| } | |
| function topHotspots(repoPath, repoIntelData, n = 10) { | |
| const paths = topPaths(repoIntelData, n); | |
| return paths.map((p) => { | |
| const abs = path.join(repoPath, p); | |
| let head = ''; | |
| try { | |
| const fd = fs.openSync(abs, 'r'); | |
| try { | |
| const buf = Buffer.alloc(500); | |
| const bytesRead = fs.readSync(fd, buf, 0, 500, 0); | |
| head = buf.toString('utf8', 0, bytesRead); | |
| } finally { | |
| fs.closeSync(fd); | |
| } | |
| } catch { /* file missing on disk, skip */ } | |
| return { path: p, head }; | |
| }); | |
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Automated sync from agent-core that consolidates repository “repo-map” functionality into a unified lib/repo-intel surface (lifecycle + queries), adds repo enrichment helpers and an opt-in embedding pipeline, and updates binary download/execution utilities to better support large analyzer outputs.
Changes:
- Deprecated
lib/repo-mapby turning it into a compat shim that re-exportslib/repo-intellifecycle APIs. - Added
lib/repo-intelmodules for lifecycle (init/update/status), query wrappers, staleness checking, caching, conversion, and post-init enrichment. - Added shared binary helper utilities and increased the analyzer
execFile*buffer limit to handle largerepo-intel.jsonoutputs.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/repo-map/index.js | Replaced repo-map implementation with a deprecated compat shim that re-exports lib/repo-intel. |
| lib/repo-intel/index.js | New unified repo-intel public surface (lifecycle, raw loading, stdin piping helpers, enrichment setters, embed lazy export). |
| lib/repo-intel/queries.js | Expanded query wrapper set and added shared argument validation. |
| lib/repo-intel/cache.js | Introduced cache management for converted repo-map view and raw repo-intel artifact + stale marker support. |
| lib/repo-intel/converter.js | Added converter from analyzer repo-intel format to legacy repo-map view format. |
| lib/repo-intel/installer.js | Added agent-analyzer availability checks leveraging the binary auto-download path. |
| lib/repo-intel/updater.js | Added staleness checking logic for cached maps relative to git HEAD. |
| lib/repo-intel/enrich.js | Added post-init enrichment helpers (README/manifests/hotspots input gathering + marker parsing). |
| lib/repo-intel/embed/index.js | Added embed module public surface (preference, binary resolver, orchestrator). |
| lib/repo-intel/embed/preference.js | Added persisted user preference handling for embedder opt-in and detail level. |
| lib/repo-intel/embed/orchestrator.js | Added orchestration to stream embed output into repo-intel set-embeddings without buffering entire JSON. |
| lib/repo-intel/embed/binary.js | Added binary resolver for agent-analyzer-embed, reusing shared download/extract helpers. |
| lib/binary/shared-helpers.js | New shared HTTP + archive helper module for both binary resolvers. |
| lib/binary/index.js | Increased analyzer execution buffer size to support large stdout payloads. |
| lib/enhance/skill-patterns.js | Simplified side-effect/manual-only frontmatter validation logic for skills. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+128
to
+133
| const startMarker = `=== ${name}_START ===`; | ||
| const endMarker = `=== ${name}_END ===`; | ||
| const startIdx = agentOutput.indexOf(startMarker); | ||
| const endIdx = agentOutput.indexOf(endMarker); | ||
| if (startIdx < 0 || endIdx < 0 || endIdx <= startIdx) return null; | ||
| const inner = agentOutput.slice(startIdx + startMarker.length, endIdx).trim(); |
| (typeof disableModelInvocation === 'string' && disableModelInvocation.toLowerCase() === 'true'); | ||
|
|
||
| if (hasSideEffects && !isManualOnly) { | ||
| if (hasSideEffects && frontmatter['disable-model-invocation'] !== true) { |
Comment on lines
170
to
171
|
|
||
| /** | ||
| * AI-authored ratio per file or project. | ||
| * @param {string} cwd | ||
| * @param {{ pathFilter?: string }} [opts] | ||
| * @returns {Object} | ||
| */ | ||
| function aiRatio(cwd, opts = {}) { | ||
| const extra = []; | ||
| if (opts.pathFilter != null) extra.push('--path-filter', opts.pathFilter); | ||
| return runQuery('ai-ratio', extra, cwd); | ||
| } | ||
|
|
Comment on lines
458
to
462
| hotspots, | ||
| coupling, | ||
| busFactor, | ||
| testGaps, | ||
| aiRatio, | ||
| diffRisk, |
Comment on lines
+100
to
+119
| function extractTarGz(buf, destDir) { | ||
| return new Promise(function (resolve, reject) { | ||
| const tarDest = process.platform === 'win32' ? destDir.replace(/\\/g, '/') : destDir; | ||
| const tar = cp.spawn('tar', ['xz', '-C', tarDest], { | ||
| stdio: ['pipe', 'pipe', 'pipe'] | ||
| }); | ||
| let stderr = ''; | ||
| tar.stderr.on('data', function (d) { stderr += d; }); | ||
| tar.stdin.write(buf); | ||
| tar.stdin.end(); | ||
| tar.on('close', function (code) { | ||
| if (code !== 0) { | ||
| reject(new Error('tar extraction failed (code ' + code + '): ' + stderr)); | ||
| } else { | ||
| resolve(); | ||
| } | ||
| }); | ||
| tar.on('error', reject); | ||
| }); | ||
| } |
Comment on lines
+128
to
+133
| const startMarker = `=== ${name}_START ===`; | ||
| const endMarker = `=== ${name}_END ===`; | ||
| const startIdx = agentOutput.indexOf(startMarker); | ||
| const endIdx = agentOutput.indexOf(endMarker); | ||
| if (startIdx < 0 || endIdx < 0 || endIdx <= startIdx) return null; | ||
| const inner = agentOutput.slice(startIdx + startMarker.length, endIdx).trim(); |
| (typeof disableModelInvocation === 'string' && disableModelInvocation.toLowerCase() === 'true'); | ||
|
|
||
| if (hasSideEffects && !isManualOnly) { | ||
| if (hasSideEffects && frontmatter['disable-model-invocation'] !== true) { |
Comment on lines
170
to
171
|
|
||
| /** | ||
| * AI-authored ratio per file or project. | ||
| * @param {string} cwd | ||
| * @param {{ pathFilter?: string }} [opts] | ||
| * @returns {Object} | ||
| */ | ||
| function aiRatio(cwd, opts = {}) { | ||
| const extra = []; | ||
| if (opts.pathFilter != null) extra.push('--path-filter', opts.pathFilter); | ||
| return runQuery('ai-ratio', extra, cwd); | ||
| } | ||
|
|
Comment on lines
458
to
462
| hotspots, | ||
| coupling, | ||
| busFactor, | ||
| testGaps, | ||
| aiRatio, | ||
| diffRisk, |
Comment on lines
+100
to
+119
| function extractTarGz(buf, destDir) { | ||
| return new Promise(function (resolve, reject) { | ||
| const tarDest = process.platform === 'win32' ? destDir.replace(/\\/g, '/') : destDir; | ||
| const tar = cp.spawn('tar', ['xz', '-C', tarDest], { | ||
| stdio: ['pipe', 'pipe', 'pipe'] | ||
| }); | ||
| let stderr = ''; | ||
| tar.stderr.on('data', function (d) { stderr += d; }); | ||
| tar.stdin.write(buf); | ||
| tar.stdin.end(); | ||
| tar.on('close', function (code) { | ||
| if (code !== 0) { | ||
| reject(new Error('tar extraction failed (code ' + code + '): ' + stderr)); | ||
| } else { | ||
| resolve(); | ||
| } | ||
| }); | ||
| tar.on('error', reject); | ||
| }); | ||
| } |
Collaborator
Author
|
Superseded by #371 (latest sync, post ReDoS + prototype-pollution + resolver fixes). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated sync of lib/ and CLAUDE.md from agent-core.