fix(wiki-worker): bound the session-events DB fallback#328
Conversation
The wiki summary workers fall back to a DB read whenever the local session-event cache is absent (session resumed on another machine, started before the cache feature, or TTL-evicted). That fallback was an unbounded `SELECT message, creation_date FROM sessions WHERE path LIKE '%<sid>%' ORDER BY creation_date ASC` — an unindexed full scan that materializes the ENTIRE fat `message` column. On a mega-session that is tens of MB (measured 87–105 MB) of cold S3 I/O, ~30s per trigger, and the periodic trigger re-pays it every ~50 events. The pi/codex workers have no local cache at all, so they hit this on every run. Bound it: a cheap `count(*)` probe (reads no fat column) gives the true total, then the newest `WIKI_FALLBACK_MAX_ROWS` rows are fetched with `ORDER BY creation_date DESC LIMIT N` and reversed to chronological. The summarizer only ever consumes the newest un-summarized rows anyway (`capLinesByBytes` discards the rest), so this changes nothing it feeds. The offset math is the subtle part: `prevOffset` indexes the FULL history, so a plain `slice(prevOffset)` breaks on a bounded window. New pure helper `newRowsFromWindow(window, total, prevOffset)` maps it correctly (last `total - prevOffset` rows; whole window when the session grew past it — those older-new rows are what the byte cap drops anyway). The stamped offset high-water uses the true `total`, not the window length, so the next run doesn't regress. The local-cache path (full session) keeps its exact `slice(prevOffset)`. Applied to all five workers (base, hermes, pi, cursor, codex). Option-1 (the local event cache) already shipped; this is the "don't re-scan the whole fat column on a cache miss" half.
📝 WalkthroughWalkthroughWiki workers now count session events separately, fetch only the newest 2,000 database rows, reverse them chronologically, and preserve global offsets. Shared offset utilities map persisted offsets onto bounded windows, while incomplete cache paths refetch from the database. ChangesBounded wiki event processing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WikiWorker
participant SessionsDB
participant OffsetHelper
participant SummaryProcess
WikiWorker->>SessionsDB: count matching session events
SessionsDB-->>WikiWorker: total event count
WikiWorker->>SessionsDB: fetch newest 2000 events
SessionsDB-->>WikiWorker: newest-first event window
WikiWorker->>OffsetHelper: compute rows after persisted offset
OffsetHelper-->>WikiWorker: chronological new rows
WikiWorker->>SummaryProcess: generate summary input
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 6 files changed
Generated for commit c338f2c. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/shared/wiki-offset.test.ts (1)
78-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage; consider adding a "stale total" regression case.
All existing cases check out against the implementation. One gap: none of these cases cover
total < window.length(window "ahead of" total) — the exact shape produced when a caller'scount(*)probe races with its row fetch (see thedbFetchfinding insrc/hooks/cursor/wiki-worker.ts/src/hooks/hermes/wiki-worker.ts). Locking in the expected behavior here would make the contract explicit for that scenario.✅ Suggested additional case
+ it("total is stale/behind the window (window.length > total) — does not silently drop rows", () => { + // Simulates a count(*) probe racing with the row fetch: the window already + // contains more rows than the earlier count reported. + const w = rows(115); // full session fetched at SELECT time + const out = newRowsFromWindow(w, /* stale */ 110, 100); + // Expect all rows from prevOffset onward, not just (total - prevOffset). + expect(out).toEqual(w.slice(100)); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/wiki-offset.test.ts` around lines 78 - 116, Add a regression test in the newRowsFromWindow test suite covering a stale total where total is less than the fetched window length, matching the race between count probing and row fetching in dbFetch. Assert the expected offset behavior and returned rows for that scenario, making the contract explicit without changing the implementation.tests/cursor/cursor-wiki-worker.test.ts (1)
229-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a specific-value assertion over separate substring checks.
The three checks here (
toContain("ORDER BY creation_date DESC"),toContain("LIMIT 2000"),includes("ORDER BY creation_date ASC")) are generic substring matches. As per path instructions, tests should prefer asserting on specific values over generic substrings. A single regex anchored on the trailing clause would pin down the exact expected SQL shape while still tolerating the dynamic escaped session-id prefix.♻️ Suggested tightening
- expect(sqls.some(s => s.startsWith("SELECT count(*) AS n"))).toBe(true); - const fetchSql = sqls.find(s => s.startsWith("SELECT message, creation_date"))!; - expect(fetchSql).toContain("ORDER BY creation_date DESC"); - expect(fetchSql).toContain("LIMIT 2000"); - expect(sqls.some(s => s.includes("ORDER BY creation_date ASC"))).toBe(false); + expect(sqls.some(s => s.startsWith("SELECT count(*) AS n"))).toBe(true); + const fetchSql = sqls.find(s => s.startsWith("SELECT message, creation_date"))!; + expect(fetchSql).toMatch(/ORDER BY creation_date DESC LIMIT 2000$/); + expect(sqls.some(s => s.includes("ORDER BY creation_date ASC"))).toBe(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cursor/cursor-wiki-worker.test.ts` around lines 229 - 234, In the cursor SQL assertions, replace the separate generic checks on fetchSql and the ASC absence check with one regex-based assertion anchored to the trailing ORDER BY creation_date DESC LIMIT 2000 clause. Preserve the existing count-query assertion and allow the dynamic escaped session-id prefix while enforcing the exact expected SQL shape.Source: Path instructions
tests/hermes/hermes-wiki-worker.test.ts (1)
234-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact SQL string instead of substrings.
These new assertions use
startsWith/toContain/includessubstring checks on the SQL text. As per path instructions fortests/**, prefer asserting on specific values over generic substrings.✅ Suggested tightening
- expect(sqls.some(s => s.startsWith("SELECT count(*) AS n"))).toBe(true); - const fetchSql = sqls.find(s => s.startsWith("SELECT message, creation_date"))!; - expect(fetchSql).toContain("ORDER BY creation_date DESC"); - expect(fetchSql).toContain("LIMIT 2000"); - expect(sqls.some(s => s.includes("ORDER BY creation_date ASC"))).toBe(false); + expect(sqls).toContainEqual(expect.stringContaining("SELECT count(*) AS n")); + const fetchSql = sqls.find(s => s.startsWith("SELECT message, creation_date"))!; + expect(fetchSql).toBe( + `SELECT message, creation_date FROM "${cfg.sessionsTable}" WHERE path LIKE '${expectedLikePattern}' ORDER BY creation_date DESC LIMIT 2000` + );As per path instructions, "Prefer asserting on specific values (paths, messages) over generic substrings."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hermes/hermes-wiki-worker.test.ts` around lines 234 - 240, Strengthen the SQL assertions in the hermes wiki worker test by comparing the generated count and fetch statements against their exact expected SQL strings rather than using startsWith, toContain, or includes. Keep the assertion that no ascending creation-date query is generated, but express it through exact statement values where possible.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/codex/wiki-worker.ts`:
- Around line 138-158: Update the LIKE predicates in the session-event queries
to use plain SQL string quotes around likePat instead of E-string literals.
Apply this in src/hooks/codex/wiki-worker.ts lines 138-158 and
src/hooks/pi/wiki-worker.ts lines 123-143, covering both count and fetch queries
while preserving the existing esc() result and query behavior.
In `@src/hooks/cursor/wiki-worker.ts`:
- Around line 135-145: The dbFetch implementations can undercount rows because
count(*) runs before the bounded SELECT. In src/hooks/cursor/wiki-worker.ts
lines 135-145 and src/hooks/hermes/wiki-worker.ts lines 135-145, update dbFetch
to fetch the bounded rows first, then query the count, and set total to the
maximum of the count and returned row length; apply the same correction to the
matching dbFetch patterns in src/hooks/wiki-worker.ts,
src/hooks/codex/wiki-worker.ts, and src/hooks/pi/wiki-worker.ts.
In `@src/hooks/wiki-worker.ts`:
- Around line 212-214: Prevent offset stamping from skipping backlog beyond the
bounded fetch window: in src/hooks/wiki-worker.ts:212-214,
src/hooks/codex/wiki-worker.ts:212-212, and src/hooks/pi/wiki-worker.ts:197-197,
update the flows around newRowsFromWindow so truncated windows advance only
through processed rows (prevOffset + newRows.length) and emit a wlog warning, or
loop fetching windows until the backlog is fully processed before recording the
full total.
---
Nitpick comments:
In `@tests/cursor/cursor-wiki-worker.test.ts`:
- Around line 229-234: In the cursor SQL assertions, replace the separate
generic checks on fetchSql and the ASC absence check with one regex-based
assertion anchored to the trailing ORDER BY creation_date DESC LIMIT 2000
clause. Preserve the existing count-query assertion and allow the dynamic
escaped session-id prefix while enforcing the exact expected SQL shape.
In `@tests/hermes/hermes-wiki-worker.test.ts`:
- Around line 234-240: Strengthen the SQL assertions in the hermes wiki worker
test by comparing the generated count and fetch statements against their exact
expected SQL strings rather than using startsWith, toContain, or includes. Keep
the assertion that no ascending creation-date query is generated, but express it
through exact statement values where possible.
In `@tests/shared/wiki-offset.test.ts`:
- Around line 78-116: Add a regression test in the newRowsFromWindow test suite
covering a stale total where total is less than the fetched window length,
matching the race between count probing and row fetching in dbFetch. Assert the
expected offset behavior and returned rows for that scenario, making the
contract explicit without changing the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b4d5f082-f461-4c2f-90db-c2c3dd752cb6
📒 Files selected for processing (14)
src/hooks/codex/wiki-worker.tssrc/hooks/cursor/wiki-worker.tssrc/hooks/hermes/wiki-worker.tssrc/hooks/pi/wiki-worker.tssrc/hooks/wiki-offset.tssrc/hooks/wiki-worker.tstests/claude-code/wiki-worker-local-cache.test.tstests/claude-code/wiki-worker-plugin-version.test.tstests/claude-code/wiki-worker.test.tstests/codex/codex-wiki-worker.test.tstests/cursor/cursor-wiki-worker.test.tstests/hermes/hermes-wiki-worker.test.tstests/pi/pi-wiki-worker.test.tstests/shared/wiki-offset.test.ts
| // 1. Fetch session events from the sessions table — BOUNDED to the newest | ||
| // WIKI_FALLBACK_MAX_ROWS rows (reversed to chronological) plus the true | ||
| // total. The old unbounded `ORDER BY ASC` materialized the whole fat | ||
| // `message` column (tens of MB, ~30s cold on a mega-session) even though | ||
| // only the newest un-summarized rows are consumed. `count(*)` reads no fat | ||
| // column, so `total` is cheap and keeps the offset math + stamped offset right. | ||
| wlog("fetching session events"); | ||
| const rows = await query( | ||
| `SELECT message, creation_date FROM "${cfg.sessionsTable}" ` + | ||
| `WHERE path LIKE E'${esc(`/sessions/%${cfg.sessionId}%`)}' ORDER BY creation_date ASC` | ||
| ); | ||
|
|
||
| if (rows.length === 0) { | ||
| const likePat = esc(`/sessions/%${cfg.sessionId}%`); | ||
| const cntRows = await query(`SELECT count(*) AS n FROM "${cfg.sessionsTable}" WHERE path LIKE E'${likePat}'`); | ||
| const total = Number(cntRows[0]?.["n"] ?? 0); | ||
| if (total === 0) { | ||
| wlog("no session events found — exiting"); | ||
| return; | ||
| } | ||
| const fetched = await query( | ||
| `SELECT message, creation_date FROM "${cfg.sessionsTable}" ` + | ||
| `WHERE path LIKE E'${likePat}' ORDER BY creation_date DESC LIMIT ${WIKI_FALLBACK_MAX_ROWS}` | ||
| ); | ||
| const rows = fetched.reverse(); | ||
|
|
||
| const jsonlLines = rows.length; | ||
| const jsonlLines = total; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate esc() to confirm whether it accounts for E''-literal backslash semantics.
rg -nP -A8 'function\s+esc\s*\(' --type=ts
rg -nP "LIKE E'" --type=ts -C2Repository: activeloopai/hivemind
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'src/hooks/**/wiki-worker.ts' 'src/hooks/**/utils.ts' 'src/hooks/**/shared.ts' 'src/hooks/**/index.ts' | sed -n '1,120p'
echo
echo "== outline codex/wiki-worker.ts =="
ast-grep outline src/hooks/codex/wiki-worker.ts --view expanded || true
echo
echo "== outline pi/wiki-worker.ts =="
ast-grep outline src/hooks/pi/wiki-worker.ts --view expanded || true
echo
echo "== search esc definitions/usages =="
rg -n "esc\s*\(" src/hooks -g '*.ts' -C 2 || true
echo
echo "== search LIKE E' patterns =="
rg -n "LIKE E'" src/hooks -g '*.ts' -C 2 || trueRepository: activeloopai/hivemind
Length of output: 16901
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== read relevant sections =="
for f in src/hooks/codex/wiki-worker.ts src/hooks/pi/wiki-worker.ts src/hooks/wiki-worker.ts; do
echo "--- $f ---"
wc -l "$f"
done
echo
echo "== src/hooks/codex/wiki-worker.ts around queries =="
sed -n '1,240p' src/hooks/codex/wiki-worker.ts | nl -ba | sed -n '1,220p'
echo
echo "== src/hooks/pi/wiki-worker.ts around queries =="
sed -n '1,220p' src/hooks/pi/wiki-worker.ts | nl -ba | sed -n '1,220p'
echo
echo "== src/hooks/wiki-worker.ts around queries =="
sed -n '1,220p' src/hooks/wiki-worker.ts | nl -ba | sed -n '1,220p'Repository: activeloopai/hivemind
Length of output: 486
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [
Path('src/hooks/codex/wiki-worker.ts'),
Path('src/hooks/pi/wiki-worker.ts'),
Path('src/hooks/wiki-worker.ts'),
]:
print(f'--- {path} ---')
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 'function esc' in line or 'LIKE E\'' in line or "LIKE '" in line:
start = max(1, i-3)
end = min(len(lines), i+8)
for j in range(start, end+1):
print(f'{j:4}: {lines[j-1]}')
print()
PYRepository: activeloopai/hivemind
Length of output: 7317
🌐 Web query:
PostgreSQL LIKE operator backslash default escape E'' standard_conforming_strings documentation
💡 Result:
In PostgreSQL, the backslash character () serves two distinct functions that often cause confusion: one as a literal string escape mechanism and another as an escape character specifically for the LIKE operator's pattern matching [1][2][3]. 1. String Literal Parsing and standard_conforming_strings: The configuration parameter standard_conforming_strings controls how backslashes are interpreted within ordinary SQL string literals ('...'). - When standard_conforming_strings is on (the default in modern PostgreSQL), backslashes in ordinary string literals are treated as literal backslash characters [2][4]. - When standard_conforming_strings is off, backslashes in ordinary string literals are treated as escape characters (e.g., '\n' becomes a newline) [2][4]. - To represent backslash escape sequences reliably, you should use E-prefixed string constants (e.g., E'\'), which treat backslashes as escape characters regardless of the standard_conforming_strings setting [2][5]. 2. LIKE Operator Escape Behavior: Regardless of the standard_conforming_strings setting, the LIKE operator always uses the backslash as its default escape character for pattern matching [1][3]. This means that within a LIKE pattern, a backslash is used to escape wildcard characters like '%' and '' [1][6]. - To match a literal backslash in a LIKE clause, you must escape it with another backslash (e.g., '\') [1][5]. - Because of the two-layer interpretation (string parsing + LIKE pattern matching), searching for a literal backslash often requires multiple backslashes, such as '\\' in an ordinary string literal, or '\\' in an E-string literal [3]. - You can override the default LIKE escape character using the ESCAPE clause [1][6]. For example, LIKE '%#%' ESCAPE '#' treats '#' as the escape character instead of the backslash [1][3]. To disable the LIKE escape mechanism entirely, you can use ESCAPE '' [1][7]. In summary, standard_conforming_strings governs how string constants are parsed, whereas the LIKE operator's use of the backslash as an escape character is a fixed behavior of the pattern matching engine [1][3]. Applications requiring reliable handling of backslashes should use E-prefixed string constants and explicitly specify an ESCAPE character for LIKE patterns if needed [1][2].
Citations:
- 1: https://www.postgresql.org/docs/current/functions-matching.html
- 2: https://www.postgresql.org/docs/current/sql-syntax-lexical.html
- 3: https://stackoverflow.com/questions/30294820/backslash-works-incorrectly-in-like-clause
- 4: https://www.postgresql.org/docs/current/runtime-config-compatible.html
- 5: https://www.postgresql.org/docs/19/sql-syntax-lexical.html
- 6: https://www.postgresql.org/docs/19/functions-matching.html
- 7: https://www.postgresql.org/docs/17/functions-matching.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path, ranges in {
'src/hooks/codex/wiki-worker.ts': [(50, 170)],
'src/hooks/pi/wiki-worker.ts': [(60, 155)],
'src/hooks/wiki-worker.ts': [(55, 170)],
'src/hooks/upload-summary.ts': [(52, 65)],
}.items():
print(f'--- {path} ---')
lines = Path(path).read_text().splitlines()
for start, end in ranges:
for i in range(start, min(end, len(lines)) + 1):
print(f'{i:4}: {lines[i-1]}')
print()
PYRepository: activeloopai/hivemind
Length of output: 17833
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path, start, end in [
('src/hooks/codex/wiki-worker.ts', 54, 160),
('src/hooks/pi/wiki-worker.ts', 64, 150),
('src/hooks/wiki-worker.ts', 59, 165),
]:
print(f'--- {path} ---')
lines = Path(path).read_text().splitlines()
for i in range(start, min(end, len(lines)) + 1):
print(f'{i:4}: {lines[i-1]}')
print()
PYRepository: activeloopai/hivemind
Length of output: 15321
🌐 Web query:
PostgreSQL E string literal backslash LIKE pattern default escape documentation
💡 Result:
In PostgreSQL, there are two distinct mechanisms involving backslashes: string constant escaping (lexical level) and LIKE pattern escaping (pattern matching level). LIKE Pattern Escape In a LIKE pattern, the default escape character is the backslash () [1][2]. This allows you to match literal underscore (_) or percent sign (%) characters by preceding them with the backslash (e.g., '%') [1][2]. To match a literal backslash, you write two backslashes ('\') [1][2]. You can change this default behavior by using the ESCAPE clause (e.g., LIKE 'pattern' ESCAPE '!') or disable it entirely by using ESCAPE '' [1][2]. PostgreSQL's default of using the backslash as an escape character is a nonstandard extension to the SQL standard, which specifies that there should be no default escape character [1][2]. E String Literal An E string literal (e.g., E'string') is a lexical extension that enables C-style backslash escape sequences (such as \n for newline or \xHH for hexadecimal bytes) [3][4]. Because these sequences are interpreted by the parser when the query is read, a backslash intended to be part of a LIKE pattern must be doubled if it is inside an E string literal [3][4]. For example, to match a literal percent sign using an E string literal, you would write E'\%'—the first backslash escapes the second one for the string constant, leaving a single backslash for the LIKE operator to use as an escape character [1][3]. Important Configuration Note Since PostgreSQL 9.1, the default setting for the configuration parameter standard_conforming_strings is ON [3][5]. This means that backslashes in standard (non-E) string constants are treated as literal characters rather than escape sequences [3][5]. Consequently, it is recommended to use E string literals only when backslash escape sequences are actually needed [3][5].
Citations:
- 1: https://www.postgresql.org/docs/current/functions-matching.html
- 2: https://www.postgresql.org/docs/19/functions-matching.html
- 3: https://www.postgresql.org/docs/current/sql-syntax-lexical.html
- 4: https://www.postgresql.org/docs/19/sql-syntax-lexical.html
- 5: https://www.postgresql.org/docs/17/sql-syntax-lexical.html
Use plain quotes for these LIKE patterns
esc() already returns a string for a plain SQL literal; E'...' adds an extra backslash-parse step before LIKE sees the pattern, so backslash-containing session IDs/paths can match differently here. Use the plain '${likePat}' form to match the other queries in these files.
src/hooks/codex/wiki-worker.ts#L145-L154src/hooks/pi/wiki-worker.ts#L130-L139
📍 Affects 2 files
src/hooks/codex/wiki-worker.ts#L138-L158(this comment)src/hooks/pi/wiki-worker.ts#L123-L143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/codex/wiki-worker.ts` around lines 138 - 158, Update the LIKE
predicates in the session-event queries to use plain SQL string quotes around
likePat instead of E-string literals. Apply this in
src/hooks/codex/wiki-worker.ts lines 138-158 and src/hooks/pi/wiki-worker.ts
lines 123-143, covering both count and fetch queries while preserving the
existing esc() result and query behavior.
| const dbFetch = async (): Promise<{ rows: Record<string, unknown>[]; total: number }> => { | ||
| const like = esc(`/sessions/%${cfg.sessionId}%`); | ||
| const cnt = await query(`SELECT count(*) AS n FROM "${cfg.sessionsTable}" WHERE path LIKE E'${like}'`); | ||
| const total = Number(cnt[0]?.["n"] ?? 0); | ||
| if (total === 0) return { rows: [], total: 0 }; | ||
| const r = await query( | ||
| `SELECT message, creation_date FROM "${cfg.sessionsTable}" ` + | ||
| `WHERE path LIKE E'${like}' ORDER BY creation_date DESC LIMIT ${WIKI_FALLBACK_MAX_ROWS}` | ||
| ); | ||
| return { rows: r.reverse(), total }; | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
dbFetch's count(*)-then-SELECT ordering is a TOCTOU race that can silently drop new session events.
Both workers implement dbFetch as two independent query() round trips: count(*) first, then the bounded SELECT ... ORDER BY creation_date DESC LIMIT 2000. query() itself can retry with up to ~30s of backoff on transient 401/403/429/5xx responses, so the gap between the two queries can be substantial while the session's capture hook keeps appending rows. When that happens — especially for the common case where the whole session fits in one fetch (rows.length < WIKI_FALLBACK_MAX_ROWS) — the earlier total understates the row count that actually existed by the time the SELECT ran. newRowsFromWindow(rows, dbTotal, prevOffset) then returns a window that starts strictly after prevOffset, skipping the rows added during the race, while the offset is still stamped from that same stale dbTotal — high enough to mark those skipped rows as already summarized. They are never fed to the agent and never revisited on a later run: a silent, permanent loss of session events from the wiki summary.
src/hooks/cursor/wiki-worker.ts#L135-L145: swap the order indbFetch— fetch the bounded rows first, thencount(*), and settotal = Math.max(count, r.length)sototalcan never understate the returned window.src/hooks/hermes/wiki-worker.ts#L135-L145: apply the identical reordering +Math.maxclamp to itsdbFetch.
The same count(*)-then-SELECT pattern also appears in src/hooks/wiki-worker.ts, src/hooks/codex/wiki-worker.ts, and src/hooks/pi/wiki-worker.ts per the supplied graph evidence; those weren't in this review's file set but should get the same fix.
📍 Affects 2 files
src/hooks/cursor/wiki-worker.ts#L135-L145(this comment)src/hooks/hermes/wiki-worker.ts#L135-L145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/cursor/wiki-worker.ts` around lines 135 - 145, The dbFetch
implementations can undercount rows because count(*) runs before the bounded
SELECT. In src/hooks/cursor/wiki-worker.ts lines 135-145 and
src/hooks/hermes/wiki-worker.ts lines 135-145, update dbFetch to fetch the
bounded rows first, then query the count, and set total to the maximum of the
count and returned row length; apply the same correction to the matching dbFetch
patterns in src/hooks/wiki-worker.ts, src/hooks/codex/wiki-worker.ts, and
src/hooks/pi/wiki-worker.ts.
| // The offset high-water (stamped into the summary) must be the TRUE total, not the | ||
| // bounded window length — else the next run's offset regresses and re-summarizes. | ||
| let jsonlLines = usedLocalCache ? rows.length : dbTotal; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Offset advances past unsummarized backlog when it exceeds the 2,000-row fetch window (silent, permanent history loss).
All three workers share one root cause: newRowsFromWindow(rows, total/dbTotal, prevOffset) can only return rows already present in the bounded 2,000-row window, but the code then stamps the offset to the full total/jsonlLines regardless of whether the backlog since prevOffset exceeded that window. Whenever total - prevOffset > WIKI_FALLBACK_MAX_ROWS (e.g., a session's first-ever summarization on a large pre-existing history, or a worker outage), the middle slice of events between prevOffset and (total - 2000) is never fed to the LLM, yet the persisted offset jumps straight to total, so that slice can never be recovered on a later run — silently, with no log line (unlike the analogous, explicitly-logged capLinesByBytes byte-budget drop).
src/hooks/wiki-worker.ts#L212-L214: stop stampingjsonlLines = dbTotalunconditionally; either loop the bounded fetch until the backlog is fully covered before stamping, or stamp onlyprevOffset + newRows.lengthwhen the window was truncated, plus awlogwarning mirroring the byte-budget drop message.src/hooks/codex/wiki-worker.ts#L212-L212: apply the same fix tojsonlLines/offset stamping aroundnewRowsFromWindow(rows, total, prevOffset).src/hooks/pi/wiki-worker.ts#L197-L197: apply the same fix tojsonlLines/offset stamping aroundnewRowsFromWindow(rows, total, prevOffset).
📍 Affects 3 files
src/hooks/wiki-worker.ts#L212-L214(this comment)src/hooks/codex/wiki-worker.ts#L212-L212src/hooks/pi/wiki-worker.ts#L197-L197
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/wiki-worker.ts` around lines 212 - 214, Prevent offset stamping
from skipping backlog beyond the bounded fetch window: in
src/hooks/wiki-worker.ts:212-214, src/hooks/codex/wiki-worker.ts:212-212, and
src/hooks/pi/wiki-worker.ts:197-197, update the flows around newRowsFromWindow
so truncated windows advance only through processed rows (prevOffset +
newRows.length) and emit a wlog warning, or loop fetching windows until the
backlog is fully processed before recording the full total.
Lift the four agent workers (hermes/cursor/pi/codex) over the 90% per-file coverage bar (were ~83–85% statements / ~84% branches). Adds tests for the bounded-fetch behaviors this PR introduced and the surrounding error/edge paths that were previously uncovered: - no-new-events skip (resume offset covers every row) - local-cache-shorter-than-offset → bounded DB refetch (hermes/cursor) - sidecar count preferred over a smaller parsed offset - NULL embedding on embed-daemon failure - existing-summary lookup throws → skip (no overwrite) - sidecar update failure / releaseLock throw → still release, swallow - capLinesByBytes drop + single-giant-row truncation - exec throws after a partial summary write → skip upload - fatal query error → release lock; retryable (503) → retry then succeed - unknown server path fallback (empty DISTINCT path) - codex formatExecFailure field branches (code/stderr/stdout, truncation) Changed-file coverage now 97.9% statements / 93.4% branches / 100% functions / 98.5% lines (all six files ≥90 per metric). No production code change; zero net-new test failures vs main.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cursor/cursor-wiki-worker.test.ts (1)
272-272: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the
LIMIT 2000bound in the "refetches from the bounded DB" tests. These tests exist to prove the fallback fetch is bounded, but they only assertORDER BY creation_date DESCon the newest-events query and never verify the row cap. A regression that dropsLIMIT 2000(unbounded fetch) would pass unnoticed. Asserting the specificLIMIT 2000value closes that gap.
tests/cursor/cursor-wiki-worker.test.ts#L272-L272: also assert the resolvedmessage, creation_dateSQL containsLIMIT 2000.tests/hermes/hermes-wiki-worker.test.ts#L300-L302: extend thefetchSqlassertions to includeLIMIT 2000alongside the existingORDER BY creation_date DESCcheck.As per path instructions, prefer asserting on specific values (paths, messages) over generic substrings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cursor/cursor-wiki-worker.test.ts` at line 272, Strengthen the bounded DB fallback assertions by verifying the resolved newest-events SQL includes the specific LIMIT 2000 value alongside ORDER BY creation_date DESC. Update the assertion at tests/cursor/cursor-wiki-worker.test.ts:272 and extend the fetchSql assertions at tests/hermes/hermes-wiki-worker.test.ts:300-302; no other behavior changes are needed.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/cursor/cursor-wiki-worker.test.ts`:
- Line 272: Strengthen the bounded DB fallback assertions by verifying the
resolved newest-events SQL includes the specific LIMIT 2000 value alongside
ORDER BY creation_date DESC. Update the assertion at
tests/cursor/cursor-wiki-worker.test.ts:272 and extend the fetchSql assertions
at tests/hermes/hermes-wiki-worker.test.ts:300-302; no other behavior changes
are needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 06ebb3b7-d418-417e-88b3-1c8df5efbe6a
📒 Files selected for processing (4)
tests/codex/codex-wiki-worker.test.tstests/cursor/cursor-wiki-worker.test.tstests/hermes/hermes-wiki-worker.test.tstests/pi/pi-wiki-worker.test.ts
1 participant
Problem
The wiki summary workers fall back to a DB read whenever the local session-event cache is absent (session resumed on another machine, started before the cache feature, or TTL-evicted). That fallback was:
— an unindexed full scan that materializes the entire fat
messagecolumn. On a mega-session that's 87–105 MB of cold S3 I/O, ~30s per trigger, re-paid every ~50 events by the periodic trigger. Measured on org1ad8b15e(Examen AI, plugin 0.7.13x) with a local pg-deeplake against the same S3 root: cold fetch of the 105 MBmessagecolumn ≈ 30s; warm ≈ 0.35s; compute is not the cost. The pi/codex workers have no local cache at all, so they hit this on every run.Fix
Bound the fallback: a cheap
count(*)probe (reads onlypath/creation_date, never the fat column) gives the true total, then the newestWIKI_FALLBACK_MAX_ROWS(2000) rows are fetched withORDER BY creation_date DESC LIMIT Nand reversed to chronological. The summarizer only ever consumes the newest un-summarized rows anyway (capLinesByBytesdiscards the rest), so this changes nothing it feeds.The offset math is the subtle part.
prevOffsetindexes the FULL history, so a plainslice(prevOffset)breaks on a bounded window. New pure helpernewRowsFromWindow(window, total, prevOffset)maps it correctly — the lasttotal - prevOffsetrows, or the whole window when the session grew past it (those older-new rows are exactly what the byte cap drops anyway). The stamped offset high-water uses the truetotal, not the window length, so the next run doesn't regress. The local-cache path (full session) keeps its exactslice(prevOffset).Applied to all five workers: base, hermes, pi, cursor, codex. Option 1 (the local event cache) already shipped in
82d3ae3e; this is the "don't re-scan the whole fat column on a cache miss" half.Tests
newRowsFromWindowunit suite (tests/shared/wiki-offset.test.ts) covering the full-window == plain-slice equivalence, bounded-window tail selection, session-grew-past-window, prevOffset 0, and copy-not-alias.count(*)/DESC LIMIT 2000/never ASCassertion added on the fallback.main(the 19 pre-existing failures are unrelated capture/plugin-version tests).🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Bug Fixes
Tests