feat: auto-create lessons in flow-compress, consolidate, and new MCP tools#279
Open
cl0ckt0wer wants to merge 27 commits into
Open
feat: auto-create lessons in flow-compress, consolidate, and new MCP tools#279cl0ckt0wer wants to merge 27 commits into
cl0ckt0wer wants to merge 27 commits into
Conversation
- 22 hook handlers across session lifecycle, messages, tool lifecycle, parts, files, permissions, tasks, commands, and config - Two-layer enrichment pipeline: /context + /enrich via system.transform - Two slash commands: /recall and /remember - Full Claude Code hook parity documented with gap analysis
mem::observe checks hookType === "prompt_submit" to extract raw.userPrompt and set session.firstPrompt. The plugin was using "user_prompt_submit" which didn't match, so sessions were never named.
- Use ctx.worktree for projectPath instead of opaque project.id - Add Array.isArray guards before output.system/.context.push() - Only delete stashed files after successful enrich POST - Defensive JSON.stringify for undefined tool inputs - Per-session Map-based dedup sets to prevent unbounded growth - Fix negative duration_ms when time.completed is unset - Validate props.file and enforce MAX_STASHED_FILES in file.edited - Deduplicate /summarize call on session idle - Buffer early config events until session.created flushes them - Move contextInjectedSessions.add after successful context fetch - Add DEBUG-gated error logging to network helpers - Guard config input.agent/mcp/provider against non-object types - Fix MCP badge count 44→51 in plugin README
…line (closes rohitg00#233) Three gaps from the Claude Code plugin port sweep: - Inject agentmemory usage instructions (memory_save, memory_recall, etc.) into the system prompt on first turn via experimental.chat.system.transform, replacing the skills mechanism that OpenCode lacks - Call /crystals/auto and /consolidate-pipeline on session.deleted, mirroring Claude's CONSOLIDATION_ENABLED behavior - Document MEMORY.md vs AGENTS.md architecture comparison (two-hop file bridge vs one-hop direct injection) Gap A (SubagentStop) is unfixable — OpenCode's SubtaskPart type has no completion/result fields. Gap C (Claude MEMORY.md bridge) is intentionally skipped — OpenCode uses direct injection.
- Guard instructions push with Array.isArray check + fix TOCTOU race by moving contextInjectedSessions.add() before the await - Session-scope stashedFiles via stashFor() helper (was process-global, could cross-contaminate concurrent sessions) - Fix tool prefix in instructions (agentmemory_memory_ not agentmemory_) - Check typeof string before pushing .context into system arrays - Change olderThanDays: 0 → 7 in consolidation fire-and-forget - Increase fire-and-forget timeout from 5s to 30s (consolidation takes minutes, 5s was guaranteed to abort)
- Fix session cross-contamination: file.edited and tool.execute.before now use props.sessionID/input.sessionID fallback instead of only activeSessionId, preventing subagent hijack of parent stash - Fix contextInjectedSessions regression from round 1: move add(sid) to after instructions push (synchronous) but before context fetch (async), so failed /context calls don't permanently skip injection - Fix Map memory leak: prune session entries (stashedFiles, seenSubtaskIds, seenToolCallIds) when session.status goes idle, preventing unbounded growth for crash-killed sessions
- Enforce MAX_STASHED_FILES cap on chat.message (was missing, could grow unbounded from 50+ file-ref messages) - Remove activeSessionId fallback from session.deleted; log warning when both info.id and sessionID are missing instead of guessing - Add subtaskSetFor/toolCallSetFor lazy-init wrappers matching stashFor pattern; fixes dedup failures for subagents spawned without a preceding session.created event - Guard chat.params against missing input.model (TypeError crash)
- Add safeSlice() helper replacing all unsafe (v as string || "").slice() calls; handles objects/BigInt/circular refs via try/catch JSON.stringify - Restore activeSessionId fallback on session.deleted (if both info.id and sessionID missing, fall back like every other handler) - Fix message.updated to check info.id before info.sessionID (matches session.created/session.updated pattern) - Add props.sessionID to message.part.updated fallback chain - Guard undefined callID and subtask ID (prevent dedup silently dropping all subsequent undefined-ID events) - Cap todo.updated at 100 entries (match session.diff's slice pattern) - Fix duration_ms || → ?? (0ms genuine duration no longer falsifies) - Fix extractErrorMessage || → ?? (0/false error values preserved) - Replace extractErrorMessage.slice with safeSlice in retry handler
CRITICAL: - Revert message.updated session ID resolution: info.id is the message UUID, not session ID. In message.updated, info is the message object (has role/tokens/modelID), not session info like in session.created. Use props.sessionID || info.sessionID || activeSessionId instead. - Fix post_tool_use_failure -> post_tool_failure: server types.ts defines "post_tool_failure" (no "use_"); compress-synthetic.ts classifies by that exact string. All error observations were misclassified as "other" instead of "error". - Fix contextInjected ordering: move add(sid) after the context fetch completes, not before. If /context times out, session is no longer permanently marked injected, allowing retry on next transform. HIGH: - Add process.cwd() fallback for projectPath (was null, causing silent 400s on every REST call when no workspace) - Guard session.start against null activeSessionId with early return (was sending sessionId: null to API) - Fix duration_ms: use typeof number checks instead of || 0 defaults; missing timing data now correctly reports null instead of 0ms
…erThanDays to 0, add compaction trigger
…ools - flow-compress.ts: save extracted <lesson> tags via mem::lesson-save - consolidate.ts: save lessons for architecture/workflow/pattern/bug memories - lessons.ts: add audit entry for mem::lesson-list - server.ts: add memory_lesson_list and memory_lesson_strengthen handlers - tools-registry.ts: add MCP tool definitions for lesson-list and lesson-strengthen - types.ts: add lesson_list to AuditEntry.operation union - README.md / plugin.json: update tool count 51 -> 53 Fixes rohitg00#274
|
@cl0ckt0wer is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Caution Review failedFailed to post review comments Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR consolidates the MCP tool surface from 53 to 15 tools (8 core + 7 extended operations), introduces action-suggest feature for heuristic/LLM-based memory action generation, implements soft-delete semantics throughout the system (governance, insights, search), wires lesson creation into consolidation/flow-compress pipelines, launches the OpenCode agentmemory-capture plugin with 22 event hooks and telemetry streaming, adds post-compact and permission-request hook infrastructure, and updates documentation and version pinning accordingly. ChangesTool Consolidation, Action-Suggest & Soft-Delete
OpenCode Plugin & Hook Infrastructure
Documentation, Metadata & Version Updates
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
605-629:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMCP tool counts are inconsistent within the same section.
Line 605 says 53 tools, but the headings still say “50 Tools” and “Extended tools (50 total …)”. Please update those labels to avoid confusing users.
🤖 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 `@README.md` around lines 605 - 629, The README shows "53 tools" at the top but the section headings read "50 Tools" and "Extended tools (50 total — set AGENTMEMORY_TOOLS=all)"; update those headings to consistently reflect the correct total (change "50 Tools" to "53 Tools" and "Extended tools (50 total — set AGENTMEMORY_TOOLS=all)" to "Extended tools (53 total — set AGENTMEMORY_TOOLS=all)") so the counts match the "53 tools" statement and avoid user confusion.
🧹 Nitpick comments (2)
plugin/opencode/agentmemory-capture.ts (1)
461-468: ⚡ Quick winExtract stash-cap logic into a helper.
The same "if stash exceeds
MAX_STASHED_FILES, keep the last N" pattern is duplicated three times (lines 463–467 here, lines 535–539 inchat.message, and lines 584–588 intool.execute.before). Consolidating reduces drift risk and keeps the eviction policy in one place.♻️ Sketch
+function addToStash(stash: Set<string>, files: Iterable<string>): void { + for (const f of files) stash.add(f); + if (stash.size > MAX_STASHED_FILES) { + const keep = [...stash].slice(-MAX_STASHED_FILES); + stash.clear(); + for (const k of keep) stash.add(k); + } +}Then call sites become
addToStash(stashFor(sid), [props.file]),addToStash(stashFor(sid), files), andaddToStash(stashFor(sid), extractFilePaths(args)).🤖 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 `@plugin/opencode/agentmemory-capture.ts` around lines 461 - 468, Extract the "cap stash to MAX_STASHED_FILES by keeping the last N items" logic into a single helper function (e.g., addToStash(stash: Set<string>, items: string[])) that adds items to the given stash and evicts older entries when stash.size > MAX_STASHED_FILES; replace the duplicated blocks that currently use stashFor(sid) and manual slice/clear loops (seen near the stash add in the current snippet and the other occurrences in chat.message and tool.execute.before) with calls to addToStash(stashFor(sid), <items>) so all eviction logic is centralized and reused.src/functions/consolidate.ts (1)
196-264: ⚡ Quick winConsider parallelizing the per-branch writes with
Promise.all.Within each branch (evolve at lines 196–236 and create at lines 237–263), the
kv.set,recordAudit, andsaveConsolidationLessoncalls are independent once their inputs are computed but execute serially. Per coding guidelines ("Use parallel operations where possible withPromise.allfor independent kv writes/reads"), these can be issued in parallel to reduce per-concept latency on the hot consolidation loop. Note that in the evolve branch, the mark-non-latest write onexistingMatchshould still complete before writing theevolvedrecord to keep ordering deterministic.♻️ Sketch of parallel writes for the create branch
await kv.set(KV.memories, memory.id, memory); - await recordAudit(kv, "remember", "mem::consolidate", [memory.id], { - action: "create_memory", - concept, - }); - existingTitles.add(memory.title.toLowerCase()); - consolidated++; - - await saveConsolidationLesson( - sdk, - memory.id, - parsed.type, - parsed.title, - parsed.content, - `Created from concept group: ${concept}`, - data.project, - ); + existingTitles.add(memory.title.toLowerCase()); + consolidated++; + await Promise.all([ + recordAudit(kv, "remember", "mem::consolidate", [memory.id], { + action: "create_memory", + concept, + }), + saveConsolidationLesson( + sdk, + memory.id, + parsed.type, + parsed.title, + parsed.content, + `Created from concept group: ${concept}`, + data.project, + ), + ]);As per coding guidelines: "Use parallel operations where possible with Promise.all for independent kv writes/reads".
🤖 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/consolidate.ts` around lines 196 - 264, The per-branch async operations are executed serially; make independent writes run in parallel with Promise.all to reduce latency. For the evolve branch (existingMatch, evolved), await kv.set(KV.memories, existingMatch.id, ...) to mark non-latest first, then create the evolved object and run the remaining independent operations in parallel: kv.set(KV.memories, evolved.id, evolved), recordAudit(..., evolved.id), and saveConsolidationLesson(..., evolved.id) via Promise.all; keep existingTitles.add and consolidated++ after those succeed. For the create branch (memory), after preparing memory, use Promise.all to run kv.set(KV.memories, memory.id, memory), recordAudit(..., memory.id), and saveConsolidationLesson(..., memory.id) concurrently, then update existingTitles and consolidated.
🤖 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 `@plugin/opencode/agentmemory-capture.ts`:
- Around line 98-136: The AGENTMEMORY_INSTRUCTIONS string incorrectly tells
users that tools start with the nonexistent prefix "agentmemory_memory_"; update
the constant AGENTMEMORY_INSTRUCTIONS in plugin/opencode/agentmemory-capture.ts
to remove or correct that sentence so it matches the listed tool names (e.g.,
state that tools use the exact names shown like memory_save, memory_recall,
memory_smart_search or that they start with "memory_"), ensuring the
instructional text and the tool-name examples are consistent.
In `@plugin/opencode/README.md`:
- Line 12: Update the stale badge in the plugin README: replace the
"MCP-51_tools" segment in the <img> tag (the current badge URL and/or alt text
showing 51) with "MCP-53_tools" so the badge and alt text reflect the new total
of 53 tools (i.e., update the image src and/or alt to show 53 consistent with
plugin.json and top-level README).
In `@ROADMAP.md`:
- Around line 38-40: The OpenCode hook bus entry ("OpenCode hook bus" /
`plugin/opencode/`) is marked shipped ([x]) but remains under the "### Planned"
section; move that entire bullet (the checked `[x] **OpenCode hook bus** (...) —
See `plugin/opencode/`" line) out of the "### Planned" block and insert it into
the "### Shipped so far in this quarter" section so the legend and item status
align. Ensure formatting and checklist markers remain intact and that
surrounding bullets keep their original order.
In `@src/functions/lessons.ts`:
- Around line 177-184: The audit call currently passes lessonIds (capped by
limit) as the targetIds but sets resultCount to lessons.length (the full
filtered list), causing ambiguity; update the recordAudit invocation in this
block (where lessonIds and lessons are prepared and recordAudit is called) to
use lessonIds.length for resultCount to reflect the returned count, or
alternatively add both totalMatched (set to lessons.length) and returnedCount
(set to lessonIds.length) to the metadata so audits record both the full match
count and the number of items returned.
In `@src/mcp/server.ts`:
- Around line 1111-1136: The MCP handlers for lesson listing and strengthening
need stricter boundary validation: for the "mem::lesson-list" branch tighten
validation on source to reject invalid values (return 400) and accept CSV
strings by splitting on commas and trimming into an array of allowed sources;
ensure minConfidence and limit accept numeric strings by parsing to numbers and
validate limit > 0, and normalize values before calling sdk.trigger for
function_id "mem::lesson-list"; for the "memory_lesson_strengthen" branch trim
args.lessonId (validate non-empty after trim) before sending it in the payload
to sdk.trigger for "mem::lesson-strengthen"; finally ensure responses use the
consistent formatted text content shape used elsewhere (body.content with text
from JSON.stringify of the normalized payload/result).
In `@src/triggers/api.ts`:
- Around line 534-543: The endpoint currently coerces non-string body.title to
undefined; instead, add explicit type validation similar to how project and cwd
are validated in api::search: if body.title is provided and typeof body.title
!== "string", return a 400 error (bad request) with a clear message; otherwise
trim and slice the string as before to produce title and set Session fields
(summary, firstPrompt) using title.slice(0,200). Reference variables/functions:
body.title, title, Session object creation in api.ts so you update the
validation before the Session is constructed.
---
Outside diff comments:
In `@README.md`:
- Around line 605-629: The README shows "53 tools" at the top but the section
headings read "50 Tools" and "Extended tools (50 total — set
AGENTMEMORY_TOOLS=all)"; update those headings to consistently reflect the
correct total (change "50 Tools" to "53 Tools" and "Extended tools (50 total —
set AGENTMEMORY_TOOLS=all)" to "Extended tools (53 total — set
AGENTMEMORY_TOOLS=all)") so the counts match the "53 tools" statement and avoid
user confusion.
---
Nitpick comments:
In `@plugin/opencode/agentmemory-capture.ts`:
- Around line 461-468: Extract the "cap stash to MAX_STASHED_FILES by keeping
the last N items" logic into a single helper function (e.g., addToStash(stash:
Set<string>, items: string[])) that adds items to the given stash and evicts
older entries when stash.size > MAX_STASHED_FILES; replace the duplicated blocks
that currently use stashFor(sid) and manual slice/clear loops (seen near the
stash add in the current snippet and the other occurrences in chat.message and
tool.execute.before) with calls to addToStash(stashFor(sid), <items>) so all
eviction logic is centralized and reused.
In `@src/functions/consolidate.ts`:
- Around line 196-264: The per-branch async operations are executed serially;
make independent writes run in parallel with Promise.all to reduce latency. For
the evolve branch (existingMatch, evolved), await kv.set(KV.memories,
existingMatch.id, ...) to mark non-latest first, then create the evolved object
and run the remaining independent operations in parallel: kv.set(KV.memories,
evolved.id, evolved), recordAudit(..., evolved.id), and
saveConsolidationLesson(..., evolved.id) via Promise.all; keep
existingTitles.add and consolidated++ after those succeed. For the create branch
(memory), after preparing memory, use Promise.all to run kv.set(KV.memories,
memory.id, memory), recordAudit(..., memory.id), and
saveConsolidationLesson(..., memory.id) concurrently, then update existingTitles
and consolidated.
🪄 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: 93923c09-ae78-4d3a-8a85-5062017da46b
📒 Files selected for processing (15)
README.mdROADMAP.mdplugin/.claude-plugin/plugin.jsonplugin/opencode/README.mdplugin/opencode/agentmemory-capture.tsplugin/opencode/commands/recall.mdplugin/opencode/commands/remember.mdplugin/opencode/plugin.jsonsrc/functions/consolidate.tssrc/functions/flow-compress.tssrc/functions/lessons.tssrc/mcp/server.tssrc/mcp/tools-registry.tssrc/triggers/api.tssrc/types.ts
Comment on lines
+98
to
+136
| const AGENTMEMORY_INSTRUCTIONS = `<agentmemory-instructions> | ||
| You have access to agentmemory for persistent cross-session memory. Use these tools proactively. | ||
|
|
||
| CORE TOOLS: | ||
|
|
||
| memory_save — Save an insight, decision, or fact to long-term memory. | ||
| Required: content (text), concepts (2-5 comma-separated keywords), type (pattern/preference/architecture/bug/workflow/fact) | ||
| Optional: files (comma-separated paths) | ||
| Use when: user says "remember this", after discovering a bug, after making an architectural decision, after learning a project convention. | ||
|
|
||
| memory_recall — Search past observations by keywords. | ||
| Use when: user says "recall", "what did we do", "do you remember", or needs context from past sessions. | ||
|
|
||
| memory_smart_search — Hybrid semantic+keyword search with progressive disclosure. | ||
| Use when: you need the most relevant past context, fuzzy/conceptual searches, or recall doesn't find what you need. | ||
|
|
||
| memory_sessions — List recent sessions with status and observation counts. | ||
| Use when: user asks about session/past history, "what did we work on". | ||
|
|
||
| memory_file_history — Get past observations about specific files (across all sessions). | ||
| Use when: you're about to edit a file and want to know its history, common pitfalls, or past edits. | ||
|
|
||
| memory_lesson_save — Save a lesson learned (what worked, what to avoid). | ||
| Use when: you discover a pattern that could help future sessions avoid mistakes. | ||
|
|
||
| memory_lesson_recall — Search lessons by query. Returns lessons sorted by confidence. | ||
| Use when: before making a decision, check if past lessons apply. | ||
|
|
||
| memory_governance_delete — Delete specific memories. Requires explicit user confirmation. | ||
| Use when: user says "forget this", "delete that memory". | ||
|
|
||
| memory_patterns — Detect recurring patterns across sessions. | ||
| Use when: you want to understand project-level trends over time. | ||
|
|
||
| memory_consolidate — Run the 4-tier memory consolidation pipeline. | ||
| Use when: you want to compress and organize accumulated session observations. | ||
|
|
||
| All memory tools start with \`agentmemory_memory_\`. Use the exact names as they appear in your tool list. Tool results are JSON. Always check what was returned before presenting to the user. | ||
| </agentmemory-instructions>`; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the actual exposed MCP tool names that the OpenCode plugin will see.
rg -nP --type=ts -C2 '\b(memory_lesson_list|memory_lesson_strengthen|memory_save|memory_recall|memory_smart_search)\b' src/mcp
rg -nP --type=ts -C2 'agentmemory_memory_'Repository: rohitg00/agentmemory
Length of output: 7075
Correct line 135: tools do not have the agentmemory_memory_ prefix.
Line 135 incorrectly states "All memory tools start with agentmemory_memory_", but the actual exposed tool names are unprefixed: memory_save, memory_recall, memory_smart_search, etc. This contradicts the correctly-listed tool names in lines 103-133. Remove or fix line 135 to avoid instructing the model to use a non-existent prefix, which would cause tool invocations to fail.
🤖 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 `@plugin/opencode/agentmemory-capture.ts` around lines 98 - 136, The
AGENTMEMORY_INSTRUCTIONS string incorrectly tells users that tools start with
the nonexistent prefix "agentmemory_memory_"; update the constant
AGENTMEMORY_INSTRUCTIONS in plugin/opencode/agentmemory-capture.ts to remove or
correct that sentence so it matches the listed tool names (e.g., state that
tools use the exact names shown like memory_save, memory_recall,
memory_smart_search or that they start with "memory_"), ensuring the
instructional text and the tool-name examples are consistent.
| </p> | ||
|
|
||
| <p align="center"> | ||
| <img src="https://img.shields.io/badge/MCP-51_tools-1f6feb?style=flat-square" alt="51 MCP tools" /> |
Contributor
There was a problem hiding this comment.
Tool count badge is stale (51 → 53).
The PR description states the tool count is being updated from 51 to 53 (with the addition of memory_lesson_list and memory_lesson_strengthen), and the top-level README.md / plugin.json reflect 53. However, this plugin's README badge still shows 51_tools. Please bump for consistency.
📝 Proposed fix
- <img src="https://img.shields.io/badge/MCP-51_tools-1f6feb?style=flat-square" alt="51 MCP tools" />
+ <img src="https://img.shields.io/badge/MCP-53_tools-1f6feb?style=flat-square" alt="53 MCP tools" />📝 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
| <img src="https://img.shields.io/badge/MCP-51_tools-1f6feb?style=flat-square" alt="51 MCP tools" /> | |
| <img src="https://img.shields.io/badge/MCP-53_tools-1f6feb?style=flat-square" alt="53 MCP tools" /> |
🤖 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 `@plugin/opencode/README.md` at line 12, Update the stale badge in the plugin
README: replace the "MCP-51_tools" segment in the <img> tag (the current badge
URL and/or alt text showing 51) with "MCP-53_tools" so the badge and alt text
reflect the new total of 53 tools (i.e., update the image src and/or alt to show
53 consistent with plugin.json and top-level README).
Comment on lines
38
to
+40
| ### Planned | ||
| - [ ] **GitHub connector** (`@agentmemory/github-watcher`) — sync issues, PRs, discussions as observations. Shares the `POST /agentmemory/observe` wire format with the filesystem connector. | ||
| - [ ] **OpenCode hook bus** (#156) — if upstream ships hook events, wire them; otherwise ship a REST-polling adapter. | ||
| - [x] **OpenCode hook bus** (#156) — wired 22 hooks covering all 12 Claude Code hook types: session lifecycle (create/idle/status/compacted/update/diff/delete/error), messages & prompts (chat.message, message.updated user+assistant, message.removed), tool capture (before + rich ToolPart lifecycle in message.part.updated), memory injection (context + enrich via system.transform), part tracking (subtask, step-finish, reasoning, file, patch, compaction, agent, retry), file enrichment pipeline (stash via tool.execute.before + file.edited + file parts), permissions (updated + replied), task tracking (todo.updated w/ priority), commands (command.executed), config & model tracking (config + chat.params). Plus 2 slash commands (recall/remember). See `plugin/opencode/`. |
Contributor
There was a problem hiding this comment.
Move the shipped OpenCode hook bus entry out of "Planned".
Line 40 is now checked [x] (shipped), but it sits under the ### Planned heading at line 38. According to the legend at lines 9–12, shipped items belong under "### Shipped so far in this quarter" (lines 25–32). Consider relocating to keep the roadmap legend accurate.
📝 Proposed move
### Shipped so far in this quarter
- [x] iii console docs in README with vendored screenshots (`#157`)
- [x] Health severity gated on RSS floor (`#158` / PR `#160`)
- [x] Standalone MCP proxies to the running server (`#159` / PR `#161`)
- [x] Audit coverage for `mem::forget` + audit policy doc (`#125` / PR `#162`)
- [x] `@agentmemory/fs-watcher` filesystem connector (`#62` / PR `#163`)
- [x] Next.js website on Vercel (PR `#164`)
- [x] CI publishes all three npm packages on release (PR `#166`)
+- [x] **OpenCode hook bus** (`#156`) — wired 22 hooks covering all 12 Claude Code hook types: ... See `plugin/opencode/`.
### Active
...
### Planned
- [ ] **GitHub connector** (`@agentmemory/github-watcher`) — ...
-- [x] **OpenCode hook bus** (`#156`) — wired 22 hooks covering all 12 Claude Code hook types: ... See `plugin/opencode/`.
- [ ] **Session replay UI** in the real-time viewer — ...🤖 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 `@ROADMAP.md` around lines 38 - 40, The OpenCode hook bus entry ("OpenCode hook
bus" / `plugin/opencode/`) is marked shipped ([x]) but remains under the "###
Planned" section; move that entire bullet (the checked `[x] **OpenCode hook
bus** (...) — See `plugin/opencode/`" line) out of the "### Planned" block and
insert it into the "### Shipped so far in this quarter" section so the legend
and item status align. Ensure formatting and checklist markers remain intact and
that surrounding bullets keep their original order.
Comment on lines
+534
to
+543
| const title = typeof body.title === "string" ? body.title.trim() : undefined; | ||
| const session: Session = { | ||
| id: sessionId, | ||
| project, | ||
| cwd, | ||
| startedAt: new Date().toISOString(), | ||
| status: "active", | ||
| observationCount: 0, | ||
| ...(title ? { summary: title.slice(0, 200) } : {}), | ||
| ...(title ? { firstPrompt: title.slice(0, 200) } : {}), |
Contributor
There was a problem hiding this comment.
Validate the type of the optional title parameter.
If body.title is provided but is not a string (e.g., a number, boolean, or object), the endpoint currently treats it as undefined and silently ignores it. For consistency with other optional string parameters in this file (see lines 359–363 for project and cwd validation in api::search), you should return a 400 error when an invalid type is provided. As per coding guidelines, input validation must occur at system boundaries.
✅ Suggested validation
}
+ if (body.title !== undefined && typeof body.title !== "string") {
+ return {
+ status_code: 400,
+ body: { error: "title must be a string" },
+ };
+ }
const title = typeof body.title === "string" ? body.title.trim() : undefined;🤖 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/triggers/api.ts` around lines 534 - 543, The endpoint currently coerces
non-string body.title to undefined; instead, add explicit type validation
similar to how project and cwd are validated in api::search: if body.title is
provided and typeof body.title !== "string", return a 400 error (bad request)
with a clear message; otherwise trim and slice the string as before to produce
title and set Session fields (summary, firstPrompt) using title.slice(0,200).
Reference variables/functions: body.title, title, Session object creation in
api.ts so you update the validation before the Session is constructed.
…o-creation # Conflicts: # README.md # plugin/.claude-plugin/plugin.json
…o-creation # Conflicts: # README.md # plugin/.claude-plugin/plugin.json # plugin/.codex-plugin/plugin.json # src/mcp/standalone.ts
Contributor
Author
|
Update pushed after rebasing/merging latest upstream main. Added in the latest commits:
Local validation:
Current non-code status:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mcp/rest-proxy.ts (1)
153-157:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset rate-limit state in test reset helper.
resetHandleForTests()now leavesrateLimitMapintact, so tests can become order-dependent once call counts accumulate across cases.Suggested fix
export function resetHandleForTests(): void { cached = null; cachedAt = 0; probeInFlight = null; + rateLimitMap.clear(); }🤖 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/mcp/rest-proxy.ts` around lines 153 - 157, The test helper resetHandleForTests currently resets cached, cachedAt, and probeInFlight but leaves rateLimitMap intact causing inter-test state leakage; update resetHandleForTests to also clear or reinitialize the rateLimitMap (e.g., call rateLimitMap.clear() or assign a new Map()) so per-test rate-limit counters are reset; locate resetHandleForTests and the module-level rateLimitMap variable to implement the change.src/mcp/standalone.ts (1)
390-399:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLocal governance delete still hard-deletes instead of soft-deleting.
In local mode this removes memory records, but the updated governance contract is soft-delete (
deleted=true). This creates mode-dependent behavior and breaks consistency between proxy/local execution.Suggested fix
case "memory_governance_delete": { let deleted = 0; + const now = new Date().toISOString(); for (const id of v.memoryIds || []) { - const existing = await kvInstance.get("mem:memories", id); - if (existing) { - await kvInstance.delete("mem:memories", id); + const existing = await kvInstance.get<Record<string, unknown>>("mem:memories", id); + if (existing && !existing["deleted"]) { + await kvInstance.set("mem:memories", id, { + ...existing, + deleted: true, + updatedAt: now, + }); deleted++; } } kvInstance.persist();🤖 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/mcp/standalone.ts` around lines 390 - 399, The local "memory_governance_delete" branch currently hard-deletes memories via kvInstance.delete, but must soft-delete to match the governance contract; change the loop in the case "memory_governance_delete" so that for each id you load the record with kvInstance.get, set existing.deleted = true (or set the proper deleted flag), write the updated record back (e.g. kvInstance.put or kvInstance.set with the same namespace "mem:memories" and id) instead of calling kvInstance.delete, increment deleted, and then call kvInstance.persist(); update references in that case handler only (the get/delete/persist sequence).
🧹 Nitpick comments (3)
test/mcp-standalone-proxy.test.ts (1)
302-305: ⚡ Quick winAssert exact core tool names, not just count.
core.toolscurrently validates only length, so a wrong pair of tools would still pass.✅ Suggested test tightening
resetHandleForTests(); process.env["AGENTMEMORY_TOOLS"] = "core"; const core = await handleToolsList(); -expect((core.tools as unknown[]).length).toBe(2); +expect((core.tools as Array<{ name: string }>).map((t) => t.name).sort()).toEqual([ + "memory_search", + "memory_store", +]); delete process.env["AGENTMEMORY_TOOLS"];🤖 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 `@test/mcp-standalone-proxy.test.ts` around lines 302 - 305, The test currently only asserts the length of core.tools; update the assertion to check the exact tool names to prevent false positives. After calling handleToolsList() (when process.env["AGENTMEMORY_TOOLS"] = "core"), assert that core.tools contains the expected tool identifiers/objects (e.g., compare arrays of names or map core.tools to names and assert equality) and then clean up by deleting process.env["AGENTMEMORY_TOOLS"]; ensure you reference handleToolsList and core.tools when locating the test.test/action-suggest.test.ts (1)
35-51: ⚡ Quick winAlign SDK mocking with the repository’s test pattern.
This suite uses a custom SDK object instead of the standard
vi.mock("iii-sdk")pattern expected in the repo.As per coding guidelines, "Mock pattern for tests: use
vi.mock("iii-sdk")with mocksdk.trigger,kv.get/set/list".Also applies to: 92-374
🤖 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 `@test/action-suggest.test.ts` around lines 35 - 51, Replace the custom mockSdk implementation with the repository standard vi.mock("iii-sdk") pattern: remove the mockSdk function and instead call vi.mock("iii-sdk", () => ({ sdk: { trigger: vi.fn(), registerFunction: vi.fn(), registerTrigger: vi.fn() }, kv: { get: vi.fn(), set: vi.fn(), list: vi.fn() } })); ensure tests that previously used mockSdk.registerFunction/trigger now use the mocked sdk.trigger and adjust calls to reference the mocked functions (sdk.trigger.mockResolvedValueOnce / mockImplementation as needed) and configure kv.get/set/list where those behaviors are required; keep references to the same identifiers (registerFunction, registerTrigger, trigger, kv.get/set/list) so test intent remains unchanged.src/functions/governance.ts (1)
45-51: ⚡ Quick winCapture operation timestamp once and reuse it.
Both delete paths generate timestamps per item; use one operation timestamp for consistency.
As per coding guidelines, "Capture timestamps once with
new Date().toISOString()and reuse throughout the operation".Also applies to: 136-138
🤖 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/governance.ts` around lines 45 - 51, Capture a single operation timestamp (e.g., const opTimestamp = new Date().toISOString()) before performing the deletion loops and reuse opTimestamp wherever you set updatedAt instead of calling new Date().toISOString() per item; update the first deletion path that sets mem.updatedAt and the other delete path that sets updatedAt (the loop handling data.memoryIds and the alternate path around the later updatedAt assignments) to use this shared opTimestamp, ensuring consistent timestamps across the operation.
🤖 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 `@plugin/opencode/README.md`:
- Around line 175-177: The fenced code blocks showing the flow "agentmemory
──write──▶ MEMORY.md ──read──▶ Claude system prompt" are missing language
tags; update those triple-backtick fences to include a language (e.g., ```text)
for both occurrences (the block at the shown diff and the similar block at lines
186-188) so markdown linting passes, ensuring the fences around the MEMORY.md /
Claude system prompt diagram and the later identical block both specify the
language.
In `@plugin/scripts/post-compact.mjs`:
- Around line 15-23: The post function currently ignores fetch responses and
only catches network errors; update post(path, body, timeoutMs) to capture the
fetch response, check response.ok (i.e., 2xx) and handle non-2xx by throwing or
logging an error with details (status, statusText and optionally response
text/json) so writes aren't silently dropped; keep the same
headers/authHeaders() and AbortSignal.timeout(timeoutMs) usage and ensure
callers get a rejected promise on failure or the response for success.
- Line 35: The code currently falls back to const sessionId = data.session_id ||
"unknown", which can cause cross-session contamination; instead, remove the
"unknown" fallback and add a guard that validates data.session_id (e.g., if
(!data?.session_id || typeof data.session_id !== 'string') return; ) at the
start of the compaction handler so the function returns early when session_id is
missing or invalid, then use the validated sessionId variable for subsequent
writes.
In `@README.md`:
- Line 431: The fenced code block ending with a plain ``` currently has no
language identifier; update that fence to include a language tag (e.g., ```text
or ```bash) so the markdown linter recognizes the block type—locate the
triple-backtick fence in the README and add an appropriate language identifier
after the opening ``` to resolve the lint warning.
In `@src/functions/governance.ts`:
- Around line 136-141: The bulk-delete loop is re-applying soft-deletes to
already-deleted memories; update the logic in the function that iterates
memories (the block that sets mem.deleted = true, mem.updatedAt = new
Date().toISOString(), calls kv.set(KV.memories, mem.id, mem),
deleteAccessLog(kv, mem.id) and cascadeStale(kv, mem.id)) to first check if
mem.deleted is already true and skip processing for those entries (i.e., only
set updatedAt, call kv.set, deleteAccessLog, and cascadeStale when mem.deleted
is false), ensuring idempotent behavior and correct deleted counts.
- Around line 25-28: Edges are being marked stale without updating their
mutation timestamp; when setting edge.stale = true in the block that checks
edge.sourceObservationIds.includes(memoryId), also set edge.updatedAt to the
current timestamp (match the node update format used elsewhere, e.g. new
Date().toISOString() or the project’s timestamp utility), then push the updated
edge via edgeUpdates and kv.set(KV.graphEdges, edge.id, edge); update the code
around edge.stale, edge.updatedAt, edgeUpdates, and kv.set to ensure consistent
metadata for stale edges.
In `@src/functions/reflect.ts`:
- Around line 484-497: The delete path currently trusts data shape, processes
IDs sequentially, and may double-delete duplicates; normalize and validate the
IDs (from data.insightId and data.insightIds) into a flat array of non-empty
strings, dedupe them, and return the validation error via the function
registration/validation flow (sdk.registerFunction) if none remain; then fetch
existing insights in parallel using Promise.all on kv.get(KV.insights, id) for
each deduped id, map the found insights to updates (set deleted=true and
updatedAt=now) and perform parallel kv.set calls with Promise.all, and finally
compute and return the deleted count based on how many insights were actually
updated (use symbols: ids, KV.insights, kv.get, kv.set, Insight, deleted,
updatedAt, now, sdk.registerFunction).
In `@src/mcp/server.ts`:
- Around line 588-591: The audit branch is passing the wrong field to
mem::audit-query: it uses args.operation but the schema exposes
args.operation_filter, so filtering is lost; update the payload in the
sdk.trigger call (the block handling operation === "audit" || name ===
"memory_audit") to send operation: args.operation_filter (falling back to
args.operation or a default if you want backward compatibility) when invoking
"mem::audit-query", ensuring the trigger payload uses the schema's
operation_filter field.
- Around line 102-103: The dispatcher currently only reads args.scope and
ignores the tool schema's operation alias, causing calls that pass operation to
be mis-routed; update the scope initialization to accept operation as a fallback
(e.g., set scope = (args.scope as string) || (args.operation as string) ||
"keyword") and ensure subsequent branching (the if that checks scope ===
"keyword" || name === "memory_recall") uses this normalized scope variable so
validation and routing respect either argument name.
- Around line 503-508: The delete branch currently expects args.insightId but
tool contracts supply insightIds (bulk/comma-separated); update the handler for
operation === "delete" to accept args.insightIds (string or string[]), normalize
it into an array of trimmed non-empty IDs (if a comma-separated string, split on
commas), return 400 if the resulting list is empty, then invoke sdk.trigger for
each id (function_id "mem::insight-delete", payload should include insightId per
call) using Promise.all and return the aggregated results in the response body
instead of a single JSON result from args.insightId.
- Around line 442-450: The "crystal"/"memory_crystallize" branch always requires
args.actionIds and always calls sdk.trigger for crystallization, so the
documented operation "list" is never handled; update the case block (the
"crystal" / "memory_crystallize" branch that reads args.actionIds and calls
sdk.trigger with function_id "mem::crystallize") to first check args.operation
(or default) and if args.operation === "list" return the list response (do not
require actionIds) by invoking the appropriate SDK call or returning the
cached/available crystallized IDs, otherwise validate args.actionIds, build
crysIds and call sdk.trigger({ function_id: "mem::crystallize", payload: {
actionIds: crysIds, project: args.project, sessionId: args.sessionId }}) for the
crystallize operation; ensure the response shape matches other branches
(status_code 200 with body.content).
In `@src/mcp/tools-registry.ts`:
- Line 49: The JSON schema for the memory_search tool currently has a top-level
required: ["query"] that forces query for all scopes; update the memory_search
schema in src/mcp/tools-registry.ts to remove the global required constraint and
instead require "query" only for the keyword/path variants (e.g., by moving the
requirement into the specific subschema or using oneOf/if-then to conditionally
require "query" when scope/type equals the keyword variant). Locate the
memory_search schema and replace the global required: ["query"] with a
conditional or scoped requirement so non-keyword variants (file/time/image) are
allowed without query.
- Line 189: The schema for the "sketch" tool incorrectly forces "title" for
every operation; change the "sketch" schema's required rules so only "operation"
is always required and "title" is not globally required—either remove "title"
from the top-level required array or implement a conditional/oneOf rule that
requires "title" only for operations that need it (e.g., create/update) and
requires "sketchId" for the "promote" operation; update the schema around the
"sketch" definition referencing the "operation", "title", "promote", and
"sketchId" fields accordingly.
In `@src/prompts/action-suggest.ts`:
- Around line 36-37: The ACTION_REGEX is brittle because it requires attributes
in a specific order and will miss valid <action> tags emitted with attributes
reordered; update parsing in src/prompts/action-suggest.ts to first match the
action block with a relaxed tag pattern (e.g.,
/<action\b([^>]*)>[\s\S]*?<\/action>/g or otherwise locate the whole opening
tag) and then extract title, priority, and description from the captured
attribute string using independent attribute-specific regexes (e.g.,
/title="([^"]*)"/, /priority="([^"]*)"/, /description="([^"]*)"/) or an HTML/XML
attribute parser; apply the same fix to the other similar matcher blocks around
where ACTION_REGEX is used (lines mentioned in the review) so attributes are
order-independent and valid actions are not dropped.
---
Outside diff comments:
In `@src/mcp/rest-proxy.ts`:
- Around line 153-157: The test helper resetHandleForTests currently resets
cached, cachedAt, and probeInFlight but leaves rateLimitMap intact causing
inter-test state leakage; update resetHandleForTests to also clear or
reinitialize the rateLimitMap (e.g., call rateLimitMap.clear() or assign a new
Map()) so per-test rate-limit counters are reset; locate resetHandleForTests and
the module-level rateLimitMap variable to implement the change.
In `@src/mcp/standalone.ts`:
- Around line 390-399: The local "memory_governance_delete" branch currently
hard-deletes memories via kvInstance.delete, but must soft-delete to match the
governance contract; change the loop in the case "memory_governance_delete" so
that for each id you load the record with kvInstance.get, set existing.deleted =
true (or set the proper deleted flag), write the updated record back (e.g.
kvInstance.put or kvInstance.set with the same namespace "mem:memories" and id)
instead of calling kvInstance.delete, increment deleted, and then call
kvInstance.persist(); update references in that case handler only (the
get/delete/persist sequence).
---
Nitpick comments:
In `@src/functions/governance.ts`:
- Around line 45-51: Capture a single operation timestamp (e.g., const
opTimestamp = new Date().toISOString()) before performing the deletion loops and
reuse opTimestamp wherever you set updatedAt instead of calling new
Date().toISOString() per item; update the first deletion path that sets
mem.updatedAt and the other delete path that sets updatedAt (the loop handling
data.memoryIds and the alternate path around the later updatedAt assignments) to
use this shared opTimestamp, ensuring consistent timestamps across the
operation.
In `@test/action-suggest.test.ts`:
- Around line 35-51: Replace the custom mockSdk implementation with the
repository standard vi.mock("iii-sdk") pattern: remove the mockSdk function and
instead call vi.mock("iii-sdk", () => ({ sdk: { trigger: vi.fn(),
registerFunction: vi.fn(), registerTrigger: vi.fn() }, kv: { get: vi.fn(), set:
vi.fn(), list: vi.fn() } })); ensure tests that previously used
mockSdk.registerFunction/trigger now use the mocked sdk.trigger and adjust calls
to reference the mocked functions (sdk.trigger.mockResolvedValueOnce /
mockImplementation as needed) and configure kv.get/set/list where those
behaviors are required; keep references to the same identifiers
(registerFunction, registerTrigger, trigger, kv.get/set/list) so test intent
remains unchanged.
In `@test/mcp-standalone-proxy.test.ts`:
- Around line 302-305: The test currently only asserts the length of core.tools;
update the assertion to check the exact tool names to prevent false positives.
After calling handleToolsList() (when process.env["AGENTMEMORY_TOOLS"] =
"core"), assert that core.tools contains the expected tool identifiers/objects
(e.g., compare arrays of names or map core.tools to names and assert equality)
and then clean up by deleting process.env["AGENTMEMORY_TOOLS"]; ensure you
reference handleToolsList and core.tools when locating the test.
🪄 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: 6ca524ed-c325-4b25-8d15-da4ac6d930c9
📒 Files selected for processing (31)
.dockerignoreREADME.mdplugin/.claude-plugin/plugin.jsonplugin/.codex-plugin/plugin.jsonplugin/hooks/hooks.codex.jsonplugin/opencode/README.mdplugin/opencode/plugin.jsonplugin/scripts/permission-request.mjsplugin/scripts/post-compact.mjssrc/config.tssrc/functions/action-suggest.tssrc/functions/governance.tssrc/functions/reflect.tssrc/functions/search.tssrc/hooks/permission-request.tssrc/hooks/post-compact.tssrc/index.tssrc/mcp/rest-proxy.tssrc/mcp/server.tssrc/mcp/standalone.tssrc/mcp/tools-registry.tssrc/prompts/action-suggest.tssrc/types.tstest/action-suggest.test.tstest/auto-compress.test.tstest/codex-plugin.test.tstest/embedding-provider.test.tstest/governance.test.tstest/mcp-standalone-proxy.test.tstest/mcp-standalone.test.tstsdown.config.ts
✅ Files skipped from review due to trivial changes (2)
- plugin/.codex-plugin/plugin.json
- plugin/.claude-plugin/plugin.json
| <details> | ||
| <summary><b>OpenClaw (paste this prompt)</b></summary> | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language identifier to this fenced block.
Use a language tag (for example, text) on this code fence to keep markdown linting clean.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 431-431: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@README.md` at line 431, The fenced code block ending with a plain ```
currently has no language identifier; update that fence to include a language
tag (e.g., ```text or ```bash) so the markdown linter recognizes the block
type—locate the triple-backtick fence in the README and add an appropriate
language identifier after the opening ``` to resolve the lint warning.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/functions/governance.ts (1)
137-145: 🏗️ Heavy liftAvoid repeated full-graph scans during bulk delete.
Line 144 calls
cascadeStaleonce per memory, and each call re-lists all graph nodes/edges (Lines 11-12). In larger batches this scales poorly and can dominate latency. Prefer a single cascade pass per batch (or per operation) using aSetof deleted memory IDs.🤖 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/governance.ts` around lines 137 - 145, The bulk-delete loop currently calls cascadeStale(mem.id) for each memory, causing repeated full-graph scans; instead, collect deleted memory IDs into a Set during the batch iteration (inside the Promise.allSettled batch.map that sets mem.deleted and kv.set) and remove the per-item cascadeStale call, then after Promise.allSettled completes call a single cascadeStaleBatch (or extend cascadeStale to accept a Set) passing that Set of deleted IDs so the graph is scanned once and stale propagation runs once per batch; update cascadeStale signature/implementation accordingly and keep deleteAccessLog calls per-item as before.test/mcp-tools-call.test.ts (1)
10-57: ⚡ Quick winAdd
vi.mock("iii-sdk")to align with coding guidelines and repo-standard test mocking pattern.The file hand-rolls SDK/KV stubs instead of the required
vi.mock("iii-sdk")approach specified in the coding guidelines. The hand-rolled mocks work correctly, but standardizing on the central mock pattern prevents drift and maintains consistency across the test suite.🤖 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 `@test/mcp-tools-call.test.ts` around lines 10 - 57, Add a vi.mock("iii-sdk") call at the top of this test and stop using the hand-rolled SDK/KV stubs; replace usages of mockKV and mockSdk with the test-suite's centralized mock implementation exported by the iii-sdk mock (or adapt the existing mock to register with that mock factory), ensuring the test imports/consumes the mocked SDK functions the same way other tests do (use the mock's trigger/override/getFunction APIs rather than the local mockSdk/registerFunction), and remove or convert mockKV usage to the repo-standard KV mock provided by vi.mock("iii-sdk") so the test follows the shared mocking pattern.
🤖 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.
Nitpick comments:
In `@src/functions/governance.ts`:
- Around line 137-145: The bulk-delete loop currently calls cascadeStale(mem.id)
for each memory, causing repeated full-graph scans; instead, collect deleted
memory IDs into a Set during the batch iteration (inside the Promise.allSettled
batch.map that sets mem.deleted and kv.set) and remove the per-item cascadeStale
call, then after Promise.allSettled completes call a single cascadeStaleBatch
(or extend cascadeStale to accept a Set) passing that Set of deleted IDs so the
graph is scanned once and stale propagation runs once per batch; update
cascadeStale signature/implementation accordingly and keep deleteAccessLog calls
per-item as before.
In `@test/mcp-tools-call.test.ts`:
- Around line 10-57: Add a vi.mock("iii-sdk") call at the top of this test and
stop using the hand-rolled SDK/KV stubs; replace usages of mockKV and mockSdk
with the test-suite's centralized mock implementation exported by the iii-sdk
mock (or adapt the existing mock to register with that mock factory), ensuring
the test imports/consumes the mocked SDK functions the same way other tests do
(use the mock's trigger/override/getFunction APIs rather than the local
mockSdk/registerFunction), and remove or convert mockKV usage to the
repo-standard KV mock provided by vi.mock("iii-sdk") so the test follows the
shared mocking pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61475458-e436-4b5f-adf2-6968f21bc17b
📒 Files selected for processing (13)
plugin/opencode/README.mdplugin/scripts/post-compact.mjssrc/functions/governance.tssrc/functions/lessons.tssrc/functions/reflect.tssrc/hooks/post-compact.tssrc/mcp/server.tssrc/mcp/standalone.tssrc/mcp/tools-registry.tssrc/prompts/action-suggest.tstest/action-suggest.test.tstest/mcp-standalone-proxy.test.tstest/mcp-tools-call.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/functions/lessons.ts
- src/functions/reflect.ts
- test/mcp-standalone-proxy.test.ts
- src/mcp/tools-registry.ts
- plugin/scripts/post-compact.mjs
- test/action-suggest.test.ts
- src/hooks/post-compact.ts
- src/mcp/server.ts
- src/mcp/standalone.ts
Contributor
Author
|
Review follow-up pushed in 3f77267. Addressed the valid governance bulk-delete nit: stale graph propagation now batches deleted IDs and scans graph nodes/edges once per batch instead of once per deleted memory. I intentionally left the test mock-style nit unchanged: this repo does not currently have a shared iii-sdk KV/function mock to consume here, and nearby tests still use local SDK/KV stubs. Rewriting this small dispatch test would add churn without changing behavior. Validation run locally:
Remaining external check: Vercel still requires deployment authorization. |
…o-creation # Conflicts: # README.md # plugin/.claude-plugin/plugin.json # plugin/.codex-plugin/plugin.json # plugin/.mcp.json # plugin/opencode/README.md # plugin/opencode/agentmemory-capture.ts # plugin/opencode/plugin.json # src/mcp/standalone.ts # test/mcp-standalone.test.ts
Contributor
Author
|
Follow-up issue for the MCP tool-surface/tool-count problem: #620 |
Contributor
Author
|
Follow-up issue for standardizing the MCP dispatch test mock pattern: #621 |
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
2 participants
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.
Fixes #274
Changes
Auto-create lessons where they were being discarded
<lesson>tags from LLM output (already parsed but previously discarded) and save viamem::lesson-savewith dedup check + warning log on failuresaveConsolidationLesson()helper. Saves lessons forarchitecture,workflow,pattern, andbugmemory types during consolidation (both new and evolved memories). Content truncated to 500 chars with proper error logging.New MCP tools (previously REST-only)
Other fixes
mem::lesson-listwith affected lesson IDs as targetIdslesson_listto AuditEntry.operation unionSummary by CodeRabbit
New Features
Improvements
Documentation