refactor(session): extract Session.isDescendantOf from auto-reply walk (F13) by tesdal · Pull Request #16 · tesdal/opencode

tesdal · 2026-04-29T19:28:31Z

Summary

F13 of the audit-remediation effort. Move the lineage walk from SessionAutoReply (where it was inline) to Session.Interface.isDescendantOf so ACP and TUI can reuse it. AutoReply still owns its policy (livelock depth limit, cross-event dedup cache) — the helper is generic.

Changes

New Session.isDescendantOf(sid, root, opts?: { maxDepth?, cache? }): Effect<boolean>.
- Default maxDepth = 64. Generic — AutoReply still passes its own MAX_LINEAGE_DEPTH = 32 explicitly.
- When opts.cache is provided, it is used both as a positive-hit short-circuit and as an accumulator: every confirmed-descendant intermediate node is added to the set. Repeated calls against the same lineage become O(1) after the first walk.
- NotFoundError defects from session.get are caught and treated as non-match.
AutoReply replaces a 23-line inline Effect.fn with a 2-line shim. Drops unused imports (Option, NotFoundError).
7 new tests on Session.isDescendantOf: identity, direct child, grandchild, unrelated, non-existent, maxDepth boundary, cache accumulation.

Verification

bun typecheck from packages/opencode/ — passes.
bun test test/session/session.test.ts — 11/11 pass (32 expects, 11s).
bun test test/session/auto-reply.test.ts test/session/subagent-hang-regression.test.ts — all pass (one pre-existing flake on first run, issue test(opencode): prompt.test.ts flaky since #23710 — 3s timeout vs. SIGTERM→exit race anomalyco/opencode#24060, unrelated).

Diamond review

codex-5.3 (spec): APPROVE
Opus (quality): APPROVE

Review-only PR against phase-ab-base. Will be closed without merging; work merges into local/integration-v2 via --no-ff after Copilot iterates clean.

…k (F13) The lineage walk in SessionAutoReply (formerly run-events) was ad hoc. ACP and TUI will eventually need the same operation. Move the walk to Session.Interface.isDescendantOf with the lookup cache as a caller-provided argument so AutoReply retains dedup across events. - Add Session.isDescendantOf(sid, root, opts?: { maxDepth?, cache? }). Default maxDepth = 64 (generic). Cache, when provided, is mutated with all confirmed-descendant intermediate nodes, turning repeated calls against the same lineage into O(1) after the first walk. NotFoundError defects are caught and treated as a non-match. - AutoReply now delegates via a 2-line shim that passes its own MAX_LINEAGE_DEPTH (32) and its descendants-Set as the cache. Drops the inline 23-line Effect.fn plus Option/NotFoundError imports. - 7 new tests on Session.isDescendantOf: identity, direct child, grandchild, unrelated, non-existent, maxDepth boundary, cache accumulation. AutoReply's existing 14 auto-reply.test tests and the subagent-hang regression continue to pass unchanged. Diamond review: codex-5.3 spec APPROVE, Opus quality APPROVE. Refs: F13 in docs/superpowers/plans/2026-04-23-audit-remediation.md

Copilot

Pull request overview

Refactors session lineage checking by extracting the parent-chain walk from SessionAutoReply into a reusable Session.Service.isDescendantOf helper so other subsystems (ACP/TUI) can share the same logic.

Changes:

Added Session.isDescendantOf(sid, root, opts?) to the Session service, including optional max-depth and a reusable cache for confirmed descendants.
Updated SessionAutoReply to delegate lineage checks to session.isDescendantOf, removing the inline walk and now-unused imports.
Added new unit tests covering common lineage cases, maxDepth behavior, and cache accumulation.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
packages/opencode/src/session/session.ts	Introduces the shared `isDescendantOf` helper and wires it into the Session service implementation.
packages/opencode/src/session/auto-reply/auto-reply.ts	Replaces the inline ancestry walk with a call to the new Session helper.
packages/opencode/test/session/session.test.ts	Adds coverage for `Session.isDescendantOf` behavior (identity, ancestry, non-existent IDs, depth limit, cache).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tesdal · 2026-04-29T19:34:03Z

+      opts?: { maxDepth?: number; cache?: Set<SessionID> },
+    ) {
+      const maxDepth = opts?.maxDepth ?? 64
+      const known = opts?.cache ?? new Set<SessionID>([root])


Good catch — fixed in 935f474 by unconditionally adding root to known at the top of isDescendantOf, regardless of whether opts.cache was provided. JSDoc updated, AutoReply now passes new Set() (no manual seed), and added a regression test that asserts an empty cache works.

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)

Copilot

Pull request overview

Refactors session lineage detection by extracting the parent-chain walk from SessionAutoReply into a reusable Session.Service.isDescendantOf helper, enabling reuse by ACP/TUI while keeping AutoReply-specific policy (depth limit, dedup cache) at the call site.

Changes:

Added Session.Service.isDescendantOf(sid, root, opts?) with optional maxDepth and cache accumulation.
Updated SessionAutoReply to call the new helper and removed now-unused imports/inline walk logic.
Added focused unit tests covering identity/lineage/non-existent IDs/depth boundary and cache accumulation behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
packages/opencode/src/session/session.ts	Adds `Interface.isDescendantOf` and its implementation (depth-bounded parent walk with optional cache).
packages/opencode/src/session/auto-reply/auto-reply.ts	Replaces inline lineage walk with a call to `session.isDescendantOf(...)` and reuses a Set cache.
packages/opencode/test/session/session.test.ts	Adds unit tests validating `isDescendantOf` behavior, including cache seeding/accumulation and depth limits.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tesdal · 2026-04-29T19:42:22Z

+      const maxDepth = opts?.maxDepth ?? 64
+      const known = opts?.cache ?? new Set<SessionID>()
+      // Always seed `root` into the working set so the parent walk has a
+      // termination anchor regardless of whether the caller pre-seeded the
+      // cache. Without this, a caller-provided cache that happens to omit
+      // `root` would cause the walk to bottom out at a not-found parent and
+      // return false even for true descendants — a silent footgun.
+      known.add(root)
+      if (sid === root) return true
+      if (known.has(sid)) return true
+      // Walk parent chain. Track the chain so we can promote every visited


Fixed in f54a1ad. At call entry, if opts.cache is non-empty AND does not contain root, isDescendantOf now dies with a clear error naming the expected root and the cache size. New regression test 'dies if cache is non-empty but missing root' pins the guard. JSDoc updated. Picked option (a) over (b) because Map<root,Set> changes the API for the AutoReply use case where a single cache is correct, while the guard catches the actual bug class without complicating the simple case.

tesdal · 2026-04-29T19:42:24Z

+        const child = await create({ title: "child", parentID: root.id })
+        // Caller passes a fresh empty Set — root must still be reachable.
+        // Without the auto-seed in isDescendantOf this would walk past root,
+        // hit a not-found parent, and return false.


Fixed in f54a1ad — wording now says 'walk PAST root (since known.has(root) would be false), bottom out at the not-found parent of root, and return false', which matches the actual code path.

…ording 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)

Copilot

Pull request overview

Refactors Session lineage traversal into a reusable Session.Interface.isDescendantOf helper so multiple subsystems (ACP/TUI/AutoReply) can share the same parent-walk logic, while preserving AutoReply’s policy choices (depth limit + dedup cache).

Changes:

Added Session.isDescendantOf(sid, root, opts?) with maxDepth and optional cache accumulation + cross-root guard.
Simplified SessionAutoReply by replacing the inline parent-walk with a thin call into session.isDescendantOf.
Added targeted tests covering identity, ancestry cases, missing sessions, depth limit, cache accumulation, and the cross-root cache guard.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
packages/opencode/src/session/session.ts	Introduces `isDescendantOf` API + implementation and wires it into the Session service.
packages/opencode/src/session/auto-reply/auto-reply.ts	Replaces inline lineage walk with `session.isDescendantOf` and reuses a shared cache.
packages/opencode/test/session/session.test.ts	Adds test suite validating `Session.isDescendantOf` behavior, including cache and guard semantics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tesdal · 2026-04-29T20:05:35Z

+   * Caches MUST NOT be shared across different roots — if a non-empty cache
+   * is passed that does not already contain `root`, the call dies with a
+   * clear error instead of returning silently-wrong results from another
+   * lineage's accumulated entries. Callers that issue many `isDescendantOf`
+   * calls against the same root (e.g. SessionAutoReply) should reuse one
+   * cache so the parent chain is traversed at most once per node.


Fair point — the guard is partial. Tightened the JSDoc in aed4e5a to spell that out: the runtime check only catches the missing-root case (a cache leaked from another lineage); a mixed cache (root present plus entries from a different root) needs cache tagging which we don't add here. Intended usage is one cache per root (AutoReply does this — fresh Set per make()); for disjoint-root cache sharing, callers should build a Map<root, Set> outside the helper. Doc now matches what the code actually enforces.

tesdal · 2026-04-29T20:05:37Z

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


Good catch on the lost trace span. Restored in aed4e5a — wrapped the call in Effect.fn("SessionAutoReply.isDescendant") so traces still show the AutoReply-specific span alongside the inner Session.isDescendantOf span. No behavior change.

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)

Copilot

Pull request overview

Refactors session lineage traversal into a reusable Session.Interface.isDescendantOf helper so multiple subsystems (AutoReply/ACP/TUI) can share the same parent-chain walk logic while allowing callers to set their own policies (depth limits, optional cache).

Changes:

Added Session.isDescendantOf(sid, root, opts?) with maxDepth and optional descendant cache (including a cross-root reuse guard).
Updated SessionAutoReply to delegate descendant checks to Session.isDescendantOf, removing the inline walk and dropping unused imports.
Added focused unit tests covering depth behavior, missing sessions, cache accumulation, and the cross-root cache guard.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
packages/opencode/src/session/session.ts	Introduces `isDescendantOf` in the Session service/interface, including cache semantics and guard behavior.
packages/opencode/src/session/auto-reply/auto-reply.ts	Replaces inline lineage walk with a thin `Effect.fn` wrapper around `session.isDescendantOf`.
packages/opencode/test/session/session.test.ts	Adds new tests validating `Session.isDescendantOf` behavior (identity, ancestry, maxDepth, cache behavior, guard).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tesdal · 2026-04-29T20:31:20Z

Review-only PR. Diamond APPROVE. Copilot 4 rounds: R1 1 substantive (cache footgun fix); R2 1 substantive (cross-root guard) + 1 nit (test wording); R3 1 substantive (JSDoc overstatement) + 1 nit (lost trace span); R4 clean. Merging into local/integration-v2 via --no-ff.

Move the lineage walk from SessionAutoReply (where it was inline) to Session.Interface.isDescendantOf. AutoReply still owns its policy (MAX_LINEAGE_DEPTH=32, per-make-call cache); the helper is generic (default maxDepth=64) and reusable by ACP/TUI. The helper auto-seeds root into the cache, guards against cross-root cache reuse (dies if non-empty cache lacks root), and accumulates walked descendants for amortized O(1) repeated lookups against the same lineage. Diamond: codex-5.3 spec APPROVE, Opus quality APPROVE. Copilot 4 rounds: - R1 1 substantive: cache footgun (callers had to remember to seed root) → fixed by unconditionally adding root to known at call entry. - R2 1 substantive + 1 nit: cross-root cache aliasing → fixed by guard that dies on non-empty cache missing root; test wording → fixed. - R3 1 substantive + 1 nit: JSDoc overstated guard scope → tightened to match what the code actually enforces; lost AutoReply trace span → restored via Effect.fn wrapper. - R4 clean. PR (review-only): #16 Refs: F13 in docs/superpowers/plans/2026-04-23-audit-remediation.md

tesdal requested a review from Copilot April 29, 2026 19:28

Copilot started reviewing on behalf of tesdal April 29, 2026 19:29 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

tesdal requested a review from Copilot April 29, 2026 19:35

Copilot started reviewing on behalf of tesdal April 29, 2026 19:36 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

tesdal requested a review from Copilot April 29, 2026 19:49

Copilot started reviewing on behalf of tesdal April 29, 2026 19:50 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

tesdal requested a review from Copilot April 29, 2026 20:05

Copilot started reviewing on behalf of tesdal April 29, 2026 20:06 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

tesdal closed this Apr 29, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(session): extract Session.isDescendantOf from auto-reply walk (F13)#16

refactor(session): extract Session.isDescendantOf from auto-reply walk (F13)#16
tesdal wants to merge 4 commits intophase-ab-basefrom
audit-f13-is-descendant-of

tesdal commented Apr 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

tesdal Apr 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

tesdal Apr 29, 2026

Uh oh!

tesdal Apr 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

tesdal Apr 29, 2026

Uh oh!

tesdal Apr 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

tesdal commented Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

Conversation

tesdal commented Apr 29, 2026

Summary

Changes

Verification

Diamond review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

tesdal Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

tesdal Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

tesdal Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

tesdal Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

tesdal Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

tesdal commented Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants