Skip to content

chore: sync core lib and CLAUDE.md from agent-core#33

Closed
avifenesh wants to merge 1 commit into
mainfrom
chore/sync-core-sync-docs-20260529-101843
Closed

chore: sync core lib and CLAUDE.md from agent-core#33
avifenesh wants to merge 1 commit into
mainfrom
chore/sync-core-sync-docs-20260529-101843

Conversation

@avifenesh

Copy link
Copy Markdown
Contributor

Automated sync of lib/ and CLAUDE.md from agent-core.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +62 to +68
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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;

Comment on lines +185 to +221
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(); });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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);
    });

Comment on lines +102 to +128
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
};
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  };
}

Comment thread lib/repo-intel/enrich.js
Comment on lines +56 to +58
if (name.endsWith('.json')) {
try { manifests[name] = JSON.parse(text); }
catch { manifests[name] = text.slice(0, 4000); }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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); }

@avifenesh

Copy link
Copy Markdown
Contributor Author

Superseded by fresh sync run (10:32) after repo-intel added to sync matrix; closing 10:18 duplicate.

@avifenesh avifenesh closed this May 29, 2026
@avifenesh
avifenesh deleted the chore/sync-core-sync-docs-20260529-101843 branch May 29, 2026 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant