fix(ideation): corrige import named do GotchasMemory (#517)#521
fix(ideation): corrige import named do GotchasMemory (#517)#521nikolasdehor wants to merge 7 commits intoSynkraAI:mainfrom
Conversation
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR fixes a named export import bug affecting GotchasMemory instantiation in IdeationEngine. Changes include updating two core files to use destructured imports with proper error handling, and introducing a regression test suite validating three initialization scenarios: auto-creation, custom memory injection, and load failure handling. Changes
Possibly related issues
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
📝 Coding Plan
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
This PR fixes a critical import bug in ideation-engine.js where GotchasMemory was being imported incorrectly. The module gotchas-memory.js exports named exports (module.exports = { GotchasMemory, ... }), but the import was treating it as a default export, causing new GotchasMemory() to throw a TypeError that was silently caught, leaving gotchasMemory always null.
Changes:
- Fixed the import in
ideation-engine.jsto use destructuring:({ GotchasMemory } = require('../memory/gotchas-memory'))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -13,7 +13,7 @@ const { execSync } = require('child_process'); | |||
| // Import dependencies with fallbacks | |||
| let GotchasMemory; | |||
| try { | |||
| GotchasMemory = require('../memory/gotchas-memory'); | |||
| ({ GotchasMemory } = require('../memory/gotchas-memory')); | |||
There was a problem hiding this comment.
This fix correctly addresses the import issue in this file, but the same bug exists in two other files that also need to be fixed:
.aios-core/core/execution/context-injector.js:24- UsesGotchasMemory = require('../memory/gotchas-memory')and tries to instantiate withnew GotchasMemory()on line 50.aios-core/core/execution/subagent-dispatcher.js:31- UsesGotchasMemory = require('../memory/gotchas-memory')and tries to instantiate withnew GotchasMemory()on line 99
Both files have the exact same bug where they import the entire module object instead of destructuring the GotchasMemory class, which will cause the same TypeError when instantiating. These should be fixed in the same PR to ensure consistency and prevent the same silent failure across the codebase.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.aios-core/core/ideation/ideation-engine.js (1)
15-19: Keep the fallback, but stop swallowing import failures silently.On Line 17,
catch {}removes error context for a critical-path module. Please capture/log minimal context so future regressions are diagnosable while still falling back tonull.Proposed adjustment
try { ({ GotchasMemory } = require('../memory/gotchas-memory')); -} catch { +} catch (error) { + console.warn('[IdeationEngine] Failed to load GotchasMemory:', error.message); GotchasMemory = null; }As per coding guidelines, "Verify error handling is comprehensive with proper try/catch and error context."
🤖 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 15 - 19, The empty catch around the dynamic import of GotchasMemory swallows errors; change the catch to capture the exception (e.g., catch (err)) and log minimal context before falling back to null so failures are diagnosable; specifically update the try/catch that wraps require('../memory/gotchas-memory') / GotchasMemory so the catch logs a concise message including err.message (or uses the existing logger if available) and then sets GotchasMemory = null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.aios-core/core/ideation/ideation-engine.js:
- Line 16: Add a regression unit test that instantiates IdeationEngine without a
gotchasMemory option and asserts that the constructor creates a GotchasMemory
instance; specifically, in your test suite import/require IdeationEngine and the
GotchasMemory symbol (from core/ideation/ideation-engine.js), call new
IdeationEngine({ /* omit gotchasMemory */ }) and verify the resulting engine has
a gotchasMemory property that is an instance of GotchasMemory; place the test
under the .aios-core/core/** tests to satisfy coverage rules and include module
load success setup/mocks as needed so the constructor path runs.
---
Nitpick comments:
In @.aios-core/core/ideation/ideation-engine.js:
- Around line 15-19: The empty catch around the dynamic import of GotchasMemory
swallows errors; change the catch to capture the exception (e.g., catch (err))
and log minimal context before falling back to null so failures are diagnosable;
specifically update the try/catch that wraps require('../memory/gotchas-memory')
/ GotchasMemory so the catch logs a concise message including err.message (or
uses the existing logger if available) and then sets GotchasMemory = null.
| @@ -13,7 +13,7 @@ const { execSync } = require('child_process'); | |||
| // Import dependencies with fallbacks | |||
| let GotchasMemory; | |||
| try { | |||
| GotchasMemory = require('../memory/gotchas-memory'); | |||
| ({ GotchasMemory } = require('../memory/gotchas-memory')); | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the ideation-engine.js file
fd -HI "ideation-engine.js"
# Check the directory structure
find .aios-core/core/ideation -type f -name "*.js" -o -name "*.ts" | head -20Repository: SynkraAI/aios-core
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for test files related to ideation engine
fd -HI "test|spec" | grep -i ideation
# Also search for gotchas-memory tests
fd -HI "test|spec" | grep -i gotchasRepository: SynkraAI/aios-core
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the ideation-engine.js file and read it to understand the constructor
fd -t f "ideation-engine.js" --path "*/core/ideation/*" -x wc -l
# Then examine the file
fd -t f "ideation-engine.js" --path "*/core/ideation/*" -x cat -nRepository: SynkraAI/aios-core
Length of output: 295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and read the ideation-engine.js file
cat -n .aios-core/core/ideation/ideation-engine.jsRepository: SynkraAI/aios-core
Length of output: 28750
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for test files anywhere in the repo matching ideation pattern
find . -type f \( -name "*test*.js" -o -name "*spec*.js" -o -name "*test*.ts" -o -name "*spec*.ts" \) | grep -iE "(ideation|gotchas)" | head -20Repository: SynkraAI/aios-core
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any test files in the ideation directory
find .aios-core/core/ideation -type f | head -20Repository: SynkraAI/aios-core
Length of output: 105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all test-like files in the entire repo
find . -type f -name "*.test.js" -o -name "*.spec.js" -o -name "*.test.ts" -o -name "*.spec.ts" | head -30Repository: SynkraAI/aios-core
Length of output: 1971
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for directory patterns that commonly contain tests
find . -type d \( -name "test" -o -name "tests" -o -name "__tests__" -o -name "spec" -o -name "specs" \) | head -20Repository: SynkraAI/aios-core
Length of output: 403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any mention of IdeationEngine in the entire codebase
rg -l "IdeationEngine" --type js --type ts | head -30Repository: SynkraAI/aios-core
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euso pipefail
# Search for gotchas-memory references
rg -l "gotchas-memory|GotchasMemory" --type js --type ts | head -30Repository: SynkraAI/aios-core
Length of output: 346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for test files mentioning ideation or gotchas
find . -type f \( -name "*.test.js" -o -name "*.spec.js" -o -name "*.test.ts" -o -name "*.spec.ts" \) -exec grep -l "ideation\|gotchas" {} \; 2>/dev/null | head -20Repository: SynkraAI/aios-core
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's a test directory under ideation
find . -path "*/ideation/*" -type f \( -name "*.test.js" -o -name "*.spec.js" -o -name "*test*.js" \) | head -20Repository: SynkraAI/aios-core
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any file that requires ideation-engine
rg "require.*ideation-engine|from.*ideation-engine" --type js --type ts | head -30Repository: SynkraAI/aios-core
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if gotchas-memory has its own tests
find . -path "*/memory/*" -type f -name "*.test.js" -o -name "*.spec.js" -o -name "*test*.js" 2>/dev/null | head -20Repository: SynkraAI/aios-core
Length of output: 1301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List all files in the memory test directory if it exists
ls -la .aios-core/core/memory/__tests__/ 2>/dev/null || echo "Directory does not exist"Repository: SynkraAI/aios-core
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and read gotchas-memory.js
find . -name "gotchas-memory.js" -type f -exec cat -n {} \;Repository: SynkraAI/aios-core
Length of output: 38907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any existing test that might test ideation-engine indirectly
rg "new IdeationEngine|ideation.*engine" --type js tests/ .aios-core/ -A 3 -B 1 | head -50Repository: SynkraAI/aios-core
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if ideation-engine is imported/used anywhere in the codebase
rg "IdeationEngine" --type js --type ts -lRepository: SynkraAI/aios-core
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any test that might be testing core modules or memory modules
find . -name "*.test.js" -o -name "*.spec.js" | xargs grep -l "GotchasMemory\|ideation" 2>/dev/null || echo "No tests found matching pattern"Repository: SynkraAI/aios-core
Length of output: 140
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check test directories near ideation or memory
ls -la .aios-core/core/ideation/ 2>/dev/null || echo "No test dir in ideation"
ls -la .aios-core/core/memory/__tests__/ 2>/dev/null || echo "No tests in memory"Repository: SynkraAI/aios-core
Length of output: 433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for getAll method usage in tests since ideation-engine calls it (line 79)
rg "getAll" --type js -l | head -20Repository: SynkraAI/aios-core
Length of output: 341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what's in those test files that mention GotchasMemory
cat -n ./tests/core/context-injector.test.js | head -100Repository: SynkraAI/aios-core
Length of output: 3865
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the other test file
cat -n ./tests/core/subagent-dispatcher.test.js | head -100Repository: SynkraAI/aios-core
Length of output: 4239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Final check: does IdeationEngine appear in any actual test that isn't just mocking?
rg "IdeationEngine" --type js tests/ .aios-core/ -B 2 -A 5 | head -50Repository: SynkraAI/aios-core
Length of output: 1313
Add regression test coverage for IdeationEngine constructor's GotchasMemory initialization path.
This core framework module lacks test coverage. Per the coding guidelines for .aios-core/core/** paths, verify test coverage exists for new/modified functions. Specifically, test that new IdeationEngine({ ...without gotchasMemory... }) instantiates a GotchasMemory instance when the module loads successfully, confirming the backwards compatibility contract for agents consuming this core module.
🤖 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 at line 16, Add a regression
unit test that instantiates IdeationEngine without a gotchasMemory option and
asserts that the constructor creates a GotchasMemory instance; specifically, in
your test suite import/require IdeationEngine and the GotchasMemory symbol (from
core/ideation/ideation-engine.js), call new IdeationEngine({ /* omit
gotchasMemory */ }) and verify the resulting engine has a gotchasMemory property
that is an instance of GotchasMemory; place the test under the
.aios-core/core/** tests to satisfy coverage rules and include module load
success setup/mocks as needed so the constructor path runs.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/core/ideation/ideation-engine-gotchas.test.js (1)
50-65: Consider usingafterEachortry/finallyfor mock cleanup.If an assertion fails before
console.warn.mockRestore()is called, the mock won't be restored, potentially affecting subsequent tests.♻️ Suggested improvement for robust cleanup
test('sets gotchasMemory to null when module fails to load', () => { // Mock the gotchas-memory module to throw on require jest.doMock('../../../.aios-core/core/memory/gotchas-memory', () => { throw new Error('Module not found'); }); // Suppress the expected console.warn - jest.spyOn(console, 'warn').mockImplementation(() => {}); + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); - IdeationEngine = require('../../../.aios-core/core/ideation/ideation-engine'); - const engine = new IdeationEngine({ rootPath: '/tmp/test' }); - - expect(engine.gotchasMemory).toBeNull(); - - console.warn.mockRestore(); + try { + IdeationEngine = require('../../../.aios-core/core/ideation/ideation-engine'); + const engine = new IdeationEngine({ rootPath: '/tmp/test' }); + + expect(engine.gotchasMemory).toBeNull(); + } finally { + warnSpy.mockRestore(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/ideation/ideation-engine-gotchas.test.js` around lines 50 - 65, The test currently mocks console.warn and the gotchas-memory require but only calls console.warn.mockRestore() at the end, which can leak if the assertion throws; wrap the test's require/assert sequence in a try/finally (or move cleanup to an afterEach) so that console.warn.mockRestore() always runs, and also call jest.resetModules() (or jest.isolateModules) in the finally/afterEach to clear the mocked require of '../../../.aios-core/core/memory/gotchas-memory' before other tests; apply this around the code that requires IdeationEngine and checks engine.gotchasMemory to ensure deterministic cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/core/ideation/ideation-engine-gotchas.test.js`:
- Around line 50-65: The test currently mocks console.warn and the
gotchas-memory require but only calls console.warn.mockRestore() at the end,
which can leak if the assertion throws; wrap the test's require/assert sequence
in a try/finally (or move cleanup to an afterEach) so that
console.warn.mockRestore() always runs, and also call jest.resetModules() (or
jest.isolateModules) in the finally/afterEach to clear the mocked require of
'../../../.aios-core/core/memory/gotchas-memory' before other tests; apply this
around the code that requires IdeationEngine and checks engine.gotchasMemory to
ensure deterministic cleanup.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.aios-core/install-manifest.yamltests/core/ideation/ideation-engine-gotchas.test.js
DevOps Review — @devops (Gage)Veredicto: APPROVED — Merge após rebase Análise
Bloqueador
Ação necessária@nikolasdehor por favor faça rebase contra main para resolver os conflitos: git fetch origin
git rebase origin/main
# Resolver conflitos (provavelmente em install-manifest.yaml)
npm run generate:manifest # Regenerar manifest
git push --force-with-leaseApós rebase limpo, merge imediato. ⚡ Gage — Repository Guardian |
nikolasdehor
force-pushed
the
fix/ideation-engine-gotchas-import
branch
2 times, most recently
from
fbbd291 to
50da339
Compare
|
@Pedrovaleriolopez, fiz o rebase que você pediu (pré-requisito do #518). O import do GotchasMemory tá corrigido com wrapper de compatibilidade no .aios-core. Pode aprovar pra desbloquear o #518? |
…ning Story 9.5 of Epic 9 (Persistent Memory Layer). Implements Phase 2 of the Agent Immortality Protocol (SynkraAI#482) — Persistence layer. Features: - Record decisions with context, rationale, and alternatives - Track outcomes (success/partial/failure) with confidence scoring - Auto-detect categories from description keywords - Find relevant past decisions for context injection (AC7) - Pattern detection across recurring decisions (AC9) - Time-based confidence decay for relevance scoring - Persistence to .aiox/decisions.json 37 unit tests covering all features.
gotchas-memory.js exporta named export { GotchasMemory, ... }, mas
ideation-engine.js atribuía o objeto module inteiro à variável.
Resultado: new GotchasMemory() lançava TypeError silenciado pelo
try/catch, e gotchasMemory ficava sempre null.
Corrige com destructuring: ({ GotchasMemory } = require(...))
Closes SynkraAI#517
CodeRabbit encontrou o mesmo bug de named export em context-injector.js
e subagent-dispatcher.js. Aplica destructuring ({ GotchasMemory }) nos 3
módulos e adiciona console.warn no catch para facilitar diagnóstico.
Módulos corrigidos:
- ideation-engine.js (catch vazio → console.warn)
- context-injector.js (import + catch)
- subagent-dispatcher.js (import + catch)
Regenera install-manifest.yaml.
Verifica que IdeationEngine instancia GotchasMemory corretamente via named export. Cobre 3 cenários: - Auto-criação quando config.gotchasMemory omitido - Uso do gotchasMemory fornecido via config - Fallback para null quando módulo falha ao carregar
Garante que o spy do console.warn é restaurado mesmo se a asserção falhar, evitando leak entre testes.
- Criado .aios-core/core/ideation/ideation-engine.js (wrapper retrocompatível) - Criado .aios-core/core/memory/gotchas-memory.js (wrapper retrocompatível) - Corrigido path do jest.doMock no teste de GotchasMemory import failure (apontava para .aios-core mas o engine importa de .aiox-core)
nikolasdehor
force-pushed
the
fix/ideation-engine-gotchas-import
branch
from
50da339 to
d15bdc9
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@Pedrovaleriolopez, esse PR é o fix original do GotchasMemory import (issue #517). Já rebased conforme você pediu. O #584 duplica esse trabalho. Pode aprovar pra desbloquear o #518 também? |
|
@Pedrovaleriolopez, fiz o rebase que você pediu no dia 26/fev e tá pronto desde então. Esse fix desbloqueia o PR de testes #518 também. Vi que o @oalanicolas abriu o #584 com o mesmo fix — mas o nosso é o original. Se precisar de qualquer mudança extra, é só avisar. |
github-actions
bot
added
area: agents
github-actions
bot
added
mcp
type: test
|
@Pedrovaleriolopez, solicitando review deste PR. Aberto desde 26/fev, corrige a issue #517 (import incorreto do O PR duplicado #584 (aberto 11/mar) inclui a mesma correção. Este PR tem conflito de merge — posso fazer rebase rapidamente se necessário. |
Problema
Em
ideation-engine.js:16, o import do GotchasMemory atribui o objeto module inteiro à variável:Como
gotchas-memory.jsexporta named exports (module.exports = { GotchasMemory, ... }),new GotchasMemory()lançaTypeError: GotchasMemory is not a constructor. O try/catch silencia o erro, egotchasMemoryfica sempre null.Correção
Destructuring no import:
Validação
engine.gotchasMemory→null(constructor silenciosamente falha)engine.gotchasMemory→GotchasMemory {}(instância correta)Closes #517
Summary by CodeRabbit
Tests
Bug Fixes