fix(runtime): make v0.9.1 dist actually start#184
Closed
Conversation
… onnx, restore KV.state The v0.9.1 dist was dead on arrival. Three independent bugs layered: 1) `getContext` was re-imported during PR #111 conflict resolution in three files (compress.ts, disk-size-manager.ts, image-quota-cleanup.ts). iii-sdk v0.11 dropped that export — the shim in src/logger.ts documents the fix. Tests mocked iii-sdk so `npm test` passed, but `node dist/cli.mjs` crashed on first import. Removed the imports, switched to the module-level logger. 2) The two PR #111 files also used the `registerFunction({ id, description }, handler)` shape. iii-sdk 0.11.2 only accepts `registerFunction(id, handler, options?)`. The dist imports succeeded but the runtime registration threw "not a function" on the test mock. Collapsed to the flat form. 3) `KV.state` was dropped from src/state/schema.ts during the same conflict resolution, which broke the disk-size-manager's persistence key. Restored `state: "mem:state"` — used only by this manager. 4) tsdown was inlining onnxruntime-node and @xenova/transformers into dist/. That rewrites the relative require() inside `onnxruntime-node/dist/binding.js` so it can't find `../bin/napi-v3/darwin/arm64/onnxruntime_binding.node` from dist/. Even without AGENTMEMORY_IMAGE_EMBEDDINGS the module graph still evaluated the bundled binding.js on startup and blew up. Added both packages (plus onnxruntime-web and the two Anthropic SDKs) to `external:` in tsdown.config.ts. Bundle shrank from 6.1 MB to 1.9 MB. The CLIP / local embedding providers lazy-load them from node_modules where relative paths work. 5) Bumped iii-sdk 0.11.0 → 0.11.2 to match the API currently shipped (Logger / durable:subscriber / durable:publisher / TriggerAction.void). 6) test/multimodal.test.ts used the old `{ id, description }` mock shape — rewrote the four registerFunction mocks to match the real `(id, cb)` signature. 812/812 pass. End-to-end smoke test with AGENTMEMORY_SLOTS=true AGENTMEMORY_REFLECT=true: - livez → 200 ok - GET /slots → all 8 defaults seeded into correct scopes (persona / user_preferences / tool_guidelines in global; rest in project) - POST /slot (invalid sizeLimit / unknown scope) → 400 with specific error - POST /slot/append overflow → 413 with currentSize + sizeLimit - POST /slot/reflect on empty session → no-op - GET /audit → every slot write + reflect emits an audit row - POST /vision-search without flag → 503 disabled
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated SDK dependency and build externals; removed unused context imports and switched to module-level logger; changed function registration to use string IDs; added KV state key and typed StateScope; test and CI workflow updates to match the API/installation changes. (48 words) Changes
Sequence Diagram(s)(omitted — changes are small, primarily refactors, typings, and config edits that do not introduce multi-component sequential flows) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/functions/image-quota-cleanup.ts (1)
56-68:⚠️ Potential issue | 🟠 MajorDo not delete images when ref-count lookup fails.
Line 61 logs the failure, but
refCountremains0, so the cleanup continues and can delete a still-referenced image after a transient KV/read error.🐛 Proposed fix
try { refCount = await getImageRefCount(kv, f.filePath); } catch (err) { logger.error("Failed to read refCount", { filePath: f.filePath, error: err instanceof Error ? err.message : String(err) }); + return; } if (refCount > 0) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/image-quota-cleanup.ts` around lines 56 - 68, The code currently treats a failed getImageRefCount call as refCount = 0 and proceeds to deleteImage; change the logic in the withKeyedLock block so that when getImageRefCount throws (caught in the catch), you do not continue with deletion: for example, set a sentinel (e.g., refCount = null) or rethrow the error and then return early if refCount is null/undefined; update the checks around refCount (> 0) to also guard for the sentinel so deleteImage(f.filePath) is only called when you successfully obtained a numeric refCount, referencing the getImageRefCount, refCount variable, withKeyedLock, and deleteImage symbols.test/multimodal.test.ts (1)
4-11:⚠️ Potential issue | 🟡 MinorRemove the stale
getContextmock from the SDK override.Lines 8-10 mock
getContext, but this export was removed iniii-sdkv0.11 and is not used anywhere in the codebase. The mock can mask future import regressions ifgetContextis accidentally reintroduced.Proposed cleanup
vi.mock("iii-sdk", async (importOriginal) => { const actual = await importOriginal<typeof import("iii-sdk")>(); return { ...actual, - getContext: () => ({ - logger: { info: vi.fn(), error: vi.fn(), warn: vi.fn() }, - }), }; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/multimodal.test.ts` around lines 4 - 11, Remove the stale getContext mock inside the vi.mock override in the test (the block returning ...actual and getContext); specifically, update the vi.mock import override that currently returns getContext: () => ({ logger: { info: vi.fn(), error: vi.fn(), warn: vi.fn() } }) to simply spread and return ...actual without defining getContext, so the test no longer mocks the removed iii-sdk export and cannot mask future import regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/state/schema.ts`:
- Line 48: The project is missing a type interface for the KV scope `KV.state`,
causing callers like `disk-size-manager.ts` to use ad-hoc
`kv.get<number>(KV.state, ...)`; add a definition in `src/types.ts` that
declares the `state` scope mapping (e.g., include an interface or type entry for
keys such as "system:currentDiskSize": number) so that `KV.state` has a typed
shape; update the exported types so callers using `kv.get`/`kv.put` can
reference the new `state` interface instead of inline generics.
---
Outside diff comments:
In `@src/functions/image-quota-cleanup.ts`:
- Around line 56-68: The code currently treats a failed getImageRefCount call as
refCount = 0 and proceeds to deleteImage; change the logic in the withKeyedLock
block so that when getImageRefCount throws (caught in the catch), you do not
continue with deletion: for example, set a sentinel (e.g., refCount = null) or
rethrow the error and then return early if refCount is null/undefined; update
the checks around refCount (> 0) to also guard for the sentinel so
deleteImage(f.filePath) is only called when you successfully obtained a numeric
refCount, referencing the getImageRefCount, refCount variable, withKeyedLock,
and deleteImage symbols.
In `@test/multimodal.test.ts`:
- Around line 4-11: Remove the stale getContext mock inside the vi.mock override
in the test (the block returning ...actual and getContext); specifically, update
the vi.mock import override that currently returns getContext: () => ({ logger:
{ info: vi.fn(), error: vi.fn(), warn: vi.fn() } }) to simply spread and return
...actual without defining getContext, so the test no longer mocks the removed
iii-sdk export and cannot mask future import regressions.
🪄 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: e6f54054-ee84-4582-96d8-09583f0bb83c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.jsonsrc/functions/compress.tssrc/functions/disk-size-manager.tssrc/functions/image-quota-cleanup.tssrc/state/schema.tstest/multimodal.test.tstsdown.config.ts
Policy: never commit lock files. Downstream developers and CI regenerate them at install time. Committing them creates churn on every transitive-dep bump and masks real dependency changes in PRs.
|
Only repository collaborators, contributors, or members can run CodeRabbit commands. |
- CI (ci.yml + publish.yml): switch from `npm ci` to `npm install --legacy-peer-deps --no-audit --no-fund`. `npm ci` requires a committed lockfile; we gitignore lockfiles by policy (#184 follow-up b1bb2b2), so `npm install` is the correct path. - types.ts: add `StateScope` interface documenting the KV.state scope shape (per CodeRabbit suggestion on schema.ts:48). Today it only holds `system:currentDiskSize: number`; future state-scope keys should register here to get compile-time key/value checks across the codebase. - disk-size-manager.ts: use the new `StateScope` / `StateScopeKey` types for get/set so type safety flows through instead of the previous ad-hoc `kv.get<number>` annotation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/types.ts (1)
853-857: Trim the WHAT-style JSDoc.This comment mostly restates the adjacent
StateScopetype. Consider removing it or limiting it to non-obvious constraints that cannot be encoded in the type. As per coding guidelines,src/**/*.ts: “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/types.ts` around lines 853 - 857, Remove the redundant WHAT-style JSDoc above the StateScope declaration: either delete the block entirely or replace it with a short, specific note only for non-obvious constraints that the type cannot encode (for example, "keys must be kept in sync with disk-size-manager and other callers"). Ensure the comment no longer restates the type and only documents constraints that need human attention while leaving the StateScope type itself as the source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish.yml:
- Around line 28-30: The workflow currently runs a fresh npm install using "npm
install --legacy-peer-deps" which lets transitive deps vary between runs; change
the job to first generate a package-lock.json deterministically with "npm
install --package-lock-only --legacy-peer-deps" (or "npm shrinkwrap" if
preferred) and then use "npm ci --legacy-peer-deps" for the actual install
before running "npm run build" and "npm test", so the build+publish steps (and
the --provenance attestation) use the same resolved dependency graph and avoid
rebuilding dependencies unpinned between steps.
---
Nitpick comments:
In `@src/types.ts`:
- Around line 853-857: Remove the redundant WHAT-style JSDoc above the
StateScope declaration: either delete the block entirely or replace it with a
short, specific note only for non-obvious constraints that the type cannot
encode (for example, "keys must be kept in sync with disk-size-manager and other
callers"). Ensure the comment no longer restates the type and only documents
constraints that need human attention while leaving the StateScope type itself
as the source of truth.
🪄 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: f293c430-4cc6-4afb-b807-c0a943de56a3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/publish.ymlsrc/functions/disk-size-manager.tssrc/types.ts
…blish Follow-up to the npm-install switch in 121322b. `npm install` resolves transitive deps fresh each step, so the version graph under which we `build` + `test` can differ from what the publish step attests with `--provenance`. Split install into two steps: 1. `npm install --package-lock-only ...` — generates a lockfile in the runner workspace (not committed, lockfiles stay gitignored). 2. `npm ci --legacy-peer-deps ...` — installs deterministically from that lockfile. Now every step in a single job run uses the same resolved dependency graph. Still no cross-run pinning (that is by design — we do not commit lockfiles), only intra-job consistency. Addresses CodeRabbit feedback on publish.yml. Applies the same pattern to ci.yml for consistency.
rohitg00
added a commit
that referenced
this pull request
Apr 22, 2026
Findings verified against current code on this branch; all four valid. 1. config.ts loadFallbackConfig (L281) — user could set FALLBACK_PROVIDERS=agent-sdk and bypass the AGENTMEMORY_ALLOW_AGENT_SDK gate added to detectProvider. Filter it out at the fallback layer too, with the same warning pointing at the opt-in flag. 2. summarize.ts (L87-92) — the empty_provider_response branch returned without recording failure metrics or a diagnostic log, unlike the parse/validation paths. Record the same metricsStore failure event and log provider name, prompt size, system size, and observation count so empty responses are visible in telemetry. 3. providers/agent-sdk.ts (L14-45) — setting process.env.AGENTMEMORY_SDK_CHILD = '1' without restoring it caused every subsequent .query() in the same parent process to hit the short-circuit guard and return '' (classified as a SDK child it is not). Capture prev, set in try, restore in finally (delete if prev was undefined). Child processes spawned during the for-await loop still inherit the marker because env is inherited at spawn time; we only restore after the loop completes. 4. plugin/scripts/sdk-guard-DI1NUOS9.mjs — tsdown extracted the shared guard helper into a hashed chunk. Hash rotates on every rebuild and churns the diff. Stopped using the shared module from hooks entirely and inlined the 6-line guard function into each hook .ts file instead. sdk-guard.ts stays in the tree because the unit tests cover it directly. Deleted the tracked hashed .mjs and confirmed no new chunk is emitted. Also applied the CI two-step install (npm install --package-lock-only then npm ci) on this branch, matching #184. Without it, npm ci fails because lockfiles are gitignored. Tests: 74 files / 819 tests pass.
rohitg00
added a commit
that referenced
this pull request
Apr 22, 2026
Pulls in #184 runtime fix, #188 viewer pipeline, lockfile gitignore, CI two-step install, StateScope types, firstPrompt typeof guard, content-addressed lesson/crystal IDs, image-quota fail-closed, and onnxruntime optionalDependencies. Resolves workflow conflicts flagged by CodeRabbit — ci.yml / publish.yml on main are already the correct two-step install pattern. # Conflicts: # .github/workflows/publish.yml
Owner
Author
|
Closing — content already landed on
|
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
v0.9.1 dist crashed on first import. Local reproduction during end-to-end slot testing surfaced four independent bugs that each block startup.
What was broken
getContextre-imported in three files — PR feat: implement multimodal image memory #111's conflict resolution re-added `import { getContext } from "iii-sdk"` to `compress.ts`, `disk-size-manager.ts`, `image-quota-cleanup.ts`. iii-sdk v0.11 dropped that export (see the shim comment in `src/logger.ts`). Tests mocked iii-sdk so `npm test` passed, but `node dist/cli.mjs` died with `The requested module 'iii-sdk' does not provide an export named 'getContext'`.What shipped
Verified end-to-end
```
$ AGENTMEMORY_SLOTS=true AGENTMEMORY_REFLECT=true node dist/cli.mjs
[agentmemory] Slots: enabled (pinned editable memory). Reflect on Stop hook: on
[agentmemory] Ready. BM25+Graph search active.
[agentmemory] Endpoints: 107 REST + 51 MCP tools + 6 MCP resources + 3 MCP prompts
$ curl :3111/agentmemory/livez → 200 { status: ok }
$ curl :3111/agentmemory/slots → 8 defaults seeded into correct scopes
$ curl -X POST :3111/agentmemory/slot/append -d '{"label":"persona","text":"..."}' → 200
$ curl -X POST :3111/agentmemory/slot -d '{"label":"x","sizeLimit":-1}' → 400 sizeLimit must be a positive integer
$ curl -X POST :3111/agentmemory/slot -d '{"label":"x","scope":"other"}' → 400 scope must be 'project' or 'global'
$ curl -X POST :3111/agentmemory/slot/append -d '{"label":"tight","text":"<600 chars>"}' → 413 append would exceed sizeLimit
$ curl -X POST :3111/agentmemory/slot/reflect -d '{"sessionId":"empty"}' → { applied: 0 }
$ curl -X POST :3111/agentmemory/vision-search -d '{"queryText":"x"}' → 503 image embeddings disabled
```
Test plan
Summary by CodeRabbit
Chores
Refactor
New
Tests