Fix Gemini thinking blocks, resync reliability, and startup UX

[wesm]

Summary

Gemini thinking blocks

• Two-pass thinking regex: Split THINKING_RE into THINKING_MARKED_RE (for [/Thinking]-delimited blocks, matched first) and THINKING_LEGACY_RE (fallback). Prevents fallback delimiters from truncating marked blocks.
• Thinking end markers: All parsers (Claude, Gemini, OpenCode) emit [/Thinking] end markers.
• Gemini thinking format: Parts joined with \n\n, ordered chronologically (thinking, content, tools).
• Merge consecutive thinking blocks: Multiple Gemini thoughts collapse into a single collapsible block.
• Thinking-only filter fix: Messages with thinking + response text are no longer hidden by the showThinking toggle.

Session count consistency

• Aligned counts across views: Session list, analytics summary, and status bar all use the same root-session filter (message_count > 0 AND relationship_type NOT IN ('subagent', 'fork')).
• Full numbers in dashboard: Removed K/M abbreviation, always shows full number with comma separators.
• Default date range: Analytics defaults to all-time instead of 1 year.
• Go HTTP integration test: TestSessionCountConsistency seeds mixed session types and asserts all three endpoints agree.
• E2e Playwright test: session-count-consistency.spec.ts verifies the same invariant through the full UI stack, asserting exact expected count.

Resync reliability

• Fresh database build: ResyncAll builds a new DB from scratch and swaps atomically, instead of in-place mutation.
• Atomic DB connections: atomic.Pointer[sql.DB] for reader/writer so HTTP handlers don't race with connection swaps.
• Windows-safe swap: Close connections before rename to avoid mandatory file locking failures.
• Empty-discovery guard: Abort resync when file discovery returns zero but old DB has file-backed sessions. Uses FileBackedSessionCount (excludes OpenCode) so OpenCode-only datasets aren't blocked, while mixed-source data loss is prevented.
• Insights preservation: Abort swap when CopyInsightsFrom fails instead of silently dropping insights.
• Failure tracking: Abort when failures exceed successful syncs.
• FTS rebuild: Drop and rebuild FTS index during resync.
• Sync serialization: SyncPaths and SyncSingleSession serialized with syncMu.

E2e test isolation

• All agent directories (Claude, Codex, Copilot, Gemini, OpenCode) point to empty dirs in e2e-server.sh, preventing real session data from leaking into tests.
• Test fixture seeds subagent, fork, and empty sessions alongside root sessions.

Other

• Session ID for all agents: Gemini, OpenCode, and Copilot sessions show the copyable session ID.
• Clean startup logging: log.Printf goes to ~/.agentsview/debug.log only. Missing directory warnings stay on stderr.
• Debug log management: Truncate debug.log on startup when >10MB.
• Windows test fix: Close log file before TempDir cleanup to avoid mandatory file locking.

Fixes #56

🤖 Generated with Claude Code

[roborev-ci]

roborev: Combined Review (db5588c)

Verdict: All reviewers agree the code is clean.

No medium, high, or critical severity issues were found by any
of the reviewing agents.

---

Synthesized from 4 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (eaf7d9d)

Review Summary: The ResyncAll optimization contains logic flaws in its change detection mechanism that risk leaving stale data in the
database and failing on non-ASCII text.

High

Lossy equality check can miss real message changes
Files: internal/sync/engine.go:954-967, internal/db/messages.go:325-342
Description:
replaceIfChanged() treats a session as unchanged when only the message count and total content length match. This is a highly collision-prone heuristic. If a session file is modified such that the text changes but the total character count remains exactly the same (e.g., replacing "hte" with "the"), or if there
are metadata-only parser changes, the engine will mistakenly assume the content is unchanged. This causes ResyncAll to silently skip updating the database, resulting in stale data persisting.
Suggested Remediation: Replace the length-based comparison with a robust change detection method. Use a stronger fingerprint (e.g.,
a fast hash like SHA-256 or xxHash of the concatenated message contents/fields) or perform a direct string equality check before deciding to skip the update.

Medium

Length-unit mismatch causes false "changed" detection for multi-byte characters
Files: internal/db/ messages.go:333, internal/sync/engine.go:954-963
Description: The database query uses SQLite's LENGTH(content) function, which returns the number of characters for TEXT. However, the Go code compares this against len(string ), which returns the number of bytes. For any session containing multi-byte UTF-8 characters (like emojis or non-English text), these length values will always mismatch even if the content is identical. This defeats the optimization entirely, silently falling back to a full FTS5 delete+reinsert.
Suggested
Remediation: Compare like-for-like units. Cast the column to a BLOB in SQLite to correctly count bytes: COALESCE(SUM(LENGTH(CAST(content AS BLOB))), 0).

---

Synthesized from 4 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (affb2ae)

Summary Verdict: The code changes look generally solid, but there is
one high-severity issue regarding potential data loss during partial failures in the new force-replace sync path.

High

Non-atomic force-replace path can lose session messages on partial failures

• Files: [engine.go](/home/roborev/.roborev/clones/
  wesm/agentsview/internal/sync/engine.go#L910), messages.go
• Description: Resync All with forceReplace=true bulk-deletes messages for the batch, commits that delete, then writes sessions/messages one-by-one. If UpsertSession or InsertMessages fails for any session, old rows are already gone and the code only logs/continues. This can
  leave sessions empty/partial, and unchanged files may then be skipped on later syncs.
• Suggested fix: Keep per-session delete+insert atomic (e.g., use ReplaceSessionMessages per session while FTS triggers are dropped), or perform delete+reinsert in one transaction per session and
  retry/report failures.

---

Synthesized from 4 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (6c715b5)

Verdict: The proposed changes introduce performance improvements and bug fixes, but there are a few significant issues with the new bulk deletion and synchronization logic that need addressing.

High

• Bulk delete before per-session writes can cause message loss on partial failures
  • File:
    internal/sync/engine.go#L917, [internal/sync/engine.go#L944](/home/roborev
    /.roborev/clones/wesm/agentsview/internal/sync/engine.go#L944)
  • Description: writeBatch(..., forceReplace=true) deletes messages for the full batch first, then writes sessions one-by-one. If Upsert Session fails (or downstream message write fails), the old messages are already gone and that session is left empty/stale.
  • Suggested Remediation: Make replacement atomic per session (delete+insert in one tx, e.g. via ReplaceSessionMessages), or abort the batch on first write error
    and surface it in SyncStats.Warnings.

Medium

• Failed bulk delete is ignored, so “replace” semantics can silently fail
  • File: [internal/sync/engine.go#L931](/home/roborev/.roborev/cl
    ones/wesm/agentsview/internal/sync/engine.go#L931)
  • Description: If DeleteMessagesForSessions fails, the code only logs and continues. The subsequent incremental path may append nothing (unchanged ordinals), leaving old parsed content in place while
    resync appears successful.
  • Suggested Remediation: Treat bulk-delete failure as a hard error for the batch, or fallback to per-session transactional replace and record warning/error in returned stats.

---

Synthesized from 4 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (fd75c9a)

The PR introduces structural improvements for database resyncs and logging, but contains high-severity concurrency and file-locking issues during the database swap phase that must be addressed.

High Severity

1. Data Corruption
& File Locking Risk from Open SQLite Database During Swap

• File: internal/sync/engine.go (Inside ResyncAll, lines 401-402)
• Description: The origDB database connections are not closed before calling os.Rename(tempPath, origPath) and forcibly deleting the original database's -wal and -shm files. This causes two major issues:

  1. Deleting WAL/SHM files while a SQLite database is open can lead to undefined behavior and irreversible corruption of the newly swapped database when SQLite attempts to checkpoint.

  2. On Windows, attempting to rename or overwrite an open file fails with a sharing violation/access denied error, causing ResyncAll to consistently fail.

• Suggested Remediation: Call origDB.Close() completely before the file rename and WAL deletion operations. Rely solely on
  os.Rename for the main .db file, and ensure origDB.Reopen() is called to re-establish connections regardless of whether the rename succeeds or fails.

2. Concurrent Handle Swap Race Condition

• File: internal/db/db.go:341

• Description: Reopen() mutates db.reader and db.writer and closes old handles while many read paths access db.reader without the same lock. This creates a race window that can surface intermittent database is closed or stale-handle behavior during a resync.

• Suggested Remediation: Make connection-handle access synchronized for both reads and writes (e.g., a shared RW lock around handle access/lifecycle), or swap to a new immutable DB instance at a higher layer and switch pointers under a lock that all DB consumers honor.

Medium Severity

1. Resync Swap
-Failure Path Drops Stats Update

• File: internal/sync/engine.go:311
• Description: In ResyncAll, if os.Rename(tempPath, origPath) fails, the function returns early after appending a warning but does not persist that into
  e.lastSyncStats. UI/state consumers can miss the failure context and see stale stats.
• Suggested Remediation: Persist lastSyncStats on all exit paths (e.g., using a common defer or helper).

2. Unsafe Connection Reuse with ATTACH
DATABASE

• File: internal/db/insights.go (Inside CopyInsightsFrom)
• Description: Executing ATTACH DATABASE and the subsequent INSERT using db.writer.Exec relies on database/sql reusing the same underlying connection. While Max OpenConns(1) makes this likely, it is not strictly guaranteed by connection pool semantics, which could cause the query to fail if a connection reconnects between statements.
• Suggested Remediation: Use conn, err := db.writer.Conn(context.Background()) to run connection-scoped
  statements (like ATTACH and the INSERT) on a guaranteed single connection, and defer conn.Close().

---

Synthesized from 4 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (9e61860)

Verdict: Changes requested to address a high-severity data loss risk during resync, and medium-severity issues with concurrency and SQL filtering.

High

Resync can still drop insights when copy
fails

• File: internal/sync/engine.go:390
• Description: In ResyncAll, newDB.Copy InsightsFrom(origPath) failure only appends a warning, then the swap continues. That replaces the original DB with a new DB that may have no insights, causing data loss.
• Suggested remediation: Abort swap on insight-copy failure (close newDB, reopen origDB, return warning
  ), or make insight copy transactional/mandatory before rename. Add an integration test for copy-failure behavior.

Medium

Data race on e.db reassignment

• File: internal/sync/engine.go (ResyncAll and FindSourceFile)

---

Description:** e.db is reassigned (e.db = newDB) during ResyncAll without holding a lock. Concurrent calls to FindSourceFile(), which reads e.db without a mutex, will trigger a Go data race.

• Suggested remediation: Protect
  e.db reads and writes using the existing e.mu (RLock/Lock), or convert e.db to an atomic.Pointer[db.DB].

NOT IN filter excludes NULL relationship types

• Files: internal/db
  /analytics.go:96, [internal/db/stats.go:19](/home/roborev/.roborev/clones/wes
  m/agentsview/internal/db/stats.go:19)
• Description: relationship_type NOT IN ('subagent', 'fork') evaluates to unknown for NULL, so legacy rows with NULL are excluded unintentionally. This can undercount sessions in stats/analytics.

Suggested remediation: Use COALESCE(relationship_type, '') NOT IN ('subagent', 'fork') (or relationship_type IS NULL OR ...). Add tests covering rows with NULL relationship_type.

---

Synthesized from 4 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (2ca8d30)

Summary Verdict: The PR introduces solid improvements to database swapping and sync logic, but requires fixes for a high-severity logic flaw in the sync failure guard, alongside medium-severity issues related to file TOCTOU, SQL NULL filtering, and database connection pooling.

High Severity

• Failure guard can be bypassed by "successful" files that produced no sessions
  • Files: [internal/sync/engine.go:745](/home/roborev/.roborev/clones/wesm/agentsview/internal/sync/engine.go#L
    745), internal/sync/engine.go:372, [internal/sync/progress.go:35](/home/rob
    orev/.roborev/clones/wesm/agentsview/internal/sync/progress.go#L35)
  • Details: filesOK is documented as “files that produced at least one session”, but it is incremented for every non-error file before checking whether that file
    produced any sessions. ResyncAll then uses Failed > filesOK to decide abort/swap. This can under-trigger aborts and permit swapping to a degraded DB when many files parse to zero sessions (e.g., non-interactive files).
  • Remediation: Increment
    filesOK only when len(r.results) > 0 (or equivalent “produced session” signal), and add a resync abort test with zero-session files plus real failures.

Medium Severity

• TOCTOU Vulnerability in Log Truncation
  *
  File: cmd/agentsview/main.go:252

  • Details: truncateLogFile performs os .Lstat(path) and then later os.Truncate(path, 0) on the same path. This is a classic Time-of-Check to Time-of-Use (TOCTOU) pattern: an attacker who can modify the path between checks could swap in a symlink and cause truncation of an
    unintended file.
  • Remediation: Avoid path-based check-then-use. Open the file once with no-follow semantics (O_NOFOLLOW where supported), fstat the opened FD, verify regular file, then ftruncate that same FD.

• Potential NULL-filter regression in root session counting (legacy DB compatibility)

  • Files: internal/db/stats.go:19,
    internal/db/analytics.go:96
  • Details: The new filter uses relationship_type NOT IN ('subagent', 'fork'). If legacy rows contain NULL in relationship_type, SQL NOT IN returns unknown and excludes those rows. That can skew stats/analytics counts and weaken the empty-discovery safety check (via FileBackedSessionCount).
  • Remediation: Use COALESCE (relationship_type, '') NOT IN ('subagent', 'fork') (or enforce/backfill non-NULL via migration), and add a test with NULL relationship_type rows.

• Connection Pool Issue with ATTACH DATABASE

  • File: internal/ db/insights.go:146 (in CopyInsightsFrom)
  • Details: w.Exec("ATTACH DATABASE ...") and the subsequent w.Exec("INSERT ...") rely on the *sql.DB connection pool. Even with MaxOpenCon ns=1, database/sql does not strictly guarantee the exact same underlying connection is reused across separate Exec calls (e.g., if an idle connection is closed and a new one is dialed). If the second Exec runs on a newly dialed connection, it will lack the attached database and
    the insert will fail.
  • Remediation: Use conn, err := w.Conn(context.Background()) to explicitly acquire a single connection, and execute the ATTACH, INSERT, and DETACH statements on conn.

---

Synthesized from 4 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (a3d5c0b)

Review Summary:
The changes introduce no security vulnerabilities, but contain medium-severity bugs related to missing UI warnings for database resyncs and regex parsing edge cases for legacy blocks.

Medium Severity

• Resync warnings are generated by backend but not surfaced in UI

  • Files: sync.ts: 14, [ResyncModal.svelte:117](/home/roborev/.roborev/clones
    /wesm/agentsview/frontend/src/lib/components/modals/ResyncModal.svelte:117), [progress.go:41](/home/roborev/.roborev/clones/wesm/agentsview/internal/sync/progress.go:
    41), engine.go:380
  • Description: The backend now emits warnings in SyncStats (
    e.g., aborted swap / copy failure), but the frontend SyncStats type and modal rendering only show synced/failed. Users can miss important failure reasons.
  • Suggested Fix: Add warnings?: string[] to frontend sync types, persist through store state, and render warnings in the
    resync done/error view.

• Legacy thinking regex can truncate old thinking blocks at first blank line

  • Files: [content-parser.ts:23](/home/roborev/.roborev/clones/wesm/agentsview/frontend/src
    /lib/utils/content-parser.ts:23), export.go:581
  • Description: The legacy
    pattern now ends on \n\n ((?=\n\[|\n\n|$)), so unmarked historical thinking blocks containing paragraph breaks may be cut early and misclassified as visible response text.
  • Suggested Fix: Tighten fallback boundary logic (for example \n\n(? =\[) or a more explicit two-pass heuristic) and add tests for legacy unmarked thinking with internal blank lines.

---

Synthesized from 4 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (8e1e3ef)

Verdict: The code is generally secure and robust, but there are two medium-severity issues regarding legacy thinking block parsing and database reopen error handling during resync.

Medium: Legacy thinking parsing truncates at first blank line

Files:

• /home/roborev/.rob orev/clones/wesm/agentsview/frontend/src/lib/utils/content-parser.ts:23
• /home/roborev/.roborev/clones/wesm/agentsview/frontend/src/lib/components/content/MessageList. svelte:44
• /home/roborev/.roborev/clones/wesm/agentsview/internal/server/export.go:581

Description:
THINKING_LEGACY_RE now uses (?=\n\[|\n\ n|$), so legacy blocks without [/Thinking] stop at any blank line. Multi-paragraph legacy thinking becomes split into thinking + normal text, which changes rendering and “hide thinking” behavior.

Suggested Fix:
For legacy fallback, terminate on explicit next block markers (or next [Tool| Read|.../[Thinking]) rather than any blank line; keep the stricter behavior only for marked blocks. Add regression tests for multi-paragraph legacy thinking.

Medium: Resync can report completion even if DB reopen fails after swap

File:

• /home/roborev/.rob orev/clones/wesm/agentsview/internal/sync/engine.go:500

Description:
After a successful rename, origDB.Reopen() failure is only appended to warnings and execution returns normal stats. At that point, connections were previously closed, so read
/write endpoints can fail until recovery/restart, leaving the app in a broken state with closed DB handles.

Suggested Fix:
Treat reopen failure as a hard resync failure path (set failed status and return error response semantics), optionally retry reopen, and ensure the UI surfaces this as fatal rather than “done
”.

---

Synthesized from 4 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (0c2f4a7)

Summary Verdict: The codebase is generally secure and structurally sound, but there are a few medium-severity issues related to database
connection handling during resync and legacy parsing logic that need to be addressed.

Medium

Resync can return “success with warning” while DB remains unavailable

• File: internal/sync/engine.go (ResyncAll, post-rename origDB.Reopen() path
  )
• Problem: If origDB.Reopen() fails after the swap, the code only appends a warning and continues. That can leave closed DB handles in service while the sync response still looks mostly successful.
• Suggested fix: Treat reopen failure as a hard failure for resync (early return with
  a failed status), and retry/recover before returning (or explicitly put the engine into a degraded/fatal state).

Legacy thinking parsing now truncates multi-paragraph legacy blocks

• Files:
  • frontend/src/lib/utils/content-parser.ts (THINKING_ LEGACY_RE)
  • frontend/src/lib/components/content/MessageList.svelte (thinking-strip regex)
  • internal/server/export.go (thinkingLegacyRe)
• Problem: The legacy regex now ends on \n\n,
  so old [Thinking] content with internal blank lines gets split, and trailing thought text can be misclassified/rendered as normal response text.
• Suggested fix: Keep the legacy fallback from stopping on generic blank lines, or add a stricter delimiter heuristic (e.g., explicit marker first, otherwise boundary only
  at known block starts), then keep the new behavior for marked [/Thinking] blocks.

---

Synthesized from 4 reviews (agents: codex, gemini | types: default, security)

[wesm]

"Finding 1 (Medium - Reopen failure): Not concerning. If Reopen() fails after a successful rename, the new DB
file is already in place. The next startup or resync will reopen it. Making this a "hard failure" doesn't help
— the swap already succeeded, and there's nothing to roll back to. The warning is the correct behavior.

Finding 2 (Medium - Legacy thinking truncation): This is by design. The two-pass regex was the entire point of
this branch — THINKING_MARKED_RE handles [/Thinking]-delimited blocks correctly, and THINKING_LEGACY_RE is a
conservative fallback that intentionally stops at \n\n to avoid consuming response text. Old blocks with
internal blank lines would need a resync to get [/Thinking] end markers (already noted in the PR). The
alternative (greedy legacy regex) was the original bug that swallowed response text."