Skip to content

Commit f54a1ad

Browse files
committed
address copilot R2: guard against cross-root cache reuse + fix test wording
R2 caught a remaining footgun: a cache populated against rootA could short-circuit a later call against rootB via known.has(sid), returning silently-wrong results. AutoReply doesn't do this (fresh Set per make call), but future ACP/TUI callers might. Fix: at call entry, if opts.cache is non-empty AND does not already contain the supplied root, die with a clear error message naming the expected root and the cache size. JSDoc updated to spell out the not-shared-across-roots invariant. Also tightened the auto-seed-empty-cache test comment: the failure mode without the auto-seed is walking PAST root (root.has(root) is false in an empty cache) and bottoming out, not necessarily a NotFoundError. New regression test: 'dies if cache is non-empty but missing root' populates a cache under rootA then asserts a subsequent rootB call dies. PR feedback: #16 - comment 3163694330 (cross-root cache aliasing) - comment 3163694378 (test wording nit)
1 parent 935f474 commit f54a1ad

2 files changed

Lines changed: 46 additions & 9 deletions

File tree

packages/opencode/src/session/session.ts

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -383,9 +383,12 @@ export interface Interface {
383383
* accumulator: every confirmed descendant in the walked chain is added to
384384
* the set. The cache is auto-seeded with `root` on every call, so callers
385385
* can pass a fresh `new Set()` and reuse it across calls without seeding.
386-
* Callers that issue many `isDescendantOf` calls against the same root
387-
* (e.g. SessionAutoReply) should reuse one cache so the parent chain is
388-
* traversed at most once per node.
386+
* Caches MUST NOT be shared across different roots — if a non-empty cache
387+
* is passed that does not already contain `root`, the call dies with a
388+
* clear error instead of returning silently-wrong results from another
389+
* lineage's accumulated entries. Callers that issue many `isDescendantOf`
390+
* calls against the same root (e.g. SessionAutoReply) should reuse one
391+
* cache so the parent chain is traversed at most once per node.
389392
*/
390393
readonly isDescendantOf: (
391394
sid: SessionID,
@@ -695,11 +698,20 @@ export const layer: Layer.Layer<Service, never, Bus.Service | Storage.Service> =
695698
) {
696699
const maxDepth = opts?.maxDepth ?? 64
697700
const known = opts?.cache ?? new Set<SessionID>()
701+
// Defend against accidental cache sharing across different roots: if a
702+
// caller passes a non-empty cache that lacks `root`, those entries were
703+
// populated under a different lineage and `known.has(sid)` could
704+
// short-circuit to true with the wrong answer. Fail loudly instead of
705+
// returning a silently wrong result.
706+
if (known.size > 0 && !known.has(root)) {
707+
return yield* Effect.die(
708+
new Error(
709+
`Session.isDescendantOf: opts.cache appears to belong to a different root (size=${known.size}, missing root=${root}). Caches must not be shared across roots.`,
710+
),
711+
)
712+
}
698713
// Always seed `root` into the working set so the parent walk has a
699-
// termination anchor regardless of whether the caller pre-seeded the
700-
// cache. Without this, a caller-provided cache that happens to omit
701-
// `root` would cause the walk to bottom out at a not-found parent and
702-
// return false even for true descendants — a silent footgun.
714+
// termination anchor and the next cross-root reuse check still works.
703715
known.add(root)
704716
if (sid === root) return true
705717
if (known.has(sid)) return true

packages/opencode/test/session/session.test.ts

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -305,13 +305,38 @@ describe("Session.isDescendantOf", () => {
305305
const root = await create({ title: "root" })
306306
const child = await create({ title: "child", parentID: root.id })
307307
// Caller passes a fresh empty Set — root must still be reachable.
308-
// Without the auto-seed in isDescendantOf this would walk past root,
309-
// hit a not-found parent, and return false.
308+
// Without the auto-seed in isDescendantOf this would walk past root
309+
// (since `known.has(root)` would be false), bottom out at the
310+
// not-found parent of root, and return false.
310311
const cache = new Set<SessionID>()
311312
expect(await isDescendantOf(child.id, root.id, { cache })).toBe(true)
312313
expect(cache.has(root.id)).toBe(true)
313314
await remove(root.id)
314315
},
315316
})
316317
})
318+
319+
test("dies if cache is non-empty but missing root (cross-root reuse guard)", async () => {
320+
await using tmp = await tmpdir({ git: true })
321+
await Instance.provide({
322+
directory: tmp.path,
323+
fn: async () => {
324+
const rootA = await create({ title: "rootA" })
325+
const rootB = await create({ title: "rootB" })
326+
const childA = await create({ title: "childA", parentID: rootA.id })
327+
// Populate a cache under rootA, then incorrectly try to reuse it
328+
// against rootB. The guard must reject this loudly.
329+
const cache = new Set<SessionID>()
330+
expect(await isDescendantOf(childA.id, rootA.id, { cache })).toBe(true)
331+
// cache now contains {rootA, childA} but not rootB.
332+
let died = false
333+
await isDescendantOf(rootA.id, rootB.id, { cache }).catch(() => {
334+
died = true
335+
})
336+
expect(died).toBe(true)
337+
await remove(rootA.id)
338+
await remove(rootB.id)
339+
},
340+
})
341+
})
317342
})

0 commit comments

Comments
 (0)