feat(memory): add decision-memory module for cross-session agent learning#564
feat(memory): add decision-memory module for cross-session agent learning#564nikolasdehor wants to merge 5 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:
WalkthroughAdds a new persistent Decision Memory module with recording, relevance scoring, outcome updates, pattern detection, events, and disk persistence; includes comprehensive Jest tests and manifest updates. Also adds new test suites for RateLimitManager and ResultAggregator. Changes
Sequence DiagramsequenceDiagram
participant Agent as Agent
participant DM as DecisionMemory
participant FS as File System
participant Events as Event Emitter
Agent->>DM: recordDecision(description, rationale, ...)
DM->>DM: detect category, extract keywords, compute id
DM->>DM: create decision object (defaults, timestamps)
DM->>FS: save to .aiox/decisions.json
DM->>Events: emit DECISION_RECORDED
Events-->>Agent: notification
Agent->>DM: updateOutcome(decisionId, outcome, notes)
DM->>DM: apply time decay & outcome adjustments, update timestamps
DM->>FS: save updated state
DM->>Events: emit OUTCOME_UPDATED
Events-->>Agent: notification
Agent->>DM: getRelevantDecisions(taskDescription, options)
DM->>DM: score by keyword similarity, apply time/outcome weights, filter
DM-->>Agent: ranked decisions
Agent->>DM: injectDecisionContext(taskDescription)
DM->>DM: format markdown block from relevant decisions
DM->>Events: emit DECISIONS_INJECTED
DM-->>Agent: contextual block
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 implements the Decision Memory module (decision-memory.js) as part of Story 9.5 of Epic 9 (Persistent Memory Layer), addressing Phase 2 of the Agent Immortality Protocol (#482). It enables AIOX agents to record decisions with context and outcomes, retrieve relevant past decisions as context for new tasks, and detect recurring decision patterns — complementing the existing gotchas-memory.js which tracks errors.
Changes:
- New
decision-memory.jsmodule with decision recording, outcome tracking, confidence/time decay, context injection, and pattern detection - New
decision-memory.test.jswith broad test coverage across all public API methods and edge cases - Updated
install-manifest.yamlto register the new module and reflect updated file sizes across the codebase
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
.aiox-core/core/memory/decision-memory.js |
Core module implementing DecisionMemory class with persistence, confidence scoring, and pattern detection |
tests/core/memory/decision-memory.test.js |
Unit test suite covering constructor, load/save, record, outcomes, relevance, patterns, stats, and decay |
.aiox-core/install-manifest.yaml |
Adds the new decision-memory.js entry and updates sizes for other modified files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _extractKeywords(text) { | ||
| const stopWords = new Set([ | ||
| 'the', 'a', 'an', 'is', 'are', 'was', 'were', 'be', 'been', | ||
| 'being', 'have', 'has', 'had', 'do', 'does', 'did', 'will', | ||
| 'would', 'could', 'should', 'may', 'might', 'can', 'shall', | ||
| 'to', 'of', 'in', 'for', 'on', 'with', 'at', 'by', 'from', | ||
| 'as', 'into', 'through', 'during', 'before', 'after', 'and', | ||
| 'but', 'or', 'nor', 'not', 'so', 'yet', 'both', 'either', | ||
| 'neither', 'each', 'every', 'all', 'any', 'few', 'more', | ||
| 'most', 'other', 'some', 'such', 'no', 'only', 'own', 'same', | ||
| 'than', 'too', 'very', 'just', 'because', 'que', 'para', | ||
| 'com', 'por', 'uma', 'como', 'mais', 'dos', 'das', 'nos', | ||
| ]); | ||
|
|
||
| return text | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9\s-]/g, ' ') | ||
| .split(/\s+/) | ||
| .filter(w => w.length > 2 && !stopWords.has(w)) | ||
| .slice(0, 20); | ||
| } |
There was a problem hiding this comment.
The _extractKeywords method instantiates a new Set of stop words on every call. Since _extractKeywords is called frequently (on every recordDecision, getRelevantDecisions, injectDecisionContext, and _detectPatterns call — including nested calls in pattern detection), this creates unnecessary garbage collection pressure. The stop words set should be defined as a module-level constant (or a class-level property) and reused across calls.
| it('should increase confidence on success (up to cap)', () => { | ||
| const mem = createMemory(); | ||
| const d = mem.recordDecision({ description: 'Enable compression' }); | ||
|
|
||
| // Reduce confidence first via a failure, then verify success increases it | ||
| mem.updateOutcome(d.id, Outcome.FAILURE); | ||
| const afterFailure = d.confidence; | ||
|
|
||
| d.outcome = Outcome.PENDING; // reset to allow re-update | ||
| mem.updateOutcome(d.id, Outcome.SUCCESS); | ||
| expect(d.confidence).toBeGreaterThan(afterFailure); |
There was a problem hiding this comment.
The test directly mutates the decision object (d.outcome = Outcome.PENDING) to bypass the updateOutcome validation in order to call updateOutcome a second time. This relies on a side effect of updateOutcome returning a reference to the same object stored in this.decisions — which is an implementation detail. The test would be more robust if it created a new decision for the success test, rather than mutating the outcome state of an existing one directly on the returned object. Additionally, as noted in the main module review, updateOutcome actually accepts Outcome.PENDING as a valid argument, so the mutation workaround in the test is not actually needed — the test could call mem.updateOutcome(d.id, Outcome.PENDING) directly to reset.
| [DecisionCategory.WORKFLOW]: [ | ||
| 'workflow', 'pipeline', 'ci', 'deploy', 'release', | ||
| 'merge', 'branch', 'review', 'approve', | ||
| ], | ||
| [DecisionCategory.TESTING]: [ | ||
| 'test', 'spec', 'coverage', 'assert', 'mock', | ||
| 'fixture', 'snapshot', 'jest', 'unit', 'integration', | ||
| ], | ||
| [DecisionCategory.DEPLOYMENT]: [ | ||
| 'deploy', 'release', 'publish', 'ship', 'staging', | ||
| 'production', 'rollout', 'canary', 'blue-green', | ||
| ], | ||
| }; |
There was a problem hiding this comment.
The keywords 'deploy' and 'release' appear in both WORKFLOW and DEPLOYMENT category keyword lists. When a description matches both categories with an equal score (e.g., "Deploy to production release"), _detectCategory will assign the first one encountered in Object.entries(CATEGORY_KEYWORDS) iteration order, which in practice means WORKFLOW wins over DEPLOYMENT due to insertion order. This makes auto-detection of deployment-related decisions unreliable. The deploy and release keywords should be exclusive to the DEPLOYMENT category, or the tie-breaking logic in _detectCategory should be made explicit.
| _detectPatterns(newDecision) { | ||
| const similar = this.decisions.filter(d => | ||
| d.id !== newDecision.id && | ||
| d.category === newDecision.category && | ||
| this._keywordSimilarity(d.keywords, newDecision.keywords) > 0.4, | ||
| ); |
There was a problem hiding this comment.
The _detectPatterns method uses a hardcoded similarity threshold of 0.4 to determine if two decisions are similar enough to form a pattern. This threshold is independent of this.config.similarityThreshold, which is the configurable threshold used in getRelevantDecisions. Using a hardcoded value means the pattern detection sensitivity cannot be tuned via configuration, unlike the rest of the system. Consider either making this threshold part of CONFIG (e.g., patternSimilarityThreshold) or deriving it from the existing similarityThreshold config value for consistency.
| const decayFactor = Math.max( | ||
| this.config.minConfidence, | ||
| 1 - (ageDays / this.config.confidenceDecayDays) * 0.5, | ||
| ); | ||
|
|
||
| return confidence * decayFactor; |
There was a problem hiding this comment.
The _applyTimeDecay method applies Math.max(minConfidence, ...) to the decayFactor, but not to the final return value confidence * decayFactor. Since decayFactor is used as a multiplier (not the final result), the returned value can be significantly below minConfidence when the input confidence is already less than 1.0.
For example, with confidence = 0.4 and decayFactor = minConfidence = 0.1, the returned value is 0.4 * 0.1 = 0.04, which is far below the minConfidence floor of 0.1.
The Math.max(this.config.minConfidence, ...) guard should be applied to the final product confidence * decayFactor, not just to decayFactor.
| const decayFactor = Math.max( | |
| this.config.minConfidence, | |
| 1 - (ageDays / this.config.confidenceDecayDays) * 0.5, | |
| ); | |
| return confidence * decayFactor; | |
| const decayFactor = 1 - (ageDays / this.config.confidenceDecayDays) * 0.5; | |
| const decayedConfidence = confidence * decayFactor; | |
| return Math.max(this.config.minConfidence, decayedConfidence); |
| if (!fs.existsSync(dir)) { | ||
| fs.mkdirSync(dir, { recursive: true }); | ||
| } | ||
|
|
||
| const data = { | ||
| schemaVersion: this.config.schemaVersion, | ||
| version: this.config.version, | ||
| updatedAt: new Date().toISOString(), | ||
| stats: this.getStats(), | ||
| decisions: this.decisions.slice(-this.config.maxDecisions), | ||
| patterns: this.patterns, | ||
| }; | ||
|
|
||
| fs.writeFileSync(filePath, JSON.stringify(data, null, 2), 'utf-8'); |
There was a problem hiding this comment.
The save() method has no error handling. If writeFileSync throws (e.g., due to a permission error or disk full), the exception will propagate uncaught to the caller. By contrast, the equivalent _saveGotchas() method in gotchas-memory.js wraps its file operations in a try/catch and logs the error gracefully. The save() method here should follow the same pattern to prevent unhandled exceptions from crashing the agent when persistence fails.
| if (!fs.existsSync(dir)) { | |
| fs.mkdirSync(dir, { recursive: true }); | |
| } | |
| const data = { | |
| schemaVersion: this.config.schemaVersion, | |
| version: this.config.version, | |
| updatedAt: new Date().toISOString(), | |
| stats: this.getStats(), | |
| decisions: this.decisions.slice(-this.config.maxDecisions), | |
| patterns: this.patterns, | |
| }; | |
| fs.writeFileSync(filePath, JSON.stringify(data, null, 2), 'utf-8'); | |
| try { | |
| if (!fs.existsSync(dir)) { | |
| fs.mkdirSync(dir, { recursive: true }); | |
| } | |
| const data = { | |
| schemaVersion: this.config.schemaVersion, | |
| version: this.config.version, | |
| updatedAt: new Date().toISOString(), | |
| stats: this.getStats(), | |
| decisions: this.decisions.slice(-this.config.maxDecisions), | |
| patterns: this.patterns, | |
| }; | |
| fs.writeFileSync(filePath, JSON.stringify(data, null, 2), 'utf-8'); | |
| } catch (err) { | |
| // Persistence failures should not crash the agent; log and continue | |
| console.error('[DecisionMemory] Failed to save decisions to disk:', err); | |
| } |
| version: this.config.version, | ||
| updatedAt: new Date().toISOString(), | ||
| stats: this.getStats(), | ||
| decisions: this.decisions.slice(-this.config.maxDecisions), |
There was a problem hiding this comment.
The save() method caps decisions at maxDecisions when writing to disk (this.decisions.slice(-this.config.maxDecisions)), but it does not update this.decisions in memory. This means the in-memory state can grow beyond maxDecisions indefinitely across a single session, while the persisted file remains capped. On the next load, the memory will be trimmed, but within a running session the in-memory array keeps growing. If the intent is to enforce a hard cap in-session too (as suggested by the config comment // Cap stored decisions), the decisions array should also be trimmed at record time or after save.
| if (!Object.values(Outcome).includes(outcome)) { | ||
| throw new Error(`Invalid outcome: ${outcome}. Use: ${Object.values(Outcome).join(', ')}`); |
There was a problem hiding this comment.
The updateOutcome validation accepts Outcome.PENDING as a valid outcome (since it's included in Object.values(Outcome)), which means callers can explicitly reset a decision back to the pending state, bypassing confidence adjustments. While the test suite mutates d.outcome = Outcome.PENDING directly to work around this, the API itself allows setting it as a valid outcome. If resetting to pending is intentional, the confidence adjustment logic (currently only handling SUCCESS and FAILURE) should also account for this case. If it is not intentional, Outcome.PENDING should be excluded from the valid outcomes in the updateOutcome method, and the error message in the JSDoc (@param {string} outcome - 'success' | 'partial' | 'failure') already suggests pending is not a valid update value.
| if (!Object.values(Outcome).includes(outcome)) { | |
| throw new Error(`Invalid outcome: ${outcome}. Use: ${Object.values(Outcome).join(', ')}`); | |
| const allowedOutcomes = [Outcome.SUCCESS, Outcome.PARTIAL, Outcome.FAILURE]; | |
| if (!allowedOutcomes.includes(outcome)) { | |
| throw new Error(`Invalid outcome: ${outcome}. Use: ${allowedOutcomes.join(', ')}`); |
![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 (4)
.aiox-core/core/memory/decision-memory.js (2)
195-228: Category input is not validated when explicitly provided.When a caller passes an explicit
categoryvalue, Line 212 uses it directly without validating it againstDecisionCategoryvalues. This could lead to inconsistent data if an invalid category string is passed.♻️ Add category validation
recordDecision({ description, rationale = '', alternatives = [], category = null, taskContext = '', agentId = 'unknown', }) { if (!description) { throw new Error('Decision description is required'); } + if (category && !Object.values(DecisionCategory).includes(category)) { + throw new Error(`Invalid category: ${category}. Use: ${Object.values(DecisionCategory).join(', ')}`); + } + const decision = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/memory/decision-memory.js around lines 195 - 228, recordDecision accepts an explicit category but assigns it directly without validation, risking invalid category values; update recordDecision to validate the provided category against the DecisionCategory enum (or allowed set) before assigning (use DecisionCategory or a helper like isValidCategory), fallback to this._detectCategory(description) or throw an Error if invalid, and ensure the assigned value in the decision object is the validated category; reference the recordDecision method, DecisionCategory, and this._detectCategory for locating code.
138-158: Consider distinguishing corruption from other I/O errors.The
load()method uses synchronous I/O with anasyncsignature, which works but is slightly misleading. More importantly, the empty catch block at Line 151 treats all errors identically—including permission errors or disk failures—as "corrupted file" scenarios. This could mask actual system issues.Consider logging or emitting an event when silently recovering from load failures, especially for non-parse errors:
♻️ Suggested improvement for error handling
} catch (error) { - // Corrupted file — start fresh + // Log recovery - helps debug issues without breaking the module + if (error.code !== 'ENOENT') { + // Not a "file not found" error - might be corruption or permission issue + this.emit('load:error', { error: error.message, path: filePath }); + } this.decisions = []; this.patterns = []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/memory/decision-memory.js around lines 138 - 158, The load() method currently swallows all errors and misleadingly mixes sync I/O with an async signature; update it to (a) either remove async or switch to async fs methods, (b) replace the empty catch with targeted handling that distinguishes JSON parse errors from filesystem/permission errors by checking error types/messages when reading/parsing the file referenced by this.projectRoot + this.config.decisionsJsonPath, (c) on JSON.parse errors recover by resetting this.decisions and this.patterns as you already do, but on fs errors (ENOENT/permission/disk) log or emit an event with the error and rethrow or fail fast instead of treating it as corruption, and (d) ensure this._loaded is only set after a successful or intentionally recovered load; refer to load(), this.decisions, this.patterns, this.config.schemaVersion for locating the logic.tests/core/memory/decision-memory.test.js (2)
201-234: Test relies on internal state mutation.Lines 209 and 229 directly mutate
d.outcome = Outcome.PENDINGto "reset" the decision for re-updating. While this works, it couples the test to internal implementation details. If the implementation changes to protect the outcome field, these tests would break.Consider recording multiple decisions to test cumulative effects, or accept this as a pragmatic trade-off for testing confidence bounds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/memory/decision-memory.test.js` around lines 201 - 234, The tests mutate internal state by setting d.outcome = Outcome.PENDING before calling mem.updateOutcome, which couples tests to implementation; instead, avoid touching internal fields and exercise the public API: create fresh decisions via createMemory().recordDecision(...) when you need a "reset" (e.g., record a second decision for the success-after-failure case rather than mutating d.outcome), and in the repeated-failure test call mem.recordDecision(...) per iteration or implement/call a public reset method if available; locate usages of recordDecision, updateOutcome, Outcome and replace direct d.outcome assignments with creating new decision instances or a public reset helper.
1-9: Use absolute import path.Line 9 uses a relative import with
../../../.aiox-core/core/memory/decision-memory. Per coding guidelines: "Use absolute imports instead of relative imports in all code."♻️ Suggested fix
-} = require('../../../.aiox-core/core/memory/decision-memory'); +} = require('.aiox-core/core/memory/decision-memory');Or configure a module alias in Jest config if the absolute path requires resolution configuration.
As per coding guidelines: "Use absolute imports instead of relative imports in all code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/memory/decision-memory.test.js` around lines 1 - 9, The test currently requires DecisionMemory and related symbols via a deep relative path; replace that relative require with an absolute import (e.g., require('aiox-core/core/memory/decision-memory') or your repo's canonical package name) so DecisionMemory, DecisionCategory, Outcome, Events, and CONFIG are imported by absolute module name, and if the absolute package name isn't resolvable in Jest add a module alias in the Jest config (moduleNameMapper) to map the chosen absolute prefix to the .aiox-core folder so the test can resolve the module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.aiox-core/core/memory/decision-memory.js:
- Around line 164-182: The save() method lacks error handling and is vulnerable
to concurrent writes; wrap the entire body of decision-memory.js::save in a
try/catch that logs or rethrows a contextual Error (include filePath and
this.config values), and implement an atomic write plus basic lock: create a
temporary file in the same dir (e.g., filePath + '.tmp'), write the JSON there,
fs.renameSync to atomically replace the target, and guard concurrent calls with
an in-memory mutex/flag on the instance (e.g., this._saving or a Promise-based
mutex) so only one save executes at a time; ensure directory creation, JSON
serialization of { schemaVersion, version, updatedAt, stats:this.getStats(),
decisions:this.decisions.slice(-this.config.maxDecisions),
patterns:this.patterns } is inside the try and any caught errors include
contextual info before being thrown or logged.
---
Nitpick comments:
In @.aiox-core/core/memory/decision-memory.js:
- Around line 195-228: recordDecision accepts an explicit category but assigns
it directly without validation, risking invalid category values; update
recordDecision to validate the provided category against the DecisionCategory
enum (or allowed set) before assigning (use DecisionCategory or a helper like
isValidCategory), fallback to this._detectCategory(description) or throw an
Error if invalid, and ensure the assigned value in the decision object is the
validated category; reference the recordDecision method, DecisionCategory, and
this._detectCategory for locating code.
- Around line 138-158: The load() method currently swallows all errors and
misleadingly mixes sync I/O with an async signature; update it to (a) either
remove async or switch to async fs methods, (b) replace the empty catch with
targeted handling that distinguishes JSON parse errors from
filesystem/permission errors by checking error types/messages when
reading/parsing the file referenced by this.projectRoot +
this.config.decisionsJsonPath, (c) on JSON.parse errors recover by resetting
this.decisions and this.patterns as you already do, but on fs errors
(ENOENT/permission/disk) log or emit an event with the error and rethrow or fail
fast instead of treating it as corruption, and (d) ensure this._loaded is only
set after a successful or intentionally recovered load; refer to load(),
this.decisions, this.patterns, this.config.schemaVersion for locating the logic.
In `@tests/core/memory/decision-memory.test.js`:
- Around line 201-234: The tests mutate internal state by setting d.outcome =
Outcome.PENDING before calling mem.updateOutcome, which couples tests to
implementation; instead, avoid touching internal fields and exercise the public
API: create fresh decisions via createMemory().recordDecision(...) when you need
a "reset" (e.g., record a second decision for the success-after-failure case
rather than mutating d.outcome), and in the repeated-failure test call
mem.recordDecision(...) per iteration or implement/call a public reset method if
available; locate usages of recordDecision, updateOutcome, Outcome and replace
direct d.outcome assignments with creating new decision instances or a public
reset helper.
- Around line 1-9: The test currently requires DecisionMemory and related
symbols via a deep relative path; replace that relative require with an absolute
import (e.g., require('aiox-core/core/memory/decision-memory') or your repo's
canonical package name) so DecisionMemory, DecisionCategory, Outcome, Events,
and CONFIG are imported by absolute module name, and if the absolute package
name isn't resolvable in Jest add a module alias in the Jest config
(moduleNameMapper) to map the chosen absolute prefix to the .aiox-core folder so
the test can resolve the module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f01103a7-46cb-4cc0-a51c-996d2b318953
📒 Files selected for processing (3)
.aiox-core/core/memory/decision-memory.js.aiox-core/install-manifest.yamltests/core/memory/decision-memory.test.js
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
.aiox-core/core/memory/decision-memory.js (1)
178-204:⚠️ Potential issue | 🟠 Major
save()still allows lost updates and hides persistence failures.This rewrites the full snapshot to a shared project file. If two agents/processes load the same file and save near-simultaneously, the later write can drop the earlier agent's decisions. The catch on Lines 199-203 also resolves instead of rejecting, so callers cannot detect or retry a failed save. Please serialize or merge writers and surface the failure to the caller.
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 @.aiox-core/core/memory/decision-memory.js around lines 178 - 204, The save() method must serialize concurrent writers, merge on-disk changes to avoid lost updates, and surface persistence errors instead of swallowing them: add an in-process mutex (e.g., a Promise-based lock stored as this._saveLock or use a simple queue) so concurrent calls to save() run one-at-a-time; inside save() before writing, read and parse the existing file (using path.resolve(this.projectRoot, this.config.decisionsJsonPath)) if present, merge its decisions with this.decisions by unique decision id (dedupe, then keep the most recent by updatedAt or insertion order), then cap to this.config.maxDecisions, build the data object (schemaVersion/version/updatedAt/stats/patterns), write atomically by writing to a temp file and fs.rename to the final path, and on any fs error log with context and rethrow the error so callers can detect/retry; reference save(), this.decisions, this.getStats(), this.patterns, this.config.maxDecisions and this.config.decisionsJsonPath when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.aiox-core/core/memory/decision-memory.js:
- Around line 139-146: The instance tracks _loaded but public APIs
(getRelevantDecisions, injectDecisionContext, recordDecision, save) use the
in-memory arrays directly, which can cause loss of persisted decisions; update
the DecisionMemory class so public methods first ensure the persisted state is
hydrated (lazy-load) or fail fast while _loaded is false: add/use an internal
async ensureLoaded() helper (or call existing load/hydrate function) at the
start of getRelevantDecisions, injectDecisionContext, recordDecision, and save
so they either await loading before operating or throw a clear error; also make
save merge with the freshly loaded state (or reload before writing) to avoid
overwriting disk with an empty buffer.
- Around line 508-538: When creating a new pattern in the DecisionMemory logic,
don't assign a negative recommendation when no outcomes exist; change the
recommendation assignment to use a neutral message if outcomes.length === 0
(e.g., "No consensus yet; awaiting resolved outcomes.") and only choose the
success/failure recommendation when outcomes.length > 0; also ensure any
existing pattern for the same cluster (this.patterns.some(...) using
_keywordSimilarity/_extractKeywords) is either updated or recalculated when
related decision outcomes change so a premature neutral/negative recommendation
doesn't persist—emit Events.PATTERN_DETECTED only after setting the
neutral/derived recommendation on pattern and add a hook in the decision outcome
handler to recompute related patterns.
- Around line 353-356: The persisted fields d.description, d.rationale, and
d.outcomeNotes are inserted verbatim into the Markdown block (via the lines.push
calls) and must be neutralized to prevent injected headings/code/controls; add a
helper like escapeMarkdown/escapeForInjection that escapes markdown/control
characters (e.g., backticks, hashes, asterisks, angle brackets, leading '>' and
triple-fence patterns, or replaces newlines with safe line breaks) and use it
when building the lines (replace uses of d.description, d.rationale,
d.outcomeNotes in the lines.push calls with the escaped values) so the block
remains reference text while leaving other logic (e.g., this._applyTimeDecay)
unchanged.
- Around line 311-326: Change the gating to use raw keyword similarity before
applying ranking bonuses: compute similarity via
this._keywordSimilarity(taskKeywords, d.keywords) for each candidate, filter out
candidates whose similarity is below this.config.similarityThreshold, then for
the remaining candidates compute decayed confidence with this._applyTimeDecay
and the outcomeBonus and build the composite score for sorting; update the block
that defines scored and the final return so the similarity-based filter happens
prior to composing the blended score (references: _keywordSimilarity,
_applyTimeDecay, config.similarityThreshold, Outcome, scored).
---
Duplicate comments:
In @.aiox-core/core/memory/decision-memory.js:
- Around line 178-204: The save() method must serialize concurrent writers,
merge on-disk changes to avoid lost updates, and surface persistence errors
instead of swallowing them: add an in-process mutex (e.g., a Promise-based lock
stored as this._saveLock or use a simple queue) so concurrent calls to save()
run one-at-a-time; inside save() before writing, read and parse the existing
file (using path.resolve(this.projectRoot, this.config.decisionsJsonPath)) if
present, merge its decisions with this.decisions by unique decision id (dedupe,
then keep the most recent by updatedAt or insertion order), then cap to
this.config.maxDecisions, build the data object
(schemaVersion/version/updatedAt/stats/patterns), write atomically by writing to
a temp file and fs.rename to the final path, and on any fs error log with
context and rethrow the error so callers can detect/retry; reference save(),
this.decisions, this.getStats(), this.patterns, this.config.maxDecisions and
this.config.decisionsJsonPath when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7825502-7fce-4590-852a-c012b844e0bd
📒 Files selected for processing (2)
.aiox-core/core/memory/decision-memory.js.aiox-core/install-manifest.yaml
| constructor(options = {}) { | ||
| super(); | ||
| this.projectRoot = options.projectRoot || process.cwd(); | ||
| this.config = { ...CONFIG, ...options.config }; | ||
| this.decisions = []; | ||
| this.patterns = []; | ||
| this._loaded = false; | ||
| } |
There was a problem hiding this comment.
Guard the public API until persisted state is hydrated.
_loaded is tracked, but none of the public methods enforce it. A fresh instance can return empty results from getRelevantDecisions() / injectDecisionContext(), or call recordDecision() + save() and rewrite .aiox/decisions.json from the empty in-memory buffer, dropping previously persisted history. Please lazy-load on first use or fail fast while _loaded is false.
As per coding guidelines, "Ensure backwards compatibility — core modules are consumed by all agents."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/core/memory/decision-memory.js around lines 139 - 146, The
instance tracks _loaded but public APIs (getRelevantDecisions,
injectDecisionContext, recordDecision, save) use the in-memory arrays directly,
which can cause loss of persisted decisions; update the DecisionMemory class so
public methods first ensure the persisted state is hydrated (lazy-load) or fail
fast while _loaded is false: add/use an internal async ensureLoaded() helper (or
call existing load/hydrate function) at the start of getRelevantDecisions,
injectDecisionContext, recordDecision, and save so they either await loading
before operating or throw a clear error; also make save merge with the freshly
loaded state (or reload before writing) to avoid overwriting disk with an empty
buffer.
05837e5 to
3c43e05
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
.aiox-core/core/memory/decision-memory.js (5)
143-144:⚠️ Potential issue | 🟠 MajorGate the public API on hydrated state.
_loadedis tracked, but these public methods never enforce it. A fresh instance can return empty context from the query APIs and can also append new in-memory decisions before the persisted history is loaded, which is how previously saved decisions get dropped on the next save. Either hydrate eagerly before first use or fail fast untilload()has completed.As per coding guidelines, "Ensure backwards compatibility — core modules are consumed by all agents."
Also applies to: 213-245, 291-355, 400-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/memory/decision-memory.js around lines 143 - 144, The class tracks a _loaded flag but public APIs (e.g., load(), query(), appendDecision(), getContext(), getDecisions(), save()) do not enforce it; update these public methods in decision-memory.js to either eagerly call/halt until load() completes or throw a clear error when _loaded is false, ensuring you call the existing load() helper (or await internal hydration) before returning results or mutating state; gate all entry points referenced in the diff (the query APIs and append/save paths across the ranges noted) so that no reads/writes happen on a not-yet-hydrated instance while preserving backwards compatibility by failing fast with a descriptive message or performing transparent lazy-awaiting of load().
502-531:⚠️ Potential issue | 🟠 MajorDon't emit an “underperformed” pattern with zero outcome data.
When the threshold is hit while all similar decisions are still
pending,outcomes.lengthis0and this code still produces the negative recommendation. Because deduplication prevents later replacements andupdateOutcome()never recalculates patterns, that unsupported conclusion can persist indefinitely. Use a neutral recommendation until resolved outcomes exist, and recompute matching patterns when outcomes change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/memory/decision-memory.js around lines 502 - 531, The pattern detection currently emits a negative recommendation when outcomes.length === 0; change the recommendation computation in the pattern creation block (where pattern is built before this.patterns.push and Events.PATTERN_DETECTED is emitted) to return a neutral message like "No resolved outcomes yet; hold judgment" when outcomes.length === 0, otherwise keep the existing success/failure logic; additionally update the updateOutcome(decisionId, ...) method to recompute affected patterns by locating patterns whose relatedDecisionIds include the updated decision (or that match by _keywordSimilarity/_extractKeywords), recalculating occurrences, successRate, recommendation and re-emitting Events.PATTERN_DETECTED (or replacing/removing patterns if metrics change) so patterns are not left with stale "underperformed" recommendations once outcomes resolve.
306-320:⚠️ Potential issue | 🟡 MinorFilter on raw similarity before ranking bonuses.
The threshold is applied to the blended score instead of keyword overlap. That lets a high-confidence or previously successful decision clear the filter with only a weak token match and get injected into unrelated tasks. Keep the composite score for ordering, but gate candidates on
similarityfirst.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/memory/decision-memory.js around lines 306 - 320, The current scoring pipeline computes a blended score then filters by this composite value, which allows high confidence/outcome bonuses to bypass weak keyword matches; change the logic in the candidates.map / scoring block so you first compute similarity via this._keywordSimilarity(taskKeywords, d.keywords) and filter out any candidate whose similarity is below this.config.similarityThreshold, then for the remaining items compute decayed = this._applyTimeDecay(d.confidence, d.createdAt), outcomeBonus via Outcome, and the final composite score (same weights: similarity*0.6 + decayed*0.25 + outcomeBonus*0.15) and sort by that score—this preserves the composite ordering but gates candidates by raw similarity.
176-199:⚠️ Potential issue | 🟠 MajorMake failed saves visible and crash-safe.
save()currently swallows every write failure and writes directly to the final file. That means callers can believe persistence succeeded when nothing was stored, and a crash duringwriteFileSync()can leavedecisions.jsontruncated. Please switch to temp-file + rename semantics and rethrow or emit a contextual error instead of silently returning.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 @.aiox-core/core/memory/decision-memory.js around lines 176 - 199, The save() method currently swallows write errors and writes directly to the final file; change it to write atomically using a temp file in the same directory (derive temp path from filePath), call fs.writeFileSync(tempPath, ...) then fs.renameSync(tempPath, filePath) to atomically replace decisions.json, and wrap the write/rename in a try/catch that cleans up the temp file on failure and rethrows or throws a new Error with context (include this.config.decisionsJsonPath, filePath and the original error) instead of silently returning so callers can observe failures; preserve the existing truncation protection by still slicing this.decisions to this.config.maxDecisions before serializing.
343-350:⚠️ Potential issue | 🟠 MajorEscape persisted text before building injected Markdown.
description,rationale, andoutcomeNotesare inserted verbatim into future task context. A stored value containing headings, fences, or prompt-like instructions can change the shape of the injected context instead of remaining reference data. Escape or neutralize those fields before appending them tolines.As per coding guidelines, "Look for potential security vulnerabilities."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/memory/decision-memory.js around lines 343 - 350, The loop that builds injected Markdown (for (const d of relevant) { ... lines.push(...) }) inserts d.description, d.rationale, and d.outcomeNotes verbatim which can break or manipulate future prompts; create and call a small sanitizer/escape helper (e.g., escapePersistedText or escapeMarkdown) and use it when constructing the lines (replace direct uses of d.description, d.rationale, d.outcomeNotes with escaped versions) to neutralize headings, code fences, backticks, angle-brackets and other prompt-like constructs before pushing them into lines; keep the existing outcome/category/confidence logic and apply the escape helper consistently wherever persisted text is interpolated.
🧹 Nitpick comments (1)
tests/core/memory/decision-memory.test.js (1)
325-381: Add regression cases for the risky injection and pattern branches.The new suite only exercises happy-path Markdown injection and pattern creation. Please add coverage for stored Markdown/control sequences and for three similar
pendingdecisions so the escaping and neutral-recommendation fixes stay locked in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/memory/decision-memory.test.js` around lines 325 - 381, Add two regression tests: one that records a decision containing Markdown/control characters (e.g., description with backticks, angle brackets, or ANSI sequences) then calls injectDecisionContext (use createMemory and mem.recordDecision / mem.injectDecisionContext) and asserts the injected context escapes/neutralizes those sequences (no raw Markdown rendered or control characters present) and still includes safe decision text; and another that records three similar decisions with status pending (use mem.recordDecision and mem.updateOutcome with Outcome.PENDING or equivalent) with nearly identical descriptions, then assert pattern detection/aggregation behaves safely (subscribe with mem.on Events.PATTERN_DETECTED and/or assert mem.getPatterns() shows a single neutral recommendation entry) to prevent duplicate or unsafe recommendations. Ensure tests reference createMemory, mem.recordDecision, mem.updateOutcome, mem.injectDecisionContext, mem.on, Events.PATTERN_DETECTED, and mem.getPatterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/memory/decision-memory.test.js`:
- Around line 3-9: Replace the brittle relative require with the repo's absolute
import alias: change the require that imports DecisionMemory, DecisionCategory,
Outcome, Events, CONFIG from "../../../.aiox-core/core/memory/decision-memory"
to the project's absolute path (e.g.,
require('aiox-core/core/memory/decision-memory') or the repo's configured
alias), ensuring the same exported symbols (DecisionMemory, DecisionCategory,
Outcome, Events, CONFIG) are imported.
---
Duplicate comments:
In @.aiox-core/core/memory/decision-memory.js:
- Around line 143-144: The class tracks a _loaded flag but public APIs (e.g.,
load(), query(), appendDecision(), getContext(), getDecisions(), save()) do not
enforce it; update these public methods in decision-memory.js to either eagerly
call/halt until load() completes or throw a clear error when _loaded is false,
ensuring you call the existing load() helper (or await internal hydration)
before returning results or mutating state; gate all entry points referenced in
the diff (the query APIs and append/save paths across the ranges noted) so that
no reads/writes happen on a not-yet-hydrated instance while preserving backwards
compatibility by failing fast with a descriptive message or performing
transparent lazy-awaiting of load().
- Around line 502-531: The pattern detection currently emits a negative
recommendation when outcomes.length === 0; change the recommendation computation
in the pattern creation block (where pattern is built before this.patterns.push
and Events.PATTERN_DETECTED is emitted) to return a neutral message like "No
resolved outcomes yet; hold judgment" when outcomes.length === 0, otherwise keep
the existing success/failure logic; additionally update the
updateOutcome(decisionId, ...) method to recompute affected patterns by locating
patterns whose relatedDecisionIds include the updated decision (or that match by
_keywordSimilarity/_extractKeywords), recalculating occurrences, successRate,
recommendation and re-emitting Events.PATTERN_DETECTED (or replacing/removing
patterns if metrics change) so patterns are not left with stale "underperformed"
recommendations once outcomes resolve.
- Around line 306-320: The current scoring pipeline computes a blended score
then filters by this composite value, which allows high confidence/outcome
bonuses to bypass weak keyword matches; change the logic in the candidates.map /
scoring block so you first compute similarity via
this._keywordSimilarity(taskKeywords, d.keywords) and filter out any candidate
whose similarity is below this.config.similarityThreshold, then for the
remaining items compute decayed = this._applyTimeDecay(d.confidence,
d.createdAt), outcomeBonus via Outcome, and the final composite score (same
weights: similarity*0.6 + decayed*0.25 + outcomeBonus*0.15) and sort by that
score—this preserves the composite ordering but gates candidates by raw
similarity.
- Around line 176-199: The save() method currently swallows write errors and
writes directly to the final file; change it to write atomically using a temp
file in the same directory (derive temp path from filePath), call
fs.writeFileSync(tempPath, ...) then fs.renameSync(tempPath, filePath) to
atomically replace decisions.json, and wrap the write/rename in a try/catch that
cleans up the temp file on failure and rethrows or throws a new Error with
context (include this.config.decisionsJsonPath, filePath and the original error)
instead of silently returning so callers can observe failures; preserve the
existing truncation protection by still slicing this.decisions to
this.config.maxDecisions before serializing.
- Around line 343-350: The loop that builds injected Markdown (for (const d of
relevant) { ... lines.push(...) }) inserts d.description, d.rationale, and
d.outcomeNotes verbatim which can break or manipulate future prompts; create and
call a small sanitizer/escape helper (e.g., escapePersistedText or
escapeMarkdown) and use it when constructing the lines (replace direct uses of
d.description, d.rationale, d.outcomeNotes with escaped versions) to neutralize
headings, code fences, backticks, angle-brackets and other prompt-like
constructs before pushing them into lines; keep the existing
outcome/category/confidence logic and apply the escape helper consistently
wherever persisted text is interpolated.
---
Nitpick comments:
In `@tests/core/memory/decision-memory.test.js`:
- Around line 325-381: Add two regression tests: one that records a decision
containing Markdown/control characters (e.g., description with backticks, angle
brackets, or ANSI sequences) then calls injectDecisionContext (use createMemory
and mem.recordDecision / mem.injectDecisionContext) and asserts the injected
context escapes/neutralizes those sequences (no raw Markdown rendered or control
characters present) and still includes safe decision text; and another that
records three similar decisions with status pending (use mem.recordDecision and
mem.updateOutcome with Outcome.PENDING or equivalent) with nearly identical
descriptions, then assert pattern detection/aggregation behaves safely
(subscribe with mem.on Events.PATTERN_DETECTED and/or assert mem.getPatterns()
shows a single neutral recommendation entry) to prevent duplicate or unsafe
recommendations. Ensure tests reference createMemory, mem.recordDecision,
mem.updateOutcome, mem.injectDecisionContext, mem.on, Events.PATTERN_DETECTED,
and mem.getPatterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b46f62c3-a778-4953-8a3c-89eb2ada7f65
📒 Files selected for processing (3)
.aiox-core/core/memory/decision-memory.js.aiox-core/install-manifest.yamltests/core/memory/decision-memory.test.js
| const { | ||
| DecisionMemory, | ||
| DecisionCategory, | ||
| Outcome, | ||
| Events, | ||
| CONFIG, | ||
| } = require('../../../.aiox-core/core/memory/decision-memory'); |
There was a problem hiding this comment.
Use the repository’s absolute import path here.
This deep relative require is brittle and violates the repo import rule. Please import the Decision Memory module through the project’s absolute path/alias instead.
As per coding guidelines, "Use absolute imports instead of relative imports in all code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/memory/decision-memory.test.js` around lines 3 - 9, Replace the
brittle relative require with the repo's absolute import alias: change the
require that imports DecisionMemory, DecisionCategory, Outcome, Events, CONFIG
from "../../../.aiox-core/core/memory/decision-memory" to the project's absolute
path (e.g., require('aiox-core/core/memory/decision-memory') or the repo's
configured alias), ensuring the same exported symbols (DecisionMemory,
DecisionCategory, Outcome, Events, CONFIG) are imported.
|
@Pedrovaleriolopez @oalanicolas, módulo de Decision Memory — permite que agentes aprendam com decisões passadas e melhorem ao longo de sessões. 126 testes unitários passando. É a base para os módulos de reflection engine e strategy optimizer. |
nikolasdehor
force-pushed
the
feat/decision-memory
branch
from
3c43e05 to
7e86ab4
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/core/memory/decision-memory.test.js (1)
370-381: Pattern deduplication assertion is weak and potentially confusing.The assertion
expect(patterns.length).toBeLessThanOrEqual(unique.size + 1)doesn't clearly verify that patterns aren't duplicated. If patterns are keyed by category, this test may pass vacuously. Consider asserting directly on pattern uniqueness:🔧 Suggested improvement
it('should not duplicate patterns', () => { const mem = createMemory({ patternThreshold: 3 }); for (let i = 0; i < 6; i++) { mem.recordDecision({ description: `Use retry pattern for service ${i}` }); } const patterns = mem.getPatterns(); - // Should not have multiple identical patterns - const unique = new Set(patterns.map(p => p.category)); - expect(patterns.length).toBeLessThanOrEqual(unique.size + 1); + // Verify no duplicate pattern entries (by checking unique identifiers or descriptions) + const patternKeys = patterns.map(p => `${p.category}:${p.description || p.keywords?.join(',')}`); + const uniqueKeys = new Set(patternKeys); + expect(patternKeys.length).toBe(uniqueKeys.size); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/memory/decision-memory.test.js` around lines 370 - 381, The test "should not duplicate patterns" uses a weak assertion; replace the current length check with a strict uniqueness assertion by verifying that no two patterns share the same identifying key (e.g., category) returned by mem.getPatterns(). Use mem.recordDecision to create entries, call mem.getPatterns(), then assert that patterns.length === new Set(patterns.map(p => p.category)).size (or equivalent) so duplicates by category are rejected; ensure you reference the patternThreshold value used when creating mem to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/memory/decision-memory.test.js`:
- Around line 201-213: The test currently compares different decisions (d vs d2)
so it doesn't assert that a SUCCESS increases a decision's own confidence;
change it to use the same decision: call createMemory(), recordDecision to get a
decision (e.g., d), capture its initial confidence (initial = d.confidence),
call mem.updateOutcome(d.id, Outcome.SUCCESS), then assert d.confidence is
greater than initial (and optionally bounded by the cap). Update references to
mem.recordDecision, mem.updateOutcome, d, and Outcome.SUCCESS accordingly.
---
Nitpick comments:
In `@tests/core/memory/decision-memory.test.js`:
- Around line 370-381: The test "should not duplicate patterns" uses a weak
assertion; replace the current length check with a strict uniqueness assertion
by verifying that no two patterns share the same identifying key (e.g.,
category) returned by mem.getPatterns(). Use mem.recordDecision to create
entries, call mem.getPatterns(), then assert that patterns.length === new
Set(patterns.map(p => p.category)).size (or equivalent) so duplicates by
category are rejected; ensure you reference the patternThreshold value used when
creating mem to keep behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b36b147-6e8b-4abf-86fe-610477af23b4
📒 Files selected for processing (3)
.aiox-core/core/memory/decision-memory.js.aiox-core/install-manifest.yamltests/core/memory/decision-memory.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- .aiox-core/core/memory/decision-memory.js
|
@Pedrovaleriolopez @oalanicolas, temos 7 features novas prontas (#564-#572, #575-#577) — Decision Memory, Reflection Engine, Proactive Sentinel, Strategy Optimizer, Time Travel, Swarm Intelligence, Cognitive Load Balancer, Predictive Pipeline, Immortality Protocol, Mesh Network. Tudo testado e rebased. Estamos investindo pesado no projeto e gostaríamos de ver essas contribuições sendo consideradas com a mesma prioridade que outros contribuidores receberam. |
…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.
- Move STOP_WORDS to module constant (perf) - Remove duplicate keywords from WORKFLOW category - Use config.similarityThreshold instead of hardcoded 0.4 - Ensure time decay never drops below minConfidence - Add try/catch to save() for disk errors - Sync in-memory decisions with maxDecisions cap on save - Reject Outcome.PENDING in updateOutcome() - Fix test that mutated internal state directly
Cobertura completa: constructor, isRateLimitError (10 cenários), calculateDelay (backoff exponencial + retryAfter + maxDelay), executeWithRetry (sucesso, retry, maxRetries, erros não-rate-limit), metrics, events, resetMetrics, formatStatus, withRateLimit wrapper e getGlobalManager singleton.
Cobertura: constructor, detectFileConflicts, assessConflictSeverity (8 padroes: critical/high/medium), suggestResolution, extractFilesFromOutput, aggregate (single wave, conflitos, history, eventos), aggregateAll.
- save() com guarda de concorrencia, escrita atomica (tmp+rename) e emissao de erro - _ensureLoaded() para lazy-load e prevenir overwrite de estado persistido - Filtro em similaridade bruta (similarity) em vez de score composto no getRelevantDecisions - _escapeMarkdown() para prevenir injecao de markdown/prompt no contexto injetado - Recomendacao neutra em patterns sem outcomes resolvidos - _recomputeRelatedPatterns() atualiza patterns quando outcomes mudam - updateOutcome rejeita PENDING via allowedOutcomes (sem duplicar validacao) - _applyTimeDecay com Math.max no resultado final - Operador ?? em vez de || para defaults (convencao do projeto) - Testes expandidos: 49 casos cobrindo novos comportamentos
nikolasdehor
force-pushed
the
feat/decision-memory
branch
from
7e86ab4 to
a10bf9e
Compare
github-actions
bot
added
area: agents
github-actions
bot
added
type: test
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.aiox-core/core/memory/decision-memory.js (2)
174-184:⚠️ Potential issue | 🟠 MajorWire
_ensureLoaded()into the public API before relying on it.Right now this helper is never called. A fresh instance can still serve empty results or overwrite an existing
decisions.jsonbecause the public entry points keep operating on the empty buffers until someone manually callsload().As per coding guidelines, "Ensure backwards compatibility — core modules are consumed by all agents."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/memory/decision-memory.js around lines 174 - 184, Public methods of the DecisionMemory class are operating on empty buffers because _ensureLoaded() is never invoked; modify each public entry point that reads or mutates state (e.g., methods like addDecision, getDecisions, getDecision, updateDecision, clearDecisions, save/persist) to call await this._ensureLoaded() at the very start so the persisted decisions are hydrated before any operation, keeping _ensureLoaded() idempotent (it already checks this._loaded) and preserving backwards compatibility.
218-220:⚠️ Potential issue | 🟠 MajorReject
save()when the write fails.Emitting
save:errorand then resolving makes persistence failures indistinguishable from success to callers. For a durability layer, failed writes need to reject with context.Suggested change
- } catch (err) { - this.emit('save:error', { error: err.message, path: filePath }); + } catch (err) { + const error = new Error(`Failed to save decision memory to ${filePath}: ${err.message}`); + error.cause = err; + this.emit('save:error', { error: error.message, path: filePath }); + throw error; } finally {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 @.aiox-core/core/memory/decision-memory.js around lines 218 - 220, The catch block in the save() method currently emits 'save:error' and then allows the function to resolve, hiding write failures; update the catch in the save() implementation to emit the error with context (as it does) and then rethrow the original error (or return a rejected Promise) so callers of save() observe the failure (include error and filePath in the emitted event and rethrow the same err object to preserve stack/context).
🧹 Nitpick comments (2)
tests/core/execution/result-aggregator.test.js (1)
17-17: Switch this test to the project's absolute import style.The relative
require()has the same brittleness and guideline violation as the other new test suite.As per coding guidelines, "Use absolute imports instead of relative imports in all code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/execution/result-aggregator.test.js` at line 17, Replace the relative require of ResultAggregator with the project's absolute import style: update the require call for ResultAggregator (the constant ResultAggregator in this test) to use the project's absolute module path instead of "../../../.aiox-core/core/execution/result-aggregator" so the test follows the codebase convention of absolute imports.tests/core/execution/rate-limit-manager.test.js (1)
13-14: Use the repo's absolute import convention for the module under test.This deep
../../../traversal is brittle to file moves and violates the project import rule.As per coding guidelines, "Use absolute imports instead of relative imports in all code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/execution/rate-limit-manager.test.js` around lines 13 - 14, Replace the brittle relative require('../../../.aiox-core/core/execution/rate-limit-manager') with the repository's absolute import for the module under test so tests use the canonical path; update the top of the file where RateLimitManager, withRateLimit and getGlobalManager are imported to require the absolute module path (e.g., require('core/execution/rate-limit-manager') or the project's configured absolute alias) instead of the deep ../../../ traversal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.aiox-core/core/memory/decision-memory.js:
- Around line 314-316: getRelevantDecisions currently passes taskDescription
straight into _extractKeywords which throws if taskDescription is
null/undefined; add an API-boundary normalization so taskDescription is coerced
to a string (e.g., taskDescription = taskDescription ?? '' and ensure it's a
string) before calling _extractKeywords and before any further processing so
callers get an empty keyword/result instead of a toLowerCase error; update
references to getRelevantDecisions and ensure injectDecisionContext will receive
the normalized '' when applicable.
- Around line 155-169: The catch currently treats any read failure as a
corrupted store and wipes this.decisions/this.patterns; change the error
handling in the block that reads filePath so you catch the error object (catch
(err)) and distinguish cases: if JSON.parse throws or the data is invalid (e.g.,
detect SyntaxError or missing/incorrect schemaVersion) then reset this.decisions
and this.patterns; if the file simply does not exist (ENOENT) treat as a
clean-empty startup and do not clear existing in-memory state unnecessarily; for
other FS errors (EACCES, EISDIR, etc.) rethrow or propagate/log the error
instead of wiping state so the caller can handle it and the store isn't marked
loaded incorrectly; keep checks around data.schemaVersion comparison and use the
symbols filePath, this.decisions, this.patterns, and this.config.schemaVersion
to locate and update the logic.
---
Duplicate comments:
In @.aiox-core/core/memory/decision-memory.js:
- Around line 174-184: Public methods of the DecisionMemory class are operating
on empty buffers because _ensureLoaded() is never invoked; modify each public
entry point that reads or mutates state (e.g., methods like addDecision,
getDecisions, getDecision, updateDecision, clearDecisions, save/persist) to call
await this._ensureLoaded() at the very start so the persisted decisions are
hydrated before any operation, keeping _ensureLoaded() idempotent (it already
checks this._loaded) and preserving backwards compatibility.
- Around line 218-220: The catch block in the save() method currently emits
'save:error' and then allows the function to resolve, hiding write failures;
update the catch in the save() implementation to emit the error with context (as
it does) and then rethrow the original error (or return a rejected Promise) so
callers of save() observe the failure (include error and filePath in the emitted
event and rethrow the same err object to preserve stack/context).
---
Nitpick comments:
In `@tests/core/execution/rate-limit-manager.test.js`:
- Around line 13-14: Replace the brittle relative
require('../../../.aiox-core/core/execution/rate-limit-manager') with the
repository's absolute import for the module under test so tests use the
canonical path; update the top of the file where RateLimitManager, withRateLimit
and getGlobalManager are imported to require the absolute module path (e.g.,
require('core/execution/rate-limit-manager') or the project's configured
absolute alias) instead of the deep ../../../ traversal.
In `@tests/core/execution/result-aggregator.test.js`:
- Line 17: Replace the relative require of ResultAggregator with the project's
absolute import style: update the require call for ResultAggregator (the
constant ResultAggregator in this test) to use the project's absolute module
path instead of "../../../.aiox-core/core/execution/result-aggregator" so the
test follows the codebase convention of absolute imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84039e1b-c8fb-46f8-8fea-d2568bc4fb8f
📒 Files selected for processing (5)
.aiox-core/core/memory/decision-memory.js.aiox-core/install-manifest.yamltests/core/execution/rate-limit-manager.test.jstests/core/execution/result-aggregator.test.jstests/core/memory/decision-memory.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/core/memory/decision-memory.test.js
- .aiox-core/install-manifest.yaml
| try { | ||
| if (fs.existsSync(filePath)) { | ||
| const raw = fs.readFileSync(filePath, 'utf-8'); | ||
| const data = JSON.parse(raw); | ||
|
|
||
| if (data.schemaVersion === this.config.schemaVersion) { | ||
| this.decisions = data.decisions ?? []; | ||
| this.patterns = data.patterns ?? []; | ||
| } | ||
| } | ||
| } catch { | ||
| // Corrupted file — start fresh | ||
| this.decisions = []; | ||
| this.patterns = []; | ||
| } |
There was a problem hiding this comment.
Only reset to an empty store for actual corruption.
This catch currently treats every read failure as "corrupted file". Permission/path I/O errors will silently clear in-memory history and still mark the store as loaded, which can later cascade into an empty-state save.
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 @.aiox-core/core/memory/decision-memory.js around lines 155 - 169, The catch
currently treats any read failure as a corrupted store and wipes
this.decisions/this.patterns; change the error handling in the block that reads
filePath so you catch the error object (catch (err)) and distinguish cases: if
JSON.parse throws or the data is invalid (e.g., detect SyntaxError or
missing/incorrect schemaVersion) then reset this.decisions and this.patterns; if
the file simply does not exist (ENOENT) treat as a clean-empty startup and do
not clear existing in-memory state unnecessarily; for other FS errors (EACCES,
EISDIR, etc.) rethrow or propagate/log the error instead of wiping state so the
caller can handle it and the store isn't marked loaded incorrectly; keep checks
around data.schemaVersion comparison and use the symbols filePath,
this.decisions, this.patterns, and this.config.schemaVersion to locate and
update the logic.
| getRelevantDecisions(taskDescription, options = {}) { | ||
| const limit = options.limit ?? this.config.maxInjectedDecisions; | ||
| const taskKeywords = this._extractKeywords(taskDescription); |
There was a problem hiding this comment.
Validate taskDescription at the API boundary.
getRelevantDecisions() forwards whatever it gets into _extractKeywords(), so null/undefined currently fail with a low-level toLowerCase error. Normalizing empty input here would also let injectDecisionContext() degrade to '' instead of throwing.
As per coding guidelines, "Check for proper input validation on public API methods."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/core/memory/decision-memory.js around lines 314 - 316,
getRelevantDecisions currently passes taskDescription straight into
_extractKeywords which throws if taskDescription is null/undefined; add an
API-boundary normalization so taskDescription is coerced to a string (e.g.,
taskDescription = taskDescription ?? '' and ensure it's a string) before calling
_extractKeywords and before any further processing so callers get an empty
keyword/result instead of a toLowerCase error; update references to
getRelevantDecisions and ensure injectDecisionContext will receive the
normalized '' when applicable.
Summary
Implementa o Decision Memory — módulo de memória de decisões cross-session para agentes AIOX. Story 9.5 do Epic 9 (Persistent Memory Layer), parte da Phase 2 do Agent Immortality Protocol (#482).
Motivação
Hoje o AIOX tem apenas
gotchas-memory.jsna camada de memória — o agente aprende a evitar erros, mas não aprende com decisões bem-sucedidas. O Decision Memory fecha essa lacuna: agentes agora podem consultar decisões passadas antes de tomar novas, aprendendo com experiência.Features
.aiox/decisions.json, sobrevive a crashes e reiníciosComo funciona
Relação com outros componentes
Arquivos
.aiox-core/core/memory/decision-memory.jstests/core/memory/decision-memory.test.jsTest plan
Summary by CodeRabbit
New Features
Tests
Chore