chore: sync core lib and CLAUDE.md from agent-core - #29
Conversation
There was a problem hiding this comment.
Code Review
This pull request unifies repository intelligence by folding the deprecated repo-map module into a new repo-intel pipeline, adding support for local embeddings via agent-analyzer-embed, and resolving several ReDoS vulnerabilities in regex patterns. Feedback from the review highlights critical security and stability issues, including the lack of checksum and SLSA verification for the new embedder binary, potential Node process crashes from unhandled stream errors on process stdin, a potential promise hang in the orchestrator's streaming pipe, memory overhead from reading entire files for hotspot headers, and a temporary directory leak on extraction failures.
| async function downloadBinary(ver) { | ||
| const platformKey = getPlatformKey(); | ||
| if (!platformKey) { | ||
| throw new Error( | ||
| 'Unsupported platform: ' + process.platform + '-' + process.arch + '. ' + | ||
| 'Supported: ' + Object.keys(PLATFORM_MAP).join(', ') | ||
| ); | ||
| } | ||
| const url = buildDownloadUrl(ver, platformKey); | ||
| process.stderr.write('Downloading ' + EMBED_BINARY_NAME + ' v' + ver + ' for ' + platformKey + '...\n'); | ||
|
|
||
| const binPath = getBinaryPath(); | ||
| const binDir = path.dirname(binPath); | ||
| fs.mkdirSync(binDir, { recursive: true }); | ||
|
|
||
| let buf; | ||
| try { | ||
| buf = await downloadToBuffer(url); | ||
| } catch (err) { | ||
| throw new Error( | ||
| 'Failed to download ' + EMBED_BINARY_NAME + ':\n' + | ||
| ' URL: ' + url + '\n' + | ||
| ' Error: ' + err.message + '\n\n' + | ||
| 'To install manually:\n' + | ||
| ' 1. Download: ' + url + '\n' + | ||
| ' 2. Extract the binary to: ' + binDir + '\n' + | ||
| ' 3. Ensure it is named: ' + path.basename(binPath) | ||
| ); | ||
| } | ||
|
|
||
| if (process.platform === 'win32') { | ||
| await extractZip(buf, binDir, path.basename(binPath)); | ||
| } else { | ||
| await extractTarGz(buf, binDir); | ||
| } | ||
| if (process.platform !== 'win32') { | ||
| fs.chmodSync(binPath, 0o755); | ||
| } | ||
| return binPath; | ||
| } |
There was a problem hiding this comment.
The downloadBinary function for the embedder binary completely lacks SHA-256 checksum verification and SLSA attestation verification, unlike the main binary resolver in lib/binary/index.js. Downloading and executing binary files from the internet without verifying their checksums or build provenance poses a critical security risk (e.g., man-in-the-middle attacks or compromised release assets). Please mirror the security verification chain (SHA-256 sidecar verification and SLSA attestation verification via gh) established in lib/binary/index.js.
| 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); | ||
| }); |
There was a problem hiding this comment.
The extractTarGz function writes directly to tar.stdin without handling potential stream errors. If the tar process fails to spawn or exits prematurely, writing to tar.stdin will emit an EPIPE error, which will crash the entire Node process if unhandled. Additionally, extracting untrusted archives directly into the destination directory without validating the entry paths (e.g., checking for parent directory traversal .. or absolute paths) poses a severe path traversal risk. Consider adding an error handler to tar.stdin and validating archive entries before extraction, similar to the validation implemented in lib/binary/index.js.
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.on('error', function () {
// Ignore EPIPE or other write errors here as they are handled by process exit/error
});
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);
});
}| 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 }); | ||
| } |
There was a problem hiding this comment.
In maybeFinish, if either embedChild or setChild exits with a non-zero code, the function returns early without settling the promise if the other process is still running (i.e., its exit code is still null). This delays error reporting and can cause the promise to hang if the other process does not exit. You should immediately reject the promise and terminate the sibling process if either process fails.
| 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 }); | |
| } | |
| function maybeFinish() { | |
| if (settled) return; | |
| if (embedExit !== null && embedExit !== 0) { | |
| return done(new Error( | |
| embedBinary.EMBED_BINARY_NAME + ' exited ' + embedExit + | |
| (embedStderr.trim() ? ': ' + embedStderr.trim().slice(0, 500) : '') | |
| )); | |
| } | |
| if (setExit !== null && setExit !== 0) { | |
| return done(new Error( | |
| 'agent-analyzer set-embeddings exited ' + setExit + | |
| (setStderr.trim() ? ': ' + setStderr.trim().slice(0, 500) : '') | |
| )); | |
| } | |
| if (embedExit === null || setExit === null) return; | |
| const m = setStdout.match(/(\d+)\s+files?/); | |
| done(null, { files: m ? parseInt(m[1], 10) : undefined }); | |
| } |
| 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.
Using fs.readFileSync(abs) reads the entire file into memory just to extract the first 500 bytes for the hotspot head. If any of the hot files are extremely large (e.g., large logs, datasets, or bundles), this can cause severe performance degradation or out-of-memory crashes. Use fs.openSync and fs.readSync to read only the first 500 bytes efficiently.
| 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 or unreadable, skip */ } | |
| return { path: p, head }; | |
| }); | |
| } |
| 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.
The runAnalyzerWithStdin function writes to proc.stdin without handling potential stream errors. If the spawned process exits immediately or fails to start, writing to proc.stdin will emit an EPIPE error, which will crash the entire Node process if unhandled. Add a no-op or simple error handler to proc.stdin to prevent unhandled exceptions.
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', () => {});
proc.stdin.write(stdinJson);
proc.stdin.end();
});
}| 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 powershell fails to spawn or exits with an error, the ps.on('error') handler is triggered and rejects the promise, but the temporary directory tmpDir created by fs.mkdtempSync is never cleaned up. This leads to a resource leak of temporary directories on disk. Ensure that tmpDir is cleaned up in the error handler as well.
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);
});
});
}|
Superseded by latest sync (post ReDoS + prototype-pollution hardening). |
Automated sync of lib/ and CLAUDE.md from agent-core.