chore: sync core lib and CLAUDE.md from agent-core - #368
Conversation
| const usesEnvVar = /AI_STATE_DIR|\$\{.*STATE.*\}/i.test(content); | ||
| // ReDoS fix: bound the .* runs to non-brace chars so they cannot cross } | ||
| // and cannot backtrack; matches the same ${...STATE...} expressions. | ||
| const usesEnvVar = /AI_STATE_DIR|\$\{[^}]*STATE[^}]*\}/i.test(content); |
There was a problem hiding this comment.
Code Review
This pull request consolidates the repository intelligence pipeline by folding lib/repo-map into lib/repo-intel and introducing an opt-in embedding orchestrator (lib/repo-intel/embed) and post-init enrichment helpers (lib/repo-intel/enrich.js). It also resolves several potential Regular Expression Denial of Service (ReDoS) vulnerabilities across multiple files by bounding previously unbounded runs. Review feedback focuses on improving robustness, suggesting relative path support and missing header checks in redirect handling, adding error listeners to prevent unhandled EPIPE crashes during tar extraction, cleaning up temporary directories on PowerShell errors to avoid resource leaks, and adding defensive null/undefined guards to prevent potential TypeErrors.
| if (sc === 301 || sc === 302 || sc === 307 || sc === 308) { | ||
| res.resume(); | ||
| 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.
The redirect handling logic does not support relative redirect paths (e.g., /foo/bar), which are standard and secure when the original request is HTTPS. It also lacks a check for a missing Location header, which could lead to calling request(undefined, ...) and throwing an error. Resolving the redirect URL against the current request URL using new URL(loc, reqUrl).href resolves both issues safely.
| if (sc === 301 || sc === 302 || sc === 307 || sc === 308) { | |
| res.resume(); | |
| 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; | |
| } | |
| if (sc === 301 || sc === 302 || sc === 307 || sc === 308) { | |
| res.resume(); | |
| const loc = res.headers.location; | |
| if (!loc) { | |
| reject(new Error('Redirect status ' + sc + ' missing Location header')); | |
| return; | |
| } | |
| try { | |
| const resolvedUrl = new URL(loc, reqUrl).href; | |
| if (!resolvedUrl.startsWith('https://')) { | |
| reject(new Error('Refusing non-HTTPS redirect to ' + resolvedUrl)); | |
| return; | |
| } | |
| request(resolvedUrl, redirectCount + 1); | |
| } catch (e) { | |
| reject(new Error('Invalid redirect URL ' + loc + ': ' + e.message)); | |
| } | |
| return; | |
| } |
| let stderr = ''; | ||
| tar.stderr.on('data', function (d) { stderr += d; }); | ||
| tar.stdin.write(buf); | ||
| tar.stdin.end(); |
There was a problem hiding this comment.
If the tar process fails to spawn or exits early, writing to tar.stdin will trigger an EPIPE error. Since there is no 'error' listener on tar.stdin, this will propagate as an unhandled exception and crash the entire Node process. Adding an error listener on tar.stdin prevents this crash.
let stderr = '';
tar.stderr.on('data', function (d) { stderr += d; });
tar.stdin.on('error', function (err) {
// EPIPE is common if tar exits early; the close event will handle the failure
if (err.code !== 'EPIPE') {
reject(err);
}
});
tar.stdin.write(buf);
tar.stdin.end();| 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 ps.on('error') is triggered (e.g., if powershell is not found on the system), the close event is never fired, and the temporary directory tmpDir is leaked in os.tmpdir(). Cleaning up the directory in both the close and error handlers prevents this resource leak.
function cleanup() {
try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch (e) { /* ignore */ }
}
ps.on('close', function (code) {
cleanup();
if (code !== 0) {
reject(new Error('zip extraction failed (code ' + code + '): ' + stderr));
} else {
resolve();
}
});
ps.on('error', function (err) {
cleanup();
reject(err);
});| function convertFile(filePath, fileSym) { | ||
| const exportNames = new Set((fileSym.exports || []).map(e => e.name)); | ||
|
|
||
| const exports = (fileSym.exports || []).map(e => ({ | ||
| name: e.name, | ||
| kind: e.kind, | ||
| line: e.line | ||
| })); | ||
|
|
||
| const functions = []; | ||
| const classes = []; | ||
| const types = []; | ||
| const constants = []; | ||
|
|
||
| for (const def of fileSym.definitions || []) { | ||
| const entry = { | ||
| name: def.name, | ||
| kind: def.kind, | ||
| line: def.line, | ||
| exported: exportNames.has(def.name) | ||
| }; | ||
| if (def.kind === 'function' || FUNCTION_LIKE_KINDS.has(def.kind)) { | ||
| functions.push(entry); | ||
| } else if (CLASS_KINDS.has(def.kind)) { | ||
| classes.push(entry); | ||
| } else if (TYPE_KINDS.has(def.kind)) { | ||
| types.push(entry); | ||
| } else if (CONSTANT_KINDS.has(def.kind)) { | ||
| constants.push(entry); | ||
| } else { | ||
| // Unknown kind - default to constants for backward compat | ||
| constants.push(entry); | ||
| } | ||
| } | ||
|
|
||
| // agent-analyzer imports: [{ from, names }] → repo-map imports: [{ source, kind, names }] | ||
| const imports = (fileSym.imports || []).map(imp => ({ | ||
| source: imp.from, | ||
| kind: 'import', | ||
| names: imp.names || [] | ||
| })); | ||
|
|
||
| return { | ||
| language: detectLanguage(filePath), | ||
| symbols: { exports, functions, classes, types, constants }, | ||
| imports | ||
| }; | ||
| } |
There was a problem hiding this comment.
If fileSym is null or undefined, accessing properties like fileSym.exports or fileSym.definitions will throw a TypeError. Adding a defensive fallback const safeFileSym = fileSym || {} at the top of the function ensures robustness against malformed or empty analyzer outputs.
function convertFile(filePath, fileSym) {
const safeFileSym = fileSym || {};
const exportNames = new Set((safeFileSym.exports || []).map(e => e.name));
const exports = (safeFileSym.exports || []).map(e => ({
name: e.name,
kind: e.kind,
line: e.line
}));
const functions = [];
const classes = [];
const types = [];
const constants = [];
for (const def of safeFileSym.definitions || []) {
const entry = {
name: def.name,
kind: def.kind,
line: def.line,
exported: exportNames.has(def.name)
};
if (def.kind === 'function' || FUNCTION_LIKE_KINDS.has(def.kind)) {
functions.push(entry);
} else if (CLASS_KINDS.has(def.kind)) {
classes.push(entry);
} else if (TYPE_KINDS.has(def.kind)) {
types.push(entry);
} else if (CONSTANT_KINDS.has(def.kind)) {
constants.push(entry);
} else {
// Unknown kind - default to constants for backward compat
constants.push(entry);
}
}
// agent-analyzer imports: [{ from, names }] → repo-map imports: [{ source, kind, names }]
const imports = (safeFileSym.imports || []).map(imp => ({
source: imp.from,
kind: 'import',
names: imp.names || []
}));
return {
language: detectLanguage(filePath),
symbols: { exports, functions, classes, types, constants },
imports
};
}| 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); |
There was a problem hiding this comment.
If intel is null or undefined (e.g., if JSON.parse returned null on empty input), accessing intel.symbols will throw a TypeError. Guarding against this with a defensive fallback const safeIntel = intel || {} prevents potential crashes.
| 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); | |
| function convertIntelToRepoMap(intel) { | |
| const safeIntel = intel || {}; | |
| const files = {}; | |
| let totalSymbols = 0; | |
| let totalImports = 0; | |
| for (const [filePath, fileSym] of Object.entries(safeIntel.symbols || {})) { | |
| files[filePath] = convertFile(filePath, fileSym || {}); |
| function topPaths(repoIntelData, n) { | ||
| const fa = repoIntelData.fileActivity || {}; |
There was a problem hiding this comment.
If repoIntelData is null or undefined, accessing repoIntelData.fileActivity will throw a TypeError. Guarding against this with a defensive fallback const safeData = repoIntelData || {} ensures robustness.
function topPaths(repoIntelData, n) {
const safeData = repoIntelData || {};
const fa = safeData.fileActivity || {};|
Superseded by #371 (latest sync, post ReDoS + prototype-pollution + resolver fixes). |
Automated sync of lib/ and CLAUDE.md from agent-core.