feat: update index#6
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe pull request introduces the Phase 3 Company Memory Engine, including reconciled memory storage, Temporal processing workflows, configurable scoring and conflict handling, organization-scoped APIs, and web pages for browsing memories, timelines, changes, and conflicts. ChangesCompany Memory Engine
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MemoryExplorer
participant MemoryAPI
participant MemoryService
participant Temporal
participant MemoryActivities
participant Prisma
MemoryExplorer->>MemoryAPI: request memory list, stats, or rebuild
MemoryAPI->>MemoryService: validate and execute organization-scoped operation
MemoryService->>Prisma: query memory data and aggregates
MemoryService->>Temporal: start memoryUpdateWorkflow
Temporal->>MemoryActivities: collect, apply, merge, timeline, conflict, score
MemoryActivities->>Prisma: persist reconciled memory state
MemoryActivities-->>Temporal: return stage statistics
Temporal-->>MemoryService: return workflow identifiers or progress
MemoryService-->>MemoryAPI: return response payload
MemoryAPI-->>MemoryExplorer: render memory data or rebuild status
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (22)
apps/api/src/modules/memory/memory.schemas.ts-58-62 (1)
58-62: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire
valueforchoice: 'custom'.resolveConflict()coerces a missingvaluetonull, so{ choice: 'custom' }can persist an unintended null resolution. Model the custom branch separately and keepvaluerequired there so explicitnullstill works.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/memory/memory.schemas.ts` around lines 58 - 62, Update resolveConflictBodySchema to use a discriminated union for choice: keep latest and previous branches with optional or absent value as appropriate, and define the custom branch with value required as z.unknown() so explicit null remains valid. Ensure resolveConflict() can no longer receive a custom choice without a value.apps/api/src/modules/memory/memory.schemas.ts-11-16 (1)
11-16: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate the memory filters against the real Prisma enums.
enumStringaccepts any all-caps token, but these values are passed straight into Prisma withas never, so unsupported inputs fail at query time instead of as a 422. Usez.nativeEnum(...)or explicitz.enum(...)per field. Same applies to thetimeline,changes, andconflictsquery schemas.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/memory/memory.schemas.ts` around lines 11 - 16, The filter schemas currently accept arbitrary uppercase strings instead of validating against Prisma enum values. Replace enumString usage in listMemoryQuerySchema and the corresponding timeline, changes, and conflicts query schemas with z.nativeEnum or explicit z.enum definitions tied to each field’s actual Prisma enum, so unsupported filters are rejected during request validation.apps/api/src/modules/memory/memory.service.ts-339-395 (1)
339-395: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftGuard conflict resolution with an OPEN check and version snapshot.
resolveConflict()updates byidonly, so the same conflict can be resolved twice.applyAttribute()readsmemory.attributesbefore the transaction, which can clobber concurrent updates.- Manual resolutions don’t write a
MemoryVersion, so/changeswill miss them.Move the memory read/update and the version write into one interactive transaction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/memory/memory.service.ts` around lines 339 - 395, Update resolveConflict() to use one interactive Prisma transaction that re-fetches the conflict with status OPEN and validates its version snapshot before resolving it. Move applyAttribute()’s memory read and update into that transaction to avoid overwriting concurrent changes, and create the corresponding MemoryVersion within the same transaction so manual resolutions appear in /changes. Ensure the conflict update is guarded against repeated resolution and return the transaction’s updated conflict.apps/web/src/app/(app)/memory/conflicts/page.tsx-53-62 (1)
53-62: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep the conflict busy until refresh completes.
load()is not returned or awaited, sobusyclears while the resolved conflict still appears OPEN, permitting duplicate submissions.Proposed fix
const load = useCallback(() => { - memoryApi + return memoryApi .listConflicts({ status, limit: 100 }) .then(setData) .catch((err) => setError(err instanceof Error ? err.message : 'Failed to load conflicts')); }, [status]); await memoryApi.resolveConflict(id, { choice }); -load(); +await load();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/`(app)/memory/conflicts/page.tsx around lines 53 - 62, Update resolve so it awaits the load() refresh after memoryApi.resolveConflict succeeds, keeping the conflict busy until refreshed data has completed loading. Preserve the existing error handling and setBusy(null) cleanup in the finally block.packages/memory-engine/src/scoring.ts-82-83 (1)
82-83: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle non-positive frequency saturation before dividing.
With zero events and
frequencySaturation = 0, Line 83 evaluates0 / 0, making bothfrequencyandcompositeNaN. Validate saturation or explicitly return zero for zero events before division.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/memory-engine/src/scoring.ts` around lines 82 - 83, Update the frequency calculation near the scoring function’s saturation logic to handle non-positive frequency saturation before division. Return a frequency of zero when the effective event count is zero, and validate or otherwise safely handle non-positive saturation for positive counts so neither frequency nor composite becomes NaN.apps/api/prisma/schema.prisma-1336-1395 (1)
1336-1395: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the memory dedupe index partial
@@unique([organizationId, memoryType, dedupeKey])is still a plain unique constraint, and the migration creates the same plain unique index, so MERGED/ARCHIVED/EXPIRED/soft-deleted rows keep blocking a replacement ACTIVE memory. Move this to a partial unique index for ACTIVE rows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/prisma/schema.prisma` around lines 1336 - 1395, Replace the plain @@unique constraint on Memory with a partial unique index that enforces uniqueness only for rows with status ACTIVE and deletedAt IS NULL. Update the corresponding migration/index definition as well, while preserving uniqueness across organizationId, memoryType, and dedupeKey for eligible memories.packages/memory-engine/src/reconciliation.ts-210-218 (1)
210-218: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not combine provenance fields from different observations.
A lower-confidence or older incoming reinforcement still replaces
source, while confidence and timestamp may remain fromcurrent. This creates a fabricated observation such as Slack provenance carrying Document confidence, affecting source trust and downstream scoring.Keep one complete
Provenancedrecord, or model reinforcement metadata separately rather than mixing its fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/memory-engine/src/reconciliation.ts` around lines 210 - 218, The same-value branch in the reconciliation logic must not mix provenance fields from separate observations. Update the merge handling around valuesEqual to retain one complete Provenanced record—prefer the observation whose confidence and timestamp are selected—or model reinforcement metadata separately, ensuring source, confidence, timestamp, and other provenance fields remain internally consistent.apps/api/prisma/migrations/20260716110000_company_memory_engine/migration.sql-273-289 (1)
273-289: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftEnforce organization ownership in relational foreign keys.
These ID-only foreign keys permit, for example, an organization A conflict or version to reference an organization B memory. Organization-scoped reads would then expose inconsistent tenant data, and the self-referencing merge constraint has the same gap.
Add unique parent keys such as
(id, organizationId)and composite foreign keys carryingorganizationIdfor every tenant-owned relationship.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/prisma/migrations/20260716110000_company_memory_engine/migration.sql` around lines 273 - 289, Update the migration’s tenant-owned relationship constraints, including memories_mergedIntoId_fkey, memory_versions_memoryId_fkey, memory_events_memoryId_fkey, memory_timeline_events_timelineId_fkey, conflict_records_memoryId_fkey, and memory_scores_memoryId_fkey, to use composite keys carrying organizationId. Add the corresponding unique parent keys such as (id, organizationId) on referenced tenant tables, and ensure each child foreign key references both the related ID and organizationId while preserving the existing delete/update actions.packages/memory-engine/src/timeline.ts-111-119 (1)
111-119: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBase timeline idempotency on
eventIdand canonicalize the fallback.Two distinct events with the same entity, source, timestamp, document, and changes currently collide, so the database unique index drops one. Conversely, equivalent change objects with different key insertion orders produce different fallback hashes.
Use
eventIdas the source-event identity when available, and sort change keys for drafts without one.Proposed fix
- const dedupeHash = stableHash( - input.entityId, - type, - input.source, - input.occurredAt, - input.documentId, - changesSummary, - ); + const dedupeHash = input.eventId + ? stableHash(input.entityId, input.eventId) + : stableHash( + input.entityId, + type, + input.source, + input.occurredAt, + input.documentId, + changesSummary, + );Also iterate over sorted entries in
describeChanges.Also applies to: 165-172
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/memory-engine/src/timeline.ts` around lines 111 - 119, Update the dedupeHash construction in the timeline event flow to use eventId as the source-event identity when present, preventing distinct events from colliding. For drafts without an eventId, retain the fallback fields but canonicalize changesSummary by sorting change keys. Update describeChanges to iterate over sorted entries so equivalent change objects produce identical hashes.packages/memory-engine/src/reconciliation.ts-179-185 (1)
179-185: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCompare structured values independently of object key order.
JSON.stringifytreats{a: 1, b: 2}and{b: 2, a: 1}as different, causing false conflicts and unnecessary memory versions when equivalent JSON arrives with different property ordering.Proposed direction
function valuesEqual(a: unknown, b: unknown): boolean { if (a === b) return true; if (typeof a === 'string' && typeof b === 'string') { return a.trim().toLowerCase() === b.trim().toLowerCase(); } - return JSON.stringify(a) === JSON.stringify(b); + return canonicalJson(a) === canonicalJson(b); }Use a recursive canonicalizer that sorts object keys while preserving array order.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/memory-engine/src/reconciliation.ts` around lines 179 - 185, Update valuesEqual to compare structured values using a recursive canonicalizer that sorts object keys while preserving array order, then compare the canonical results. Keep the existing identity and case-insensitive trimmed string handling, and ensure objects with identical properties in different insertion orders are treated as equal.packages/activities/src/memory-engine.activities.ts-837-842 (1)
837-842: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist conflicts discovered while merging duplicate memories.
reconcileAttributescan return conflicts here, but they are ignored and the loser is deleted. This silently resolves cross-source disagreements without creating reviewableConflictRecordrows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/activities/src/memory-engine.activities.ts` around lines 837 - 842, Update the duplicate-memory merge flow around reconcileAttributes so any returned conflicts are persisted as reviewable ConflictRecord rows before the loser memory is deleted. Reuse the existing conflict persistence mechanism and ensure conflicts from the existing and incoming attributes are not discarded while preserving the current reconciliation behavior.packages/activities/src/memory-engine.activities.ts-549-556 (1)
549-556: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not report a successful workflow after event failures.
This catch converts errors into
failedstatistics, but the workflow only loops onprocessedand never checksfailed; it therefore continues and finalizes the run as successful. Propagate a batch failure or make the workflow fail whenfailed > 0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/activities/src/memory-engine.activities.ts` around lines 549 - 556, Update the workflow’s event-processing flow around the catch block that increments stats.failed so failures are propagated or explicitly cause the workflow to fail when failed > 0. Ensure the finalization path cannot report a successful run after any memory event application fails, while preserving the existing failed-event status update.packages/activities/src/memory-engine.activities.ts-945-965 (1)
945-965: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHonor MANUAL and SOURCE_PRIORITY during post-resolution.
MANUALconflicts can currently be auto-resolved, whileSOURCE_PRIORITYfalls through to timestamp comparison. Skip automatic resolution forMANUALand reuse the domain conflict resolver for every other strategy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/activities/src/memory-engine.activities.ts` around lines 945 - 965, Update the post-resolution logic around clearWinner and the conflictRecord update to skip conflicts using the MANUAL strategy. For all other strategies, determine the winning value through the existing domain conflict resolver so SOURCE_PRIORITY and other strategies are handled consistently instead of falling back to timestamp comparison; preserve the existing AUTO_RESOLVED persistence behavior.packages/activities/src/memory-engine.activities.ts-321-343 (1)
321-343: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake all bounded stages resumable.
The tuning limits currently truncate stable queries without cursors, so each run can revisit the same rows while permanently starving the remainder.
packages/activities/src/memory-engine.activities.ts#L321-L343: paginate objects and mentions, preserving continuation across collection rounds.packages/activities/src/memory-engine.activities.ts#L895-L900: paginate timeline rebuilding so every timeline is eventually refreshed.packages/activities/src/memory-engine.activities.ts#L938-L941: rotate or cursor past unresolved conflicts so later records are examined.packages/activities/src/memory-engine.activities.ts#L978-L982: paginate scoring so decaying scores are eventually recomputed for every active memory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/activities/src/memory-engine.activities.ts` around lines 321 - 343, Make every bounded memory-engine stage resumable instead of repeatedly selecting the same first page: in packages/activities/src/memory-engine.activities.ts lines 321-343, add stable cursor pagination for objects and their mentions across collection rounds; in lines 895-900, paginate timeline rebuilding so each timeline is eventually refreshed; in lines 938-941, rotate or cursor past unresolved conflicts; and in lines 978-982, paginate scoring so every active memory is eventually recomputed. Preserve the existing tuning limits and processing behavior while carrying continuation state between rounds.packages/activities/src/memory-engine.activities.ts-299-306 (1)
299-306: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMerge entity state instead of replacing it with one memory’s attributes.
Every event overwrites
currentState; an episodic mention with empty attributes therefore clears status, priority, and assignee previously derived from semantic memory. Merge into the existing entity state and avoid regressinglastEventAt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/activities/src/memory-engine.activities.ts` around lines 299 - 306, Update the entity-state persistence logic around the currentState update to merge attrs into the existing currentState rather than replacing it, preserving previously derived status, priority, and assignee when attrs are empty or partial. Ensure lastEventAt only advances and cannot regress when processing an older event, while retaining the version increment and other update behavior.packages/activities/src/memory-engine.activities.ts-847-888 (1)
847-888: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSnapshot the merged values rather than the stale survivor values.
The memory row receives recalculated confidence and importance, but the new version snapshot stores the pre-merge values from
survivor. Compute both once and use them in the update and snapshot.Proposed fix
const sources = distinctSources(reconciled.merged); const version = survivor.version + 1; + const confidence = aggregateConfidence(reconciled.merged, sources); + const importance = Math.max(survivor.importance, loser.importance); await tx.memory.update({ ... - confidence: aggregateConfidence(reconciled.merged, sources), - importance: Math.max(survivor.importance, loser.importance), + confidence, + importance, ... - confidence: survivor.confidence, - importance: survivor.importance, + confidence, + importance,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/activities/src/memory-engine.activities.ts` around lines 847 - 888, In the merge flow, compute the reconciled confidence and importance once before the survivor update, then reuse those values in both the tx.memory.update call and the snapshot invocation. Replace the snapshot’s stale survivor.confidence and survivor.importance references while preserving the recalculated merged attributes and existing update behavior.packages/activities/src/memory-engine.activities.ts-363-401 (1)
363-401: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the object’s creation event when its current version exceeds one.
On first collection of an already-updated object, Line 365 emits an
UPDATEDevent atcreatedAt, followed by another update event. The timeline never receives its creation event.Proposed fix
- type: obj.version > 1 ? 'KNOWLEDGE_OBJECT_UPDATED' : 'KNOWLEDGE_OBJECT_CREATED', + type: 'KNOWLEDGE_OBJECT_CREATED',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/activities/src/memory-engine.activities.ts` around lines 363 - 401, Update the initial event construction in the object collection flow so it always emits the object’s creation event at createdAt, including when obj.version exceeds one; reserve KNOWLEDGE_OBJECT_UPDATED for the later updatedAt event guarded by the existing version check. Preserve the existing payload and ordering while changing the initial event type selection.packages/activities/src/memory-engine.activities.ts-658-676 (1)
658-676: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winVersion summary-only changes.
changedonly considers reconciled attributes and conflicts, so an updated knowledge-object summary is discarded when its attributes remain unchanged. Compare the incoming summary independently, update it, and increment/snapshot the version when it changes.Proposed direction
- changed = reconciled.changed.length > 0 || reconciled.conflicts.length > 0; + const summary = payload.summary ?? existing.summary; + const summaryChanged = summary !== existing.summary; + changed = + summaryChanged || + reconciled.changed.length > 0 || + reconciled.conflicts.length > 0; ... - const summary = payload.summary && changed ? payload.summary : existing.summary;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/activities/src/memory-engine.activities.ts` around lines 658 - 676, The update flow around changed, summary, and version must detect summary-only changes. Compare payload.summary with existing.summary independently, use that result to update changed and increment the version, and preserve the existing summary when no incoming summary change is present.services/temporal-worker/src/config.ts-116-123 (1)
116-123: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate
MEMORY_SCORE_WEIGHTSat runtime.JSON.parsehere still accepts arrays,null, extra keys, and nonnumeric or negative values, and those can flow into scoring asNaNor unintended weights. Use a strict Zod object with the five finite, nonnegative numeric fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/temporal-worker/src/config.ts` around lines 116 - 123, The parseScoreWeights function currently accepts invalid JSON shapes and score values. Replace the unchecked JSON.parse result with strict Zod validation requiring exactly the five score-weight fields, each a finite nonnegative number; preserve the existing invalid-input error behavior and return the validated MemoryTuning['scoreWeights'] value.packages/activities/src/memory-engine.activities.ts-518-542 (1)
518-542: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAtomically claim pending events before applying them. Concurrent runs can read the same
PENDINGrows before either one marks themPROCESSED, so both may reconcile the same event and duplicate references, versions, or conflicts. Claim each row first with a conditional status transition, or enforce one active organization run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/activities/src/memory-engine.activities.ts` around lines 518 - 542, Update applyMemoryEvents to atomically claim each event before calling applyOne: conditionally transition the row from PENDING to an in-progress status and continue only when that update succeeds. Use the event identifier and organization scope in the conditional update, preserving processing for uniquely claimed rows and preventing concurrent runs from applying the same event.packages/activities/src/memory-engine.activities.ts-594-677 (1)
594-677: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftHandle terminal memories before the normal update path
findUniquecan returnMERGED,EXPIRED, orARCHIVEDrows, and this code updates them in place without restoring visibility. Merged-loser dedupe keys also stay attached to the hidden row instead of the survivor, so define redirect/reactivation behavior explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/activities/src/memory-engine.activities.ts` around lines 594 - 677, Handle terminal statuses immediately after the findUnique lookup, before the normal create/update branches. For MERGED memories, resolve the dedupe key to the surviving memory and apply the observation there rather than updating the hidden loser; for EXPIRED or ARCHIVED memories, explicitly reactivate them or create a replacement according to the intended visibility rules. Ensure dedupe keys no longer remain attached only to merged losers, and preserve the existing reconciliation flow for active memories.packages/workflows/src/workflows/memory-engine.workflow.ts-115-123 (1)
115-123: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not report success unless the event queue is confirmed drained.
After 25 non-empty batches, execution proceeds to later stages and returns
COMPLETEDeven though pending events may remain. Extend the activity result withhasMore/remaining, then fail or continue processing when the cap is reached without confirmed drainage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workflows/src/workflows/memory-engine.workflow.ts` around lines 115 - 123, Update the memory-event processing loop in the workflow to track whether the queue is fully drained, using the ingest result’s hasMore or remaining indicator alongside processed counts. When MAX_APPLY_ROUNDS is reached with pending events, do not proceed to later stages or return COMPLETED; instead continue processing or return the established failure/incomplete outcome, while preserving normal completion when drainage is confirmed.
🟡 Minor comments (8)
apps/api/src/modules/memory/memory.service.ts-463-487 (1)
463-487: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not let malformed observability data fail the stats endpoint.
An invalid Redis value makes
JSON.parsethrow and turns the entire statistics request into a 500. Parse defensively and returnnullor a structured unavailable state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/memory/memory.service.ts` around lines 463 - 487, Update the statistics response’s processingStatus handling around lastRunRaw so malformed Redis data cannot throw from JSON.parse. Catch parsing failures and return null (or the existing structured unavailable representation), while preserving valid JSON parsing and the rest of the stats response.apps/api/src/modules/memory/memory.service.ts-100-112 (1)
100-112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude soft-deleted memories from detail lookups.
Unlike listing and entity queries, this lookup omits
deletedAt: null, so a removed memory remains accessible by its ID.- where: { id, organizationId }, + where: { id, organizationId, deletedAt: null },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/memory/memory.service.ts` around lines 100 - 112, Update the Prisma query in getMemory to require deletedAt to be null alongside id and organizationId, ensuring soft-deleted memories are not returned while preserving the existing includes and not-found behavior.apps/web/src/components/memory/util.ts-3-25 (1)
3-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse accessible badge color pairs.
Several backgrounds, including
ORGANIZATIONAL,WORKING,KNOWLEDGE, andGIT, lack sufficient contrast with the fixedtext-whiteused by consumers. Darken these colors or expose a matching foreground color.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/memory/util.ts` around lines 3 - 25, Update the color definitions in MEMORY_TYPE_COLOR and SOURCE_COLOR so ORGANIZATIONAL, WORKING, KNOWLEDGE, and GIT provide sufficient contrast with consumers’ fixed text-white foreground; prefer darkening those background values while preserving the existing color-map API and all other mappings.apps/web/src/app/(app)/memory/page.tsx-149-177 (1)
149-177: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the call-to-action match its destination.
“View entity timeline” is inside the card link to
/memory/${m.id}, so clicking it opens memory details. Rename it to “View memory details” or render a separate link to/memory/entity/${m.entityId}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/`(app)/memory/page.tsx around lines 149 - 177, Update the entityId call-to-action inside the memory card rendered by the memories map so its label matches the existing `/memory/${m.id}` destination; rename “View entity timeline” to “View memory details” and preserve the current conditional rendering and styling.apps/web/src/app/(app)/memory/page.tsx-39-47 (1)
39-47: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear stale request errors before retrying.
Each loader only sets
erroron rejection. After a filter, range, status, or route change succeeds, the previous error remains rendered.
apps/web/src/app/(app)/memory/page.tsx#L39-L47: clear the list error when starting or successfully completingload().apps/web/src/app/(app)/memory/[id]/page.tsx#L37-L41: clear the previous detail error whenidchanges.apps/web/src/app/(app)/memory/changes/page.tsx#L20-L25: clear the error when loading another date range.apps/web/src/app/(app)/memory/conflicts/page.tsx#L44-L48: clear the error when changing status or refreshing after resolution.apps/web/src/app/(app)/memory/entity/[id]/page.tsx#L20-L26: clear the previous entity error whenidchanges.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/`(app)/memory/page.tsx around lines 39 - 47, Clear stale errors before each relevant memory request: in apps/web/src/app/(app)/memory/page.tsx lines 39-47, reset the list error at the start or successful completion of load(); in apps/web/src/app/(app)/memory/[id]/page.tsx lines 37-41 and apps/web/src/app/(app)/memory/entity/[id]/page.tsx lines 20-26, reset the detail/entity error when id changes; in apps/web/src/app/(app)/memory/changes/page.tsx lines 20-25, reset the error when loading a new date range; and in apps/web/src/app/(app)/memory/conflicts/page.tsx lines 44-48, reset it when status changes or refresh follows resolution.apps/web/src/app/(app)/memory/conflicts/page.tsx-105-107 (1)
105-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHighlight previous-value resolutions.
The previous side is always passed
active={false}, soPREVIOUS_WINSresolutions are displayed incorrectly.-<Side label="Previous" side={c.previous} active={false} /> +<Side label="Previous" side={c.previous} active={c.resolution === 'PREVIOUS_WINS'} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/`(app)/memory/conflicts/page.tsx around lines 105 - 107, Update the Previous Side invocation in the conflicts page to set active based on whether c.resolution is 'PREVIOUS_WINS', while preserving the existing Latest-side behavior for 'LATEST_WINS'.packages/activities/src/memory-engine.activities.ts-987-991 (1)
987-991: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFind the latest reference timestamp instead of trusting array order.
A late-arriving historical event is appended last and incorrectly becomes
lastEventAt, reducing the memory’s recency score. Select the maximum reference timestamp.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/activities/src/memory-engine.activities.ts` around lines 987 - 991, Update the lastEventAt calculation near readReferences and frequencyCount to derive the latest timestamp by comparing all reference at values, rather than using refs[refs.length - 1]. Preserve memory.updatedAt.toISOString() as the fallback when refs is empty or timestamps are unavailable.packages/workflows/src/definitions.ts-45-45 (1)
45-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winModel and assign terminal progress states accurately.
The progress contract lacks
FAILED, while the workflow assignsCOMPLETEbefore finalization and retains it if finalization fails.
packages/workflows/src/definitions.ts#L45-L45: addFAILEDto the stage union.packages/workflows/src/workflows/memory-engine.workflow.ts#L156-L197: assignCOMPLETEafter successful finalization and assignFAILEDin the catch path.Proposed fix
- stage: 'COLLECT' | 'APPLY' | 'MERGE' | 'TIMELINE' | 'CONFLICT' | 'SCORE' | 'COMPLETE'; + stage: 'COLLECT' | 'APPLY' | 'MERGE' | 'TIMELINE' | 'CONFLICT' | 'SCORE' | 'COMPLETE' | 'FAILED';- progress.stage = 'COMPLETE'; await finalize.finalizeMemoryRun({ // ... }); + progress.stage = 'COMPLETE'; } catch (error) { const message = error instanceof Error ? error.message : String(error); + progress.stage = 'FAILED'; progress.error = message;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workflows/src/definitions.ts` at line 45, Extend the stage union in packages/workflows/src/definitions.ts at lines 45-45 with FAILED. In packages/workflows/src/workflows/memory-engine.workflow.ts at lines 156-197, move the COMPLETE assignment to after successful finalization and set the progress stage to FAILED in the catch path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Major comments:
In
`@apps/api/prisma/migrations/20260716110000_company_memory_engine/migration.sql`:
- Around line 273-289: Update the migration’s tenant-owned relationship
constraints, including memories_mergedIntoId_fkey,
memory_versions_memoryId_fkey, memory_events_memoryId_fkey,
memory_timeline_events_timelineId_fkey, conflict_records_memoryId_fkey, and
memory_scores_memoryId_fkey, to use composite keys carrying organizationId. Add
the corresponding unique parent keys such as (id, organizationId) on referenced
tenant tables, and ensure each child foreign key references both the related ID
and organizationId while preserving the existing delete/update actions.
In `@apps/api/prisma/schema.prisma`:
- Around line 1336-1395: Replace the plain @@unique constraint on Memory with a
partial unique index that enforces uniqueness only for rows with status ACTIVE
and deletedAt IS NULL. Update the corresponding migration/index definition as
well, while preserving uniqueness across organizationId, memoryType, and
dedupeKey for eligible memories.
In `@apps/api/src/modules/memory/memory.schemas.ts`:
- Around line 58-62: Update resolveConflictBodySchema to use a discriminated
union for choice: keep latest and previous branches with optional or absent
value as appropriate, and define the custom branch with value required as
z.unknown() so explicit null remains valid. Ensure resolveConflict() can no
longer receive a custom choice without a value.
- Around line 11-16: The filter schemas currently accept arbitrary uppercase
strings instead of validating against Prisma enum values. Replace enumString
usage in listMemoryQuerySchema and the corresponding timeline, changes, and
conflicts query schemas with z.nativeEnum or explicit z.enum definitions tied to
each field’s actual Prisma enum, so unsupported filters are rejected during
request validation.
In `@apps/api/src/modules/memory/memory.service.ts`:
- Around line 339-395: Update resolveConflict() to use one interactive Prisma
transaction that re-fetches the conflict with status OPEN and validates its
version snapshot before resolving it. Move applyAttribute()’s memory read and
update into that transaction to avoid overwriting concurrent changes, and create
the corresponding MemoryVersion within the same transaction so manual
resolutions appear in /changes. Ensure the conflict update is guarded against
repeated resolution and return the transaction’s updated conflict.
In `@apps/web/src/app/`(app)/memory/conflicts/page.tsx:
- Around line 53-62: Update resolve so it awaits the load() refresh after
memoryApi.resolveConflict succeeds, keeping the conflict busy until refreshed
data has completed loading. Preserve the existing error handling and
setBusy(null) cleanup in the finally block.
In `@packages/activities/src/memory-engine.activities.ts`:
- Around line 837-842: Update the duplicate-memory merge flow around
reconcileAttributes so any returned conflicts are persisted as reviewable
ConflictRecord rows before the loser memory is deleted. Reuse the existing
conflict persistence mechanism and ensure conflicts from the existing and
incoming attributes are not discarded while preserving the current
reconciliation behavior.
- Around line 549-556: Update the workflow’s event-processing flow around the
catch block that increments stats.failed so failures are propagated or
explicitly cause the workflow to fail when failed > 0. Ensure the finalization
path cannot report a successful run after any memory event application fails,
while preserving the existing failed-event status update.
- Around line 945-965: Update the post-resolution logic around clearWinner and
the conflictRecord update to skip conflicts using the MANUAL strategy. For all
other strategies, determine the winning value through the existing domain
conflict resolver so SOURCE_PRIORITY and other strategies are handled
consistently instead of falling back to timestamp comparison; preserve the
existing AUTO_RESOLVED persistence behavior.
- Around line 321-343: Make every bounded memory-engine stage resumable instead
of repeatedly selecting the same first page: in
packages/activities/src/memory-engine.activities.ts lines 321-343, add stable
cursor pagination for objects and their mentions across collection rounds; in
lines 895-900, paginate timeline rebuilding so each timeline is eventually
refreshed; in lines 938-941, rotate or cursor past unresolved conflicts; and in
lines 978-982, paginate scoring so every active memory is eventually recomputed.
Preserve the existing tuning limits and processing behavior while carrying
continuation state between rounds.
- Around line 299-306: Update the entity-state persistence logic around the
currentState update to merge attrs into the existing currentState rather than
replacing it, preserving previously derived status, priority, and assignee when
attrs are empty or partial. Ensure lastEventAt only advances and cannot regress
when processing an older event, while retaining the version increment and other
update behavior.
- Around line 847-888: In the merge flow, compute the reconciled confidence and
importance once before the survivor update, then reuse those values in both the
tx.memory.update call and the snapshot invocation. Replace the snapshot’s stale
survivor.confidence and survivor.importance references while preserving the
recalculated merged attributes and existing update behavior.
- Around line 363-401: Update the initial event construction in the object
collection flow so it always emits the object’s creation event at createdAt,
including when obj.version exceeds one; reserve KNOWLEDGE_OBJECT_UPDATED for the
later updatedAt event guarded by the existing version check. Preserve the
existing payload and ordering while changing the initial event type selection.
- Around line 658-676: The update flow around changed, summary, and version must
detect summary-only changes. Compare payload.summary with existing.summary
independently, use that result to update changed and increment the version, and
preserve the existing summary when no incoming summary change is present.
- Around line 518-542: Update applyMemoryEvents to atomically claim each event
before calling applyOne: conditionally transition the row from PENDING to an
in-progress status and continue only when that update succeeds. Use the event
identifier and organization scope in the conditional update, preserving
processing for uniquely claimed rows and preventing concurrent runs from
applying the same event.
- Around line 594-677: Handle terminal statuses immediately after the findUnique
lookup, before the normal create/update branches. For MERGED memories, resolve
the dedupe key to the surviving memory and apply the observation there rather
than updating the hidden loser; for EXPIRED or ARCHIVED memories, explicitly
reactivate them or create a replacement according to the intended visibility
rules. Ensure dedupe keys no longer remain attached only to merged losers, and
preserve the existing reconciliation flow for active memories.
In `@packages/memory-engine/src/reconciliation.ts`:
- Around line 210-218: The same-value branch in the reconciliation logic must
not mix provenance fields from separate observations. Update the merge handling
around valuesEqual to retain one complete Provenanced record—prefer the
observation whose confidence and timestamp are selected—or model reinforcement
metadata separately, ensuring source, confidence, timestamp, and other
provenance fields remain internally consistent.
- Around line 179-185: Update valuesEqual to compare structured values using a
recursive canonicalizer that sorts object keys while preserving array order,
then compare the canonical results. Keep the existing identity and
case-insensitive trimmed string handling, and ensure objects with identical
properties in different insertion orders are treated as equal.
In `@packages/memory-engine/src/scoring.ts`:
- Around line 82-83: Update the frequency calculation near the scoring
function’s saturation logic to handle non-positive frequency saturation before
division. Return a frequency of zero when the effective event count is zero, and
validate or otherwise safely handle non-positive saturation for positive counts
so neither frequency nor composite becomes NaN.
In `@packages/memory-engine/src/timeline.ts`:
- Around line 111-119: Update the dedupeHash construction in the timeline event
flow to use eventId as the source-event identity when present, preventing
distinct events from colliding. For drafts without an eventId, retain the
fallback fields but canonicalize changesSummary by sorting change keys. Update
describeChanges to iterate over sorted entries so equivalent change objects
produce identical hashes.
In `@packages/workflows/src/workflows/memory-engine.workflow.ts`:
- Around line 115-123: Update the memory-event processing loop in the workflow
to track whether the queue is fully drained, using the ingest result’s hasMore
or remaining indicator alongside processed counts. When MAX_APPLY_ROUNDS is
reached with pending events, do not proceed to later stages or return COMPLETED;
instead continue processing or return the established failure/incomplete
outcome, while preserving normal completion when drainage is confirmed.
In `@services/temporal-worker/src/config.ts`:
- Around line 116-123: The parseScoreWeights function currently accepts invalid
JSON shapes and score values. Replace the unchecked JSON.parse result with
strict Zod validation requiring exactly the five score-weight fields, each a
finite nonnegative number; preserve the existing invalid-input error behavior
and return the validated MemoryTuning['scoreWeights'] value.
---
Minor comments:
In `@apps/api/src/modules/memory/memory.service.ts`:
- Around line 463-487: Update the statistics response’s processingStatus
handling around lastRunRaw so malformed Redis data cannot throw from JSON.parse.
Catch parsing failures and return null (or the existing structured unavailable
representation), while preserving valid JSON parsing and the rest of the stats
response.
- Around line 100-112: Update the Prisma query in getMemory to require deletedAt
to be null alongside id and organizationId, ensuring soft-deleted memories are
not returned while preserving the existing includes and not-found behavior.
In `@apps/web/src/app/`(app)/memory/conflicts/page.tsx:
- Around line 105-107: Update the Previous Side invocation in the conflicts page
to set active based on whether c.resolution is 'PREVIOUS_WINS', while preserving
the existing Latest-side behavior for 'LATEST_WINS'.
In `@apps/web/src/app/`(app)/memory/page.tsx:
- Around line 149-177: Update the entityId call-to-action inside the memory card
rendered by the memories map so its label matches the existing `/memory/${m.id}`
destination; rename “View entity timeline” to “View memory details” and preserve
the current conditional rendering and styling.
- Around line 39-47: Clear stale errors before each relevant memory request: in
apps/web/src/app/(app)/memory/page.tsx lines 39-47, reset the list error at the
start or successful completion of load(); in
apps/web/src/app/(app)/memory/[id]/page.tsx lines 37-41 and
apps/web/src/app/(app)/memory/entity/[id]/page.tsx lines 20-26, reset the
detail/entity error when id changes; in
apps/web/src/app/(app)/memory/changes/page.tsx lines 20-25, reset the error when
loading a new date range; and in
apps/web/src/app/(app)/memory/conflicts/page.tsx lines 44-48, reset it when
status changes or refresh follows resolution.
In `@apps/web/src/components/memory/util.ts`:
- Around line 3-25: Update the color definitions in MEMORY_TYPE_COLOR and
SOURCE_COLOR so ORGANIZATIONAL, WORKING, KNOWLEDGE, and GIT provide sufficient
contrast with consumers’ fixed text-white foreground; prefer darkening those
background values while preserving the existing color-map API and all other
mappings.
In `@packages/activities/src/memory-engine.activities.ts`:
- Around line 987-991: Update the lastEventAt calculation near readReferences
and frequencyCount to derive the latest timestamp by comparing all reference at
values, rather than using refs[refs.length - 1]. Preserve
memory.updatedAt.toISOString() as the fallback when refs is empty or timestamps
are unavailable.
In `@packages/workflows/src/definitions.ts`:
- Line 45: Extend the stage union in packages/workflows/src/definitions.ts at
lines 45-45 with FAILED. In
packages/workflows/src/workflows/memory-engine.workflow.ts at lines 156-197,
move the COMPLETE assignment to after successful finalization and set the
progress stage to FAILED in the catch path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 682f1964-1c51-4c0f-af02-b71ebb5810e4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
.env.exampleapps/api/prisma/migrations/20260716110000_company_memory_engine/migration.sqlapps/api/prisma/schema.prismaapps/api/src/app.tsapps/api/src/modules/memory/memory.routes.tsapps/api/src/modules/memory/memory.schemas.tsapps/api/src/modules/memory/memory.service.tsapps/web/src/app/(app)/layout.tsxapps/web/src/app/(app)/memory/[id]/page.tsxapps/web/src/app/(app)/memory/changes/page.tsxapps/web/src/app/(app)/memory/conflicts/page.tsxapps/web/src/app/(app)/memory/entity/[id]/page.tsxapps/web/src/app/(app)/memory/page.tsxapps/web/src/components/memory/util.tsapps/web/src/lib/api.tsdocs/memory-engine.mdpackages/activities/package.jsonpackages/activities/src/index.tspackages/activities/src/memory-engine.activities.tspackages/activities/src/memory.context.tspackages/memory-engine/eslint.config.mjspackages/memory-engine/package.jsonpackages/memory-engine/src/config.tspackages/memory-engine/src/conflict.tspackages/memory-engine/src/index.tspackages/memory-engine/src/reconciliation.test.tspackages/memory-engine/src/reconciliation.tspackages/memory-engine/src/scoring.test.tspackages/memory-engine/src/scoring.tspackages/memory-engine/src/timeline.test.tspackages/memory-engine/src/timeline.tspackages/memory-engine/src/types.tspackages/memory-engine/tsconfig.jsonpackages/workflows/src/constants.tspackages/workflows/src/definitions.tspackages/workflows/src/index.tspackages/workflows/src/workflows/memory-engine.workflow.tsservices/temporal-worker/package.jsonservices/temporal-worker/src/config.tsservices/temporal-worker/src/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation