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 2 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. |
WalkthroughAdds a persistent Decision Memory module that records decisions with metadata, computes relevance via keyword similarity and time-decay, supports outcome updates and pattern detection, emits lifecycle events, and persists state to Changes
Sequence DiagramsequenceDiagram
participant Agent as AI 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: persist (save queued)
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: persist (save queued)
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: fetch relevant decisions, format markdown block
DM->>Events: emit DECISIONS_INJECTED
DM-->>Agent: contextual block
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
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
| async save() { | ||
| const filePath = path.resolve(this.projectRoot, this.config.decisionsJsonPath); | ||
| const dir = path.dirname(filePath); | ||
|
|
||
| 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.
Missing error handling and potential race condition in save().
The save() method performs synchronous file operations without try/catch. If the disk is full, permissions are denied, or another I/O error occurs, this will throw an unhandled exception. Additionally, concurrent save() calls could interleave writes and corrupt the file.
Per coding guidelines for core modules: "Verify error handling is comprehensive with proper try/catch and error context."
🛡️ Recommended fix with error handling and basic locking
async save() {
+ if (this._saving) {
+ // Prevent concurrent writes
+ return;
+ }
+ this._saving = true;
+
const filePath = path.resolve(this.projectRoot, this.config.decisionsJsonPath);
const dir = path.dirname(filePath);
- if (!fs.existsSync(dir)) {
- fs.mkdirSync(dir, { recursive: true });
- }
+ 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,
- };
+ 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');
+ fs.writeFileSync(filePath, JSON.stringify(data, null, 2), 'utf-8');
+ } catch (error) {
+ this.emit('save:error', { error: error.message, path: filePath });
+ throw error;
+ } finally {
+ this._saving = false;
+ }
}🤖 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 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.
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.
| // Score by keyword similarity + confidence with time decay | ||
| const scored = candidates.map(d => { | ||
| const similarity = this._keywordSimilarity(taskKeywords, d.keywords); | ||
| const decayed = this._applyTimeDecay(d.confidence, d.createdAt); | ||
| const outcomeBonus = d.outcome === Outcome.SUCCESS ? 0.2 : | ||
| d.outcome === Outcome.FAILURE ? 0.1 : 0; // Failures are also valuable to learn from | ||
|
|
||
| return { | ||
| decision: d, | ||
| score: (similarity * 0.6) + (decayed * 0.25) + (outcomeBonus * 0.15), | ||
| }; | ||
| }); | ||
|
|
||
| return scored | ||
| .filter(s => s.score >= this.config.similarityThreshold) | ||
| .sort((a, b) => b.score - a.score) |
There was a problem hiding this comment.
Filter on raw similarity before adding ranking bonuses.
Line 325 applies similarityThreshold to the blended score, even though the config describes it as minimum keyword overlap. A very successful or high-confidence decision can pass with only a trivial token match and get injected into unrelated tasks. Keep the composite score for ranking, but gate candidates on similarity itself.
Possible fix
const scored = candidates.map(d => {
const similarity = this._keywordSimilarity(taskKeywords, d.keywords);
const decayed = this._applyTimeDecay(d.confidence, d.createdAt);
const outcomeBonus = d.outcome === Outcome.SUCCESS ? 0.2 :
d.outcome === Outcome.FAILURE ? 0.1 : 0; // Failures are also valuable to learn from
return {
decision: d,
+ similarity,
score: (similarity * 0.6) + (decayed * 0.25) + (outcomeBonus * 0.15),
};
});
return scored
- .filter(s => s.score >= this.config.similarityThreshold)
+ .filter(s => s.similarity >= this.config.similarityThreshold)
.sort((a, b) => b.score - a.score)🤖 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 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).
| lines.push(`### ${outcomeIcon} ${d.description}`); | ||
| if (d.rationale) lines.push(`**Rationale:** ${d.rationale}`); | ||
| if (d.outcomeNotes) lines.push(`**Outcome:** ${d.outcomeNotes}`); | ||
| lines.push(`**Category:** ${d.category} | **Confidence:** ${Math.round(this._applyTimeDecay(d.confidence, d.createdAt) * 100)}%`); |
There was a problem hiding this comment.
Escape persisted fields before building the injected Markdown block.
description, rationale, and outcomeNotes are inserted verbatim into future task context. A crafted value with headings, code fences, or instruction text can change the structure of the injected prompt instead of remaining reference material. Escape Markdown/control sequences or wrap these fields in a neutral container before joining the lines.
🤖 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 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.
| if (similar.length >= this.config.patternThreshold - 1) { | ||
| const outcomes = similar.map(d => d.outcome).filter(o => o !== Outcome.PENDING); | ||
| const successCount = outcomes.filter(o => o === Outcome.SUCCESS).length; | ||
| const failureCount = outcomes.filter(o => o === Outcome.FAILURE).length; | ||
|
|
||
| const pattern = { | ||
| id: `pattern-${this.patterns.length + 1}`, | ||
| category: newDecision.category, | ||
| description: `Recurring ${newDecision.category} decision: "${newDecision.description}"`, | ||
| occurrences: similar.length + 1, | ||
| successRate: outcomes.length > 0 ? successCount / outcomes.length : 0, | ||
| recommendation: successCount > failureCount | ||
| ? 'This approach has historically worked well. Consider reusing.' | ||
| : 'This approach has historically underperformed. Consider alternatives.', | ||
| detectedAt: new Date().toISOString(), | ||
| relatedDecisionIds: [...similar.map(d => d.id), newDecision.id], | ||
| }; | ||
|
|
||
| // Avoid duplicate patterns | ||
| const exists = this.patterns.some(p => | ||
| p.category === pattern.category && | ||
| this._keywordSimilarity( | ||
| this._extractKeywords(p.description), | ||
| this._extractKeywords(pattern.description), | ||
| ) > 0.6, | ||
| ); | ||
|
|
||
| if (!exists) { | ||
| this.patterns.push(pattern); | ||
| this.emit(Events.PATTERN_DETECTED, pattern); | ||
| } |
There was a problem hiding this comment.
Don't emit an "underperformed" pattern when there is no outcome data yet.
When the threshold is hit before any similar decision leaves pending, outcomes.length is 0, successCount > failureCount is false, and the pattern gets a negative recommendation with no evidence. Because patterns are append-only and duplicates are skipped later, that wrong recommendation can persist indefinitely. Use a neutral recommendation until resolved outcomes exist, or recalculate existing patterns when related 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 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.
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. |
…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
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
| 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; | ||
|
|
||
| // Record a fresh decision and mark it as success | ||
| const d2 = mem.recordDecision({ description: 'Enable compression v2' }); | ||
| mem.updateOutcome(d2.id, Outcome.SUCCESS); | ||
| expect(d2.confidence).toBeGreaterThan(afterFailure); | ||
| }); |
There was a problem hiding this comment.
Test logic may not verify the intended behavior.
This test compares d2.confidence (a fresh decision marked SUCCESS) against afterFailure (the confidence of a different decision d after failure). This demonstrates that successful decisions have higher confidence than failed ones, but doesn't actually verify that success increases confidence from its initial value.
Consider restructuring to test the same decision:
🔧 Suggested fix
- it('should increase confidence on success (up to cap)', () => {
+ it('should maintain or increase confidence on success', () => {
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;
-
- // Record a fresh decision and mark it as success
- const d2 = mem.recordDecision({ description: 'Enable compression v2' });
- mem.updateOutcome(d2.id, Outcome.SUCCESS);
- expect(d2.confidence).toBeGreaterThan(afterFailure);
+ const d = mem.recordDecision({ description: 'Enable compression' });
+ const initial = d.confidence;
+
+ mem.updateOutcome(d.id, Outcome.SUCCESS);
+ expect(d.confidence).toBeGreaterThanOrEqual(initial);
});🤖 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 - 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.
|
@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. |
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