fix(db): promote orphan subagent sessions to sidebar roots - #725
Conversation
roborev: Combined Review (
|
roborev triage (
|
mjacobs
left a comment
There was a problem hiding this comment.
The orphan-as-root approach (option (a) from #382) is the right call, and the unpaginated SQLite path reads correctly.
roborev's two findings should block merge and I agree with both: the PostgreSQL sidebar not getting the fix (parity), and the paginated path promoting children of soft-deleted parents. Not repeating them here.
A few additional points, inline where localized:
- The soft-deleted divergence exists because the PR ships two independent promotion implementations — worth consolidating to one shared definition (note on
buildCanonicalRootWhere). - The new tests exercise the CTE path, not the paginated sidebar that actually ships (note on the tests).
- Small relationship-set drift between the two implementations (note on the orphan seed).
|
Addressed the review requests in the latest push:
Validation run locally:
|
roborev: Combined Review (
|
|
Follow-up fix in the latest push: The shared canonical-root refactor accidentally changed paginated sidebar grouping for Adjusted the split so the two semantics stay distinct:
That keeps the reviewer-requested orphan promotion change while preserving the pre-existing continuation pagination behavior. Validation run locally:
|
roborev: Combined Review (
|
mjacobs
left a comment
There was a problem hiding this comment.
Re-reviewed — the shared-QueryDialect refactor is the right call: it closes the SQLite/PG parity gap at the seam rather than per-path, and splitting sidebar vs canonical relationship sets is a clean way to keep continuations attached. I confirmed the soft-deleted-parent non-promotion is deliberate and now consistent for subagent/fork across both paths.
A few residuals (inline where localized), all minor:
- One continuation-only divergence is left between the paginated and unpaginated paths — inline note on the orphan seed.
- DuckDB is the third sidebar backend and didn't get the change — inline note.
- Tiny cleanup: in
BuildCanonicalRootWhere,basealready promotes every orphan (NOT (canonicalChild AND parent_exists)is true whenever the parent is absent), so theincludeOrphansOR-branch is a no-op and the parameter is effectively dead — worth dropping, or making it actually gate, so a futurefalsecaller can't silently re-diverge from the unpaginated path. - Tests: the soft-deleted-parent assertion lives only in a
pgtest(behind the build tag, so not in the default unit run), and live-parent continuation isn't exercised through the paged path — a SQLite-layer test for both would guard exactly the cases this PR turns on.
|
Addressed the residual sidebar-root feedback in the latest push:
Focused validation passed locally: |
roborev: Combined Review (
|
fix(db): promote orphan subagent sessions to sidebar roots via IncludeOrphans flag
Summary
IncludeOrphanstoSessionFilterso sidebar reads can promote missing-parent canonical child sessions as synthetic roots instead of silently dropping them.subagent,fork, andcontinuation.subagentandforkremain hidden from normal root lists, whilecontinuationstays attached to its live parent for sidebar grouping.IncludeOrphansin all three sidebar entrypoints.Scope
Changed files:
internal/db/sessions.go- SQLite sidebar opts into orphan recovery and paginated roots compose base filtering with shared canonical-root logic.internal/db/query_dialect.go,internal/db/query_dialect_test.go- shared canonical-root predicates, dialect relationship sets, and SQL-shape coverage.internal/db/filter_test.go- SQLite orphan, continuation, and paginated sidebar regression coverage.internal/postgres/sessions.go,internal/postgres/store_test.go- PostgreSQL sidebar parity and pgtest coverage.internal/duckdb/store.go- DuckDB sidebar opts into orphan recovery and uses the populated canonical child set.No schema change. No frontend change. No
dataVersionbump required.Validation
Focused checks run locally:
Fixes #382