Skip to content

Commit 935f474

Browse files
committed
address copilot R1: auto-seed root into isDescendantOf cache
Copilot caught a footgun: when callers passed opts.cache without seeding it with root, the parent walk would never terminate at root (the loop checks known.has(cur), not cur === root) and would return false even for true descendants. Fix in Session.isDescendantOf: unconditionally known.add(root) at the top of every call, regardless of whether the caller pre-seeded. JSDoc updated to say a fresh empty Set is sufficient. AutoReply now seeds with new Set() (no manual root pre-seed). Also added a regression test that explicitly passes an empty cache and asserts the call succeeds. PR feedback: #16 (comment)
1 parent 980f5e0 commit 935f474

3 files changed

Lines changed: 34 additions & 6 deletions

File tree

packages/opencode/src/session/auto-reply/auto-reply.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,10 @@ export const make = Effect.fn("SessionAutoReply.make")(function* (config: Config
4646
livelockWarned: false,
4747
}
4848

49-
const descendants = new Set<SessionID>([config.rootSessionID])
49+
// Reused across calls so the parent walk is traversed at most once per
50+
// descendant id. Session.isDescendantOf auto-seeds `rootSessionID` on every
51+
// call, so an empty Set is the simplest seed.
52+
const descendants = new Set<SessionID>()
5053

5154
const isDescendant = (sid: SessionID) =>
5255
session.isDescendantOf(sid, config.rootSessionID, { maxDepth: MAX_LINEAGE_DEPTH, cache: descendants })

packages/opencode/src/session/session.ts

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -381,10 +381,11 @@ export interface Interface {
381381
* stops at any `NotFoundError` (returns false). When `opts.cache` is
382382
* provided, it is used both as a positive-hit short-circuit and as an
383383
* accumulator: every confirmed descendant in the walked chain is added to
384-
* the set. Callers that issue many `isDescendantOf` calls against the same
385-
* root (e.g. SessionAutoReply) should seed the cache with `new Set([root])`
386-
* and reuse it across calls so the parent chain is traversed at most once
387-
* per node.
384+
* the set. The cache is auto-seeded with `root` on every call, so callers
385+
* 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.
388389
*/
389390
readonly isDescendantOf: (
390391
sid: SessionID,
@@ -693,7 +694,13 @@ export const layer: Layer.Layer<Service, never, Bus.Service | Storage.Service> =
693694
opts?: { maxDepth?: number; cache?: Set<SessionID> },
694695
) {
695696
const maxDepth = opts?.maxDepth ?? 64
696-
const known = opts?.cache ?? new Set<SessionID>([root])
697+
const known = opts?.cache ?? new Set<SessionID>()
698+
// 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.
703+
known.add(root)
697704
if (sid === root) return true
698705
if (known.has(sid)) return true
699706
// Walk parent chain. Track the chain so we can promote every visited

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,4 +296,22 @@ describe("Session.isDescendantOf", () => {
296296
},
297297
})
298298
})
299+
300+
test("auto-seeds root into cache so callers can pass an empty Set", async () => {
301+
await using tmp = await tmpdir({ git: true })
302+
await Instance.provide({
303+
directory: tmp.path,
304+
fn: async () => {
305+
const root = await create({ title: "root" })
306+
const child = await create({ title: "child", parentID: root.id })
307+
// 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.
310+
const cache = new Set<SessionID>()
311+
expect(await isDescendantOf(child.id, root.id, { cache })).toBe(true)
312+
expect(cache.has(root.id)).toBe(true)
313+
await remove(root.id)
314+
},
315+
})
316+
})
299317
})

0 commit comments

Comments
 (0)