README.md updated#322
Conversation
|
@brukcodes is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe README is restructured into consistent markdown tables across navigation, Overview, benchmarks, agent integrations, features, MCP tooling, iii console pages, configuration, and API docs. Separately, migrate.ts loads better-sqlite3 via dynamic import and normalizes session/observation/summary IDs with fallbacks and generated IDs. ChangesREADME Documentation Table-Driven Restructure
Migration robustness and sqlite import
Possibly Related PRs
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/functions/migrate.ts (1)
72-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd audit trail for migration operations.
The migration function performs state-changing operations (writing sessions, observations, and summaries to KV) but does not call
recordAudit()to record these operations. As per coding guidelines, functions insrc/functions/**/*.tsmust record state-changing operations viarecordAudit()for audit trail tracking.📋 Proposed fix to add audit recording
Import
recordAuditat the top of the file:import { recordAudit } from "../state/audit.js";Then add an audit record after the migration completes (after line 139):
await kv.set(KV.summaries, summarySessionId, summary); summaryCount++; } + + await recordAudit({ + operation: "mem::migrate", + metadata: { + dbPath: data.dbPath, + sessionCount, + obsCount, + summaryCount, + }, + }); logger.info("Migration complete", {As per coding guidelines, state-changing operations in
src/functions/**/*.tsmust callrecordAudit()for audit trail tracking.🤖 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 `@src/functions/migrate.ts` at line 72, The migration writes state to KV (e.g., await kv.set(KV.sessions, session.id, session)) but doesn't record an audit entry; import recordAudit from "../state/audit.js" and call recordAudit(...) from the migrate function after the migration's state-changing operations complete (include context such as operation type "migration", target identifiers like session.id or observation ids, and any summary data) so every write (sessions, observations, summaries) is logged for audit trail tracking.
🧹 Nitpick comments (1)
src/functions/migrate.ts (1)
60-74: ⚡ Quick winConsider parallelizing independent KV writes.
The migration loops perform sequential
await kv.set()operations. Since each write is independent, batching them withPromise.all()would improve performance for large migrations. As per coding guidelines, "Use parallel operations where possible with Promise.all for independent kv writes/reads."⚡ Example refactor for session migration
- for (const row of sessions) { + await Promise.all(sessions.map(async (row) => { const sessionId = String(row.session_id ?? row.id ?? generateId("mig")); const session: Session = { id: sessionId, project: row.project_path || row.project || "unknown", cwd: row.cwd || row.project_path || "", startedAt: row.created_at || row.started_at || new Date().toISOString(), endedAt: row.ended_at || row.updated_at, status: "completed", observationCount: 0, }; await kv.set(KV.sessions, session.id, session); - sessionCount++; - } + })); + sessionCount = sessions.length;Apply the same pattern to observations and summaries loops.
As per coding guidelines, parallel operations should be used where possible for independent KV writes.
🤖 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 `@src/functions/migrate.ts` around lines 60 - 74, The migration currently awaits each independent kv.set sequentially inside the sessions loop (for const row of sessions) which is slow; instead collect promises for each write (using kv.set with the computed session id and session object) into an array and then await Promise.all(promises), and update sessionCount by sessions.length (or the number of promises) after the Promise.all resolves; locate the session migration logic (variables: sessions, sessionId, Session, sessionCount, generateId, kv.set) to implement this change and apply the same Promise.all batching pattern to the observations and summaries migration loops as well.
🤖 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.
Inline comments:
In `@src/functions/migrate.ts`:
- Around line 123-125: summarySessionId currently falls back to the hardcoded
string "migrated" when row.session_id, row.sessionId, and row.id are all
missing, which causes key collisions when calling kv.set(KV.summaries,
summarySessionId, summary); change the fallback to a guaranteed-unique string
(for example by appending a stable unique identifier like a UUID, a timestamp
plus the loop index, or a hash of the row contents) so each summary gets a
distinct key; update the code paths that build summarySessionId (references:
summarySessionId, row.session_id, row.sessionId, row.id) and ensure the new key
is a string before calling kv.set(KV.summaries, summarySessionId, summary).
- Line 94: The fallback for sessionId currently uses the hardcoded string
"migrated" which groups unrelated rows together; change the assignment of
sessionId in migrate.ts (the const sessionId = ...) to call generateId() when
neither row.session_id nor row.sessionId exist so each migrated observation gets
a unique session identifier, and ensure generateId is imported/available in the
module (or reference the existing utility function) so the code compiles and
produces unique fallback IDs.
- Around line 35-36: The dynamic import of better-sqlite3 uses a fallback
`sqlite.default ?? sqlite` which can yield a namespace object (not a
constructor); replace that pattern by destructuring the default export directly
so Database references the constructor. Change the import to use `const {
default: Database } = await import("better-sqlite3");` and remove the fallback
assignment; update any subsequent references to the existing Database variable
if needed to match the destructured name.
---
Outside diff comments:
In `@src/functions/migrate.ts`:
- Line 72: The migration writes state to KV (e.g., await kv.set(KV.sessions,
session.id, session)) but doesn't record an audit entry; import recordAudit from
"../state/audit.js" and call recordAudit(...) from the migrate function after
the migration's state-changing operations complete (include context such as
operation type "migration", target identifiers like session.id or observation
ids, and any summary data) so every write (sessions, observations, summaries) is
logged for audit trail tracking.
---
Nitpick comments:
In `@src/functions/migrate.ts`:
- Around line 60-74: The migration currently awaits each independent kv.set
sequentially inside the sessions loop (for const row of sessions) which is slow;
instead collect promises for each write (using kv.set with the computed session
id and session object) into an array and then await Promise.all(promises), and
update sessionCount by sessions.length (or the number of promises) after the
Promise.all resolves; locate the session migration logic (variables: sessions,
sessionId, Session, sessionCount, generateId, kv.set) to implement this change
and apply the same Promise.all batching pattern to the observations and
summaries migration loops as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const sqlite = await import("better-sqlite3"); | ||
| Database = sqlite.default ?? sqlite; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify better-sqlite3 export structure and correct import pattern
# Check if better-sqlite3 is installed and examine its package.json
if [ -f "node_modules/better-sqlite3/package.json" ]; then
echo "=== better-sqlite3 package.json ==="
cat node_modules/better-sqlite3/package.json | jq '{name, version, main, module, type, exports}'
fi
# Look for TypeScript definitions to understand export structure
if [ -f "node_modules/better-sqlite3/lib/index.d.ts" ]; then
echo "=== Type definitions ==="
cat node_modules/better-sqlite3/lib/index.d.ts | head -20
fi
# Check package-lock.json for installed version
echo "=== Installed version ==="
cat package-lock.json | jq '.packages["node_modules/better-sqlite3"].version // .dependencies["better-sqlite3"].version' 2>/dev/null || echo "Not found in package-lock.json"Repository: rohitg00/agentmemory
Length of output: 158
🌐 Web query:
How to dynamically import better-sqlite3 in ESM TypeScript with proper default export handling
💡 Result:
In ESM TypeScript, better-sqlite3 should be treated as a CommonJS module whose module.exports becomes the ESM default export, so you must read the constructor from the default property when using dynamic import(). 1) Correct dynamic import with proper default export handling (Node ESM) import better-sqlite3 dynamically and create the DB: const { default: BetterSqlite3 } = await import('better-sqlite3'); const db = new BetterSqlite3('foobar.db'); This is required because when a CommonJS module is imported into ESM via import(), Node maps module.exports to the ESM default export; with dynamic import() the module.exports value is available under namespace.default [1]. The better-sqlite3 package is intended to be instantiated as a default export Database/constructor in ES module notation (import Database from 'better-sqlite3') [2][3]. 2) TypeScript typing (constructor vs Database type) The typings define a DatabaseConstructor interface with new(...) and call signatures, and a Database interface for instances [4]. In practice, you can type the default-imported value and/or the return type: // db.ts import type { Database, DatabaseConstructor } from 'better-sqlite3'; export async function openDb(path: string): Promise { const { default: BetterSqlite3 } = await import('better-sqlite3'); const ctor = BetterSqlite3 as unknown as DatabaseConstructor; return new ctor(path); } If you previously used incorrect interop (e.g., import BetterSqlite3 from 'better-sqlite3' with dynamic import without reading .default, or used the wrong import shape), you can hit runtime errors like better_sqlite3_1.default is not a constructor; dynamic import interop via .default is the fix [5] (and the underlying rule is the CommonJS→ESM default mapping for import() [1]). 3) When you don’t need dynamic import If you can use static ESM imports, the simplest form is: import Database from 'better-sqlite3'; const db = new Database('foobar.db'); This is the documented/typical ES module usage [2][3]. Notes - This pattern is especially relevant for native addons: ESM static/dynamic loading still has to synchronize the module loading shape, but the default export mapping rule is what determines whether the constructor is at namespace.default or not [1].
Citations:
- 1: https://nodejs.github.io/package-examples/04-cjs-esm-interop/shipping-cjs-for-esm/
- 2: https://www.npmjs.com/package/better-sqlite3/v/9.6.0
- 3: https://github.com/JoshuaWise/better-sqlite3
- 4: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/better-sqlite3/index.d.ts
- 5: https://stackoverflow.com/questions/79367852/typeerror-better-sqlite3-1-default-is-not-a-constructor-in-typescript-but-not
Use explicit destructuring for the better-sqlite3 dynamic import.
The fallback pattern sqlite.default ?? sqlite is problematic because sqlite (the namespace object) is not a constructor. Since better-sqlite3 is a CommonJS module imported via ESM, the constructor is always at the .default property. Replace with explicit destructuring:
Current code
const sqlite = await import("better-sqlite3");
Database = sqlite.default ?? sqlite;
Recommended change
const { default: Database } = await import("better-sqlite3");
🤖 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 `@src/functions/migrate.ts` around lines 35 - 36, The dynamic import of
better-sqlite3 uses a fallback `sqlite.default ?? sqlite` which can yield a
namespace object (not a constructor); replace that pattern by destructuring the
default export directly so Database references the constructor. Change the
import to use `const { default: Database } = await import("better-sqlite3");`
and remove the fallback assignment; update any subsequent references to the
existing Database variable if needed to match the destructured name.
1 participant
I have enhanced the README.md to make it more professional. Here are the key improvements I made:
1 Added an Overview Section: Introduced a concise, formal description of agentmemory at the beginning, explaining its purpose, architecture, and key features.
2 Improved Language: Rephrased the introductory paragraph to use more professional and formal language, replacing casual phrases with clear, objective descriptions.
3 Added a Contributing Section: Included a dedicated section on how to contribute, with guidelines for forking, development setup, issue reporting, and adherence to the code of conduct.
4 Updated Table of Contents: Added links to the new Overview and Contributing sections for better navigation.
Summary by CodeRabbit