chore: sync core lib and CLAUDE.md from agent-core - #28
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors and unifies repository intelligence features under lib/repo-intel, introducing an opt-in embedding capability with a separate binary, shared HTTP/archive helpers, and post-init enrichment utilities, while deprecating the old lib/repo-map module. The review feedback identifies several critical security and robustness issues: a lack of checksum or SLSA verification for the new embed binary download, a command injection vulnerability in PowerShell zip extraction, potential resource leaks and uncaught exceptions during archive extraction, a potential hang in the dual-process streaming pipe if one child fails early, unconstrained JSON manifest parsing that could exceed LLM context limits, and a missing null check on parsed analyzer output.
|
|
||
| let buf; | ||
| try { | ||
| buf = await downloadToBuffer(url); |
There was a problem hiding this comment.
The embed binary resolver downloads and extracts the agent-analyzer-embed binary directly from GitHub releases without performing any SHA-256 checksum verification or SLSA build provenance verification. In contrast, the main binary resolver in lib/binary/index.js implements a robust security verification chain (verifying against a .sha256 sidecar and using gh attestation verify for SLSA provenance). Bypassing these checks for the embed binary introduces a significant security risk, potentially allowing the execution of tampered or malicious binaries if the release or download channel is compromised.
| var ps = cp.spawn( | ||
| 'powershell', | ||
| ['-NoProfile', '-NonInteractive', '-Command', | ||
| 'Expand-Archive', '-Path', tmpZip, '-DestinationPath', destDir, '-Force'], | ||
| { stdio: ['ignore', 'pipe', 'pipe'] } | ||
| ); |
There was a problem hiding this comment.
The extractZip function uses powershell -Command Expand-Archive with unquoted/unescaped variables (tmpZip and destDir) passed directly as arguments. In PowerShell, arguments passed to -Command are concatenated with spaces and re-evaluated as a script. If the user's temporary directory or destination directory contains spaces (e.g., C:\Users\John Doe\AppData\...), brackets, or other special characters, this will fail with syntax errors or lead to arbitrary command execution/injection. Passing the paths safely via environment variables (similar to how lib/binary/index.js handles it) ensures they are never parsed as part of the command string.
const ps = cp.spawn(
'powershell',
['-NoProfile', '-NonInteractive', '-Command',
'Expand-Archive -Path $env:SRC_ZIP -DestinationPath $env:DEST_DIR -Force'],
{
stdio: ['ignore', 'pipe', 'pipe'],
env: Object.assign({}, process.env, {
SRC_ZIP: tmpZip,
DEST_DIR: destDir
})
}
);| resolve(); | ||
| } | ||
| }); | ||
| ps.on('error', reject); |
There was a problem hiding this comment.
If ps.on('error') is triggered (e.g., if powershell fails to spawn or is missing on the system), the promise is rejected, but the temporary directory tmpDir is leaked because the cleanup logic is only registered in the close event handler. Ensure tmpDir is cleaned up in the error handler as well.
ps.on('error', function (err) {
try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch (e) { /* ignore */ }
reject(err);
});| tar.stdin.write(buf); | ||
| tar.stdin.end(); |
There was a problem hiding this comment.
In extractTarGz, tar.stdin.write(buf) is called directly. If the tar process fails to start (e.g., tar is missing on the system) or exits prematurely, writing to its stdin will emit an error event on tar.stdin. Since there is no error listener registered on tar.stdin, this will result in an uncaught exception (typically EPIPE or ERR_STREAM_WRITE_AFTER_END) that crashes the entire Node.js application. Registering a no-op error listener on tar.stdin before writing to it safely catches and ignores write errors (which are already handled by checking the process exit code).
tar.stdin.on('error', function () {});
tar.stdin.write(buf);
tar.stdin.end();| if (name.endsWith('.json')) { | ||
| try { manifests[name] = JSON.parse(text); } | ||
| catch { manifests[name] = text.slice(0, 4000); } |
There was a problem hiding this comment.
In readManifests, if a JSON manifest (like package.json) is successfully parsed, the entire parsed object is stored in manifests[name]. In large projects or monorepos, package.json can be extremely large (hundreds of kilobytes). When this is stringified and embedded into the summarizer prompt in buildSummarizerPrompt, it can easily exceed LLM context limits or consume an excessive number of tokens. Slicing is only applied to non-JSON manifests or when JSON parsing fails. Consider limiting the size of the parsed JSON manifest or pruning it to only include essential fields (such as name, version, description, dependencies, devDependencies, and scripts) to keep the prompt size bounded.
| 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) : '') | ||
| )); | ||
| } |
There was a problem hiding this comment.
In streamEmbedToSetEmbeddings, the maybeFinish function waits for both embedExit and setExit to be non-null before taking action. If embedChild exits with a non-zero code (fails) while setChild is still running, the promise will not reject immediately; instead, it waits for setChild to exit. If setChild hangs or takes a long time to process the partial stream, the entire execution will hang. Implementing a fail-fast mechanism that immediately rejects the promise and terminates the sibling process when either process exits with a non-zero code improves robustness.
| 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) : '') | |
| )); | |
| } | |
| 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; |
| function convertIntelToRepoMap(intel) { | ||
| const files = {}; |
There was a problem hiding this comment.
convertIntelToRepoMap assumes intel is a valid object. However, if JSON.parse in the caller receives 'null' or an empty/invalid output, intel could be null. Accessing intel.symbols will then throw a TypeError and crash the execution. Adding a defensive check at the beginning of convertIntelToRepoMap to handle a null or undefined intel object gracefully is recommended.
function convertIntelToRepoMap(intel) {
if (!intel) {
return {
version: '2.0',
generated: new Date().toISOString(),
project: { languages: [] },
stats: { totalFiles: 0, totalSymbols: 0, totalImports: 0, errors: [] },
files: {}
};
}
const files = {};|
Superseded by latest sync (post ReDoS + prototype-pollution hardening). |
Automated sync of lib/ and CLAUDE.md from agent-core.