feat(workspace): load workspace memory on demand, and show it in the memory tool - #1123
feat(workspace): load workspace memory on demand, and show it in the memory tool#1123sahrizvi wants to merge 5 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Previous Review Summaries (10 snapshots, latest commit 9765de5)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9765de5)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Previous review (commit 8b6af3c)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Previous review (commit c1e8089)Status: No Issues Found | Recommendation: Merge Files Reviewed (10 files)
Previous review (commit 7fc24db)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit 9c26064)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Previous reviewThis review did not run. Your provider API key hit its rate limit, so the Previous reviewThis review did not run. Your provider API key hit its rate limit, so the Previous reviewThis review did not run. Your provider API key hit its rate limit, so the Previous reviewThis review did not run. Your provider API key hit its rate limit, so the Previous reviewThis review did not run. Your provider API key hit its rate limit, so the Reviewed by deepseek-v4-pro · Input: 105.9K · Output: 46.6K · Cached: 1.1M Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
3 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/memory/tools/memory-read.ts">
<violation number="1" location="packages/opencode/src/memory/tools/memory-read.ts:57">
P2: `scope: "all"` currently suppresses every project read exception, not just the no-instance-context case described in the comment. This can hide real project-store failures and return incomplete or misleading ID lookup results. Restrict suppression to the specific expected context error and rethrow other exceptions.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| try { | ||
| block = await MemoryStore.read(scope, args.id) | ||
| } catch (e) { | ||
| if (args.scope === "all" && scope === "project") continue |
There was a problem hiding this comment.
P2: scope: "all" currently suppresses every project read exception, not just the no-instance-context case described in the comment. This can hide real project-store failures and return incomplete or misleading ID lookup results. Restrict suppression to the specific expected context error and rethrow other exceptions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/memory/tools/memory-read.ts, line 57:
<comment>`scope: "all"` currently suppresses every project read exception, not just the no-instance-context case described in the comment. This can hide real project-store failures and return incomplete or misleading ID lookup results. Restrict suppression to the specific expected context error and rethrow other exceptions.</comment>
<file context>
@@ -45,15 +45,17 @@ export const MemoryReadTool = Tool.define("altimate_memory_read", {
- } catch {
- continue
+ } catch (e) {
+ if (args.scope === "all" && scope === "project") continue
+ throw e
}
</file context>
ce874ec to
fae029a
Compare
db315dd to
9652073
Compare
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
fae029a to
c068ed6
Compare
9652073 to
9c26064
Compare
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| waitTimedOut?: boolean | ||
| /** Set when the last fetch for this session failed, so a caller can tell an | ||
| * empty workspace apart from an unreadable one. */ | ||
| hydrateFailed?: boolean |
There was a problem hiding this comment.
SUGGESTION: hydrateFailed is now dead code — it is written but never read.
The refresh restructure in this PR replaced the hydrateFailed read with a direct outcome.status === "error" check, but the three writes remain (commitLoad at lines 838/841 and refresh at line 887). Nothing reads the field anymore, so it and its doc comment can be removed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 7fc24db1 — removed the hydrateFailed?: boolean field from SessionMemory plus its three write sites (commitLoad at lines 835 and 838, and refresh at line 884). The refresh restructure already replaced the read with a direct outcome.status === "error" check, so the field was purely dead weight after that landed.
| return [...(sessions.get(sessionID)?.overlay ?? [])] | ||
| } | ||
|
|
||
| /** Re-read this session's workspace memory, discarding what it already holds. |
There was a problem hiding this comment.
SUGGESTION: This doc comment duplicates the one on refresh (line 867) and is now stale — it still says "Returns how many blocks the session now holds", but refresh returns a RefreshResult. It is also attached to the RefreshResult type rather than the function. Remove this block so the two cannot drift.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 7fc24db1 — dropped the duplicate/stale doc block above export type RefreshResult. The comment on refresh itself (line 867 in the old file / line 862 now) is the canonical one; the RefreshResult type stays as a plain shape declaration.
…lt doc Two cleanups on packages/opencode/src/altimate/workspace/memory-sync.ts: 1. Remove the `hydrateFailed` field on `SessionMemory` and its three write sites. The `refresh` restructure replaced the read with a direct `outcome.status === "error"` check, so the field is written but never read. (kilo-code-bot #1123 comment 3840696504.) 2. Remove the doc comment above `export type RefreshResult` — it duplicated the doc on `refresh` (line 867) and had drifted stale (still claimed "Returns how many blocks the session now holds", but `refresh` returns a `RefreshResult`). The doc was also attached to the wrong element (the type, not the function). (kilo-code-bot #1123 comment 3840696513.)
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
5517f96 to
d011067
Compare
c1e8089 to
d011067
Compare
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
…lt doc Two cleanups on packages/opencode/src/altimate/workspace/memory-sync.ts: 1. Remove the `hydrateFailed` field on `SessionMemory` and its three write sites. The `refresh` restructure replaced the read with a direct `outcome.status === "error"` check, so the field is written but never read. (kilo-code-bot #1123 comment 3840696504.) 2. Remove the doc comment above `export type RefreshResult` — it duplicated the doc on `refresh` (line 867) and had drifted stale (still claimed "Returns how many blocks the session now holds", but `refresh` returns a `RefreshResult`). The doc was also attached to the wrong element (the type, not the function). (kilo-code-bot #1123 comment 3840696513.)
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
358fa87 to
86403e1
Compare
…lt doc Two cleanups on packages/opencode/src/altimate/workspace/memory-sync.ts: 1. Remove the `hydrateFailed` field on `SessionMemory` and its three write sites. The `refresh` restructure replaced the read with a direct `outcome.status === "error"` check, so the field is written but never read. (kilo-code-bot #1123 comment 3840696504.) 2. Remove the doc comment above `export type RefreshResult` — it duplicated the doc on `refresh` (line 867) and had drifted stale (still claimed "Returns how many blocks the session now holds", but `refresh` returns a `RefreshResult`). The doc was also attached to the wrong element (the type, not the function). (kilo-code-bot #1123 comment 3840696513.)
e38db61 to
8b6af3c
Compare
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
| // Keep what the session had. Emptying it because the network hiccuped is | ||
| // strictly worse than not reloading, and the user asked for a reload. | ||
| const state = sessionState(sessionID) | ||
| state.overlay = previous |
There was a problem hiding this comment.
WARNING: state.overlay = previous is redundant and can overwrite a fresher overlay.
loadWorkspaceMemory() is now pure — it returns a LoadOutcome and never touches sessions/state.overlay — so by the time this error branch runs, the session already retains whatever overlay it had. The previous capture/restore is a leftover from the pre-refactor refresh, which called sessions.delete(sessionID) before loading and therefore had to restore on failure; the restore no longer does anything useful.
It is also not merely dead code: hydrate runs fire-and-forget on every turn (session/prompt.ts:1056) and commits into the same state object. If a concurrent in-flight hydrate resolves between the previous snapshot (line 880) and this assignment, this line writes the stale snapshot over the fresher overlay — inverting the "keep what the session had" guarantee it claims to enforce — and hydrate's idempotence means the session never refetches it.
Drop the previous capture and this assignment; on error, return { count: overlayBlocks(sessionID).length, ok: false, status: "error" } and leave the overlay untouched.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…lt doc Two cleanups on packages/opencode/src/altimate/workspace/memory-sync.ts: 1. Remove the `hydrateFailed` field on `SessionMemory` and its three write sites. The `refresh` restructure replaced the read with a direct `outcome.status === "error"` check, so the field is written but never read. (kilo-code-bot #1123 comment 3840696504.) 2. Remove the doc comment above `export type RefreshResult` — it duplicated the doc on `refresh` (line 867) and had drifted stale (still claimed "Returns how many blocks the session now holds", but `refresh` returns a `RefreshResult`). The doc was also attached to the wrong element (the type, not the function). (kilo-code-bot #1123 comment 3840696513.)
8b6af3c to
9765de5
Compare
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
…memory tool Closes the half of the read requirement that was never built. Memory loaded at session start, but "on the user's demand" had no entry point at all, and the memory tool could not see workspace memory even when the model had been given it. **A session could never pick up newer memory.** `hydrate` is idempotent for the life of a session — deliberately, so the per-turn injection stays cheap — but nothing reset it and no command forced a reload. A session started before a teammate (or this user on another machine) wrote a block never saw it, for its entire life. Adds `refresh(sessionID)`, which drops the session's cached state so the next hydrate genuinely refetches, exposed as a new `altimate_memory_refresh` tool so the user can just ask for it. **The memory tool disagreed with the prompt.** `altimate_memory_read` read the local store only, while injection merged the workspace overlay — so a user asking "what do you remember?" was shown strictly less than the model actually had. It now merges the same overlay, filtered to the requested scope. `mergeOverlay` is exported rather than duplicated, so the tool and the injection path cannot drift. Verified against a live backend and real mem0, with controls that fail on the old behaviour: a block written mid-session reaches the workspace, plain `hydrate` still cannot see it, `refresh` pulls it in, and the read tool then lists both the early and the late block. Both fixes are mutation-checked — removing the state clear in `refresh`, or the merge in the read tool, each fails a test. Tests: 11286 pass. The two failures in the full run are unrelated and pre-existing (`Truncate > cleanup` fails identically at the merge-base; the subprocess suite is load-flaky and passes 5/5 on its own).
…riting project
Both defects were found by running the on-demand work end to end against a live
backend, not by the unit suite.
**A failed refresh destroyed the session's memory.** `doHydrate` empties the
overlay when a fetch fails — correct at session start, where there was nothing
to lose — but `refresh` clears the session first, so a network hiccup during a
user-requested reload wiped everything the session had. The user asks to
refresh, and silently ends up with less. `refresh` now snapshots the overlay,
restores it when the fetch failed, and returns `{count, ok}` so the caller can
tell "reloaded, workspace is empty" from "could not reach the workspace". The
tool says which happened instead of reporting a count that reads like success.
**The mirror resolved the wrong project.** `existsLocally` — added last round to
stop a delete-during-sweep resurrecting a block — called `MemoryStore.read`
without a directory, so it resolved project scope from the *ambient* instance.
`mirrorBlock` is fire-and-forget, so by the time it ran the ambient instance
could be a different project, the block was not found there, and a perfectly
good write was skipped as "deleted". Same class as the `list`/`read` mismatch
fixed earlier in this stack. The owning directory is now captured at write time,
while the writing context is still current, and threaded through `mirrorBlock`
→ `push` → `existsLocally`; `backfill` passes the directory it swept.
Reproduced directly: writing inside a nested instance context, the block reads
back `false` from the outer context and `true` from the captured directory, and
mirrors in 3s with the fix.
E2E: 11/11 against a live backend and real mem0, including the edge cases the
happy path misses — a block archived elsewhere disappearing on refresh, scope
filtering not leaking project blocks into a global read, refresh on a session
that never hydrated, and refresh on an unlinked project being a quiet no-op.
Both fixes mutation-checked. 431 tests pass across the affected suites.
…ss, by-id lookup **C1 — a stale load could publish over a newer one.** `doHydrate` resolved its write target *after* its awaits by calling `sessionState()` again, so a slow hydration that a refresh had superseded wrote its stale blocks over the fresh result — or, on failure, emptied the session. Loading is now pure (`loadWorkspaceMemory`) and the caller commits into the state that launched it, with a generation check as backstop. `refresh` no longer clears the overlay before fetching, so memory can't blink out of the prompt mid-reload. **C2 — a transient failure reported success and wiped the session.** `refresh` judged failure solely from `hydrateFailed`, which only the `catch` set — but the binding/`memory_enabled` check returns early *without throwing*, and `memoryEnabled` fails closed on any transport error. So an unreachable workspace emptied the overlay and told the user "Reloaded... no memory blocks", which is strictly worse than not reloading. Reads now use a three-way `memoryStatus` (`enabled`/`disabled`/`error`); `refresh` returns a status; the tool says "could not reach the workspace", "not linked" or "memory disabled" accordingly. The write path keeps failing closed. **M3 — concurrent refreshes could discard real memory.** Two refreshes racing let the second capture the first's not-yet-filled state as `previous` and restore emptiness over live memory. `refresh` is now serialized per session. **M1 — `memory_read` still disagreed on the by-id path.** Only the list branch merged the overlay, so looking up a workspace-only block by id — the natural follow-up to seeing it listed — answered "not found" for a block the model was holding. Both branches now share one resolved overlay, and the id path returns every match, since sibling projects may legitimately share an id. It also no longer dies when one scope is unreadable: project scope throws outside an instance, which turned an id lookup into a tool error instead of returning the global and workspace matches. **M2 — directory threading stopped short of the binding.** The mirror confirmed the block against the writing project but still resolved the *binding* from the ambient instance, so project A's memory could upload carrying project B's workspace metadata — a cross-workspace disclosure. The delete path was wholly ambient. Both now resolve `currentBinding(directory)` from the captured directory, and the capture covers global blocks too, whose upload is still gated by the writing project's binding. **M4** gates the refresh tool on `ALTIMATE_WORKSPACE`, so non-pilot users are no longer shipped a tool that can only answer "not enabled". **m2** applies expiry to overlay blocks on read. **m3** marks unsuccessful refreshes `success: false` with a reason, so telemetry stops counting them as successful tool calls. **m5/m6/n1** are cleanups. Every fix above is mutation-checked except the C1 generation check, which is defence-in-depth: the load-owned commit is the actual fix and reverting *that* fails the stale-load test. `MemoryRefreshTool` now has tests for all four branches, which it previously had none of. Tests: 11298 pass. Two failures in the full run are unrelated — `Truncate > cleanup` fails at the merge-base, and `httpapi-session` passes 17/17 alone and 436/436 beside the memory suites.
… flag leaking Bot review on the current head. **An unreadable store reported "not found".** The id path swallowed every read error so that project scope, which throws outside an instance context, could not hide a global or workspace match. But probing both scopes is our choice only when the caller passes `scope: "all"` — when they name a scope, a failure there is a real error they need to see, not missing memory. Suppression is now limited to exactly the project-scope-under-`all` case. **The test flag leaked into the process.** `store-directory.test.ts` set `ALTIMATE_WORKSPACE=1` at module load and never restored it, so unrelated suites sharing the process inherited the pilot flag. Saved and restored in teardown, deleting it when it was previously unset. Both mutation-checked. The reviewer's other two findings were raised against `de4201f7b` and are already fixed in `568b7cfae9`: the id path merges the overlay (it no longer returns before the merge), and hydration commits into the state that launched it, so a stale in-flight fetch cannot clobber a refreshed overlay. Tests: 4481 pass across memory + altimate.
…lt doc Two cleanups on packages/opencode/src/altimate/workspace/memory-sync.ts: 1. Remove the `hydrateFailed` field on `SessionMemory` and its three write sites. The `refresh` restructure replaced the read with a direct `outcome.status === "error"` check, so the field is written but never read. (kilo-code-bot #1123 comment 3840696504.) 2. Remove the doc comment above `export type RefreshResult` — it duplicated the doc on `refresh` (line 867) and had drifted stale (still claimed "Returns how many blocks the session now holds", but `refresh` returns a `RefreshResult`). The doc was also attached to the wrong element (the type, not the function). (kilo-code-bot #1123 comment 3840696513.)
822a2fd to
2ad4754
Compare
9765de5 to
50a70db
Compare
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 participant
Summary
Closes the half of the read requirement that was never built: memory loaded at session start, but "on the user's demand" had no entry point at all, and the memory tool could not see workspace memory even when the model had been given it.
Both gaps were found by auditing the shipped code against the requirement, then confirmed live before being fixed — not assumed.
1. A session could never pick up newer memory
hydrateis idempotent for the life of a session — deliberately, so the per-turn injection stays cheap. But nothing ever reset it, and the CLI exposed no command to force a reload. A session started before a teammate (or this user on another machine) wrote a block never saw it, for its entire life.Proven against a live backend: a running session re-hydrated and still could not see a block written after it started, while a brand-new session saw it immediately.
Adds
refresh(sessionID)— drops the session's cached state (overlay, hydration promise and timed-out latch together) so the next hydrate genuinely refetches — exposed as a newaltimate_memory_refreshtool. In an agentic CLI the natural "on demand" surface is the user asking; the agent calls the tool.2. The memory tool disagreed with the prompt
altimate_memory_readreadMemoryStoreonly, while injection merged the workspace overlay. A user asking "what do you remember?" was shown strictly less than the model actually had.It now merges the same overlay, filtered to the requested scope.
mergeOverlayis exported rather than duplicated, so the tool and the injection path cannot drift.Verification
Live E2E (local backend + real mem0), with controls that fail on the old behaviour:
hydratestill cannot see itrefreshpulls it inaltimate_memory_readlists both the early and the late blockMutation-checked — removing the state clear in
refresh, or the merge in the read tool, each fails a test. The read-tool test drives the real tool through theinitToolfixture rather than asserting onmergeOverlaydirectly, because the helper-level assertion passed even with the tool's merge deleted.Suite: 11286 pass, typecheck clean. The two failures in the full run are unrelated and pre-existing —
Truncate > cleanupfails identically at the merge-base, and the subprocess suite is load-flaky and passes 5/5 on its own.Notes for review
feat/workspace-memory.refreshreturns 0 and the tool reports that there is nothing to reload.🤖 Generated with Claude Code
https://claude.ai/code/session_012wmN54fRA4WLgdVJVunaNk
Summary by cubic
Loads workspace memory on demand and shows it in the memory tool so users see what the model sees. Before: sessions never picked up memory written after start and the read tool omitted workspace memory; after: refresh is safe and race-proof, and reads return the merged local + workspace view with explicit outcomes when the workspace is disabled, unlinked, or unreachable.
ALTIMATE_WORKSPACEin@opencode-ai/coreis on) with clear user outcomes and no turn failures.Written for commit 50a70db. Summary will update on new commits.