fix(opencode): bound shared-container sync to the changed session

[colinmollenhour]

Problem

Every session in an OpenCode root lives in one physical opencode.db, but each
session's SourceFingerprint carried that shared container's file size
(openCodeFormatSourceSet.Fingerprint). The freshness gate in
providerSourceUnchangedInDB requires both size and mtime to match, so any
single session's write moved the container size and thereby changed the
fingerprint of every other session in the root, dropping their freshness skip.

The result: one changed session re-parses the entire root. On a large container
that means thousands of sessions re-read out of a multi-GB SQLite database every
time the watcher fires, which on an always-on daemon with a live OpenCode
session is effectively continuous. dropUnchangedSharedSQLiteResults discards
the redundant results afterwards, so the writes stayed bounded and the problem
was invisible in correctness tests — only the reads, JSON decoding and transcript
building were wasted.

This is the same failure mode providerFingerprintHashEstablishesFreshness
already documents for Hermes ("any single-member change invalidates every
member's stat identity"). Hermes got a per-member escape hatch; the OpenCode
family never did, and it computes no fingerprint hash, so it had no fallback.

Fix

Replace the shared container's size with a per-session composite mtime:

MAX(session.time_updated,
    project.time_updated,
    MAX(message.time_updated),
    MAX(part.time_updated))

Each component is per-session (or, for project, correctly per-project — a
worktree rename genuinely re-resolves cwd for every session in that project).
Discovery, single-session source lookup, and the parse path all resolve mtime
through one helper, so a stored file_mtime is always directly comparable to
the value the freshness gate checks it against.

The container is still os.Stat-ed, but for existence only.

Impact

Synthetic container, one session changed:

Sessions in container	Before	After
20	0 skipped, 52ms	19 skipped, 33ms
400	0 skipped, 401ms	399 skipped, 115ms

Before, the cost of one changed session scaled with the number of unchanged
sessions. After, unchanged sessions skip and the pass is bounded by the change.

Measured against an isolated clone of a production OpenCode container
(13.5 GB, 5,981 sessions, 104k messages, 508k parts):

Pass	Result
Full cold reindex	5,898 sessions written, 0 failed, 1m35s
Steady-state pass	0 written, 5,981 skipped, 1.20s

The steady-state figure is the point. Before this change, any write to that
container dropped the freshness skip for all 5,981 sessions, so a pass that now
costs about a second instead re-parsed the entire container — every session read
back out of a 13.5 GB SQLite database, JSON-decoded, and rebuilt into a
transcript, only for dropUnchangedSharedSQLiteResults to discard all but the
one that actually changed. With a live OpenCode session writing continuously,
that ran on every watcher batch.

Discovery itself, including the composite, resolves all 5,981 sessions in ~0.5s.
It is cheap because OpenCode keeps each part's data in SQLite overflow pages,
so scanning (session_id, time_updated) never touches transcript bytes.

Compatibility

Containers whose schema lacks the child time_updated columns (older OpenCode,
Kilo, MiMoCode, ICodeMate) are detected by a cached PRAGMA probe and keep the
previous session-only mtime and the container-size fallback, so their
behaviour is unchanged. The legacy JSON storage-tree layout is untouched.

Because the composite differs from the previously stored value for most rows,
the first sync after upgrading re-parses the archive once to re-stamp
file_mtime, then settles into the steady-state pass above. On the 13.5 GB
container that one-time migration is the 1m35s reindex; it is self-healing and
does not repeat.

TL;DR on repeat automated-review findings

Recent review rounds keep re-raising two things that are settled design, not
defects:

1. "The watcher still scans every session row." True, and irreducible:
   OpenCode's schema has no watermark index and is not ours to alter, so any
   sound candidate selection must read the session table once — a few
   thousand small fixed-width rows, single-digit milliseconds. What this PR
   removed is the archive-scaling part: child-table scans (hundreds of
   thousands of rows) and per-event materialization, both now bounded by the
   changed batch. A test asserting "rows scanned stays constant as sessions
   grow" cannot honestly exist for this design.
2. "Child-only edits are deferred." Deliberate, documented, and
   test-pinned: a write that touches only child rows waits for the next
   periodic full pass (minutes), whose digest catches it; live-viewed
   sessions are covered by the 1.5s per-session poll. Detecting it per event
   requires reading child rows — exactly the archive-sized work earlier
   reviews required removing. The same trade cannot be simultaneously a
   required optimization and a reported bug.

Watcher passes are bounded by the changed batch

The full digest listing aggregates every message and part row of a
container, so running it on each watcher event made a one-session write do
work proportional to the archive (the 1.20s steady-state figure above).
Watcher changed-path passes now avoid it entirely: they list sessions through
a bounded session-row watermark — MAX(session.time_updated, project.time_updated), touching no child tables — compare it per session and
like-for-like against the stored session/project metadata watermark
(recovered from the persisted child digest, which already embeds those times;
loaded in one indexed range query), and drop covered sessions before they are
even materialized into discovered files. Only the changed batch flows into
the sync pipeline and resolves the full composite and child digest through
the indexed per-session lookup. Like-for-like matters: the stored composite
can be dominated by a newer child timestamp, and comparing the session-row
watermark against it would hide a metadata update stamped below that child
maximum. A session or project row that advances past its own stored metadata
watermark is always a candidate, wherever other sessions' watermarks or its
own child timestamps sit.

Periodic full passes and streamed reconciliation passes get the same
treatment when there is nothing to do: a container whose captured state
still matches the last fully verified pass is listed through the watermark
form, because the container gate then skips every member before
fingerprinting and the child identity scan would be archive-sized work
nothing reads. Any write breaks that trust and the next pass carries the
complete digest again — and watermark-only skips additionally require the
pass's container capture to survive its recapture check, so a container
that changes mid-pass resolves full per-session digests instead of skipping
on a stale premise.

The trade is explicit: a child write at or below the stored composite that
leaves the session and project rows untouched is invisible to a watcher pass
and is reconciled by the next full-discovery pass over the now-untrusted
container, whose carried digest still catches it. Cutoff passes,
reconciliation, and tombstoning keep the complete digest listing;
changed-path passes already never promote container trust, so gate promotion
soundness is unchanged; legacy containers are never watermark-skipped.
Actively watched sessions do not rely on this path at all — their poll
resolves the composite per session. Instrumentation counts the two query
shapes, and regression tests assert a watcher event runs zero
whole-container child scans with sources processed and per-session lookups
that do not grow between 20- and 200-session archives, and that idle full
passes run zero child scans.

Deletions, and a correction

A MAX over timestamps cannot see a deletion. Removing a message or part only
moves the composite when the deleted row happened to hold the maximum, and on
the production clone 5,758 of 5,981 sessions (96%) carry a session or
project timestamp at or above every child — so for almost every session a delete
would have changed nothing and the removed content would have stayed archived
indefinitely.

An earlier revision of this description claimed a revert stays detectable
because it lowers the max. That was wrong for those 96%. The fingerprint hash
now carries a per-session digest of the watermark plus the child row counts, and
freshness compares it (FingerprintHashRequiredForFreshness), so a delete
changes the fingerprint even when the watermark cannot. A regression test
deletes a child while the session row holds a higher timestamp; it fails against
the max-only signal.

The watermark is also now read before the message and part queries. Those are
separate autocommit reads, so a concurrent write landing between them previously
produced a torn transcript stamped with a watermark newer than the content read,
which every later sync would skip as fresh. Reading it first inverts the race:
the stamp is never newer than the content, so a concurrent change leaves the
stored value behind the source and the next pass re-syncs it.

Remaining gap: a write that leaves the watermark, the message count and the part
count all unchanged is not attributed to any session, which requires an in-place
edit that does not stamp time_updated. It is recorded with its supporting
evidence in docs/internal/session-format-sources.md.

Where to look

• internal/parser/opencode.go — composite expression, cached schema probe,
  the single helper that discovery/lookup/parse all resolve mtime through, and
  the bounded session-row watermark listing used by changed-path passes.
• internal/parser/opencode_provider.go — the fingerprint no longer stamps the
  shared container's size when the composite is available; watermark-only
  sources adopt the looked-up composite and digest together.
• internal/sync/opencode_container_gate.go — the batched pre-materialization
  filter for watermark-only changed-path sources, the per-file watermark skip,
  and the discovery trust probe that lets idle full passes list trusted
  containers without the child scan.
• internal/sync/test_helpers_test.go — the fixture schema previously omitted
  time_updated on project, message and part entirely, so it could not
  model shared-container freshness at all. That gap is why this regression was
  not caught. The schema now matches production and the mutation helpers
  maintain those columns the way OpenCode does.

Three existing assertions changed as a consequence, and reviewers should weigh
them: two asserted file_mtime stays constant across a rewrite, which no longer
holds now that the composite legitimately advances with the changed rows; the
third asserted a cwd-vetoed container produces zero skips, which is now one —
the persisted session skips on its own per-session freshness while the vetoed
session is still reprocessed. A trusted-container gate skip would produce two,
which is the promotion violation that test exists to catch, so the invariant it
guards is still pinned.

A cardinality-scaling regression test
(TestOpenCodeSharedContainerChangeIsPerSessionBounded) pins the invariant and
fails without the fix.

[Copilot]

Pull request overview

This pull request fixes a performance regression in the OpenCode SQLite “shared container” format by changing the per-session freshness signal so that a single session write no longer invalidates every other session in the same opencode.db. This keeps background sync work bounded by the changed batch rather than scaling with total sessions in a container.

Changes:

• Replace shared-container size-based invalidation with a per-session composite mtime derived from session/project/message/part time_updated values.
• Update OpenCode provider fingerprinting to omit container size when the composite mtime is available, while preserving legacy fallback behavior when the schema lacks required columns.
• Add/adjust integration and performance-invariant tests and fixtures to model production OpenCode schemas and pin the bounded-work invariants.

Reviewed changes

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

Show a summary per file

File	Description
internal/parser/opencode.go	Adds composite-mtime discovery/lookup helpers, cached schema probing, and stamps stored file_mtime from the same composite signal used for freshness gating.
internal/parser/opencode_provider.go	Threads composite-mtime knowledge through discovery/meta and drops container-size fingerprinting when the composite is supported.
internal/parser/opencode_provider_test.go	Updates fingerprint expectations to reflect composite-mtime behavior and size omission.
internal/sync/hash.go	Introduces an indirection seam for prefix hashing so perf invariant tests can count content reads.
internal/sync/engine.go	Routes freshness-gate prefix hashing through the indirection seam.
internal/sync/test_helpers_test.go	Makes OpenCode test fixtures mirror production schema (adds time_updated columns) and updates helpers to maintain them.
internal/sync/engine_integration_test.go	Adjusts integration assertions for composite-mtime semantics and project/worktree updates.
internal/sync/perf_invariant_test.go	Adds a new invariant test ensuring warm sync passes don’t rehash unchanged Claude archives.
internal/sync/opencode_container_perf_test.go	Adds a new cardinality-scaling regression test to pin per-session bounded work for shared OpenCode containers.
docs/internal/session-format-sources.md	Records the OpenCode SQLite change-detection evidence and known gaps in the format sources doc.

Comments suppressed due to low confidence (1)

internal/sync/opencode_container_perf_test.go:67

• This map is currently tracking stats.Synced, but it’s named unchangedSkips and the final assertion message talks about “work” growth. After renaming the map (above), update the assignment and final assertion to use the new name and describe what’s being compared.

			unchangedSkips[n] = stats.Synced
		})
	}

	assert.Equal(t, unchangedSkips[20], unchangedSkips[200],
		"work for one changed session must not grow with container size")

---

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

[roborev-ci]

roborev: Combined Review (c515e09)

Code freshness logic is sound in intent, but the OpenCode mtime lookup introduces a Medium-severity scaling regression.

Medium

• Archive-wide scans occur once per session — internal/parser/opencode.go:155, internal/parser/opencode_provider.go:882

  The single-session mtime query reuses aggregate subqueries that group every message and part in the container. Because discovery does not populate SourceRef.DiscoveryMTimeNS, parse-diff triggers this archive-wide scan per session, resulting in O(sessions × child rows) work. Single-session syncs also scan the entire archive.

  Suggested fix: Use session-filtered or correlated aggregates for single-session lookups, populate DiscoveryMTimeNS from meta.FileMtime, and update the scaling test to measure query/source work rather than only sync counters.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 8m3s

[roborev-ci]

roborev: Combined Review (101c877)

The PR needs changes: two medium-severity correctness issues could leave archived OpenCode sessions permanently stale.

Medium

• internal/parser/opencode.go:494 — Deletions can be missed by the composite fingerprint. The composite uses only maximum timestamps. Deleting a message or part is invisible when session.time_updated, project.time_updated, or another child retains the same higher maximum. Since container size is omitted, the session may be considered fresh and deleted content can remain archived indefinitely. Include deletion-sensitive state such as child counts or revisions in the fingerprint, or avoid the pre-parse skip after container changes. Add a test where the session timestamp exceeds the deleted child’s timestamp.

• internal/parser/opencode.go:799 — Concurrent updates can permanently stamp stale content as current. Messages and parts are read through separate autocommit queries, followed by the composite-watermark query. An update between these reads can produce stale parsed content stamped with the newer watermark, causing later syncs to skip it. Read all inputs and the composite within one read transaction, or capture the watermark before reading inputs so a concurrent change forces another sync.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 8m4s

[roborev-ci]

roborev: Combined Review (4ceaf0d)

Medium findings remain in OpenCode SQLite freshness detection and watcher-path scaling.

Medium

• internal/parser/opencode_provider.go:732 — Reconstructed SQLite sources, including normal reconciliation sources, do not carry ChildDigest. The fallback lookup returns only the watermark and composite flag, leaving fingerprint.Hash empty; deletion-only changes can therefore pass freshness checks. Return the child digest from the single-session lookup and use it when discovery metadata lacks one. Add a deletion test through ReconcileWatchRoots.

• internal/parser/opencode.go:189 — The digest uses only (watermark, message count, part count). Replacing one child with another while preserving counts and the existing maximum timestamp produces the same fingerprint, potentially leaving stale archived content. Include a stable aggregate of child identities and update timestamps, and test a same-count replacement below an already-higher session timestamp.

• internal/parser/opencode.go:529 — Every database/WAL event still scans both complete child tables and materializes every session through sqliteSources. The performance test compares only stats.Synced, so it does not verify that watcher-triggered work remains bounded by the changed batch. Avoid archive-wide child aggregation on watcher paths and instrument the regression test to compare sessions and child rows examined across small and large archives.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 5m34s

[colinmollenhour]

Apologies for the noise, I hope it's ok to continue fixing in this manner... I'm very impressed with Roborev which I see is one of your projects - very nice!

[colinmollenhour]

AI Review Response · Commit: 1839ab8 · By: Claude Code with Opus 5

All three findings verified as real. Two are fixed in 1839ab8; the third I am
confirming rather than fixing, with numbers, because I do not think it can be
closed without a design decision.

1. Reconstructed sources carry no ChildDigest — confirmed, fixed.
Correct. Fingerprint read the digest only from discovery metadata, so any
source rebuilt by FindSource or reconciliation produced an empty hash, and an
empty hash is treated as no constraint by the gate. The single-session lookup now
returns the digest and Fingerprint uses it whenever the source did not carry
one. Added TestOpenCodeDeletedChildDetectedViaReconciliation, driven through
ReconcileWatchRoots as you suggested; it fails against the previous commit.

2. Same-count replacement produces the same digest — confirmed, fixed.
Correct. (watermark, message count, part count) is invariant under a
replacement that preserves counts and keeps new timestamps below an
already-higher session watermark. The digest now also folds in the child time
sums and the child id range: the sums catch an edit whose timestamp stays below
the watermark, the id range catches a same-count replacement reusing old
timestamps. Added
TestOpenCodeSameCountChildReplacementIsDetected, which also fails against the
previous commit. Everything read still lives in the child tables' main b-tree
pages, so the transcript text in overflow pages is never touched.

Cost on the production clone (13.5 GB, 5,981 sessions, 508k parts): cold reindex
1m15s → 1m20s, steady-state pass 641ms → 918ms.

3. Archive-wide child aggregation per watcher event — confirmed, not fixed.
Also correct, and worth stating plainly: discovery aggregates both child tables
on every pass where the container changed, which is 918ms on this container
regardless of how few sessions changed. That is genuinely O(child rows) per
event rather than bounded by the changed batch.

I have not fixed it because I do not think it can be, cheaply: a write to a
shared SQLite container does not say which session it touched, so identifying
the changed subset requires consulting the child tables. The mitigation already
present is the container change-counter gate, which skips discovery entirely
when the container has not changed — so this cost is paid per container change,
not per watcher tick. The realistic options are (a) cache per-session digests
keyed on the container change counter and recompute only on change, which helps
bursty writers but not a continuously-active session, or (b) accept it. Prior to
this PR the same event re-parsed every session out of the container, so this is
roughly three orders of magnitude better but not asymptotically bounded.

On the test instrumentation point: the scaling test does assert
stats.Skipped == n-1 alongside Synced, so per-session parse work is
pinned. You are right that it does not measure child rows examined, which is
exactly the dimension finding 3 is about — I did not add that instrumentation
because it would assert a bound the implementation does not currently hold.

Happy to take direction on (3) if maintainers want it closed in this PR rather
than tracked separately.

[roborev-ci]

roborev: Combined Review (1839ab8)

Medium-severity issues remain in OpenCode freshness detection and polling performance.

Medium

• internal/parser/opencode.go:212 — Digest can miss metadata updates below the current watermark. The digest includes only the maximum session/project/child timestamp. If a session or project timestamp changes but remains below an already-higher watermark, both the mtime and digest remain unchanged, potentially skipping metadata updates such as a worktree rename. Include session.time_updated and project.time_updated as separate digest fields, with coverage for updates below the existing watermark.

• internal/parser/opencode.go:167 — Mtime-only polling performs unnecessary aggregate scans. openCodeSessionCompositeMtime always runs the deletion-digest COUNT/SUM/MIN/MAX subqueries, even when callers such as buildOpenCodeSession and openCodeSQLiteSessionMtime discard the digest. Because session watching polls the latter every 1.5 seconds, this causes eight unnecessary child-range scans per poll. Add a watermark-only query for mtime-only callers and reserve the full aggregate query for fingerprint generation.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 8m26s

[colinmollenhour]

AI Review Response · Commit: 585d9d3 · By: Claude Code with Opus 5

Both findings verified and fixed in 585d9d3.

1. Digest misses metadata updates below the current watermark — confirmed,
fixed. Correct. Session and project timestamps only entered the digest through
the composite MAX, so an update that landed below an already-higher child
watermark moved neither the mtime nor the digest. They are now separate digest
fields. Added TestOpenCodeMetadataUpdateBelowWatermarkIsDetected, which
raises a child above the session/project timestamps and then renames the project
underneath it; it fails against the previous commit.

Worth noting for the record that this is narrower in practice than the other
findings: a real rename stamps time_updated at write time, which normally
exceeds the existing watermark, so reaching it needs a child timestamp ahead of
wall clock. The fix is essentially free though — those two columns were already
being selected — so there was no reason to leave the hole open on that argument.

2. Mtime-only polling runs the digest aggregates — confirmed, fixed.
Correct, and the call path is worse than it first looks:
openCodeSQLiteSessionMtime backs OpenCodeSourceMtime, which the session
watcher polls every 1.5s per watched session, so an open session view was
running eight child-range subqueries per tick and discarding every one of them.
buildOpenCodeSession did the same on the parse path.

Added openCodeSessionWatermark, a watermark-only query, and routed both
mtime-only callers at it; the full aggregate is now reserved for fingerprint
generation. TestOpenCodeWatermarkOnlyQuerySkipsDigestScans pins that the
mtime-only query contains no COUNT/SUM while the fingerprint query still
does.

Measured on the production clone (13.5 GB, 5,981 sessions, 508k parts): the
steady-state pass over every session drops from 918ms to 715ms, and the cold
reindex is unchanged at ~1m20s. The polling win does not show up in that number
at all — it is per watched session per tick, so it scales with open session
views rather than archive size.

Both suites pass, go vet and go fmt clean.

[roborev-ci]

roborev: Combined Review (585d9d3)

Medium

• internal/parser/opencode.go:109 — Each opencode.db/WAL event aggregates every row in message and part to compute per-session fingerprints, making a one-session write trigger archive-sized reads. The performance test only measures rewritten sessions, so it misses this regression. Use a changed-candidate path for watcher events, reserve full aggregation for periodic reconciliation, and add a small-vs-large test measuring rows examined or equivalent source-read work.

• internal/parser/opencode.go:596 — The child digest is not collision-resistant for the complete child identity set. Replacing a non-boundary row with another row having the same timestamp preserves counts, timestamp sums, and min/max IDs, so the session can remain incorrectly classified as fresh. Digest every ordered (id, time_updated) pair or an equivalent complete representation, and test replacement of a middle row while preserving timestamps and extrema.

---

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

[colinmollenhour]

AI Review Response · Commit: c74c07d · By: Claude Code with Opus 5

Pushed three commits (108a8c26, 000a31ec, c74c07db). The first answers
the review of 585d9d3; the other two come from a local review pass run before
pushing. One finding is deliberately left open and called out at the end.

Digest collisions on a middle-row replacement — fixed (108a8c26).
Confirmed: swapping a non-boundary row for one carrying the same timestamp
preserves counts, timestamp sums and min/max ids alike, so the digest was
identical and the session stayed classified as fresh. The digest is now a hash
over every ordered (id, time_updated) pair.
TestOpenCodeMiddleRowReplacementIsDetected reproduces the collision and fails
against the reduced aggregates. Cost on the production clone: cold reindex
1m20s → 1m35s, steady-state pass 715ms → 1.197s.

Legacy containers re-queried for a digest they never emit — fixed
(000a31ec). A container without composite support carries an empty digest by
design, but the fingerprint treated empty as missing and reopened the shared
database once per session — precisely the archive-scaled work this branch
exists to remove. The condition is now
mtime == 0 || (composite && digest == "").

Missing indexes made an existing test vacuous — fixed (c74c07db). Worth
spelling out, because it invalidates an earlier claim in this PR. Both test
fixtures declared message and part with no indexes at all, while production
OpenCode declares three. The query-plan test added earlier to prove the
single-session lookup reaches the child tables through an index was therefore
running against a schema where an indexed plan was impossible — it could not
have caught what it claimed to. The fixtures now declare the production indexes,
the assertion requires an explicit USING INDEX/USING COVERING INDEX instead
of merely rejecting SCAN (SQLite reports SEARCH for some unindexed
aggregate plans), and composite support now probes for a leftmost session_id
index before enabling the correlated lookups, so an unindexed container falls
back to the session-only mtime rather than putting a full child-table scan on
the session watcher's 1.5s poll.

Still open: archive-wide child aggregation per watcher event. Discovery
aggregates every child row on each pass where the container changed, so a
one-session write does work proportional to the archive rather than the change,
and the group_concat identity adds a memory cost the earlier reduced
aggregates did not have. I have not fixed it. A write to a shared SQLite
container does not say which session it touched, so narrowing the candidate set
means consulting the child tables; hashing incrementally from a streamed ordered
scan would remove the memory growth but not the scan. A real fix needs a
changed-candidate path backed by either an index on time_updated — which is
OpenCode's schema, not ours — or persisted per-session state to diff against.
That is a design decision rather than a patch, so I would rather surface it than
guess, and I have not added a test asserting a bound the implementation does not
hold.

For scale, the container gate still short-circuits discovery entirely when the
container has not changed, so this cost is paid per container change, not per
watcher tick; prior to this PR the same event re-parsed every session out of the
container. Measured steady-state pass over all 5,981 sessions is 1.197s.

[roborev-ci]

roborev: Combined Review (c74c07d)

Changes improve SQLite freshness detection, but introduce an archive-scaling regression that should be fixed before merge.

Medium

• internal/parser/opencode.go:587 — Every database change performs discovery ordering, aggregation, and group_concat across all message and part rows before checking individual session freshness. Updating one session therefore does work proportional to the entire archive and may create large temporary identity sets. The performance test only checks rewritten-session counts, so it misses this regression.

  Limit digest computation to candidate sessions or reconcile the archive in bounded batches. Add instrumentation verifying that examined child rows remain bounded as the unchanged archive grows.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 5m39s

[colinmollenhour]

AI Review Response · Commit: c74c07d · By: Claude Code with Opus 5

Confirmed, and I am not going to fix it in this PR without a maintainer
decision. This is the fourth consecutive review to raise it and the first to
mark it blocking, so it deserves a straight answer rather than another
acknowledgement.

The finding is correct. Discovery orders and aggregates every message and
part row of a container on each pass where that container changed, and builds
group_concat identity strings for all sessions before any per-session
freshness check. A one-session write therefore does archive-proportional work
and materializes a large temporary identity set. On a 13.5 GB / 5,981-session
container that is 1.20s per container change. The scaling test does not catch
it: it pins rewritten and skipped session counts, which stayed bounded
throughout, and says nothing about rows examined.

Why it is still open. A write to a shared SQLite container does not say
which session it touched, so producing a candidate set means consulting the
child tables. The two obvious filters both fail:
session.time_updated is cheap but misses child-only edits, which is precisely
the case this PR added the child digest for; per-session on-demand digests are
index seeks (~5ms each) but cost far more than one archive scan once most
sessions are checked — about 30s across 5,981 sessions versus 1.20s today.

On the two suggestions. Bounded batches are implementable: cap per-pass
digest work and rotate coverage so per-event work has a hard ceiling, with full
coverage amortized over several passes. That trades detection latency for the
bound — a deletion in an un-batched session stays archived until its turn
comes round. That is a product decision about staleness, not a refactor, which
is why I would rather have it agreed than assume it. Limiting to candidate
sessions I do not currently see a sound way to do without an index on
time_updated, which is OpenCode's schema and not ours to add.

On the instrumentation. I have deliberately not added a test asserting that
examined child rows stay bounded as the archive grows, because the
implementation does not hold that bound — the test would have to be written to
pass. I would rather ship the gap visible than a green assertion that hides it.

For context on the trade already banked: before this PR the same event re-parsed
every session out of the container. The measured numbers are 13m52s → 1m35s for
a cold reindex and, for a pass where one session changed, 0 sessions skipped →
5,980 skipped. The remaining cost is a bounded-constant scan of index pages
rather than a full re-parse, but it is still O(archive) and I am not claiming
otherwise.

Happy to implement bounded batching if that trade is acceptable, or to leave
this for a follow-up if you would rather land the correctness work first.

[colinmollenhour]

AI Review Response · Commit: c7e006f · By: Claude Code with Fable 5

Following up on my previous comment: the author opted to fix this rather than
defer it, and c7e006f8 implements the "limit digest computation to candidate
sessions" variant the review suggested, plus the requested instrumentation.

What changed. Changed-path (watcher) classification no longer runs the
grouped whole-container child scan. It lists sessions through a bounded
session-row watermark — MAX(session.time_updated, project.time_updated),
touching no message/part rows — and marks those sources watermark-only.
The engine skips a watermark-only source when its stored composite watermark
already covers the carried value; only sessions whose watermark advances
resolve the full composite and child digest through the existing indexed
per-session lookup (session_id-index seeks). Per-event work is now
O(session-table) + O(changed batch): the session table is ~6k tiny rows on the
production container where the child tables are ~612k rows, so the 1.2s
per-event scan becomes a few milliseconds plus ~5ms per actually-changed
session.

Why this is sound. The stored MTimeNS is the composite
MAX(session, project, children) from the last parse, so a session-row
watermark at or below it proves the session and project rows did not advance.
What the watermark cannot see — a child write at or below the stored composite
that leaves the session row untouched — is deliberately deferred to the next
full-discovery pass (the periodic sync), whose carried digest still catches
it. Full-sync discovery, cutoff passes, reconciliation, and tombstoning keep
the complete digest listing; changed-path passes already never promote
container trust, so the gate's promotion soundness is unchanged, and legacy
containers without composite support are never watermark-skipped. Actively
watched sessions never relied on this path; their 1.5s poll resolves the
composite per session directly.

Instrumentation. parser.OpenCodeContainerChildScans /
OpenCodeSessionChildLookups count the two query shapes, and
TestOpenCodeWatcherEventIsWatermarkBounded asserts a changed-path pass runs
zero whole-container child scans with per-session lookups that do not grow
between 20- and 200-session archives — the bound now holds, so the assertion
is honest. TestOpenCodeWatcherPassDefersChildOnlyEditToFullDiscovery pins
the staleness contract explicitly, and
TestOpenCodeFullPassSkipsAfterWatcherPassParse pins that watcher-pass parses
store the true composite (Fingerprint adopts the looked-up composite alongside
the digest), so the next full pass still skips everything unchanged.

[colinmollenhour]

AI Review Response · Commit: c7e006f · By: Claude Code with Fable 5 · Summary

Fixed

1. roborev (Medium, review of c74c07d) — archive-proportional discovery
   on every database change. Previously answered as "needs a maintainer
   decision"; the author opted to implement the bounded design, and
   c7e006f8 delivers it. Watcher changed-path passes now list sessions
   through a bounded session-row watermark (no child-table access) and skip
   sessions whose stored composite already covers it; only the changed batch
   pays the indexed per-session digest lookup. Full-discovery passes keep the
   complete child digest and reconcile what the watermark cannot see (child
   writes at or below the stored composite), so correctness properties from
   earlier rounds — deletion sensitivity, ordered child identity, metadata
   below the watermark — are all preserved with a detection latency bounded
   by the periodic full sync. The requested instrumentation exists and is now
   honestly assertable: zero whole-container child scans per watcher event,
   and per-session lookups that do not grow between 20- and 200-session
   archives.

Previously resolved (unchanged this round): both Copilot inline threads
(fixed/rebutted, resolved) and all roborev findings from c515e09 through
585d9d3.

PR state: no open review items remain. go fmt, go vet, the full
internal/parser and internal/sync suites pass locally with the new
regression tests included.

[roborev-ci]

roborev: Combined Review (c7e006f)

Changes improve OpenCode SQLite change detection, but three medium-severity performance and freshness issues remain.

Medium

• internal/sync/opencode_container_gate.go:308 — The watermark-only freshness check ignores child-only changes. If a child timestamp advances while the session/project watermark remains unchanged, the update is delayed until periodic discovery. Include a child-change signal or per-session child delta, with a watcher regression test covering this case.

• internal/parser/opencode_provider.go:937 — Each SQLite watcher event still lists and materializes every session, leaving total work and allocations proportional to archive size. Query only candidate session IDs affected since the previous container state, and test the total number of sources processed.

• internal/parser/opencode.go:226 — Periodic discovery aggregates every child identity before checking whether the container is unchanged, causing archive-sized scans and large string allocations even for trusted, untouched containers. Use lightweight discovery while captured state matches trusted state, resolving the full digest only when that check fails.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 9m12s

[colinmollenhour]

AI Review Response · Commit: 8abd803 · By: Claude Code with Fable 5

Three findings on c7e006f: one is the documented design contract and I am
rebutting it; the other two are real and fixed in 8abd803f.

1. opencode_container_gate.go — watermark check ignores child-only
changes: rebuttal. This is the deliberate, documented trade at the center of
the design, not an oversight — bounding per-event work requires some change
class to defer, and this is the one chosen, with its reconcile path built in.
The watcher regression test this finding requests already exists:
TestOpenCodeWatcherPassDefersChildOnlyEditToFullDiscovery pins exactly this
case, including the full-pass reconcile. The suggested fix — "include a
child-change signal or per-session child delta" — is precisely the
archive-sized child aggregation the previous review (of c74c07d) required
removing from this path; a per-event child signal cannot be computed without
reading child rows, which is the cost being bounded. Detection latency for
this class is capped by the periodic full pass, actively watched sessions
bypass it entirely via the 1.5s per-session composite poll, and on the
production container 96% of sessions carry a session/project timestamp at or
above every child. The contract is recorded in
docs/internal/session-format-sources.md.

2. opencode_provider.go — every session still listed and materialized per
event: confirmed, fixed. The child tables were no longer read, but every
session still became a SourceRef and a DiscoveredFile, and each skip cost
two point queries against the archive. Changed-path classification now loads
the container members' stored composite watermarks and data versions in one
indexed range query (ListVirtualContainerMemberFreshness, riding
idx_sessions_file_path) and drops covered watermark-only sources before
they are materialized into discovered files. The comparison is per-session —
a watermark that advances past its own stored composite is always kept,
wherever other sessions' watermarks sit — which an existing regression test
(TestOpenCodeSQLiteRootSyncPathsAndStaleReparse) enforces against
interleaved timestamps; a container-wide max cutoff was tried first and that
test correctly rejected it. Sessions with no stored row or a stale data
version are kept unconditionally, and the filter fails open to the per-file
gate on query errors. The perf test now asserts, per the review's request,
that sources processed per event do not grow between 20- and 200-session
archives. The one remaining O(session-count) term is the session-table scan
itself plus the range query over stored members — small fixed-width rows with
no child access, unavoidable without an index OpenCode's schema does not
have, and roughly three orders of magnitude lighter than the child scan this
PR removed.

3. opencode.go — full discovery aggregates child identity for trusted,
untouched containers: confirmed, fixed. Exactly the suggested shape:
discovery now receives a trust probe
(ProviderConfig.SQLiteContainerUnchangedSinceTrust, keyed to the pass's
pre-discovery state captures) and lists a trusted container through the
bounded watermark form — the container gate then skips every member before
fingerprinting, so the digest was archive-sized work nothing read. Any write
changes the container state and breaks trust, so the next full pass carries
the complete child digest again; that keeps the deferral contract from
point 1 intact, because a trusted container by definition has no pending
deferred edits. A container that changes between capture and listing is
failed for the pass by the existing gate bookkeeping and re-verified by
content next pass. Reconciliation, tombstoning, and all other provider
constructions keep full-fidelity listings.
TestOpenCodeIdleFullPassSkipsContainerChildScan pins zero child scans and
zero per-session lookups on an idle pass, and that a subsequent child-only
edit below every watermark still reconciles through the digest.

[colinmollenhour]

AI Review Response · Commit: 8abd803 · By: Claude Code with Fable 5 · Summary

Review of c7e006f (three Medium findings): two fixed in 8abd803f, one
rebutted.

Fixed

1. Watcher events materialized every session — changed-path
   classification now compares the watermark-only listing against stored
   member freshness loaded in one indexed range query and drops covered
   sessions before they become discovered files; per-event pipeline work is
   bounded by the changed batch, and the perf test asserts sources processed
   do not grow with container size.
2. Idle full passes aggregated child identity for trusted containers —
   discovery now lists containers whose captured state matches the last
   fully verified pass through the bounded watermark form; any write breaks
   trust and restores the full digest listing, keeping the deferred-edit
   reconcile contract intact. New test pins zero child scans on idle passes.

Rebutted

3. Watermark check ignores child-only changes — this is the documented
   design contract, not a defect: the requested "child-change signal" per
   event is the archive-sized aggregation the previous review required
   removing, and the requested watcher regression test already exists
   (TestOpenCodeWatcherPassDefersChildOnlyEditToFullDiscovery). Bounded by
   the periodic full pass; watched sessions bypass it via the 1.5s composite
   poll.

PR state: no other open review items. go fmt, go vet, and the full
internal/parser and internal/sync suites pass locally, including the
pre-existing TestOpenCodeSQLiteRootSyncPathsAndStaleReparse, which
correctly rejected a first-cut container-wide cutoff and drove the
per-session comparison that shipped.

[roborev-ci]

roborev: Combined Review (8abd803)

Medium-severity issues remain in OpenCode watcher performance and child-update detection.

Medium

• internal/parser/opencode.go:118, internal/db/sessions.go:2182 — Each watcher event still scans and materializes every session twice before filtering, leaving per-event CPU and allocation work O(total sessions). The performance test only measures downstream lookups and processed files, so it does not detect this scaling. Select and stream only candidate session IDs before constructing SourceRef values, and add a regression counter comparing rows scanned/materialized for small and large containers.

• internal/sync/opencode_container_gate.go:207 — The watermark filter can discard child-only updates whenever session/project timestamps remain unchanged. For example, a child timestamp advancing beyond the stored composite may never reach fingerprinting, leaving the session stale until a full pass. Preserve candidates when child state may have advanced or add a bounded child-change signal, plus a watcher test covering this case.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 8m41s

[colinmollenhour]

AI Review Response · Commit: dee21db · By: Claude Code with Fable 5

Both findings on 8abd803 restate positions already litigated in this thread;
one of them nonetheless caught an inaccuracy in my documentation, which
dee21db6 corrects. Nothing in the design changes.

1. "Scans and materializes every session twice per event": rebutted. This
is the residual O(session-count) floor explicitly acknowledged and defended
in the previous response, now re-raised without engaging that defense. The
floor is one scan of the session table (a few thousand small fixed-width
rows, no child access) plus one indexed range query over the stored members —
per-session candidate selection requires per-session state on both sides, and
OpenCode's schema indexes neither time_updated column, so no sound query
can avoid reading the session rows. The suggested "select only candidate
session IDs" was implemented in its only SQL-expressible form (a
container-wide cutoff) during the 8abd803 work and was rejected by an
existing regression test (TestOpenCodeSQLiteRootSyncPathsAndStaleReparse):
a container-level cutoff misses a session whose watermark advances past its
own stored composite while staying below a sibling's. The requested
regression counter — rows scanned bounded as the container grows — is an
assertion this design cannot satisfy, for the same reason, and writing a test
that must be written to pass is a pattern already refused twice on this PR.
What is asserted, because it actually holds: zero child-table access per
event, and sources materialized into the pipeline bounded by the changed
batch across 20- vs 200-session archives. These positions are now recorded
at the code sites themselves (ForEachOpenCodeSessionWatermarkMeta,
ListVirtualContainerMemberFreshness) and in the PR description's TL;DR.

2. "Watermark filter can discard child-only updates": the trade is the
documented contract and stands, but the finding's example exposed a wording
error, fixed. The deferral itself was rebutted in the previous response
and that rebuttal stands unchanged: a per-event child signal requires
reading child rows, which is the archive-sized work the review of c74c07d
required removing — the same cost cannot be both a mandated optimization and
a reported defect. However, the example ("a child timestamp advancing beyond
the stored composite") is correct and my comments and doc entry drew the
miss class too narrowly as "at or below the stored composite". In truth any
child-only write that leaves the session and project rows untouched is
invisible to the session-row watermark, above the stored composite as well
as below it. dee21db6 corrects the wording everywhere it appears and adds
the requested watcher test variant:
TestOpenCodeWatcherPassDefersChildOnlyEditToFullDiscovery now pins both
the below-composite replacement and an above-composite append — each
deferred by the watcher pass with zero child-table access, each reconciled
by the next full-discovery pass (the write breaks container trust, so that
pass carries the complete digest). Detection latency stays bounded by the
periodic sync, and actively watched sessions bypass this path via the 1.5s
per-session composite poll.

[colinmollenhour]

AI Review Response · Commit: dee21db · By: Claude Code with Fable 5 · Summary

Review of 8abd803 (two Medium findings): both are restatements of
previously litigated positions; no design change. One caught a documentation
inaccuracy, corrected in dee21db6.

Rebutted

1. Per-event O(session-count) scan — the acknowledged, irreducible floor:
   one session-table scan plus one indexed range query, both over small
   fixed-width rows with zero child access. The suggested candidate-only SQL
   was tried and rejected by an existing regression test (container-wide
   cutoffs are unsound), and the requested bounded-rows-scanned counter is an
   assertion the design cannot honestly satisfy without a schema index that
   is OpenCode's, not ours.
2. Child-only deferral — the documented contract; a per-event child
   signal is the archive-sized aggregation the review of c74c07d required
   removing.

Fixed (wording + coverage)

• The finding's above-the-composite example was right that my prose drew the
  deferral class too narrowly. Comments, doc entry, and the PR description
  now state it exactly (any child-only write, wherever its timestamps land),
  and the deferral test pins both variants, including the requested
  above-composite watcher case.

For maintainers: the two rebutted items are now recorded at the code
sites and in a TL;DR section of the PR description. Further automated
re-raises of these two points can be answered by that section; anything
beyond them will be triaged on its merits as usual.

[roborev-ci]

roborev: Combined Review (dee21db)

The change is generally sound, but one medium-severity synchronization bug can delay valid OpenCode metadata updates.

Medium

• internal/sync/opencode_container_gate.go:220 (also fallback at line 427): The watcher compares the session/project watermark with a stored composite watermark that may be dominated by a newer child timestamp. If metadata advances but remains below that composite value, valid changes such as title, directory, or project worktree updates are ignored until a full reconciliation.

  Suggested fix: Persist or recover the prior session/project-only watermark and compare like-for-like. Add a watcher-path test where child timestamps exceed both the previous and newly advanced metadata timestamps.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 4m46s

[colinmollenhour]

AI Review Response · Commit: ede3001 · By: Claude Code with Fable 5

Confirmed and fixed in ede3001d — unlike the two previous rounds, this
finding is genuinely new and correct, and it exposed an overclaim in my own
comments.

The defect. The watermark gates compared the live session-row watermark
against the stored composite MTimeNS, which is MAX(session, project, child
times). On a session whose composite is dominated by a child timestamp (7% of
sessions on the measured production container end in that state), a later
metadata update — title, session, or project change stamped below that child
maximum — advanced the session row without advancing past the composite, so
the watcher skipped a change class the watermark path exists to catch. My
gate comment claimed the comparison "proves the session and project rows did
not advance"; that was false for child-dominated composites.

The fix is the suggested one, and it needed no schema change. The
"prior session/project-only watermark" the review asks to persist was already
persisted: the child digest embeds sessionTime and projectTime in their
own right (they were folded in earlier on this PR precisely so the full
pass could detect below-watermark metadata updates).
OpenCodeChildDigestMetadataWatermarkNS parses them back out of the stored
fingerprint hash — rejecting any hash shape this digest version did not
write — and both gates (the batched pre-materialization filter and the
per-file fallback the finding also cites) now compare like-for-like against
that recovered metadata watermark. Rows without a parseable digest
(pre-digest fingerprints) fall back to the composite, the conservative old
behavior, and self-heal on their next reparse. The digest layout is now
load-bearing for this parse, so its comment requires a prefix version bump
on any field change.

Tests. The requested watcher-path case is covered end to end:
TestOpenCodeWatcherCatchesMetadataUpdateUnderChildDominatedComposite sets
child timestamps above both the previous and the newly advanced metadata
times, and asserts the watcher pass re-parses the session (with zero
container child-table access) and archives the update — it fails against the
previous comparison. TestOpenCodeChildDigestMetadataWatermarkNS pins the
digest round-trip and the rejection of foreign or malformed hash shapes.
The child-only deferral contract is unchanged and its tests still pass:
writes touching neither the session nor project row still defer to the full
pass; what changed is that session/project-row advances are now always
candidates regardless of where their own child timestamps sit.

[colinmollenhour]

AI Review Response · Commit: ede3001 · By: Claude Code with Fable 5 · Summary

Review of dee21db6 (one Medium finding): genuinely new, confirmed, and
fixed in ede3001d.

Fixed

1. Metadata updates hidden under a child-dominated composite — the
   watermark gates compared the live session-row watermark against the
   stored composite, which a newer child timestamp can dominate; a metadata
   update stamped below that maximum was wrongly deferred to the full pass.
   Both gates now compare like-for-like against the stored session/project
   metadata watermark, recovered from the persisted child digest (which
   already embedded those times — no schema change), with the composite as
   the conservative fallback for pre-digest rows. The requested watcher test
   (child timestamps above both old and new metadata times) is added and
   fails against the previous comparison; a parser test pins the digest
   round-trip and rejection of foreign hash shapes.

Unchanged: the child-only deferral contract and the bounded per-event
floor (see the PR description's TL;DR); all their tests still pass.

PR state: no open review items. go fmt, go vet, and the full
internal/parser and internal/sync suites pass locally.

[roborev-ci]

roborev: Combined Review (ede3001)

Two medium-severity correctness and performance issues remain in OpenCode SQLite reconciliation.

Medium

• internal/parser/opencode.go:239 — ForEachOpenCodeSessionMeta performs archive-wide child aggregation unconditionally for composite-capable databases. Streamed reconciliation calls it through DiscoverEach, but the trusted-container watermark optimization applies only to materialized Discover. Idle ReconcileWatchRoots and ReconcileProviderRoots passes therefore still scan every message and part, making background work scale with archive size.

  Suggested fix: Pass the captured trusted-container predicate into streamed reconciliation, add a watermark-only streaming lister for trusted containers, and test that an idle trusted pass performs zero container-child scans.

• internal/sync/opencode_container_gate.go:432 — watermarkOnlySQLiteSourceFresh can miss concurrent child-only updates. A trusted full discovery may select watermark-only sources, after which beginSQLiteContainerPass detects a container-state mismatch and invalidates trust; however, this helper can still skip the source because its metadata watermark is unchanged.

  Suggested fix: Allow the skip only while the current container pass retains a valid, successful capture, or distinguish full-pass watermark sources from watcher sources and resolve the full digest after a recapture mismatch.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 8m4s

[colinmollenhour]

AI Review Response · Commit: ddc9894 · By: Claude Code with Fable 5

Both findings on ede3001 are real and both are fixed in ddc9894b, each by
the review's suggested shape.

1. Streamed reconciliation missed the trusted-container optimization:
confirmed, fixed. Correct on all points — the trust probe was wired into
materialized Discover only, and streamReconciliationCandidates built its
providers without it, so an idle ReconcileWatchRoots /
ReconcileProviderRoots pass still aggregated every child row during
DiscoverEach. Reconciliation already captures container states before
streaming, so the exact same predicate full discovery uses now rides
ProviderConfig into the reconcile providers, and discoverRootEach streams
the bounded watermark listing for trusted containers (per-agent
streamSQLiteWatermark, covering the Kilo/MiMoCode/ICodeMate relabeled
forms). Trusted candidates all gate-skip before fingerprinting, so the pass
touches no child rows at all. The requested test exists:
TestOpenCodeIdleReconcilePassSkipsContainerChildScan pins zero container
child scans and zero per-session child lookups on an idle trusted reconcile,
and that a child-only edit below every watermark breaks trust and reconciles
with full digest fidelity on the next pass.

2. Watermark skip could outlive an invalidated capture: confirmed, fixed
with the first suggested option. The race is exactly as described: a
trusted full discovery lists watermark-only sources, the container changes
before beginSQLiteContainerPass recaptures, the capture is invalidated —
and watermarkOnlySQLiteSourceFresh could still skip on an unchanged
metadata watermark, hiding a concurrent child-only write for one extra pass.
The skip now requires the current pass to hold a live, unfailed capture for
the source's container (sqliteContainerPassCaptureValid: captured before
discovery, recapture matched, not poisoned by a processing failure); sources
processed under an invalidated capture fall through to Fingerprint and
resolve the full per-session digest. The cost of that fallback is paid only
on the rare mid-pass race, is bounded per session by the indexed lookup, and
buys back full-fidelity freshness for exactly the pass that observed the
race. The mid-stream variant of the same race on the reconcile path resolves
identically: the recapture check runs after streaming completes, so a
container that changed mid-stream fails it and its candidates resolve full
fingerprints.

[colinmollenhour]

AI Review Response · Commit: ddc9894 · By: Claude Code with Fable 5 · Summary

Review of ede3001 (two Medium findings): both genuine, both fixed in
ddc9894b following the suggested fixes.

Fixed

1. Idle reconcile passes still scanned child tables — the trusted
   predicate now rides into streamed reconciliation's providers, which list
   trusted containers through the bounded watermark form; new test pins zero
   child scans and zero per-session lookups on an idle trusted reconcile,
   plus full-digest reconciliation after a child-only edit breaks trust.
2. Watermark skip could outlive an invalidated capture — watermark-only
   skips now require the pass to hold a live, unfailed container capture
   (sqliteContainerPassCaptureValid); sources processed after a mid-pass
   container change resolve full per-session digests instead.

PR state: no open review items. go fmt, go vet, and the full
internal/parser and internal/sync suites pass locally. The two
previously rebutted items (irreducible session-table scan; child-only
deferral contract) remain as documented in the PR description's TL;DR.

[roborev-ci]

roborev: Combined Review (ddc9894)

The synchronization optimization looks sound, but one medium-severity test query is broken.

Medium

• internal/parser/opencode_provider_test.go:1163 — The single-session test query uses openCodeCompositeCountsExpr, which references undefined m and p aliases and fails with no such column: m.n. Use openCodeSessionCompositeCountsExpr to match the production single-session query.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 5m16s

[colinmollenhour]

AI Review Response · Commit: ddc9894 · By: Claude Code with Fable 5

Rebutted — the finding is factually incorrect on all three of its claims.

1. internal/parser/opencode_provider_test.go:1163 uses
   openCodeSessionCompositeCountsExpr — the exact constant the finding
   recommends. The grouped openCodeCompositeCountsExpr (the one whose m
   and p aliases require openCodeCompositeMtimeJoins) appears nowhere in
   this test file; the two similarly named constants appear to have been
   confused.
2. The query cannot be silently broken: the test executes it against a real
   SQLite container and scans the result under require.NoError, by design
   ("Both must be executable, not merely string-shaped"). A
   no such column: m.n error would fail the test.
3. TestOpenCodeWatermarkOnlyQuerySkipsDigestScans passes, verified with
   -count=1 at ddc9894b, as does the full internal/parser suite.

No change made.

[colinmollenhour]

AI Review Response · Commit: ddc9894 · By: Claude Code with Fable 5 · Summary

Review of ddc9894 (one Medium finding): rebutted as factually incorrect —
the cited test already uses openCodeSessionCompositeCountsExpr, the
constant the finding recommends; the grouped variant it claims is referenced
appears nowhere in the file; and the test executes the query against a real
container under require.NoError and passes. No change made.

The review otherwise assesses the synchronization work as sound, and no
other findings remain. PR state: everything raised across all review
rounds is either fixed on the branch (through ddc9894b) or documented as a
deliberate trade in the PR description's TL;DR — the branch is ready for
maintainer review.

[roborev-ci]

roborev: Combined Review (5b64288)

The OpenCode freshness changes have three medium-severity correctness and scalability issues; no security issues were found.

Medium

• Unbounded watcher-event work — internal/parser/opencode_provider.go:984
  Every SQLite watcher event materializes all session metadata and all SourceRefs before unchanged members are filtered, so allocation and classification still scale with archive size. The performance test measures only the later source-processing stage.
  Fix: Stream watermark rows through the freshness comparison, accumulate only changed candidates, and add an allocation/cardinality regression covering classification.

• Active watcher misses child deletions and replacements — internal/parser/opencode.go:318
  The active-session watcher checks only the composite MAX watermark and ignores the deletion-sensitive digest. Deleting a child, or replacing a child below an existing higher timestamp, can leave the watermark unchanged and prevent fallback synchronization.
  Fix: Use a deletion-sensitive per-session source version, such as the full fingerprint/digest, and test child deletion during active watching.

• Performance test queries reference undefined aliases — internal/parser/opencode_provider_test.go:1159
TestOpenCodeWatermarkOnlyQuerySkipsDigestScans uses container-wide expressions referencing m and p, but its joins define only pr, causing both queries to fail with missing-column errors.
  Fix: Pair openCodeSessionCompositeMtimeExpr and openCodeSessionCompositeCountsExpr with openCodeSessionCompositeMtimeJoins.

---

Reviewers: 2 done | Synthesis: codex, 12s | Total: 6m23s

[colinmollenhour]

I think this can be closed now that #1319 was merged. Thanks @wesm and @rodboev !

The branch was squashed and includes #1291 changes