Skip to content

Commit aed4e5a

Browse files
committed
address copilot R3: tighten JSDoc + restore AutoReply trace span
R3 caught two issues: 1. JSDoc overstated the cross-root guard. The runtime check only catches a non-empty cache that lacks root; it does not catch a cache that has been mixed (root present plus entries from a different root). Detecting that would require tagging the cache. JSDoc rewritten to be honest: the guard is partial, intended usage is one cache per root, and a Map<root,Set> is the correct shape if disjoint roots must share state. 2. AutoReply lost its 'SessionAutoReply.isDescendant' trace span when the walk delegated to a plain arrow function. Restored Effect.fn wrapper so traces still show the AutoReply-specific call site alongside the inner Session.isDescendantOf span. No behavior change, only observability. PR feedback: #16 - comment 3163766945 (JSDoc overstates guard scope) - comment 3163766986 (lost trace span name)
1 parent f54a1ad commit aed4e5a

2 files changed

Lines changed: 19 additions & 9 deletions

File tree

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,11 +48,17 @@ export const make = Effect.fn("SessionAutoReply.make")(function* (config: Config
4848

4949
// Reused across calls so the parent walk is traversed at most once per
5050
// descendant id. Session.isDescendantOf auto-seeds `rootSessionID` on every
51-
// call, so an empty Set is the simplest seed.
51+
// call, so an empty Set is the simplest seed. Wrapped in Effect.fn so
52+
// AutoReply-specific lineage checks remain identifiable in traces alongside
53+
// the inner Session.isDescendantOf span.
5254
const descendants = new Set<SessionID>()
5355

54-
const isDescendant = (sid: SessionID) =>
55-
session.isDescendantOf(sid, config.rootSessionID, { maxDepth: MAX_LINEAGE_DEPTH, cache: descendants })
56+
const isDescendant = Effect.fn("SessionAutoReply.isDescendant")(function* (sid: SessionID) {
57+
return yield* session.isDescendantOf(sid, config.rootSessionID, {
58+
maxDepth: MAX_LINEAGE_DEPTH,
59+
cache: descendants,
60+
})
61+
})
5662

5763
const bump = (kind: "question" | "permission", sid: SessionID) => {
5864
if (kind === "question") stats.autoRejectedQuestions++

packages/opencode/src/session/session.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -383,12 +383,16 @@ 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-
* 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.
386+
*
387+
* Cache reuse is only safe within a single root. As a partial guard, if a
388+
* non-empty cache is passed that does not already contain `root`, the call
389+
* dies — that case can only arise from a cache leaked from another lineage.
390+
* The guard does NOT catch caches that have been mixed (root present plus
391+
* entries from a different root); detecting that would require tagging the
392+
* cache. Each `SessionAutoReply` instance owns its own cache, which is the
393+
* intended usage. If you have a use case that needs disjoint roots to
394+
* share a cache, build a `Map<root, Set<SessionID>>` outside this helper
395+
* and pass the per-root inner Set.
392396
*/
393397
readonly isDescendantOf: (
394398
sid: SessionID,

0 commit comments

Comments
 (0)