Skip to content

fix(sync): bound agent-scoped reconciliation to its requested roots - #1319

Merged
wesm merged 5 commits into
kenn-io:mainfrom
rodboev:pr/1208-reconciliation-scoping
Aug 1, 2026
Merged

fix(sync): bound agent-scoped reconciliation to its requested roots#1319
wesm merged 5 commits into
kenn-io:mainfrom
rodboev:pr/1208-reconciliation-scoping

Conversation

@rodboev

@rodboev rodboev commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Part of the prep work for #1208, along with #1307 and #1318. The branch was squashed and includes #1291's changes; if #1291 merges on its own, this needs a rebase.

The bug

Asking reconciliation to check one directory could delete sessions in other directories. The requested path got widened to the whole configured root, and that widened claim was treated as proof when deciding what to tombstone.

The fix

Each provider now says exactly what a request covers:

  • Discovery may walk a wider directory, but the pass can only touch what was actually asked for.
  • Deleting anything requires proof the pass really looked there.
  • A pass that covered one whole container (a SQLite file, a Hermes archive) can clean up its members — after checking the session didn't just move somewhere else.
  • Blank or unrelated paths now do nothing instead of matching everything.

Hermes, OpenCode, Zed, Shelley, Trae, Aider, VS Copilot, and Omnigent each get topology rules for their own layouts (shared databases, archives, profile folders).

Also in here (from #1291)

Watcher performance for shared SQLite databases: one session's write used to re-parse the whole database. Now each session has its own change signal, and a file event does work proportional to what changed, not to the archive size.

Where to look

  • internal/parser/provider.go — the scope plan
  • internal/sync/engine.go — how the engine uses it
  • internal/parser/hermes_provider.go, opencode_provider.go, multi_session_container.go — per-provider rules

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (b14c50e)

Code review found two medium-severity reconciliation issues in OpenCode SQLite container handling.

Medium

  • internal/parser/provider.go:521 — OpenCode inherits the generic descendant scope, so requesting opencode.db proves only that exact path without IncludeVirtualMembers. Discovered opencode.db#<session> sources are filtered out, allowing reconciliation to succeed without syncing or tombstoning them. Override OpenCode-family scope resolution so database, WAL, and virtual-member paths share the container’s virtual-member proof, and add an exact-container reconciliation test.

  • internal/sync/engine.go:3430 — Reconciliation narrowed to one OpenCode virtual member records only that member as discovered, but finishSQLiteContainerPass can still promote the captured container state as trusted. Other changed sessions may then be skipped indefinitely during later full passes. Promote trust only when the scope enumerated the container’s complete membership, or disable promotion for narrowed scopes; add a multi-session scoped-reconciliation regression test.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 15m45s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (e717dfd)

No issues found.


Reviewers: 2 done | Synthesis: codex | Total: 15m58s

@wesm

wesm commented Jul 31, 2026

Copy link
Copy Markdown
Member

working on this

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (0838610)

Changes need revision: two medium-severity synchronization issues could leave sessions stale or incorrectly active.

Medium

  • internal/sync/opencode_container_gate.go:223 — Watermark-only sources are filtered before the pre-discovery container state is revalidated. A concurrent commit between the watermark query and filtering can remove the changed session from files; if all entries are filtered, SyncPathsContext returns before beginSQLiteContainerPass, bypassing the later capture-validity guard. Thread the pre-discovery capture into classification and filter only when a post-listing capture still matches; otherwise retain all sources for full fingerprinting. Add a regression test for mutation between watermark listing and filtering.

  • internal/parser/hermes_provider.go:871 — Plain transcript-directory Hermes roots do not resolve descendant requests. Because their reconciliation unit has an empty stateDB, deleting root/session.jsonl can produce an empty plan, leaving the session active. For non-archive, non-profile units, use generic descendant-scope behavior: traverse the configured root while validating the requested descendant using the configured path spelling. Add coverage for transcript deletion from a custom flat Hermes directory.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 13m56s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (ad7ae9e)

The change needs revision to prevent archive-sized memory allocation in the watcher fast path.

Medium

  • internal/parser/opencode_provider.go:1028, internal/sync/opencode_container_gate.go:228 — The watcher fast path materializes all session metadata as SourceRefs and all stored members as a map before filtering. Each database event therefore allocates O(total sessions), violating the bounded daemon-memory invariant. The performance test only measures post-filter processing and child lookups, so it does not catch this scaling issue. Stream or page live watermarks and stored freshness in ordered passes, emit only changed sources, and add a cardinality test covering peak retained rows or allocations.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 15m59s

@roborev-ci

roborev-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (49ad79d)

Medium-severity issues remain in reconciliation scoping and freshness pagination.

Medium

  • internal/parser/provider.go:473 — Multi-session providers using SourceSetProvider inherit the generic directory scope resolver. Reconciling a physical container path (such as Aider history, Zed/Shelley SQLite, or Omnigent chat.db) proves only that path, while discovery emits container#member paths. The engine discards those members as outside the proof and reports a successful no-op, regressing the prior widening to the configured root.

    Fix: Add container-aware reconciliation topology for multiSessionContainerSourceSet, mapping physical containers and sidecars to proof scopes with IncludeVirtualMembers: true, while retaining exact scopes for individual members. Add coverage for non-OpenCode container providers.

  • internal/sync/opencode_container_gate.go:199 — The freshness adapter filters stale-data-version rows after fetching a database page. If a full raw page contains only stale rows, it returns an empty page with done=false; storedMemberFreshnessCursor treats that as exhaustion. Later current-version members are unnecessarily emitted and materialized, causing one event’s work to scale with the remainder of the archive.

    Fix: Advance internally across raw pages using the last raw path until a vouchable row is found or the source is exhausted. Add a test where an all-stale first page is followed by covered current-version rows.


Reviewers: 2 done | Synthesis: codex, 11s | Total: 15m12s

Squash of the reconciliation-scoping branch, including its base branch
perf/bound-quick-sync-cutoff-io (kenn-io#1291) and review fixes.

- fix(opencode): bound shared-container sync to the changed session
- fix(opencode): keep single-session mtime lookup off the whole container
- fix(opencode): detect deleted children and stop stamping torn reads
- fix(opencode): carry the child digest on non-discovery source lookups
- fix(opencode): carry metadata times in the digest and skip it when unused
- fix(opencode): digest the complete ordered child identity
- fix(opencode): do not re-query legacy containers for a digest they never emit
- fix(opencode): require session_id indexes before composite freshness
- perf(opencode): bound watcher-event container listing to the session row
- perf(opencode): bound watcher materialization and idle full-pass scans
- docs(opencode): state the full deferral contract and its bounded floor
- fix(opencode): compare the watcher watermark like-for-like with stored metadata times
- fix(opencode): bound reconcile discovery and guard watermark skips by capture
- test(sync): pin steady-state Claude sync to zero source re-reads
- fix(sync): bound reconciliation authority to the roots a pass was asked about
- fix(sync): prove a scope in the spelling its traversal roots discover
- test(db): bound ownership paging on the query rather than on what follows it
- fix(sync): withhold deletion authority from a root that has no bounded proof
- fix(db): keep a separator-terminated proof scope from matching no stored child
- fix(parser): prove a reconciliation scope in the spelling its sources were stored under
- fix(parser): prove a hermes profile in its container's configured spelling
- fix(parser): resolve a hermes profile to its on-disk spelling, not the request's
- fix(parser): keep an unowned profile child out of hermes traversal
- fix(parser): resolve a hermes profile scope only for a profile the container owns
- fix(parser): widen an unowned hermes profile child to its container instead of dropping it
- fix(parser): widen an opencode container alias to its whole virtual membership
- fix(parser): keep a covered request's descendant proof under its remaining ancestor
- test(sync): stop NewProvider from writing shared test provider state
- perf(sync): walk a shared reconciliation gateway once per pass
- fix(sync): group traversal scopes without indexing an unproven slice
- fix(parser): resolve a flat hermes root's descendants generically
- fix(sync): revalidate the container capture before watermark-only filtering
- perf(sync): bound watcher fast-path memory to one page plus the changed batch
- fix(sync): advance the freshness pager past all-stale raw pages
- fix(parser): widen multi-session container requests to their virtual membership
@wesm
wesm force-pushed the pr/1208-reconciliation-scoping branch from 49ad79d to f70b0fd Compare August 1, 2026 09:45
@roborev-ci

roborev-ci Bot commented Aug 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (f70b0fd)

Medium-severity issue found in Hermes scope deduplication; no security issues were identified.

Medium

  • internal/parser/hermes_provider.go:805 — Deduplicating Hermes scopes preserves only the first scope’s authority. With overlapping configuration ordered as [profile, container], the individual profile creates a non-covering scope first; the later covering scope shares its identity but loses CoverageIdentities. Reconciliation can therefore appear incomplete and fail to reclaim removed aggregate-owned state.db sessions even though all configured roots were requested.
    • Fix: On key collisions, merge or upgrade traversal, proof, and coverage fields—not only RetryRoots—and add a reversed-order overlapping-root test.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 17m47s

…g the first

One archive can resolve through several branches — its own configured
root and a container's profile enumeration — in any order when
overlapping roots are configured and requested. The scope dedup kept
only the first arrival's fields and appended retry roots, so a
non-covering profile-branch scope arriving first silently dropped the
later covering scope's CoverageIdentities: a pass requesting every
configured root could never satisfy its required identities and
withheld aggregate-member deletion. Colliding scopes describe the same
unit, so traversal roots, proof scopes, and coverage identities now
merge on collision.
@roborev-ci

roborev-ci Bot commented Aug 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (9d1b897)

Medium-severity reconciliation issues remain; no security vulnerabilities were identified.

Medium

  • internal/sync/engine.go:4455 — Aggregate-member deletion still requires full provider-root coverage. Reconciliation scoped to one complete Hermes archive can prove that a state.db member was removed, but aggregateOwnedMemberGone exits early if another configured archive/profile is uncovered, leaving the deleted session active.

    Fix: Permit aggregate-member checks when all provider roots are covered or the current scope proves complete membership of the aggregate container. Pass this authority into aggregateOwnedMemberGone and add a targeted Hermes member-deletion test.

  • internal/sync/engine.go:5932 — Incremental cutoff filtering trusts watermark-only metadata even when post-discovery container recapture invalidates the pass. A child-only commit during discovery can retain an old session/project watermark, fall below the cutoff, and be removed before full fingerprinting detects the update.

    Fix: Use the carried watermark only while sqliteContainerPassCaptureValid is true; otherwise fall through to providerSourceMtime so the composite fingerprint and digest are resolved.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 19m57s

…th a relocation check

A pass scoped to one complete archive proves a member gone from that
container, but two guards still demanded full provider-root coverage:
aggregate-owned rows bailed in aggregateOwnedMemberGone, and virtual
rows bailed in the resolver branch, so a state.db member deleted from a
Hermes archive stayed active whenever any other configured root was
unrequested. Both paths now accept a completed scope whose proof spans
the row's whole container membership — and, since a scoped spool cannot
see other roots, they first ask the provider across its full configured
scope whether the session still resolves anywhere: a same-ID copy under
another root is a move and is preserved, a session found nowhere is
reclaimed.

Cutoff filtering had the inverse hole: it trusted a carried
watermark-only mtime even after the post-discovery recapture
invalidated the pass, so a child-only commit landing during discovery
could leave a stale session-row watermark below the cutoff and drop the
file before full fingerprinting saw the update. The carried watermark
now decides the cutoff only while the pass's container capture is live;
otherwise the live composite is resolved.
@roborev-ci

roborev-ci Bot commented Aug 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (183133c)

Code review found one medium-severity reconciliation issue.

Medium

  • internal/sync/engine.go:4797 — Scoped container reconciliation does not check whether PersistentArchive members were relocated. If a persistent-archive session moves from container A to configured container B, reconciling only A can incorrectly tombstone it even though FindSource resolves it under B. Call reconciliationMemberRelocated before setting persistentMemberContainerExists and tombstoning, and add a cross-container move regression test for a persistent-archive provider.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 16m44s

The persistent-archive branch gained scoped container-proof authority
without the relocation check the other member paths carry: a session
moved from the requested container to another configured root's
container was tombstoned even though the provider still resolves it
there. The branch now asks the provider across its full configured
scope before claiming the member, and the relocation helper withholds
deletion when no provider is available, which also removes the nil flow
NilAway flagged.

The OpenCode test fixtures now close their writer handle in cleanup;
Windows cannot delete a database file with an open handle, which failed
TempDir removal for tests that discarded the writer.
@roborev-ci

roborev-ci Bot commented Aug 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (5ba5f35)

Code changes are generally sound, but one medium-severity reconciliation bug can incorrectly tombstone relocated sessions.

Medium

  • internal/sync/engine.go:4873 — Scoped container reconciliation checks relocation only when the stale virtual path still resolves. If a non-persistent container is deleted after its member moves elsewhere—for example, a Visual Studio Copilot conversation moving to another trace—SourceForReconciliation returns not found and the code tombstones the session even though FindSource could locate it under another configured container.

    Fix: Before tombstoning any scoped, container-authorized virtual member, call reconciliationMemberRelocated regardless of whether the stale path resolves. Add coverage for moving a member while deleting its original container.


Reviewers: 2 done | Synthesis: codex, 11s | Total: 20m54s

The relocation guard ran only inside the branch paths — the persistent
branch and the resolver branch when the stale spelling still resolved —
so a virtual member whose home container was itself deleted reached the
shared missing-path tombstone with no relocation check at all, and a
scoped pass reclaimed a session that had moved to another configured
root. The guard now sits once at the shared pre-tombstone site: any
scoped pass about to tombstone a virtual-member row first asks the
provider across its full configured scope whether the session still
resolves anywhere, and the branch-local copies are removed.
@roborev-ci

roborev-ci Bot commented Aug 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (86c1cdd)

No issues found.


Reviewers: 2 done | Synthesis: codex | Total: 15m6s

@wesm
wesm merged commit deff98a into kenn-io:main Aug 1, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants