Fix nested subagent hierarchy: re-parent depth≥2 subagents to their spawner

[nmishaan]

Fixes #1246.

Problem

Nested Claude Code subagents (main -> orchestrator -> grandchild, spawnDepth >= 2) all
live in the same flat <main-session>/subagents/ directory, so path derivation assigns
parent_session_id = main session at every depth and tags each row
relationship_type = 'subagent'. LinkSubagentSessions() — which corrects the parent
from the authoritative tool_calls.subagent_session_id spawn edge — skipped those rows
because of its WHERE relationship_type != 'subagent' guard. The hierarchy stayed flat
past depth 1: GET /sessions/{orchestrator}/children was empty and per-session usage
rollups attributed the whole subtree to the main session.

Change

All in internal/db/sessions.go, plus one call-site change in internal/sync/engine.go:

• Re-parent from the spawn edge. The linking predicate now updates a session when it
  is not yet tagged subagent (the pre-existing upgrade path) or when its stored parent
  differs from the spawn edge (null-safe IS NOT). The tool-call edge records the actual
  spawn, so it is authoritative over the path heuristic.
• Deterministic conflict resolution (subagentSpawnerExpr). Copied or forked history
  can leave several sessions claiming the same child. Resolution is a pure function of
  the stored edges — never of ingestion order — so a link written from a partial view
  self-corrects on a later sync: candidates are ordered by start time (a fork derives
  from its source, so it starts later), with started_at normalized via strftime
  because raw RFC3339Nano strings do not sort chronologically when fractional seconds
  are stripped. Unknown or malformed start times rank last, and the session id breaks
  remaining ties.
• Cost bounded by edges, not archive. The global statement is driven from the
  idx_tool_calls_subagent partial index (s.id IN (SELECT tc.subagent_session_id ...))
  instead of a full sessions scan, so bulk-sync linking cost tracks the number of spawn
  edges rather than archive size.
• Watcher syncs bounded by the batch. SyncSingleSessionContext — called by the
  session watcher on every file change — now uses LinkSubagentSessionsForSessions,
  which restricts the pass to children reachable from the sessions that sync wrote
  (spawner-side and child-side branches, both index seeks). Bulk paths (full sync,
  reconciliation, resync) keep their coalesced global pass. Query-plan tests pin both
  cost shapes at two archive sizes.
• All single-session branches link. The incremental append path (which can
  introduce a spawn edge when the Task tool_use and its subagent mapping land in the
  same append) links scoped to the appended session, and children referenced by a
  batch's pre-write edges are captured (SubagentChildSessionIDs) before rewrites or
  parser-exclusion deletes cascade those edges away, so a former child still
  re-resolves to its remaining spawner in the same sync.

Tradeoffs and limitations

• When conflicting edges exist and the real spawner has no usable started_at, a dated
  copy wins; protecting it would require reading the child's stored parent, which
  reintroduces ingestion-order dependence. The link self-corrects once the spawner's
  start time becomes known. Documented in subagentSpawnerExpr.
• Fork/copy provenance policy (self-edge guards, deeper lineage tracking) is scoped to
  LinkSubagentSessions: provenance for conflicting fork/copy spawn edges (follow-up to #1247) #1250.
• No frontend change: transcript drill-down already follows tool_calls at any depth;
  this fix makes /children and usage rollups correct.

Reviewers should start at subagentSpawnerExpr and the two linking statements in
internal/db/sessions.go, then the SyncSingleSessionContext call site.

---

Disclosure: investigated and written with Claude Code (AI).

[roborev-ci]

roborev: Combined Review (3d41ce5)

The change needs a medium-severity fix to correctly classify already-parented subagents.

Medium

• internal/db/sessions.go:1586 — The update predicate only runs when the parent differs. If the parser already assigned the authoritative parent but classified the session as continuation, fork, or empty, relationship_type is not upgraded to subagent, causing incorrect grouping. Update when either the parent differs or relationship_type != 'subagent', and add a test where the existing parent already matches the tool-call parent.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 1m17s

[nmishaan]

Good catch confirmed and fixed in 9d2e0b6.

You're right that LinkSubagentSessions sets both parent_session_id and relationship_type = 'subagent', so gating only on "parent differs" skipped rows whose parent already matched the tool-call spawner but were still classified continuation/fork/empty the type upgrade never ran and they grouped wrong.

The predicate is now:

AND (
    relationship_type != 'subagent'
    OR parent_session_id IS NOT (SELECT tc.session_id FROM tool_calls tc
                                 WHERE tc.subagent_session_id = sessions.id LIMIT 1)
)

so the type upgrade fires even when the parent is unchanged, while already-correct subagents match neither branch (no local_modified_at churn). Added TestLinkSubagentSessionsUpgradesTypeWhenParentAlreadyMatches (existing parent already equals the tool-call parent, relationship_type = 'continuation' → upgraded to subagent); it fails on the previous predicate and passes now. Existing Subagent|LinkSubagent tests stay green.

(Disclosure: investigated/authored with Claude Code; changes verified locally.)

[nmishaan]

Pushed a small polish commit (9e58019) after a second review pass:

• Updated the LinkSubagentSessions doc comment, which previously described only the type upgrade and not the nested re-parenting this PR adds.
• Added TestLinkSubagentSessionsLinksNullParentSubagent — a subagent-tagged session with a NULL parent + a spawn edge must be linked to its spawner. Mutation-verified: it fails if the null-safe IS NOT is swapped for !=. (Two reviewers independently suggested this case.)

All internal/db Subagent|LinkSubagent tests pass locally.

One pre-existing observation, intentionally left out of scope: the SELECT tc.session_id … LIMIT 1 has no ORDER BY, so if a single subagent session were ever referenced by spawn edges from two different parents, the chosen parent would be arbitrary. This predates the PR (the SET subquery already used bare LIMIT 1) and shouldn't arise from valid Claude Code data (an agentId maps to one spawn). Flagging only in case you'd want a deterministic ORDER BY / dedupe as a separate follow-up.

(Disclosure: iterated with Claude Code and a second-opinion review pass; all changes verified locally.)

[roborev-ci]

roborev: Combined Review (9e58019)

Medium-severity issue found in subagent re-parenting logic.

Medium

• internal/db/sessions.go:1595 — The scalar subquery uses LIMIT 1 without ensuring a child has exactly one distinct spawner. If copied or forked history creates links from multiple sessions to the same child, a correctly parented subagent may be re-parented arbitrarily. Resolve only unambiguous edges, preserving the current parent when multiple references exist, and add a conflicting-edge regression test.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 2m58s

[roborev-ci]

roborev: Combined Review (c544f11)

The PR has one medium-severity correctness issue in session hierarchy reconciliation.

Medium

• internal/db/sessions.go:1569 — Conflict handling depends on ingestion order. If a copied or forked edge is initially the only stored edge, linking provisionally re-parents the child to it. When the real edge arrives later, the relationship becomes ambiguous, but COALESCE(..., parent_session_id) permanently retains the incorrect provisional parent.
  • Suggested fix: Resolve using stable provenance, or retain enough source-parent state to reverse provisional linkage when candidates become ambiguous. Add a regression test that calls LinkSubagentSessions before and after inserting the conflicting edge.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 3m48s

[nmishaan]

Thanks — real catch. The temporal case is genuine but narrow: it needs a forked/copied edge ingested before its source's edge, and a fork derives from its source (so the source is normally parsed first, where the current COALESCE-preserve behavior is already correct). It's reachable mainly through reconciliation/copy orderings.

Fully resolving it needs stable provenance — e.g. resolving ambiguous edges to the earliest-started spawner, or tracking source-parent state to reverse a provisional link — a fork-lineage design decision I scoped out of this depth-2 parenting fix. The core change here is a strict improvement over the previous bare-LIMIT 1 behavior, and its worst case for conflicting edges is bounded (parent-chain consumers use UNION recursive CTEs that dedupe/terminate — no loop or corruption).

Opened #1250 to track the provenance work (ingestion-order determinism + a self-edge guard), with your suggested before/after-conflicting-edge regression test noted. Happy to send a follow-up PR against whichever resolution approach you prefer.

[mjacobs]

Thanks for the investigation and for opening #1250 — agreed the broader fork/copy provenance policy is separate design work. But the ingestion-order case needs to be resolved in this PR, because it's introduced here: on main, a correctly-parented subagent row can never be re-parented; with this change, a copied edge that is temporarily the only stored edge overwrites the correct parent, and once the real edge arrives the COALESCE preserve-on-ambiguity rule locks the wrong parent in — with no convergence path until follow-up work that has no timeline.

The existing conflicting-edge test inserts both edges before linking, so it doesn't exercise this sequence. Please add: (1) child correctly parented, (2) insert the copied edge, link, (3) insert the real edge, link again, (4) assert the child converges to and remains under the real spawner.

Concretely, resolving the edge to the earliest-started spawner (join sessions, ORDER BY started_at) instead of requiring a unique spawner would make step 4 pass: a fork always starts after its source, so this is deterministic across ingestion orders and self-corrects already-linked rows on the next sync. The self-edge guard and any deeper provenance tracking can stay in #1250.

[roborev-ci]

roborev: Combined Review (c544f11)

The hierarchy reconciliation is functionally sound, but introduces an archive-scaling performance regression.

Medium

• internal/db/sessions.go:1597 — Every watcher-driven sync executes a global UPDATE. The correlated aggregate rechecks every historical tool-linked subagent, so per-event work scales with total archive size. Limit reconciliation to child IDs affected by the current sync, reserve global repair for full resync or migration paths, and add a small-versus-large archive cardinality regression test.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 2m43s

[roborev-ci]

roborev: Combined Review (00660f1)

The change is functionally sound overall, but one medium-severity timestamp-ordering bug can select the wrong parent.

Medium

• internal/db/sessions.go:1579 — Raw RFC3339Nano strings do not always sort chronologically when fractional seconds are optional. For example, a later ...00.1Z timestamp sorts lexically before ...00Z, so a copied spawner may incorrectly win and re-parent the child. Order by parsed or normalized timestamps, and add a regression test covering production-style ...00Z versus ...00.1Z values.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 3m32s

[nmishaan]

Pushed 00660f1 + aeb3313, covering @mjacobs's ingestion-order request and both outstanding roborev findings.

1. Ingestion-order convergence (@mjacobs) — done in 00660f1

Took your suggested resolution: the spawner is now resolved to the earliest-started candidate (LEFT JOIN sessions ps, ordered by start time) instead of requiring a unique one. You were right that the previous rule was the problem rather than the mitigation — COALESCE(unambiguous_edge, parent_session_id) read the row's own parent, so its result depended on what earlier syncs had written, which is exactly how a provisional parent got locked in. Resolution is now a pure function of the stored edges, so a link written from a partial view self-corrects on the next sync.

TestLinkSubagentSessionsConvergesAcrossIngestionOrder implements your four steps literally:

1. child correctly parented under the real spawner,
2. insert the copied edge → LinkSubagentSessions(),
3. insert the real edge → LinkSubagentSessions() again,
4. assert the child converges to the real spawner and stays there on a third link (idempotent, not oscillating).

Plus the mirror order (real edge first, later-arriving copy must not steal the child), so neither order is privileged.

The self-edge guard and deeper provenance tracking remain in #1250 as you scoped.

2. Timestamp ordering (roborev on 00660f1) — real bug, fixed in aeb3313

This one was a genuine defect in the fix above, and worse than a corner case. started_at is TEXT written by timeutil.Format → time.RFC3339Nano, which strips trailing zeros from the fractional second. So production values are not fixed width:

time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)        -> "2026-01-01T00:00:00Z"
                       + 100 * time.Millisecond    -> "2026-01-01T00:00:00.1Z"

. (0x2E) sorts before Z (0x5A), so ordering the raw strings puts the later timestamp first. Any spawner whose start lands on a whole second stores no fractional part at all and sorts behind every fractional one — precisely undercutting the earliest-started rule.

Fixed by ordering on strftime('%Y-%m-%dT%H:%M:%fZ', ps.started_at), which re-renders each value as fixed-width ...T00:00:00.000Z where lexical order is chronological. That's the same normalization the sync_marker triggers in db.go already use. It also subsumes the old NULLIF(...,'') wrapper: strftime returns NULL for an started_at that is unset, empty or malformed, so the leading IS NULL key now sorts unparseable timestamps last too — which the previous form did not. Resolution is millisecond-granular (SQLite's parser truncates there); anything closer falls through to the existing tc.session_id key, so the order stays total.

TestLinkSubagentSessionsOrdersStartTimesChronologically builds both timestamps with timeutil.Format itself so it can't drift from what the parsers write, and asserts up front that the raw strings really do sort inverted — otherwise it would pass without the fix. Mutation-verified: restoring the raw ordering fails it with parent copied-spawner.

3. Archive scaling (roborev on c544f11) — fixed in aeb3313, and it's now faster than main

Worth stating plainly: the global UPDATE is not introduced by this PR. main plans the same way —

-- main today
QUERY PLAN
|--SCAN sessions                       <-- full scan of the archive
`--CORRELATED SCALAR SUBQUERY
   `--SEARCH tc USING INDEX idx_tool_calls_subagent (subagent_session_id=?)

EXISTS(...) reads like an index probe but plans as a scan of sessions with a correlated probe per row. Driving the row set from the edges instead — s.id IN (SELECT tc.subagent_session_id ...) — seeks the idx_tool_calls_subagent partial index and then looks each child up by primary key:

|--SEARCH s USING INDEX sqlite_autoindex_sessions_1 (id=?)
|--LIST SUBQUERY
|  `--SEARCH tc USING COVERING INDEX idx_tool_calls_subagent (subagent_session_id>?)

Measured on a seeded archive in the steady state a watcher-driven sync actually hits (every subagent already linked, UPDATE matches zero rows), holding spawn edges constant at 8k:

variant	40k sessions	200k sessions	growth
main today	17.1 ms	102.5 ms	6.0×
this PR, EXISTS form	35.9 ms	109.4 ms	3.0×
this PR, IN form (shipped)	25.1 ms	30.9 ms	1.2×

So this isn't just a recovered regression — at 200k sessions linking is now 3.3× faster than main, because per-sync cost tracks the number of spawn edges rather than the size of the archive. (I also measured a single-pass UPDATE .. FROM with a ROW_NUMBER() window: 27.4 ms — it trades 8k tiny sorts for one large materialized one, so it's a bigger diff for no gain. Rejected.)

On "limit reconciliation to child IDs affected by the current sync": I deliberately didn't, because it would break the convergence in §1. A later-arriving spawn edge is written into the spawner's transcript, not the child's — re-checking a child that this sync didn't touch is exactly what lets a provisional link self-correct. Narrowing the scope to sessions written this sync reintroduces the lock-in @mjacobs asked to fix. Making the global statement index-driven gets the scaling without that trade.

TestLinkSubagentSessionsPlanScalesWithSpawnEdges asserts the query plan rather than wall-clock time — the plan is what decides the scaling and it's deterministic in CI (same approach as the existing EXPLAIN QUERY PLAN assertions in source_path_hints_test.go). It runs at two archive sizes so it can't pass merely for want of statistics. Mutation-verified: swapping the IN back to the equivalent-reading EXISTS re-introduces SCAN s and fails it while every behavioural test stays green — which is the silent regression it exists to catch.

Verification

Go 1.26.5, -tags fts5:

• ./internal/db — full package, green (all LinkSubagent* tests included)
• ./internal/duckdb — full package, green, including TestPushIncrementalMirrorsSubagentLinkBackfill, which depends on this function bumping local_modified_at so the linked session re-enters the incremental mirror window
• ./internal/sync — all Subagent|Link|Parent|Hierarch tests green

Both new tests are mutation-verified fail→pass as described above.

Two flaky-test notes, both pre-existing and outside internal/db:

• I could not get a clean full-package internal/sync run on either branch: TestFileChangeTimeDetectsSameStatRewrite fails for me on unmodified main (9e5324c) in 4 of 5 isolated runs. Looks environment-sensitive rather than related to any of this.
• Go Test (windows-latest) is red on main too, with a different test each run — TestPushWatchProductionOwnersRetryPartialAuthoritativeBatch and TestWatcherOverflowCollapsesPathAndByteLimitsToOneFullSync on main, TestVerifiedSourceGateRechecksAfterStatAndWatcherInvalidation on this branch. All three are in internal/sync; this PR touches only internal/db.

Happy to open a separate issue for the internal/sync flakes if that's useful — they're unrelated to this change but they do make the signal here harder to read.

(The CI workflow runs for both pushes are sitting at action_required pending maintainer approval, so the only check reporting on this PR right now is roborev.)

---

(Disclosure: investigated and authored with Claude Code; the timing/plan numbers above were measured locally on a seeded SQLite archive, and both new tests were mutation-verified fail→pass.)

[roborev-ci]

roborev: Combined Review (aeb3313)

Deterministic subagent linking has two medium-severity correctness and scalability issues.

Medium

• internal/db/sessions.go:1590 — Candidates with unknown or malformed started_at always lose to candidates with known timestamps. This can replace the correctly linked original spawner with a later copied spawner. Apply chronological resolution only when timestamps are comparable; otherwise preserve a verified/current parent or retain ambiguity. Add regression coverage for this case.

• internal/db/sessions.go:1628 — Every linking pass enumerates all historical spawn edges and resolves every linked child, even when the current sync changed nothing. Watcher and periodic-sync work therefore scales with total archive size rather than the changed batch. Track affected child IDs or maintain a bounded dirty-child set, and add a scaling test that increases archived spawn edges while holding the changed batch constant.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 3m50s

[roborev-ci]

roborev: Combined Review (32be42b)

Changes need revision: two medium-severity issues can leave subagent hierarchy links stale.

Medium

• internal/sync/engine.go:12459 — Incremental single-session sync returns after updating tool-call subagent links without invoking the new scoped linker. A newly appended agentId edge leaves the child hierarchy stale until a later bulk sync. After writeIncremental, call LinkSubagentSessionsForSessions with the incremental session ID and add a regression test for an incrementally populated spawn edge.

• internal/db/sessions.go:1707 — The scoped query discovers affected children only from post-write edges. If a full rewrite or parser exclusion removes a spawner edge, the former child is omitted and cannot be re-resolved to another remaining spawner. Capture referenced child IDs before replacing or deleting sessions, then include them in the post-write scoped linking batch.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 4m18s

[roborev-ci]

roborev: Combined Review (985126c)

Changes need revision: two medium-severity parent-linking correctness issues remain.

Medium

• internal/db/sessions.go:1598 — Conflicting spawn edges with equal, unknown, or sub-millisecond-different start times fall back to tc.session_id. This can incorrectly re-parent a linked child merely because another session ID sorts first, especially when copied histories share timestamps or %f drops sub-millisecond precision. Preserve explicit parent provenance or use trustworthy lineage data for ties; do not overwrite an established parent based solely on session-ID ordering. Add equal-start and sub-millisecond tests.

• internal/db/sessions.go:1707 — A child whose sole spawn edge was removed is excluded from the update because both UNION branches select only from remaining tool_calls. The child therefore retains a stale parent indefinitely, even when its pre-write ID was captured. Explicitly process captured child IDs when no edge remains, restoring or clearing the edge-derived parent and relationship as appropriate. Add a test covering removal of the sole spawn edge.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 7m1s

[wesm]

I can't push here anymore for some reason so opening a new PR to supersede this