fix(ideation): corrigir import de GotchasMemory como named export#519
fix(ideation): corrigir import de GotchasMemory como named export#519nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Conversation
gotchas-memory.js exporta { GotchasMemory } (named export) mas
ideation-engine.js usava require() sem destructuring, recebendo o
objeto do módulo em vez da classe. Isso causa TypeError ao tentar
instanciar com new GotchasMemory().
Corrige aplicando destructuring no require:
({ GotchasMemory } = require('../memory/gotchas-memory'))
Closes SynkraAI#517
nikolasdehor
requested review from
Pedrovaleriolopez and
oalanicolas
as code owners
|
@nikolasdehor is attempting to deploy a commit to the Pedro Valério Lopez's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe import statement for Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Este PR corrige um bug no IdeationEngine onde o módulo gotchas-memory.js era importado como se fosse default export, apesar de ele expor GotchasMemory como named export. Isso causava falha ao tentar instanciar GotchasMemory e degradava silenciosamente a funcionalidade de filtragem de gotchas conhecidos (Issue #517).
Changes:
- Ajusta o
requirede../memory/gotchas-memorypara usar destructuring e obter a classeGotchasMemorycorretamente.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.aios-core/core/ideation/ideation-engine.js (4)
504-533:⚠️ Potential issue | 🟠 Major
JSON.parseonnpm auditoutput may throw on non-JSON content.
npm auditcan prefix its JSON with deprecation warnings or other plain-text lines in some npm versions, makingauditResultnot strictly valid JSON. While the outercatchswallows any resultingSyntaxError, it also silently discards legitimate vulnerability data, defeating the purpose of this check.🛡️ Proposed defensive parse
- const audit = JSON.parse(auditResult); + // npm can prepend non-JSON warnings; extract only the JSON portion + const jsonStart = auditResult.indexOf('{'); + if (jsonStart === -1) return findings; + const audit = JSON.parse(auditResult.slice(jsonStart));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/ideation/ideation-engine.js around lines 504 - 533, The JSON.parse can fail on npm audit output with leading non-JSON lines; update checkDependencyVulnerabilities to defensively extract the JSON payload from auditResult (returned by execSync) before parsing — e.g., trim output, locate the first '{' (or first '[') and parse the substring from that index, handle empty or non-JSON results by returning no findings, and keep the outer try/catch but avoid silently swallowing valid vulnerability data; refer to checkDependencyVulnerabilities, execSync, auditResult and audit when implementing this fix.
63-69:⚠️ Potential issue | 🔴 Critical
summary.quickWinswill always be0— mutation after spread does not propagate.JavaScript evaluates object-literal properties left-to-right. In the expression:
{ ...f, area, priority: this.calculatePriority(f) }
...fis spread (snapshottingf's properties) beforecalculatePriority(f)is invoked.calculatePrioritythen mutatesf.category = 'quick-win'on Line 126, but that mutation has no effect on the already-created snapshot. Consequently, no suggestion object in thesuggestionsarray ever carriescategory: 'quick-win', andsummary.quickWins(Line 97, filtered bys.category === 'quick-win') is always0.The fix is to either include
categoryexplicitly after callingcalculatePriority, or move the category assignment outside the mutating function.🐛 Proposed fix
- const findings = await analyzer.analyze(); - suggestions.push( - ...findings.map((f) => ({ - ...f, - area, - priority: this.calculatePriority(f), - })), - ); + const findings = await analyzer.analyze(); + suggestions.push( + ...findings.map((f) => { + const priority = this.calculatePriority(f); + return { + ...f, // spread after calculatePriority so mutated category is included + area, + priority, + }; + }), + );And in
calculatePriority, returncategoryexplicitly rather than relying on side-effect mutation:calculatePriority(finding) { const impact = finding.impact || 0.5; const effortMultiplier = { low: 1.5, medium: 1.0, high: 0.6 }[finding.effort] || 1.0; if (finding.effort === 'low' && impact >= 0.7) { - finding.category = 'quick-win'; - return impact * 1.5; + finding.category = 'quick-win'; // keep for callers that rely on the side-effect being spread after this call + return impact * 1.5; } return impact * effortMultiplier; }As per coding guidelines,
.aios-core/core/**is a CRITICAL PATH and requires thorough review of logic correctness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/ideation/ideation-engine.js around lines 63 - 69, The suggestions mapping spreads f before calculatePriority mutates f, so category set inside calculatePriority never makes it into the created suggestion objects (causing summary.quickWins to be 0); fix by having calculatePriority return the computed category (or compute category separately) and include that returned category explicitly when building each suggestion in the mapping (e.g., call this.calculatePriority(f) -> { priority, category } and then push { ...f, area, priority, category }), or alternatively set category on the suggestion after calling calculatePriority rather than relying on mutation; update usages of calculatePriority accordingly.
283-313:⚠️ Potential issue | 🟠 MajorShell injection via unsanitised
this.rootPathacross all analyzerexecSynccalls.
this.rootPathis interpolated directly into shell command strings (e.g., Line 289, and equivalently on Lines 322, 353, 386, 559, 595, 623, 672, 705, 767, 800). A path containing shell metacharacters (;,$, backticks, spaces) would allow arbitrary command execution. BecauserootPathdefaults toprocess.cwd()but can also be supplied viaconfig.rootPath(Line 24), any caller controlling that value can trigger injection.Sanitise the path before interpolation, or use
spawnSyncwith argument arrays to avoid shell interpretation entirely.🛡️ Suggested approach — use spawnSync with argument arrays
-const { execSync } = require('child_process'); +const { spawnSync } = require('child_process');Replace each
execSyncgrep call with aspawnSyncequivalent. Example forcheckSyncOperations:- const result = execSync( - `grep -rn "readFileSync\\|writeFileSync\\|existsSync" --include="*.js" --include="*.ts" ${this.rootPath}/src ${this.rootPath}/.aios-core 2>/dev/null || true`, - { encoding: 'utf8', maxBuffer: 5 * 1024 * 1024 }, - ); + const proc = spawnSync( + 'grep', + ['-rn', 'readFileSync|writeFileSync|existsSync', '--include=*.js', '--include=*.ts', + `${this.rootPath}/src`, `${this.rootPath}/.aios-core`], + { encoding: 'utf8', maxBuffer: 5 * 1024 * 1024 }, + ); + const result = proc.stdout || '';As per coding guidelines,
.aios-core/core/**requires input validation on public API methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/ideation/ideation-engine.js around lines 283 - 313, The execSync invocations (e.g., in checkSyncOperations) interpolate this.rootPath into shell strings, enabling shell injection; replace those execSync grep calls with child_process.spawnSync using argument arrays (pass grep pattern and --include and paths as separate args) or else strictly sanitize/validate this.rootPath (e.g., resolve with path.resolve and reject or escape any unsafe metacharacters) before use; update the methods that call execSync (checkSyncOperations and the other analyzer methods that use execSync) to preserve encoding/maxBuffer behavior, capture stdout/stderr from spawnSync, and handle errors explicitly instead of an empty catch so the analyzer remains safe and robust.
470-501:⚠️ Potential issue | 🟡 MinorMissing
maxBufferoncheckInsecurePatternsexecSynccall.Line 475's
execSyncforevalusage omits amaxBufferoption. On large codebases the default 1 MB buffer can be exceeded, causing a silent exception that is swallowed by thecatchblock, meaning realeval()usages would go unreported.🛡️ Proposed fix
const evalResult = execSync( `grep -rn "\\beval\\s*(" --include="*.js" --include="*.ts" ${this.rootPath} 2>/dev/null || true`, { encoding: 'utf8', + maxBuffer: 5 * 1024 * 1024, }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/ideation/ideation-engine.js around lines 470 - 501, The execSync call inside checkInsecurePatterns (the one that assigns evalResult) is missing a maxBuffer option which can cause silent failures on large repos; update that execSync invocation to include a sufficiently large maxBuffer (e.g. 10 * 1024 * 1024) in its options object (alongside encoding), and apply the same maxBuffer addition to any other execSync calls in checkInsecurePatterns to avoid swallowed exceptions and ensure findings are reported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.aios-core/core/ideation/ideation-engine.js:
- Around line 504-533: The JSON.parse can fail on npm audit output with leading
non-JSON lines; update checkDependencyVulnerabilities to defensively extract the
JSON payload from auditResult (returned by execSync) before parsing — e.g., trim
output, locate the first '{' (or first '[') and parse the substring from that
index, handle empty or non-JSON results by returning no findings, and keep the
outer try/catch but avoid silently swallowing valid vulnerability data; refer to
checkDependencyVulnerabilities, execSync, auditResult and audit when
implementing this fix.
- Around line 63-69: The suggestions mapping spreads f before calculatePriority
mutates f, so category set inside calculatePriority never makes it into the
created suggestion objects (causing summary.quickWins to be 0); fix by having
calculatePriority return the computed category (or compute category separately)
and include that returned category explicitly when building each suggestion in
the mapping (e.g., call this.calculatePriority(f) -> { priority, category } and
then push { ...f, area, priority, category }), or alternatively set category on
the suggestion after calling calculatePriority rather than relying on mutation;
update usages of calculatePriority accordingly.
- Around line 283-313: The execSync invocations (e.g., in checkSyncOperations)
interpolate this.rootPath into shell strings, enabling shell injection; replace
those execSync grep calls with child_process.spawnSync using argument arrays
(pass grep pattern and --include and paths as separate args) or else strictly
sanitize/validate this.rootPath (e.g., resolve with path.resolve and reject or
escape any unsafe metacharacters) before use; update the methods that call
execSync (checkSyncOperations and the other analyzer methods that use execSync)
to preserve encoding/maxBuffer behavior, capture stdout/stderr from spawnSync,
and handle errors explicitly instead of an empty catch so the analyzer remains
safe and robust.
- Around line 470-501: The execSync call inside checkInsecurePatterns (the one
that assigns evalResult) is missing a maxBuffer option which can cause silent
failures on large repos; update that execSync invocation to include a
sufficiently large maxBuffer (e.g. 10 * 1024 * 1024) in its options object
(alongside encoding), and apply the same maxBuffer addition to any other
execSync calls in checkInsecurePatterns to avoid swallowed exceptions and ensure
findings are reported.
1. Mock gotchas-memory com shape de constructor em vez de null — funciona com import atual (sem destructuring) e com fix SynkraAI#519 (com destructuring). Captura mismatch de import/export. 2. Reconfigurar spies de fs no beforeEach (existsSync, mkdirSync, writeFileSync) para evitar leak de estado entre testes após clearAllMocks limpar as implementações. 3. Suprimir console.warn no teste de analyzer rejection para evitar ruído no output do CI.
|
Fechando como duplicata — substituído pelo PR #521 que corrige o bug em todos os 3 módulos (ideation-engine, context-injector, subagent-dispatcher). |
Problema
gotchas-memory.jsexporta usando named exports:Mas
ideation-engine.jsimporta sem destructuring:Isso faz com que
GotchasMemoryreceba o objeto do módulo inteiro em vez da classe. Na linha 30,new GotchasMemory()tenta instanciar o objeto do módulo → TypeError.Correção
Aplicar destructuring no require:
Testes
Rodei os 54 testes do ideation-engine (PR #518) com este fix aplicado — todos passam.
Closes #517
Summary by CodeRabbit
Release Notes
No user-visible changes in this release. This update includes internal code organization improvements to enhance maintainability.