chore: sync core lib and CLAUDE.md from agent-core#33
Conversation
There was a problem hiding this comment.
Code Review
This pull request unifies repository intelligence by introducing the lib/repo-intel module, which manages repository analysis, queries, and embedding generation using the agent-analyzer and agent-analyzer-embed binaries. Key changes include increasing execution buffer limits, adding shared binary download and extraction helpers, implementing cache and update management, and deprecating the old repo-map module. Feedback from the review highlights several critical issues: a potential process crash on redirects missing a Location header, a potential hang in the dual-process pipeline upon signal termination, a security risk from missing checksum and SLSA verification for the embed binary, a resource leak if PowerShell fails to spawn, potential TypeErrors on null inputs in the converter, and token bloat from unpruned package.json manifests.
| var loc = res.headers.location; | ||
| if (loc && !loc.startsWith('https://')) { | ||
| reject(new Error('Refusing non-HTTPS redirect to ' + loc)); | ||
| return; | ||
| } | ||
| request(loc, redirectCount + 1); | ||
| return; |
There was a problem hiding this comment.
If the redirect response does not contain a Location header, loc will be undefined. Passing undefined to request(loc, ...) will cause https.get(undefined, ...) to throw a TypeError synchronously. Since this synchronous throw occurs inside an asynchronous callback, it will not be caught by the Promise executor and will result in an unhandled exception that crashes the Node.js process. Always validate that loc is a non-empty string before proceeding.
| var loc = res.headers.location; | |
| if (loc && !loc.startsWith('https://')) { | |
| reject(new Error('Refusing non-HTTPS redirect to ' + loc)); | |
| return; | |
| } | |
| request(loc, redirectCount + 1); | |
| return; | |
| var loc = res.headers.location; | |
| if (!loc) { | |
| reject(new Error('Redirect response missing Location header')); | |
| return; | |
| } | |
| if (!loc.startsWith('https://')) { | |
| reject(new Error('Refusing non-HTTPS redirect to ' + loc)); | |
| return; | |
| } | |
| request(loc, redirectCount + 1); | |
| return; |
| 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 child process is terminated by a signal (e.g., SIGPIPE or SIGTERM), the code argument passed to the close event will be null. Because embedExit or setExit will remain null, the condition embedExit === null || setExit === null in maybeFinish will always evaluate to true, causing the function to return early and the Promise to hang indefinitely. Update the state tracking to handle signal termination correctly.
function maybeFinish() {
if (settled || embedExit === null || setExit === null) return;
if (embedExit.code !== 0 || embedExit.signal) {
return done(new Error(
embedBinary.EMBED_BINARY_NAME + ' failed' +
(embedExit.code !== null ? ' with code ' + embedExit.code : '') +
(embedExit.signal ? ' due to signal ' + embedExit.signal : '') +
(embedStderr.trim() ? ': ' + embedStderr.trim().slice(0, 500) : '')
));
}
if (setExit.code !== 0 || setExit.signal) {
return done(new Error(
'agent-analyzer set-embeddings failed' +
(setExit.code !== null ? ' with code ' + setExit.code : '') +
(setExit.signal ? ' due to signal ' + setExit.signal : '') +
(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, signal) { embedExit = { code, signal }; maybeFinish(); });
setChild.on('close', function (code, signal) { setExit = { code, signal }; maybeFinish(); });| const extractTarGz = sharedHelpers.extractTarGz; | ||
| const extractZip = sharedHelpers.extractZip; | ||
|
|
||
| async function downloadBinary(ver) { |
There was a problem hiding this comment.
Unlike the main binary resolver in lib/binary/index.js, this resolver completely lacks any SHA-256 checksum verification or SLSA build provenance attestation verification. Downloading and executing unverified binaries from GitHub releases poses a significant security risk (e.g., in the event of a compromised release or MITM attack). Please implement the same verification chain here.
| resolve(); | ||
| } | ||
| }); | ||
| ps.on('error', reject); |
There was a problem hiding this comment.
If the PowerShell process fails to spawn (e.g., if powershell is not found on the system PATH), the error event is emitted and the promise is rejected, but the temporary directory tmpDir is never cleaned up. This results in a resource leak of both the directory and the temporary zip file. Ensure cleanup is performed in the error handler.
ps.on('error', function (err) {
try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch (e) { /* ignore */ }
reject(err);
});| function convertIntelToRepoMap(intel) { | ||
| const files = {}; | ||
| let totalSymbols = 0; | ||
| let totalImports = 0; | ||
|
|
||
| for (const [filePath, fileSym] of Object.entries(intel.symbols || {})) { | ||
| files[filePath] = convertFile(filePath, fileSym); | ||
| const s = files[filePath].symbols; | ||
| totalSymbols += s.functions.length + s.classes.length + | ||
| s.types.length + s.constants.length; | ||
| totalImports += files[filePath].imports.length; | ||
| } | ||
|
|
||
| return { | ||
| version: '2.0', | ||
| generated: intel.generated || new Date().toISOString(), | ||
| git: intel.git ? { commit: intel.git.analyzedUpTo } : undefined, | ||
| project: { languages: detectLanguagesFromFiles(Object.keys(files)) }, | ||
| stats: { | ||
| totalFiles: Object.keys(files).length, | ||
| totalSymbols, | ||
| totalImports, | ||
| errors: [] | ||
| }, | ||
| files | ||
| }; | ||
| } |
There was a problem hiding this comment.
If intel is null or undefined (for example, if the analyzer output is empty or invalid), calling intel.symbols or intel.generated will throw a TypeError and crash the execution. Add defensive checks to safely handle null/undefined inputs.
function convertIntelToRepoMap(intel) {
const files = {};
let totalSymbols = 0;
let totalImports = 0;
const symbols = (intel && intel.symbols) || {};
for (const [filePath, fileSym] of Object.entries(symbols)) {
files[filePath] = convertFile(filePath, fileSym);
const s = files[filePath].symbols;
totalSymbols += s.functions.length + s.classes.length +
s.types.length + s.constants.length;
totalImports += files[filePath].imports.length;
}
return {
version: '2.0',
generated: (intel && intel.generated) || new Date().toISOString(),
git: (intel && intel.git) ? { commit: intel.git.analyzedUpTo } : undefined,
project: { languages: detectLanguagesFromFiles(Object.keys(files)) },
stats: {
totalFiles: Object.keys(files).length,
totalSymbols,
totalImports,
errors: []
},
files
};
}| if (name.endsWith('.json')) { | ||
| try { manifests[name] = JSON.parse(text); } | ||
| catch { manifests[name] = text.slice(0, 4000); } |
There was a problem hiding this comment.
If package.json is extremely large (e.g., containing thousands of lines of dependencies or scripts), parsing and storing the entire object in manifests can cause severe token bloat when serialized into the LLM prompt in buildSummarizerPrompt. Prune the parsed JSON to only keep essential metadata fields (like name, version, description, dependencies, devDependencies, and scripts).
if (name.endsWith('.json')) {
try {
const parsed = JSON.parse(text);
if (name === 'package.json') {
manifests[name] = {
name: parsed.name,
version: parsed.version,
description: parsed.description,
dependencies: parsed.dependencies,
devDependencies: parsed.devDependencies,
scripts: parsed.scripts
};
} else {
manifests[name] = parsed;
}
}
catch { manifests[name] = text.slice(0, 4000); }|
Superseded by fresh sync run (10:32) after repo-intel added to sync matrix; closing 10:18 duplicate. |
Automated sync of lib/ and CLAUDE.md from agent-core.