fix: iii-sdk v0.11 getContext crash — add logger shim, v0.8.11#146
Merged
fix: iii-sdk v0.11 getContext crash — add logger shim, v0.8.11#146
Conversation
…ility
iii-sdk v0.11 dropped getContext() entirely. 32 src/functions/*.ts files still
imported and called it, crashing node dist/index.mjs on first import with:
SyntaxError: module 'iii-sdk' does not provide an export named 'getContext'
- Add src/logger.ts: thin stderr shim (info/warn/error) with identical call signature
- Remove getContext import and const ctx = getContext() from all 32 function files
- Replace ctx.logger.* with logger.* throughout
- Fix search.ts registerFunction({ id: ... }) -> registerFunction('id') for v0.11 API
- Update 45 test mock blocks from vi.mock("iii-sdk") to vi.mock("../src/logger.js")
- Bump to v0.8.11
Tests: 731/731 pass. Binary starts cleanly. Closes #116.
📝 WalkthroughWalkthroughThis PR removes usage of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…ersions, and test
![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 (3)
src/functions/export-import.ts (1)
179-180:⚠️ Potential issue | 🟠 MajorAdd
0.8.11to the import compatibility allowlist.Line 179 currently omits the current release version, which causes
mem::importto reject exports produced by this build and breaks the consistency check.Proposed patch
- const supportedVersions = new Set(["0.3.0", "0.4.0", "0.5.0", "0.6.0", "0.6.1", "0.7.0", "0.7.2", "0.7.3", "0.7.4", "0.7.5", "0.7.6", "0.7.7", "0.7.9", "0.8.0", "0.8.1", "0.8.2", "0.8.3", "0.8.4", "0.8.5", "0.8.6", "0.8.7", "0.8.8", "0.8.9", "0.8.10"]); + const supportedVersions = new Set(["0.3.0", "0.4.0", "0.5.0", "0.6.0", "0.6.1", "0.7.0", "0.7.2", "0.7.3", "0.7.4", "0.7.5", "0.7.6", "0.7.7", "0.7.9", "0.8.0", "0.8.1", "0.8.2", "0.8.3", "0.8.4", "0.8.5", "0.8.6", "0.8.7", "0.8.8", "0.8.9", "0.8.10", "0.8.11"]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/export-import.ts` around lines 179 - 180, supportedVersions set in export/import.ts is missing the current release "0.8.11", causing importData.version to be rejected; update the Set literal referenced by supportedVersions to include the string "0.8.11" (e.g., add "0.8.11" among the other version entries) so that the if-check using supportedVersions.has(importData.version) accepts the new release version.test/export-import.test.ts (1)
122-123:⚠️ Potential issue | 🟡 MinorUpdate the exported version assertion to the bumped release.
CI is failing because this still expects
0.8.10while the PR bumps to0.8.11.Patch
- expect(result.version).toBe("0.8.10"); + expect(result.version).toBe("0.8.11");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/export-import.test.ts` around lines 122 - 123, The test assertion for the exported package version is stale: update the expectation in test/export-import.test.ts where the assertion checks result.version (currently expecting "0.8.10") to the new bumped release "0.8.11" so the export-import.test.ts check matches the PR version bump.CHANGELOG.md (1)
9-22:⚠️ Potential issue | 🟡 MinorMinor formatting: missing blank line before
## [0.8.10].There should be a blank line between the end of the 0.8.11 section and the 0.8.10 header for consistent changelog formatting.
📝 Suggested fix
- **iii-sdk v0.11 getContext crash** ([`#116`](https://github.com/rohitg00/agentmemory/issues/116)) — `SyntaxError: The requested module 'iii-sdk' does not provide an export named 'getContext'` on startup. Removed all `getContext` imports from 32 function files, added `src/logger.ts` shim, updated 45 test mock blocks. ### Changed + - Upgraded `iii-sdk` dependency from `^0.11.0-next.8` to stable `^0.11.0`.Wait, looking more closely, the issue is between lines 21-22:
- Upgraded `iii-sdk` dependency from `^0.11.0-next.8` to stable `^0.11.0`. - Aligned stream send payloads with v0.11 wire format by using `type` for `stream::send` events in observe/compress/session-activity paths. - Updated migration guidance/examples and diagnostics plugin registration snippets to v0.11 function registration and trigger request shapes. + ## [0.8.10] — 2026-04-15🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 9 - 22, The CHANGELOG.md is missing a blank line before the "## [0.8.10]" header causing inconsistent formatting; edit the file to insert a single blank line between the end of the 0.8.11 section and the "## [0.8.10]" header (i.e., add a newline after the last line of the 0.8.11 block so the "## [0.8.10]" header is separated by an empty line).
🧹 Nitpick comments (7)
test/routines.test.ts (1)
3-5: Remove the unused logger mock from this test file.The
registerRoutinesFunctionimplementation does not import or use../src/logger.js, making this mock unnecessary setup noise that adds no value to the test.Proposed cleanup
-vi.mock("../src/logger.js", () => ({ - logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, -}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/routines.test.ts` around lines 3 - 5, Remove the unused logger mock in test/routines.test.ts by deleting the vi.mock("../src/logger.js", () => ({ logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, })); line(s); the test only exercises registerRoutinesFunction and that implementation does not import or use ../src/logger.js so the mock is unnecessary noise—ensure no other tests in the file rely on that mock before removing.test/crystallize.test.ts (1)
3-5: Align this test setup with the repo’s standard SDK mock pattern.Line 3 currently mocks only
../src/logger.js; please keep/add the canonicalvi.mock("iii-sdk")scaffold (sdk.trigger,kv.get/set/list) used across function tests, then layer logger mocking as needed.As per coding guidelines "Test mock pattern: use vi.mock("iii-sdk") with mock sdk.trigger, kv.get/set/list".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/crystallize.test.ts` around lines 3 - 5, The test currently only mocks "../src/logger.js" but must follow the repo's standard SDK mock scaffold; add a top-level vi.mock("iii-sdk") that provides the canonical mock shape (exporting sdk with trigger and kv methods: sdk.trigger, kv.get, kv.set, kv.list) and then layer or keep the existing logger mock (logger.info/warn/error) so tests use the standard iii-sdk stubbed interface; update the test to call vi.mock("iii-sdk") with those mocked methods before or alongside the vi.mock("../src/logger.js") mock.test/consistency.test.ts (1)
5-7: Use the standard SDK mock baseline in this test too.Line 5 currently switches to a logger-only mock; please retain/add the
vi.mock("iii-sdk")pattern used by the suite, then compose logger mocks on top.As per coding guidelines "Test mock pattern: use vi.mock("iii-sdk") with mock sdk.trigger, kv.get/set/list".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/consistency.test.ts` around lines 5 - 7, Replace the current logger-only mock with the standard SDK mock baseline by adding vi.mock("iii-sdk") (the test-suite pattern) and then compose the logger mock on top of it instead of replacing the whole module; ensure the mocked SDK includes the usual symbols (sdk.trigger and kv.get/kv.set/kv.list) and then apply the logger override similar to the rest of tests (keep the existing logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() } composition).test/mcp-standalone.test.ts (1)
3-5: Please align this mock bootstrap with the repo’s SDK test pattern.Line 3 should retain the standard
vi.mock("iii-sdk")scaffold for tool/function tests; keep logger mocks as an additional layer.As per coding guidelines "Test mock pattern: use vi.mock("iii-sdk") with mock sdk.trigger, kv.get/set/list".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mcp-standalone.test.ts` around lines 3 - 5, Replace the current logger-only mock with the repo test pattern: call vi.mock("iii-sdk") and inside provide a mocked sdk object exposing trigger (vi.fn()) and a kv namespace with get, set, list (vi.fn()) as needed, then also mock the logger layer (logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }) as an additional export; update any references in the test to use the mocked sdk.trigger and kv.get/set/list instead of relying solely on the logger mock.test/sentinels.test.ts (1)
3-5: Keep test mock setup consistent with the suite standard.Line 3 should follow the baseline
vi.mock("iii-sdk")pattern for function tests, with logger mocking layered as an additional mock where required.As per coding guidelines "Test mock pattern: use vi.mock("iii-sdk") with mock sdk.trigger, kv.get/set/list".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sentinels.test.ts` around lines 3 - 5, The test currently mocks the logger via vi.mock("../src/logger.js") which breaks the suite standard; change the test to use the baseline vi.mock("iii-sdk") pattern for function tests and layer the logger mock on top only where needed: replace the direct vi.mock("../src/logger.js") usage with vi.mock("iii-sdk") that provides mocked sdk.trigger and kv.get/kv.set/kv.list as per other tests, then add a secondary mock for logger (mocking logger.info/warn/error) to keep logger behavior consistent with the suite; reference the existing vi.mock("../src/logger.js") and logger symbols to locate the change and ensure sdk.trigger and kv.get/set/list are included in the iii-sdk mock.test/relations.test.ts (1)
3-4: Align this test mock with the repo’s standard SDK mock pattern.Line 3 switches to mocking
../src/logger.js; current repository guidance expectsvi.mock("iii-sdk")as the base test mock shape (withsdk.triggerandkv.get/set/list). Consider restoring that base mock and layering logger stubs on top if needed.As per coding guidelines
test/**/*.test.ts: “Test mock pattern: use vi.mock("iii-sdk") with mock sdk.trigger, kv.get/set/list”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/relations.test.ts` around lines 3 - 4, Replace the ad-hoc logger-only mock with the repository standard SDK mock: call vi.mock("iii-sdk") and provide a mock object that includes sdk.trigger and kv.get/set/list (as used across tests), then extend that mock to include logger stubs (logger.info/warn/error) so existing tests can use both the standard sdk API and the logger; update the mock declaration in relations.test.ts (replace vi.mock("../src/logger.js", ...) with the unified vi.mock("iii-sdk") shape and add the logger methods on the returned mock).src/logger.ts (1)
1-25: Trim the large migration narrative from inline code comments.This block is very detailed and mostly explains what changed in the migration flow. Consider moving most of it to PR/ADR docs and keeping only a brief module intent note inline.
As per coding guidelines, "Avoid code comments explaining WHAT — use clear naming instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logger.ts` around lines 1 - 25, The top-of-file comment in the logger module is overly long and historical; trim it to a concise intent note that describes the exported logger singleton and its public API (logger.info/logger.warn/logger.error) and the stderr output format, and move the detailed migration narrative to PR/ADR or external docs; update the module comment block around the logger export (symbol: logger) to a short summary only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 3: Update the ExportData interface's version union to include the new
release string by adding "0.8.11" to the existing union in the ExportData type;
locate the ExportData interface and append the new literal to the version
field's union so it matches the package.json and other updated version
references.
In `@src/logger.ts`:
- Around line 29-35: The emptiness check using Object.keys(fields) currently
sits outside the try/catch and can itself throw (e.g., for proxy objects); move
the Object.keys(fields) call and the "no fields" branch inside the existing try
block in function fmt so any exception from accessing fields is caught, then
keep the current behavior: if fields is empty return the base "[agentmemory]
${level} ${msg}" string, otherwise return the string with
JSON.stringify(fields); ensure the catch still returns a safe fallback and
references the same symbols (fmt, Object.keys, fields, JSON.stringify) so the
function never throws on malformed field objects.
In `@src/version.ts`:
- Line 1: Update the ExportData.version union in src/types.ts to include
"0.8.11" so it matches the new VERSION, and reconcile the version unions in
src/version.ts by adding the missing entries ("0.7.7", "0.7.9", "0.8.0") that
appear in supportedVersions/ExportData; ensure the exported constant VERSION
remains set to "0.8.11" and that the union types in src/version.ts and
ExportData.version are consistent with supportedVersions.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 9-22: The CHANGELOG.md is missing a blank line before the "##
[0.8.10]" header causing inconsistent formatting; edit the file to insert a
single blank line between the end of the 0.8.11 section and the "## [0.8.10]"
header (i.e., add a newline after the last line of the 0.8.11 block so the "##
[0.8.10]" header is separated by an empty line).
In `@src/functions/export-import.ts`:
- Around line 179-180: supportedVersions set in export/import.ts is missing the
current release "0.8.11", causing importData.version to be rejected; update the
Set literal referenced by supportedVersions to include the string "0.8.11"
(e.g., add "0.8.11" among the other version entries) so that the if-check using
supportedVersions.has(importData.version) accepts the new release version.
In `@test/export-import.test.ts`:
- Around line 122-123: The test assertion for the exported package version is
stale: update the expectation in test/export-import.test.ts where the assertion
checks result.version (currently expecting "0.8.10") to the new bumped release
"0.8.11" so the export-import.test.ts check matches the PR version bump.
---
Nitpick comments:
In `@src/logger.ts`:
- Around line 1-25: The top-of-file comment in the logger module is overly long
and historical; trim it to a concise intent note that describes the exported
logger singleton and its public API (logger.info/logger.warn/logger.error) and
the stderr output format, and move the detailed migration narrative to PR/ADR or
external docs; update the module comment block around the logger export (symbol:
logger) to a short summary only.
In `@test/consistency.test.ts`:
- Around line 5-7: Replace the current logger-only mock with the standard SDK
mock baseline by adding vi.mock("iii-sdk") (the test-suite pattern) and then
compose the logger mock on top of it instead of replacing the whole module;
ensure the mocked SDK includes the usual symbols (sdk.trigger and
kv.get/kv.set/kv.list) and then apply the logger override similar to the rest of
tests (keep the existing logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn()
} composition).
In `@test/crystallize.test.ts`:
- Around line 3-5: The test currently only mocks "../src/logger.js" but must
follow the repo's standard SDK mock scaffold; add a top-level vi.mock("iii-sdk")
that provides the canonical mock shape (exporting sdk with trigger and kv
methods: sdk.trigger, kv.get, kv.set, kv.list) and then layer or keep the
existing logger mock (logger.info/warn/error) so tests use the standard iii-sdk
stubbed interface; update the test to call vi.mock("iii-sdk") with those mocked
methods before or alongside the vi.mock("../src/logger.js") mock.
In `@test/mcp-standalone.test.ts`:
- Around line 3-5: Replace the current logger-only mock with the repo test
pattern: call vi.mock("iii-sdk") and inside provide a mocked sdk object exposing
trigger (vi.fn()) and a kv namespace with get, set, list (vi.fn()) as needed,
then also mock the logger layer (logger: { info: vi.fn(), warn: vi.fn(), error:
vi.fn() }) as an additional export; update any references in the test to use the
mocked sdk.trigger and kv.get/set/list instead of relying solely on the logger
mock.
In `@test/relations.test.ts`:
- Around line 3-4: Replace the ad-hoc logger-only mock with the repository
standard SDK mock: call vi.mock("iii-sdk") and provide a mock object that
includes sdk.trigger and kv.get/set/list (as used across tests), then extend
that mock to include logger stubs (logger.info/warn/error) so existing tests can
use both the standard sdk API and the logger; update the mock declaration in
relations.test.ts (replace vi.mock("../src/logger.js", ...) with the unified
vi.mock("iii-sdk") shape and add the logger methods on the returned mock).
In `@test/routines.test.ts`:
- Around line 3-5: Remove the unused logger mock in test/routines.test.ts by
deleting the vi.mock("../src/logger.js", () => ({ logger: { info: vi.fn(), warn:
vi.fn(), error: vi.fn() }, })); line(s); the test only exercises
registerRoutinesFunction and that implementation does not import or use
../src/logger.js so the mock is unnecessary noise—ensure no other tests in the
file rely on that mock before removing.
In `@test/sentinels.test.ts`:
- Around line 3-5: The test currently mocks the logger via
vi.mock("../src/logger.js") which breaks the suite standard; change the test to
use the baseline vi.mock("iii-sdk") pattern for function tests and layer the
logger mock on top only where needed: replace the direct
vi.mock("../src/logger.js") usage with vi.mock("iii-sdk") that provides mocked
sdk.trigger and kv.get/kv.set/kv.list as per other tests, then add a secondary
mock for logger (mocking logger.info/warn/error) to keep logger behavior
consistent with the suite; reference the existing vi.mock("../src/logger.js")
and logger symbols to locate the change and ensure sdk.trigger and
kv.get/set/list are included in the iii-sdk mock.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfa8c25d-545a-4417-acce-8b1c63f59571
📒 Files selected for processing (81)
CHANGELOG.mdpackage.jsonsrc/functions/access-tracker.tssrc/functions/audit.tssrc/functions/auto-forget.tssrc/functions/claude-bridge.tssrc/functions/compress.tssrc/functions/consolidate.tssrc/functions/consolidation-pipeline.tssrc/functions/context.tssrc/functions/enrich.tssrc/functions/evict.tssrc/functions/export-import.tssrc/functions/file-index.tssrc/functions/governance.tssrc/functions/graph.tssrc/functions/migrate.tssrc/functions/observe.tssrc/functions/patterns.tssrc/functions/profile.tssrc/functions/query-expansion.tssrc/functions/relations.tssrc/functions/remember.tssrc/functions/retention.tssrc/functions/search.tssrc/functions/skill-extract.tssrc/functions/sliding-window.tssrc/functions/smart-search.tssrc/functions/snapshot.tssrc/functions/summarize.tssrc/functions/team.tssrc/functions/temporal-graph.tssrc/functions/timeline.tssrc/functions/working-memory.tssrc/logger.tssrc/version.tstest/access-tracker.test.tstest/actions.test.tstest/audit.test.tstest/auto-compress.test.tstest/auto-forget.test.tstest/cascade.test.tstest/checkpoints.test.tstest/claude-bridge.test.tstest/confidence.test.tstest/consistency.test.tstest/consolidation-pipeline.test.tstest/crystallize.test.tstest/diagnostics.test.tstest/enrich.test.tstest/export-import.test.tstest/facets.test.tstest/frontier.test.tstest/governance.test.tstest/graph.test.tstest/leases.test.tstest/lessons.test.tstest/mcp-prompts.test.tstest/mcp-resources.test.tstest/mcp-standalone.test.tstest/mesh.test.tstest/obsidian-export.test.tstest/profile.test.tstest/query-expansion.test.tstest/reflect.test.tstest/relations.test.tstest/retention-access.test.tstest/retention.test.tstest/routines.test.tstest/sentinels.test.tstest/signals.test.tstest/sketches.test.tstest/skill-extract.test.tstest/sliding-window.test.tstest/smart-search.test.tstest/snapshot.test.tstest/team.test.tstest/temporal-graph.test.tstest/timeline.test.tstest/verify.test.tstest/working-memory.test.ts
| { | ||
| "name": "@agentmemory/agentmemory", | ||
| "version": "0.8.10", | ||
| "version": "0.8.11", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TARGET="0.8.11"
FILES=(
"package.json"
"src/version.ts"
"src/types.ts"
"src/functions/export-import.ts"
"test/export-import.test.ts"
"plugin/.claude-plugin/plugin.json"
)
for f in "${FILES[@]}"; do
if [ -f "$f" ]; then
echo "== $f =="
rg -n --fixed-strings "$TARGET" "$f" || echo "No $TARGET found in $f"
else
echo "MISSING: $f"
fi
echo
doneRepository: rohitg00/agentmemory
Length of output: 897
🏁 Script executed:
rg -n "version" src/types.ts -A 2 -B 2 | head -50Repository: rohitg00/agentmemory
Length of output: 664
Add version 0.8.11 to the ExportData version union.
The version update is missing from src/types.ts line 255. Add "0.8.11" to the version field's type union in the ExportData interface:
Location: src/types.ts line 255
export interface ExportData {
version: "0.3.0" | "0.4.0" | "0.5.0" | "0.6.0" | "0.6.1" | "0.7.0" | "0.7.2" | "0.7.3" | "0.7.4" | "0.7.5" | "0.7.6" | "0.7.9" | "0.8.0" | "0.8.1" | "0.8.2" | "0.8.3" | "0.8.4" | "0.8.5" | "0.8.6" | "0.8.7" | "0.8.8" | "0.8.9" | "0.8.10";
Should include "0.8.11" at the end of the union.
All other version references (package.json, src/version.ts, src/functions/export-import.ts, test/export-import.test.ts, plugin/.claude-plugin/plugin.json) are correctly updated to 0.8.11.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 3, Update the ExportData interface's version union to
include the new release string by adding "0.8.11" to the existing union in the
ExportData type; locate the ExportData interface and append the new literal to
the version field's union so it matches the package.json and other updated
version references.
Comment on lines
+29
to
+35
| function fmt(level: string, msg: string, fields: Fields): string { | ||
| if (!fields || Object.keys(fields).length === 0) { | ||
| return `[agentmemory] ${level} ${msg}`; | ||
| } | ||
| try { | ||
| return `[agentmemory] ${level} ${msg} ${JSON.stringify(fields)}`; | ||
| } catch { |
There was a problem hiding this comment.
Guard the Object.keys check inside the fallback try/catch.
Object.keys(fields) can throw on edge-case objects (e.g., proxies), so fmt can still throw before reaching your fallback. Keep the emptiness check inside the try path.
Proposed patch
function fmt(level: string, msg: string, fields: Fields): string {
- if (!fields || Object.keys(fields).length === 0) {
- return `[agentmemory] ${level} ${msg}`;
- }
try {
+ if (!fields || Object.keys(fields).length === 0) {
+ return `[agentmemory] ${level} ${msg}`;
+ }
return `[agentmemory] ${level} ${msg} ${JSON.stringify(fields)}`;
} catch {
// Fields contained a circular reference or a BigInt — fall back
// to the plain message so a log line never throws.
return `[agentmemory] ${level} ${msg}`;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function fmt(level: string, msg: string, fields: Fields): string { | |
| if (!fields || Object.keys(fields).length === 0) { | |
| return `[agentmemory] ${level} ${msg}`; | |
| } | |
| try { | |
| return `[agentmemory] ${level} ${msg} ${JSON.stringify(fields)}`; | |
| } catch { | |
| function fmt(level: string, msg: string, fields: Fields): string { | |
| try { | |
| if (!fields || Object.keys(fields).length === 0) { | |
| return `[agentmemory] ${level} ${msg}`; | |
| } | |
| return `[agentmemory] ${level} ${msg} ${JSON.stringify(fields)}`; | |
| } catch { | |
| // Fields contained a circular reference or a BigInt — fall back | |
| // to the plain message so a log line never throws. | |
| return `[agentmemory] ${level} ${msg}`; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/logger.ts` around lines 29 - 35, The emptiness check using
Object.keys(fields) currently sits outside the try/catch and can itself throw
(e.g., for proxy objects); move the Object.keys(fields) call and the "no fields"
branch inside the existing try block in function fmt so any exception from
accessing fields is caught, then keep the current behavior: if fields is empty
return the base "[agentmemory] ${level} ${msg}" string, otherwise return the
string with JSON.stringify(fields); ensure the catch still returns a safe
fallback and references the same symbols (fmt, Object.keys, fields,
JSON.stringify) so the function never throws on malformed field objects.
| @@ -1 +1 @@ | |||
| export const VERSION: "0.3.0" | "0.4.0" | "0.5.0" | "0.6.0" | "0.6.1" | "0.7.0" | "0.7.2" | "0.7.3" | "0.7.4" | "0.7.5" | "0.7.6" | "0.8.1" | "0.8.2" | "0.8.3" | "0.8.4" | "0.8.5" | "0.8.6" | "0.8.7" | "0.8.8" | "0.8.9" | "0.8.10" = "0.8.10"; | |||
| export const VERSION: "0.3.0" | "0.4.0" | "0.5.0" | "0.6.0" | "0.6.1" | "0.7.0" | "0.7.2" | "0.7.3" | "0.7.4" | "0.7.5" | "0.7.6" | "0.8.1" | "0.8.2" | "0.8.3" | "0.8.4" | "0.8.5" | "0.8.6" | "0.8.7" | "0.8.8" | "0.8.9" | "0.8.10" | "0.8.11" = "0.8.11"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify supportedVersions and ExportData.version include 0.8.11
echo "=== Checking supportedVersions in export-import.ts ==="
rg -n "supportedVersions" src/functions/export-import.ts -A2
echo ""
echo "=== Checking ExportData version union in types.ts ==="
rg -n "export interface ExportData" src/types.ts -A2Repository: rohitg00/agentmemory
Length of output: 863
Add "0.8.11" to ExportData.version union in src/types.ts.
The ExportData.version type union at line 255 is missing "0.8.11". This will cause TypeScript errors when exporting data with the new version.
Also note: Version union types are inconsistent across files. src/version.ts is missing "0.7.7", "0.7.9", and "0.8.0" that are defined in both supportedVersions and ExportData.version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/version.ts` at line 1, Update the ExportData.version union in
src/types.ts to include "0.8.11" so it matches the new VERSION, and reconcile
the version unions in src/version.ts by adding the missing entries ("0.7.7",
"0.7.9", "0.8.0") that appear in supportedVersions/ExportData; ensure the
exported constant VERSION remains set to "0.8.11" and that the union types in
src/version.ts and ExportData.version are consistent with supportedVersions.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/functions/export-import.ts (1)
179-180: Consider derivingsupportedVersionsfromVERSIONto reduce future drift.The new
"0.8.11"entry is correct, but future bumps are easy to miss with a fully hardcoded set. AppendingVERSIONdynamically can prevent that class of miss.♻️ Suggested refactor
- const supportedVersions = new Set(["0.3.0", "0.4.0", "0.5.0", "0.6.0", "0.6.1", "0.7.0", "0.7.2", "0.7.3", "0.7.4", "0.7.5", "0.7.6", "0.7.7", "0.7.9", "0.8.0", "0.8.1", "0.8.2", "0.8.3", "0.8.4", "0.8.5", "0.8.6", "0.8.7", "0.8.8", "0.8.9", "0.8.10", "0.8.11"]); + const supportedVersions = new Set([ + "0.3.0", "0.4.0", "0.5.0", "0.6.0", "0.6.1", + "0.7.0", "0.7.2", "0.7.3", "0.7.4", "0.7.5", "0.7.6", "0.7.7", "0.7.9", + "0.8.0", "0.8.1", "0.8.2", "0.8.3", "0.8.4", "0.8.5", "0.8.6", "0.8.7", "0.8.8", "0.8.9", "0.8.10", + VERSION, + ]);Based on learnings: “When bumping version, update: package.json (version field), src/version.ts (VERSION constant and type union), src/types.ts (ExportData version union), src/functions/export-import.ts (supportedVersions set), test/export-import.test.ts (version assertion), and plugin/.claude-plugin/plugin.json (version field)”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/export-import.ts` around lines 179 - 180, The supportedVersions set in export-import.ts is hardcoded and can drift from the VERSION constant; modify the logic that builds supportedVersions so it includes the current VERSION automatically (e.g., append or merge VERSION into the existing set/array) and then check importData.version against that set; update the symbol references: supportedVersions and importData.version (and ensure you use the exported VERSION constant from src/version.ts) so future version bumps to VERSION do not require manual edits to supportedVersions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/functions/export-import.ts`:
- Around line 179-180: The supportedVersions set in export-import.ts is
hardcoded and can drift from the VERSION constant; modify the logic that builds
supportedVersions so it includes the current VERSION automatically (e.g., append
or merge VERSION into the existing set/array) and then check importData.version
against that set; update the symbol references: supportedVersions and
importData.version (and ensure you use the exported VERSION constant from
src/version.ts) so future version bumps to VERSION do not require manual edits
to supportedVersions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5f83a2c-9c3e-41d7-ae5b-eba3c5d1abd7
📒 Files selected for processing (3)
plugin/.claude-plugin/plugin.jsonsrc/functions/export-import.tstest/export-import.test.ts
✅ Files skipped from review due to trivial changes (1)
- plugin/.claude-plugin/plugin.json
🚧 Files skipped from review as they are similar to previous changes (1)
- test/export-import.test.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.
Problem
PR #116 merged the iii-sdk v0.11 migration but left a runtime regression:
node dist/index.mjscrashed immediately with:iii-sdk v0.11 dropped
getContext()entirely. 32src/functions/*.tsfiles still imported and called it. The bug was invisible to CI because:iii-sdkwithvi.mock(...)so the real module never loadstsdownbundles without type-checking, so the missing export wasn't caught at build timeFix
src/logger.ts— new thin stderr shim with the same.info/.warn/.errorsignature the oldgetContext().loggerhad. Output goes tostderras[agentmemory] <level> <msg>, whichiii-execforwards intodocker logs agentmemory-iii-engine-1.src/functions/*.tsfiles — removedgetContextimports, deletedconst ctx = getContext()lines, replacedctx.logger.*withlogger.*src/functions/search.ts— also fixedregisterFunction({ id: '...' })→registerFunction('...')for the v0.11 string-ID APIvi.mock("iii-sdk", () => ({ getContext: ... }))→vi.mock("../src/logger.js", () => ({ logger: ... }))Verification
npm run build— cleannode dist/index.mjs— starts, logs all startup lines, connects to engine (or retries gracefully if no engine running)npm test— 731/731 passSummary by CodeRabbit
Bug Fixes
Chores