Skip to content

fix(viewer): repair empty tabs + enrich session detail + auto-derive lessons/crystals on import#188

Merged
rohitg00 merged 8 commits intomainfrom
fix/viewer-empty-tabs-and-import-pipeline
Apr 22, 2026
Merged

fix(viewer): repair empty tabs + enrich session detail + auto-derive lessons/crystals on import#188
rohitg00 merged 8 commits intomainfrom
fix/viewer-empty-tabs-and-import-pipeline

Conversation

@rohitg00
Copy link
Copy Markdown
Owner

@rohitg00 rohitg00 commented Apr 22, 2026

Summary

Fixes a pile of viewer issues where tabs rendered empty despite data existing, sessions looked like useless hex IDs with no context, and JSONL imports silently broke the search + consolidation pipelines.

Stacks on #184

Includes the runtime fix from #184 (a12767b drop getContext / externalize onnx / restore KV.state) and the lockfile gitignore (b1bb2b2). Merge #184 first, rebase drops to 3 commits. Kept bundled because without #184 the dist will not start and none of this UI work can be verified locally.

What was broken

  • Import pipeline: import-jsonl wrote RawObservation shape directly to KV, never ran synthetic compression, never indexed BM25. Result: imported sessions were invisible to search, consolidation reported "fewer than 5 summaries", memories / semantic / procedural stayed empty.
  • Audit tab: /agentmemory/audit returned a bare array. Viewer expected { entries, success }. Rendered empty despite populated data.
  • Replay tab: /agentmemory/replay/sessions went via sdk.trigger → mem::replay::sessions, which timed out past ~400 sessions. Users with real history hit a 30s hang.
  • Sessions list: just showed "rohitghumare · sess_xxxx · 6 obs" with no preview of what the session was about.
  • Sessions detail: clicking did nothing visible — panel rendered below the list but without observations context.
  • Memories strength: m.strength = 7 scored × 100 rendered as "700%".
  • Memories tab: no header, users could not tell if those were all memories or a filtered subset.
  • CONNECTING…: WS upgrade to iii-stream port sometimes accepted TCP but never completed handshake. new WebSocket() has no browser-level timeout, so the banner stuck on "CONNECTING…" forever and the polling fallback never kicked in.
  • Crystals tab: empty by default, and when populated, showed raw lesson_xxx IDs instead of lesson content.
  • Lessons tab: no explainer for how lessons are sourced / scored / decayed.

What changes

Pipeline (src/functions/replay.ts, src/functions/observe.ts, src/types.ts, src/functions/summarize.ts ← N/A — left alone)

  • import-jsonl: runs buildSyntheticCompression on each parsed observation, writes compressed shape to KV.observations, adds to BM25 via getSearchIndex().add(). Same path live observe uses, so imports behave like captured observations from Claude Code hooks.
  • Session enrichment: capture the first userPrompt from parsed JSONL as session.firstPrompt. Live observe does the same when it sees its first prompt_submit.
  • Auto-derivation on import:
    • Crystals — one per imported session. Narrative = first prompt (or observation preview fallback), keyOutcomes = distinct tool titles, filesAffected = extracted file paths, lessons = derived lesson IDs.
    • Lessons — regex match imperative patterns in user/assistant text (always / never / don't / prefer / avoid / caveat / note / warning), dedupe, save up to 20 per session with confidence: 0.4, tag auto-import, source consolidation.
    • Actions deliberately NOT auto-created. Actions are user goals, not tool logs.
    • Semantic / procedural still require the consolidation pipeline (needs LLM summaries).
  • Session interface gets firstPrompt? + summary? fields.

API shape fixes (src/triggers/api.ts)

  • api::audit wraps response as { entries, success: true } so the viewer's expected shape is satisfied.
  • api::replay::sessions now calls kv.list<Session>(KV.sessions) + .sort() directly instead of sdk.trigger. Sub-50ms on 600+ sessions.

Viewer (src/viewer/index.html)

  • Sessions list now shows a firstPrompt / summary preview row under each item.
  • Session click handler is async: fetches observations for the clicked session, renders a detail panel with a preview callout, a 4-stat grid (observations / tools / files / duration), a top-10 tool bar chart, activity-type breakdown badges, a file list, and metadata.
  • Crystals tab: header explainer card, redesigned cards with pills (project / session / counts), tool badges, file list, and lesson content resolved via a lessons fetch (stored in state.crystals.lessonMap).
  • Lessons tab: header explainer card describing the rule + confidence + decay model so the auto-import tag is not confusing.
  • Memories tab: header explainer card; strength display handles both 0–1 and 0–10 scales and clamps at 100%.
  • Actions tab: fixed wrong field read — frontier response is { frontier: [...] }, not { actions: [...] }.
  • WS: 5-second connect timeout. If the socket is still CONNECTING after that, forcibly ws.close()onclose fires → retry / polling-fallback chain takes over. Stops the forever "CONNECTING…" banner.

Test plan

  • npm run build — clean
  • npm test — 812 / 812 pass
  • Local: AGENTMEMORY_SLOTS=true AGENTMEMORY_REFLECT=true GRAPH_EXTRACTION_ENABLED=true CONSOLIDATION_ENABLED=true node dist/cli.mjs start + node dist/cli.mjs import-jsonl ~/.claude/projects/-Users-<user>/<uuid>.jsonl
    • Sessions tab shows preview for newly imported sessions.
    • Lessons tab has 10 auto-extracted rules tagged auto-import at 40% confidence.
    • Crystals tab has 1 crystal per session with tool badges, file list, lesson content.
    • Audit tab renders populated entries.
    • Replay tab loads in under a second.
    • LIVE banner settles green instead of stuck CONNECTING.

What is still empty (not a bug)

Summary by CodeRabbit

  • New Features

    • Sessions show preview text (firstPrompt/summary); richer Session detail view with observations, activity, files, tools and duration.
    • Replay import now creates compressed observations and auto-extracts/persists Lessons and Crystals.
    • Crystals and Lessons surfaced in UI; Memories header and improved empty states.
    • WebSocket connect timeout added.
  • Bug Fixes

    • Safer cleanup: skip eviction on read failures and clearer logging.
    • Improved memory strength percentage calculation.
  • Chores

    • Updated package deps, build config, CI/publish install steps, and .gitignore.
  • Tests

    • Updated test mocks to match new registration signature.

… 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
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.
- import-jsonl: run synthetic compression + BM25 index per observation
  (previously raw obs never got indexed; search/summaries never saw them)
- import-jsonl + observe: capture first userPrompt as session.firstPrompt
  so sessions list shows what each session was about, not just an ID
- api::audit: wrap bare array in { entries, success } — viewer expected
  { entries } shape and was rendering empty despite populated data
- api::replay::sessions: call kv.list directly instead of trigger hop
  (was timing out at 10s+ with 600+ sessions)
- viewer sessions tab: show firstPrompt preview under each session
- viewer memories tab: fix strength display (7 * 100 = 700%; now scaled
  properly for both 0-1 and 0-10 representations, clamped to 100)
- viewer memories tab: add explainer card describing what memories are
- viewer WS: add 5s connect timeout so stuck WS upgrades fall through
  to polling mode instead of showing CONNECTING... forever
import-jsonl now runs lightweight heuristic extraction on imported
sessions so the Lessons and Crystals tabs populate from replay data
instead of staying empty until a user manually invokes the MCP tools.

- Lessons: regex-match imperative patterns in user/assistant text
  (always/never/don't/prefer/avoid/caveat) and save top 20 per session
  with low confidence (0.4) and an 'auto-import' tag so they're clearly
  derived rather than hand-curated.
- Crystals: one crystal per imported session — narrative = first user
  prompt or observation preview, keyOutcomes = distinct tool names,
  filesAffected = extracted file paths, lessons = derived lesson IDs.
- Actions stay deliberately empty (user-initiated goals, not tool logs).
- Semantic/procedural still require consolidation pipeline summaries.
- Crystals tab: add header card explaining what crystals are; redesign
  each card with narrative, pills (project/session/counts), tool badges,
  files list, and resolved lesson content (fetched via /lessons and
  mapped by id) instead of showing raw lesson UUIDs.
- Lessons tab: add header card explaining the rule/confidence/decay
  model so users understand the low-confidence auto-import tag.
- Sessions tab: clicking a session now fetches its observations and
  renders a full detail panel — preview callout, 4-card grid
  (observations / tools / files / duration), top tool bar chart,
  activity type breakdown, file list, metadata block, actions row.
- Pull in lessons when loading crystals so auto-generated lessons are
  actually readable in context.
@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 22, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
agentmemory Ready Ready Preview, Comment Apr 22, 2026 10:56am

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 22, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d85e1248-1ec1-4ada-85c9-72050ef29e0e

📥 Commits

Reviewing files that changed from the base of the PR and between 81be407 and 510d6ff.

📒 Files selected for processing (1)
  • src/config.ts
✅ Files skipped from review due to trivial changes (1)
  • src/config.ts

📝 Walkthrough

Walkthrough

Adds synthetic observation compression and lesson/crystal derivation during replay import; extends Session with firstPrompt and summary; introduces a typed StateScope and KV.state; switches function registration to string IDs and module-level loggers; updates API handlers to read from KV; enhances viewer UI (session previews, lesson resolution, WS connect timeout); updates deps, build externals, and CI install steps.

Changes

Cohort / File(s) Summary
Config & CI
\.gitignore, package.json, tsdown.config.ts, .github/workflows/ci.yml, .github/workflows/publish.yml
Added lockfile ignores; bumped iii-sdk; added onnxruntime-* to optionalDependencies; marked several packages as externals for bundling; changed CI/publish to two-step lockfile-generation + npm ci with audit/fund disabled.
Types & State schema
src/types.ts, src/state/schema.ts
Added Session.firstPrompt? and Session.summary?; introduced StateScope with "system:currentDiskSize": number, StateScopeKey type alias, and added KV.state = "mem:state".
Function registration & logging
src/functions/compress.ts, src/functions/disk-size-manager.ts, src/functions/image-quota-cleanup.ts, test/multimodal.test.ts
Switch to string IDs in sdk.registerFunction; removed getContext() usage and ctx.logger in favor of module logger; disk-size registration uses "mem::disk-size-delta"; tests updated to mock (id, cb) signature and assert by id.
Observation & Replay logic
src/functions/observe.ts, src/functions/replay.ts
observe now conditionally persists normalized/truncated firstPrompt; replay import builds/stores synthetic/compressed observations, indexes them, persists earliest firstPrompt, and calls deriveCrystalAndLessons(...) to extract and persist lessons/crystals.
Quota & disk management
src/functions/disk-size-manager.ts, src/functions/image-quota-cleanup.ts
Typed DISK_SIZE_KEY as StateScopeKey; KV access typed via StateScope[...]; logging simplified; image cleanup now fails-closed when ref-count read errors (skip eviction and log error).
API triggers
src/triggers/api.ts
api::replay::sessions now reads/sorts sessions directly from KV and returns { success: true, sessions }; api::audit response wrapped as { entries, success: true }.
Viewer UI
src/viewer/index.html
Load lessons + build lessonMap; session previews use `firstPrompt
Build/test config
tsdown.config.ts, test/multimodal.test.ts
Marked several deps external for bundling; updated tests to new registerFunction signature and id-based assertions.

Sequence Diagram(s)

sequenceDiagram
    participant Import as JSONL Import
    participant SessionKV as Session KV
    participant ObsKV as Observation KV
    participant Compress as Compression Module
    participant Index as Search Index
    participant Deriver as Lesson/Crystal Deriver

    Import->>Import: Parse JSONL into raw observations
    Import->>Import: Find earliest non-empty userPrompt -> firstPrompt
    Import->>SessionKV: Conditionally persist firstPrompt if missing

    loop for each raw observation
        Import->>Compress: buildSyntheticCompression(raw)
        Compress-->>Import: syntheticObs
        Import->>ObsKV: Store syntheticObs under sessionId
        Import->>Index: add(syntheticObs)
    end

    Import->>Deriver: deriveCrystalAndLessons(rawObsList, syntheticObsList, firstPrompt)
    Deriver->>Deriver: Extract lesson texts, create KV.lessons (content-addressed)
    Deriver->>Deriver: Aggregate lesson IDs/files -> KV.crystals
    Deriver-->>Import: return created lesson/crystal IDs
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

Poem

🐰 I nibble lines and stitch each clue,
First prompts caught and compressed anew,
Lessons sprout and crystals shine bright,
Sessions hum, the viewer lights the night. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately summarizes the three main changes: viewer tab repairs, session detail enrichment, and auto-derivation of lessons/crystals on import, matching the detailed objectives.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/viewer-empty-tabs-and-import-pipeline

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/image-quota-cleanup.ts (1)

56-68: ⚠️ Potential issue | 🟠 Major

Skip eviction when the ref-count lookup fails.

Line 61 logs the failure but leaves refCount at 0, so a transient KV/read error falls through to deleteImage() for an image that may still be referenced.

🛡️ 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 ref-count
read failure leaves refCount at 0 and causes deleteImage to run; update the
withKeyedLock callback (surrounding getImageRefCount in
src/functions/image-quota-cleanup.ts) so that if getImageRefCount throws you do
not proceed with eviction: on catch of the error from getImageRefCount, log the
error as you do and then return early (or rethrow) from the callback to skip
deleteImage; reference the withKeyedLock callback, getImageRefCount, refCount
variable, and deleteImage to locate and change the logic.
🧹 Nitpick comments (2)
tsdown.config.ts (1)

25-31: onnxruntime packages are purely transitive dependencies; no direct imports found.

Verification found no direct imports of onnxruntime-node or onnxruntime-web anywhere in the source code—only in the external array of tsdown.config.ts. This means the concern about non-lazy code paths triggering MODULE_NOT_FOUND errors does not apply. These packages are only ever accessed transitively through @xenova/transformers, which is already optional and lazy-loaded (see clip.ts).

For clarity and explicit intent, consider declaring onnxruntime-node and onnxruntime-web as optionalDependencies alongside @xenova/transformers, but this is not required.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tsdown.config.ts` around lines 25 - 31, The external array in
tsdown.config.ts lists "onnxruntime-node" and "onnxruntime-web" even though
there are no direct imports and they are only transitively used via
"@xenova/transformers"; either remove those two packages from the external:
[...] declaration to avoid misleading configuration, or document the intent with
an inline comment and instead add them as optionalDependencies in package.json
alongside "@xenova/transformers" to make their optional/transitive nature
explicit; update the external array and package.json accordingly and ensure the
change references the external array symbol in tsdown.config.ts and the
optionalDependencies section in package.json.
src/state/schema.ts (1)

48-48: Add a typed interface for the new KV.state scope.

KV.state now stores state entries such as system:currentDiskSize, but src/types.ts does not define the corresponding value shape/key map. Add a small exported interface so callers do not keep using ad-hoc number generics for this scope. Based on learnings: When adding new KV scopes, add to the KV object in src/state/schema.ts and add corresponding interface in src/types.ts.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/state/schema.ts` at line 48, Add a typed interface for the KV.state scope
and export it from src/types.ts so callers stop using ad-hoc number generics;
define an interface (e.g., KVState or StateKV) that maps known keys like
"system:currentDiskSize" to their value types (number) and export it, then
update usages to use that interface as the generic for KV.state. Ensure the
KV.state entry remains in src/state/schema.ts (state: "mem:state") and wire the
new exported interface name into any type declarations referencing KV.state so
the scope has a proper key->value shape.
🤖 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/functions/observe.ts`:
- Around line 206-211: The code pushes a sanitized firstPrompt using
raw.userPrompt without ensuring it's actually a string; add a runtime guard
(e.g. typeof raw.userPrompt === "string") before calling replace/trim/slice in
the observe flow so non-string truthy values won't throw. Update the conditional
that currently reads (!session.firstPrompt && raw.userPrompt) to validate
stringness and only push the update to the updates array when raw.userPrompt is
a string; keep the same sanitization (replace(/\s+/g," ").trim().slice(0,200))
but only apply it after the type check so functions like replace/trim are never
called on non-strings.

In `@src/functions/replay.ts`:
- Around line 112-156: Replace non-deterministic IDs with content-addressed IDs
and upserts: for lessons, stop using generateId("lesson")—use
fingerprintId("lsn", content.trim().toLowerCase()) to derive lessonId, check
kv.get(KV.lessons, lessonId) and only create/kv.set if missing (or merge/update
existing) and push the canonical lessonId into lessonIds; for crystals, avoid
generateId("crystal") by either fingerprinting on sessionId (e.g.,
fingerprintId("crl", sessionId)) or first querying existing crystals for this
sessionId and reusing that id, then perform an upsert via kv.set(KV.crystals,
crystalId, crystal) so repeated imports are idempotent (update existing records
instead of always creating new ones).

In `@src/viewer/index.html`:
- Around line 2686-2691: The search currently only uses raw c.lessons (IDs) in
the items filter, so visible lesson text doesn't match; update the filter inside
the items = items.filter(...) to resolve each lesson ID via lessonMap (use
state.crystals.lessonMap) and include the lesson content/string (e.g.,
lessonMap[id].content or the appropriate field) when building the haystack,
replacing or augmenting (c.lessons || []).join(' ') so the combined string uses
the resolved lesson text for matching.

---

Outside diff comments:
In `@src/functions/image-quota-cleanup.ts`:
- Around line 56-68: The ref-count read failure leaves refCount at 0 and causes
deleteImage to run; update the withKeyedLock callback (surrounding
getImageRefCount in src/functions/image-quota-cleanup.ts) so that if
getImageRefCount throws you do not proceed with eviction: on catch of the error
from getImageRefCount, log the error as you do and then return early (or
rethrow) from the callback to skip deleteImage; reference the withKeyedLock
callback, getImageRefCount, refCount variable, and deleteImage to locate and
change the logic.

---

Nitpick comments:
In `@src/state/schema.ts`:
- Line 48: Add a typed interface for the KV.state scope and export it from
src/types.ts so callers stop using ad-hoc number generics; define an interface
(e.g., KVState or StateKV) that maps known keys like "system:currentDiskSize" to
their value types (number) and export it, then update usages to use that
interface as the generic for KV.state. Ensure the KV.state entry remains in
src/state/schema.ts (state: "mem:state") and wire the new exported interface
name into any type declarations referencing KV.state so the scope has a proper
key->value shape.

In `@tsdown.config.ts`:
- Around line 25-31: The external array in tsdown.config.ts lists
"onnxruntime-node" and "onnxruntime-web" even though there are no direct imports
and they are only transitively used via "@xenova/transformers"; either remove
those two packages from the external: [...] declaration to avoid misleading
configuration, or document the intent with an inline comment and instead add
them as optionalDependencies in package.json alongside "@xenova/transformers" to
make their optional/transitive nature explicit; update the external array and
package.json accordingly and ensure the change references the external array
symbol in tsdown.config.ts and the optionalDependencies section in package.json.
🪄 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: e235bdfd-5ecf-4d42-bc9f-9e31dec407ce

📥 Commits

Reviewing files that changed from the base of the PR and between 480d8e8 and 9066086.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (13)
  • .gitignore
  • package.json
  • src/functions/compress.ts
  • src/functions/disk-size-manager.ts
  • src/functions/image-quota-cleanup.ts
  • src/functions/observe.ts
  • src/functions/replay.ts
  • src/state/schema.ts
  • src/triggers/api.ts
  • src/types.ts
  • src/viewer/index.html
  • test/multimodal.test.ts
  • tsdown.config.ts

Comment thread src/functions/observe.ts Outdated
Comment thread src/functions/replay.ts
Comment thread src/viewer/index.html
All six findings verified against current code on this branch; all valid.

1. observe.ts firstPrompt (L206-211) — raw.userPrompt is typed as
   optional string but populated from payload.data which is unknown at
   runtime; a non-string truthy value (array, object) would crash at
   .replace(). Guard with typeof === 'string' before sanitizing, and
   skip the update when the trimmed value is empty.

2. replay.ts deriveCrystalAndLessons — generateId('lesson') /
   generateId('crystal') produced fresh IDs on every import, so
   re-importing the same JSONL duplicated lessons and crystals. Switch
   to fingerprintId:
   - lessons keyed by fingerprintId('lesson', content.toLowerCase()) +
     merge on existing (bump reinforcements, append sessionId to
     sourceIds, union tags).
   - crystals keyed by fingerprintId('crystal', sessionId) + preserve
     original createdAt and sourceActionIds when upserting.

3. viewer/index.html crystals filter (L2686-2691) — search haystack
   previously joined raw lesson IDs (lesson_xxx), so lesson text never
   matched. Resolve each ID via state.crystals.lessonMap and include
   resolved content in the haystack; also fold in filesAffected and
   project so the search feels complete.

4. image-quota-cleanup.ts (L56-68) — when getImageRefCount threw, the
   refCount variable stayed at its default 0 and the code happily
   proceeded to deleteImage(). That could delete a still-referenced
   image on any transient KV error. Fail-closed: log and return from
   the withKeyedLock callback on error so deleteImage is never reached
   without a confirmed refCount === 0.

5. types.ts / disk-size-manager.ts — add a StateScope interface that
   maps KV.state scope keys to their value types
   (system:currentDiskSize: number), export StateScopeKey, and have
   disk-size-manager use StateScope[typeof DISK_SIZE_KEY] in kv.get /
   kv.set generics instead of the ad-hoc <number>.

6. tsdown.config.ts + package.json — onnxruntime-node / onnxruntime-web
   are not imported directly; they come in transitively via
   @xenova/transformers. Keep them in tsdown's external array (bundling
   them breaks the relative .node binding paths in dist/) and add both
   to package.json optionalDependencies alongside @xenova/transformers
   so the optional/lazy nature is explicit. Expand the tsdown comment
   to reference the new optionalDependencies entries and name the exact
   src files that lazy-import them.

812/812 tests pass.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/types.ts (1)

855-860: Drop the WHAT-style comment here.

StateScope and "system:currentDiskSize" are already self-descriptive, and this comment mostly restates the shape/usage.

Proposed cleanup
-/**
- * KV.state scope — long-lived system counters + flags keyed by string.
- * Keep keys/types in sync with the state-scope callers (e.g.,
- * disk-size-manager) so TypeScript enforces consistent value shapes
- * instead of every caller using ad-hoc `<number>` generics.
- */
 export interface StateScope {

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/types.ts` around lines 855 - 860, Remove the redundant WHAT-style comment
block above StateScope that describes the KV.state scope and the
"system:currentDiskSize" key; since StateScope and the key name are
self-descriptive, delete the comment lines and leave the type definition and any
brief doc that explains WHY only if necessary, ensuring symbols StateScope and
"system:currentDiskSize" remain unchanged.
🤖 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/types.ts`:
- Around line 855-860: Remove the redundant WHAT-style comment block above
StateScope that describes the KV.state scope and the "system:currentDiskSize"
key; since StateScope and the key name are self-descriptive, delete the comment
lines and leave the type definition and any brief doc that explains WHY only if
necessary, ensuring symbols StateScope and "system:currentDiskSize" remain
unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 46e6f2af-5a43-45ba-b0c1-c57242439454

📥 Commits

Reviewing files that changed from the base of the PR and between fab246a and 81be407.

📒 Files selected for processing (8)
  • package.json
  • src/functions/disk-size-manager.ts
  • src/functions/image-quota-cleanup.ts
  • src/functions/observe.ts
  • src/functions/replay.ts
  • src/types.ts
  • src/viewer/index.html
  • tsdown.config.ts
✅ Files skipped from review due to trivial changes (1)
  • src/functions/observe.ts
🚧 Files skipped from review as they are similar to previous changes (6)
  • package.json
  • src/functions/disk-size-manager.ts
  • tsdown.config.ts
  • src/functions/image-quota-cleanup.ts
  • src/functions/replay.ts
  • src/viewer/index.html

Follow-up to CodeRabbit review on #188. detectProvider's agent-sdk
fallback at L93-97 hardcoded maxTokens: 4096, ignoring the maxTokens
variable computed from env["MAX_TOKENS"] at L45. Every other provider
branch (minimax, anthropic, gemini, openrouter) uses the computed
value; the hardcoded one was the odd one out. Replace with the
maxTokens identifier for consistency.

Rejecting the two lockfile-related findings: .gitignore on this branch
lists package-lock.json / pnpm-lock.yaml / yarn.lock, so the workflow
comments ("Lockfiles are gitignored at the repo level") are accurate
and the two-step install (npm install --package-lock-only then npm ci)
is the correct pattern. Reverting to a single npm ci would fail CI
because there is no committed lockfile to install from.
@rohitg00 rohitg00 merged commit a654dd7 into main Apr 22, 2026
5 checks passed
@rohitg00 rohitg00 deleted the fix/viewer-empty-tabs-and-import-pipeline branch April 22, 2026 11:27
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
rohitg00 added a commit that referenced this pull request Apr 22, 2026
- README provider table: surface the no-op default and call out the
  opt-in Claude-subscription fallback with AGENTMEMORY_ALLOW_AGENT_SDK
  (from #187) instead of listing 'Claude subscription' as the default.
- README env block: document OPENAI_BASE_URL / OPENAI_EMBEDDING_MODEL
  (#186) and OPENAI_EMBEDDING_DIMENSIONS (#189), plus MINIMAX_API_KEY.
- Hero stat-tests SVG: 654 -> 827 (both dark and light variants) to
  match current suite size after recursion guard + idempotent
  lesson/crystal tests + openai dimension tests landed.
- website/lib/generated-meta.json regenerated by
  website/scripts/gen-meta.mjs (v0.9.1, 51 tools, 12 hooks,
  119 endpoints, 848 tests).
@rohitg00 rohitg00 mentioned this pull request Apr 22, 2026
rohitg00 added a commit that referenced this pull request Apr 22, 2026
Rolls up #186 (OPENAI_BASE_URL / OPENAI_EMBEDDING_MODEL), #187 (Stop-hook
recursion 5-layer defense + NoopProvider + AGENTMEMORY_ALLOW_AGENT_SDK opt-in),
#188 (viewer empty-tabs + import-jsonl synthetic compression + auto-derived
lessons/crystals + richer session detail + audit/replay/frontier shape fixes),
#189 (OPENAI_EMBEDDING_DIMENSIONS + model-dimensions table), and #190
(README/website docs refresh).

Bumps: package.json, plugin/.claude-plugin/plugin.json, src/version.ts,
src/types.ts ExportData.version union, src/functions/export-import.ts
supportedVersions, test/export-import.test.ts assertion, and
packages/mcp/package.json shim (was stuck at 0.9.0).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant