Skip to content

Commit db315dd

Browse files
committed
fix(workspace): narrow the id-lookup error suppression, stop the test 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.
1 parent 568b7cf commit db315dd

3 files changed

Lines changed: 38 additions & 8 deletions

File tree

packages/opencode/src/memory/tools/memory-read.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,15 +45,17 @@ export const MemoryReadTool = Tool.define("altimate_memory_read", {
4545

4646
const matches: (MemoryBlock & { origin?: string })[] = []
4747
for (const scope of scopes) {
48-
// One scope failing must not take the others down with it. Project
49-
// scope throws outside an instance context, which would otherwise
50-
// turn an id lookup into a tool error instead of returning the
51-
// global and workspace matches. `listAll` already behaves this way.
48+
// Narrowly suppressed: probing BOTH scopes is our choice, not the
49+
// caller's, and project scope throws outside an instance context --
50+
// that must not hide a global or workspace match. A scope the caller
51+
// asked for explicitly failing is a real error they need to see,
52+
// rather than a misleading "not found".
5253
let block: MemoryBlock | undefined
5354
try {
5455
block = await MemoryStore.read(scope, args.id)
55-
} catch {
56-
continue
56+
} catch (e) {
57+
if (args.scope === "all" && scope === "project") continue
58+
throw e
5759
}
5860
if (!block) continue
5961
// Respect include_expired for ID reads

packages/opencode/test/memory/overlay-merge.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,27 @@ describe("the memory read tool", () => {
212212
expect(String(res.output ?? "")).not.toContain("EXPIRES SOON")
213213
})
214214

215+
test("an explicitly requested scope that cannot be read is an error, not 'not found'", async () => {
216+
// Probing both scopes is our choice when scope=all, so a project-scope
217+
// failure there is suppressed. When the caller names a scope, swallowing
218+
// the failure would report missing memory for a store that is merely
219+
// unreadable.
220+
const { MemoryStore } = await import("../../src/memory/store")
221+
const original = MemoryStore.read
222+
;(MemoryStore as any).read = async () => {
223+
throw new Error("store unreadable")
224+
}
225+
try {
226+
const tool = await initTool(MemoryReadTool)
227+
const res: any = await tool.execute({ id: "x", scope: "project" }, { sessionID: SES, agent: "build" })
228+
const text = String(res.output ?? "")
229+
expect(text).not.toContain("No memory block found")
230+
expect(text).toContain("store unreadable")
231+
} finally {
232+
;(MemoryStore as any).read = original
233+
}
234+
})
235+
215236
test("surfaces workspace memory, not just the local store", async () => {
216237
// The tool read MemoryStore only, so it disagreed with what the model was
217238
// actually given -- injection merges the overlay and the tool did not.

packages/opencode/test/memory/store-directory.test.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
// The workspace flag is read at module load, so it must be set before the
2-
// modules under test are imported.
2+
// modules under test are imported -- and restored afterwards so it does not
3+
// leak into unrelated suites sharing this process.
4+
const ORIGINAL_WORKSPACE_FLAG = process.env.ALTIMATE_WORKSPACE
35
process.env.ALTIMATE_WORKSPACE = "1"
46

5-
import { describe, test, expect, beforeEach, afterEach } from "bun:test"
7+
import { describe, test, expect, beforeEach, afterEach, afterAll } from "bun:test"
68
import fs from "fs/promises"
79
import path from "path"
810
import os from "os"
@@ -12,6 +14,11 @@ import { MemoryStore } from "@/memory/store"
1214
// and so cannot catch path-resolution bugs. Callers outside an Instance context
1315
// -- the `link` subcommand is one -- pass `directory` explicitly; every step of
1416
// the read path has to honour it, not just the directory scan.
17+
afterAll(() => {
18+
if (ORIGINAL_WORKSPACE_FLAG === undefined) delete process.env.ALTIMATE_WORKSPACE
19+
else process.env.ALTIMATE_WORKSPACE = ORIGINAL_WORKSPACE_FLAG
20+
})
21+
1522
describe("MemoryStore project scope with an explicit directory", () => {
1623
let proj: string
1724

0 commit comments

Comments
 (0)