Skip to content

feat(web,hub): add opt-in search for session message content - #1598

Open
techotaku39 wants to merge 31 commits into
tiann:mainfrom
techotaku39:feat/session-content-search
Open

feat(web,hub): add opt-in search for session message content#1598
techotaku39 wants to merge 31 commits into
tiann:mainfrom
techotaku39:feat/session-content-search

Conversation

@techotaku39

@techotaku39 techotaku39 commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add an explicit Default / Content scope switch to session search.
  • Add SQLite FTS5-backed search for visible user and assistant message content.
  • Support opening a matching session at the target message, including very old messages.
  • Add persistent match highlighting and previous/next navigation for multiple matches.
  • Add bounded historical message loading to avoid fetching an entire large session.
  • Add migration, API, type, locale, unit, component, and E2E coverage.

Problem / Motivation

Session search previously searched session metadata only. Users could not find a session from a phrase remembered from the conversation, and opening a content-search result did not reliably navigate to the matching historical message.

Content search is intentionally opt-in so the existing metadata search remains the default, predictable, and lightweight behavior.

Implementation

Search scope

  • Keep metadata search as the default.
  • Preserve searching by session title, path, agent, machine name, session ID, and existing metadata fields.
  • Add a compact scope control with Default and Content options.
  • Only issue a content-search request after the user explicitly selects Content.
  • Apply the active-only, date, unread, and machine sidebar filters to the candidate session scope before the Hub applies the global result limit.
  • Carry scoped session IDs in a POST body and bind them as one JSON value inside SQLite, so scopes larger than 500 sessions remain supported without exceeding SQLite variable limits.
  • Enforce a defensive serialized scope-size bound before the Hub executes the scoped query.
  • Enforce a request-body limit before parsing scoped content-search JSON and return 413 for oversized requests.
  • Show a representative matching snippet in the session list.
  • Preserve existing session visibility, namespace, and access boundaries.

Content indexing and APIs

  • Add a SQLite FTS5 trigram index for searchable message content.
  • Add an ordinary two-character n-gram index for short content queries, avoiding corpus-scale LIKE scans.
  • Gate content searches at two characters; one-character queries show an explanatory message instead of ranking the entire corpus.
  • Index only visible user and assistant text.
  • Exclude system messages, tool payloads, hidden reasoning, queued messages, and sidechain assistant prose that has no standalone rendered timeline anchor.
  • Normalize Antigravity planner text with the same cleanup rules used by the renderer, excluding echoed task results and task-log-only markers.
  • Maintain the derived indexes when messages are inserted, imported, copied, merged, truncated, or deleted.
  • Keep canonical message writes and derived-index writes atomic.
  • Keep normal session deletion and bulk cleanup-script deletion atomic with derived-index cleanup.
  • Track the renderer's stream identity in the derived lookup and retain the latest raw message ID as the navigation target, so coalesced stream snapshots count as one visible match and stale/empty snapshots remove obsolete text.
  • Add a schema migration from v25 to v26 that creates and backfills the short-query index for existing databases, including the target-message lookup column needed for stream coalescing.
  • Add global and session-scoped content-search endpoints, with a POST request for scoped searches.
  • Return raw message IDs and snippets so the UI can navigate to the exact match.

Historical message navigation

  • Add a bounded message-context endpoint around a target message.
  • Load at most the target message plus a limited number of surrounding messages instead of fetching the entire session.
  • Coordinate the message window, router, and rendered message anchors so repeated jumps to ancient messages remain reliable.
  • Keep the target alive until the context is loaded and rendered.
  • Dismiss the route target and release the history lock when context loading reaches a terminal failure.
  • Reconcile the bounded window with the real tail when the user explicitly returns to the latest messages.
  • Preserve every raw HAPI message ID when streaming snapshots are coalesced, so a search hit for a later snapshot still resolves to the rendered card.

Multiple matches and UI

  • Keep the target match highlighted persistently.
  • Split highlights across Markdown text nodes when a match crosses elements such as <strong>.
  • Add previous/next controls with wraparound navigation.
  • Show the current match position and total match count.
  • Add a locating state while historical context is being loaded.
  • Use application theme variables for the navigation bar, status indicator, and highlight styles, including dark mode.
  • Add English and Chinese translations.
  • Show a minimum-query hint for one-character content queries.

Migration / Performance

  • Bump the database schema from version 25 to 26.
  • The default metadata search path does not query message content or scan the message table.
  • Content searches use FTS5 for normal queries and an indexed two-character table for short queries.
  • The one-character guard prevents common single-character terms from ranking most of the corpus on the synchronous Hub SQLite path.
  • Scoped content-search IDs are normalized, filtered to visible sessions, serialized once, and bounded to 256 KiB before the Hub query runs.
  • Historical navigation loads only a bounded context window rather than the complete conversation.
  • Existing messages are indexed during migration, while future message changes update the indexes incrementally.

Validation

  • bun typecheck
  • bun run --cwd shared test — 269 tests passed
  • bun run --cwd hub test -- src/store/messageContentSearch.test.ts src/store/migration-v23.test.ts src/web/routes/sessions.test.ts scripts/cleanup-sessions.test.ts — 88 tests passed
  • bun run test:web — 260 test files and 2508 tests passed
  • pwsh -NoProfile -File .\scripts\Invoke-HapiTaskPlaywright.ps1 -Name session-content-search -Suite Root -TestArgs terminal-wrap-fidelity.spec.ts — 2 passed
  • pwsh -NoProfile -File .\scripts\Invoke-HapiTaskPlaywright.ps1 -Name session-content-search -Suite Live -TestArgs session-content-search-jump.spec.ts — 5 passed
  • bun run build
  • git diff --check

Related Issues

Fixes #1554

AI Assistance

Implemented with AI-assisted pair programming using OpenAI Codex and GPT-5.6.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Deduplicate by session before applying the global result limit — a high-frequency recent session can consume the entire candidate row set and hide other matching sessions. Evidence hub/src/store/messageContentSearch.ts:209.
  • [Major] Release target locks when clearing a message window — the existing invalidation path clears the window and immediately syncs, but the new lock makes that sync a no-op. Evidence web/src/lib/message-window-store.ts:788.
  • [Minor] Clear stale content results before the debounce — old rows remain clickable under the new query for 180 ms and can open a mismatched message/query pair. Evidence web/src/components/SessionList.tsx:1295.
  • [Minor] Remove fallback card highlights when changing or dismissing a target — class-only highlights are not selected by the current cleanup. Evidence web/src/components/AssistantChat/HappyThread.tsx:1530.

Summary
Review mode: initial

  • Four findings: 2 Major, 2 Minor.
  • Coverage gaps: no test for per-session result starvation, invalidation while a search target lock is held, query changes during debounce, or cleanup of the class-only highlight fallback.

Testing

  • Not run (automation); static review only because PR code is untrusted and was not executed.

HAPI Bot

Comment thread hub/src/store/messageContentSearch.ts Outdated
const safeLimit = Number.isFinite(limit) ? Math.max(1, Math.min(100, Math.floor(limit))) : 50
// Keep enough rows to deduplicate to one hit per session without making a
// broad content search scan an unbounded number of snippets.
const candidateLimit = Math.min(10_000, Math.max(200, safeLimit * 50))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Deduplicate by session before applying the result limit

candidateLimit is applied to matching message rows before seenSessions deduplicates them. Because rows are ordered by session recency first, one recent session with at least 2,500 matches (for the default limit) can occupy the entire candidate set; every other matching session is then omitted, so the endpoint may return only one session even though many match.

Suggested fix:

const rows = db.prepare(`
    WITH ranked AS (
        SELECT f.message_id, f.session_id, f.role, f.seq, f.created_at,
               s.updated_at,
               snippet(message_content_search, 0, '', '', '…', 24) AS snippet,
               ROW_NUMBER() OVER (
                   PARTITION BY f.session_id
                   ORDER BY CAST(f.seq AS INTEGER) DESC
               ) AS session_rank
        FROM message_content_search AS f
        INNER JOIN sessions AS s
            ON s.id = f.session_id AND s.namespace = ?
        WHERE message_content_search MATCH ?
    )
    SELECT message_id, session_id, role, seq, created_at, snippet
    FROM ranked
    WHERE session_rank = 1
    ORDER BY updated_at DESC, CAST(seq AS INTEGER) DESC
    LIMIT ?
`).all(namespace, escapeFtsPhrase(normalizedQuery), safeLimit)

Apply the same per-session ranking shape to the short-query LIKE branch, and add a test where one session has more than candidateLimit hits while another session also matches.

sessionId: string,
options: { ensureAfterCurrent?: boolean } = {}
): Promise<void> {
if (messageWindowTargetLocks.has(sessionId)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Release target locks when clearing the message window

The new early return means syncTailMessages() cannot repopulate a locked session. Existing invalidation handling does clearMessageWindow(event.sessionId) followed by syncTailMessages(...) (web/src/App.tsx:294), but clearMessageWindow does not remove messageWindowTargetLocks. While the retained search target is active, an invalidation therefore empties the chat and the recovery sync is silently skipped.

Suggested fix:

export function clearMessageWindow(sessionId: string): void {
    messageWindowTargetLocks.delete(sessionId)
    tailSyncControllers.delete(sessionId)
    clearPersistedState(sessionId)
    // existing state reset
}

Add a regression test that locks a session, clears it, then verifies a subsequent tail sync calls getMessages and repopulates the window.

Comment thread web/src/components/SessionList.tsx Outdated
}

const controller = new AbortController()
const timer = window.setTimeout(() => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Clear stale content results before starting the debounce

When normalizedQuery changes, the previous contentSearchResponse remains rendered until this timer fires 180 ms later. During that interval the rows belong to the old query, but clicks pass the new normalizedQuery as targetMessageQuery, producing a mismatched message/query jump.

Suggested fix:

setContentSearchResponse(null)
setContentSearchLoading(true)
setContentSearchError(false)

const timer = window.setTimeout(() => {
    void api.searchSessionContent(normalizedQuery, 50, controller.signal)
        // existing handlers
}, 180)

Add a component test that changes the query before the debounce expires and asserts that the old result is no longer clickable.

scrollSearchTargetIntoView(matchMarker ?? target, viewportRef.current)
scheduleSearchTargetScroll(targetMessageId, targetSearchQuery, target)
if (!matchMarker) {
target.classList.add(SEARCH_TARGET_HIGHLIGHT_CLASS)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Clean up the class-only fallback highlight

When no text range can be created, this adds hapi-message-search-target directly to the message card. clearSearchTargetMarker() only selects elements with data-hapi-source-search-match="true", so this fallback highlight survives navigation to another match and dismissal of the search bar.

Suggested fix:

const highlighted = document.querySelectorAll<HTMLElement>(
    `.${SEARCH_TARGET_HIGHLIGHT_CLASS}`
)
for (const element of highlighted) {
    if (element.getAttribute(SEARCH_TARGET_MATCH_ATTRIBUTE) === 'true' && element.parentElement) {
        removeSearchMatchMarker(element, element.parentElement)
    } else {
        element.classList.remove(SEARCH_TARGET_HIGHLIGHT_CLASS)
    }
}

Cover the fallback path with a test that dismisses the target and asserts the card class is removed.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Batch the FTS rebuild instead of materializing the full message scope — the v24 migration and session-merge rebuild load every selected content blob into JS before indexing, so peak memory scales with the entire history. Evidence hub/src/store/messageContentSearch.ts:121.
    Suggested fix:
    let afterRowId = 0
    while (true) {
        const batch = selectBatch.all(afterRowId, 500) as Array<DbMessageRow & { row_id: number }>
        if (batch.length === 0) break
        for (const row of batch) indexRow(row)
        afterRowId = batch[batch.length - 1]!.row_id
    }
  • [Major] Resolve the rendered text part that actually contains the query — one stored assistant message can render several elements with the same source id; returning the first one makes later-block matches wait through retries and then highlight/scroll to the wrong block. Evidence web/src/chat/outline.ts:102.
    Suggested fix:
    export function findConversationMessageAnchor(messageId: string, query?: string): HTMLElement | null {
        const candidates = getSourceMessageCandidates(messageId)
        if (query) {
            const matching = candidates.find((element) =>
                findConversationMessageTextRange(element, query) !== null
            )
            if (matching) return matching
        }
        return candidates[0] ?? null
    }
  • [Minor] Exclude hidden AGENT_NOTIFY_SUMMARY footer text from the index — the default renderer strips this footer, but content search currently returns it in snippets and cannot highlight it in chat. Evidence shared/src/messages.ts:188.
    Suggested fix:
    const rawText = directText ?? extractAssistantPlainText(record.content) ?? ''
    const text = normalizeSearchablePlainText(stripNotifySummaryFooter(rawText))

Summary
Review mode: follow-up after new commits

  • The four prior findings no longer reproduce in the current diff. Three current findings remain: two Major, one Minor.

Testing

  • Not run (automation); static review only because PR code is untrusted and was not executed. Current GitHub test and integration checks pass.

HAPI Bot

Comment thread hub/src/store/messageContentSearch.ts Outdated
SELECT id, session_id, content, created_at, seq, invoked_at
FROM messages
WHERE invoked_at IS NOT NULL
`).all() as DbMessageRow[]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Batch the FTS rebuild instead of materializing the whole message scope

Both SELECT branches call .all(), so the v24 startup migration loads every invoked message content blob into JS at once; the session-specific branch does the same for merges. HAPI histories are not size-bounded, and compressed BLOB rows remain retained in rows while each item is decoded and inserted. A large existing database can therefore exhaust memory and prevent the hub from completing the upgrade.

Suggested fix:

let afterRowId = 0
while (true) {
    const batch = db.prepare(`
        SELECT rowid AS row_id, id, session_id, content, created_at, seq, invoked_at
        FROM messages
        WHERE invoked_at IS NOT NULL AND rowid > ?
        ORDER BY rowid
        LIMIT ?
    `).all(afterRowId, 500) as Array<DbMessageRow & { row_id: number }>
    if (batch.length === 0) break
    for (const row of batch) indexRow(row)
    afterRowId = batch[batch.length - 1]!.row_id
}

Apply the same rowid pagination with the session predicate when sessionIds is present. Add a rebuild test that spans multiple batches and verifies every eligible row is indexed.

Comment thread web/src/chat/outline.ts Outdated
*/
export function findConversationMessageAnchor(messageId: string): HTMLElement | null {
for (const element of document.querySelectorAll<HTMLElement>('[data-hapi-source-message-id]')) {
if (element.getAttribute('data-hapi-source-message-id') === messageId) return element

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Choose the source element that actually contains the query

A single stored Claude assistant message may contain multiple visible text blocks. extractAssistantPlainText() concatenates all of them into one indexed document, while NotifySummaryText renders each part with the same data-hapi-source-message-id. This loop always returns the first part. If the hit is in a later part, HappyThread retries the range lookup on the wrong element for about six seconds and finally applies the fallback highlight/scroll to that wrong part.

Suggested fix:

export function findConversationMessageAnchor(
    messageId: string,
    query?: string
): HTMLElement | null {
    const exact = Array.from(document.querySelectorAll<HTMLElement>(
        `[data-hapi-source-message-id="${CSS.escape(messageId)}"]`
    ))
    const grouped = Array.from(document.querySelectorAll<HTMLElement>(
        '[data-hapi-source-message-ids]'
    )).filter((element) =>
        element.getAttribute('data-hapi-source-message-ids')?.split(/\s+/).includes(messageId)
    )
    const candidates = [...exact, ...grouped]
    if (query) {
        const matching = candidates.find((element) =>
            findConversationMessageTextRange(element, query) !== null
        )
        if (matching) return matching
    }
    return candidates[0] ?? document.getElementById(getConversationMessageAnchorId(messageId))
}

Pass targetSearchQuery from HappyThread. Add a regression test with one assistant message containing two text blocks and a query present only in the second block.

Comment thread shared/src/messages.ts Outdated
&& typeof record.content.text === 'string'
? record.content.text
: null
const text = normalizeSearchablePlainText(directText ?? extractAssistantPlainText(record.content) ?? '')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Do not index the hidden AGENT_NOTIFY_SUMMARY footer

The default chat renderer strips this machine footer when sessionSummaryInChat is off (NotifySummaryText.tsx:123-127), but the index stores the full assistant text here. Searching for footer-only summary/action text therefore exposes it in the session-list snippet, then opens a target whose rendered DOM does not contain the query and falls back to a card-only highlight. That contradicts the visible-text scope of this index.

Suggested fix:

const rawText = directText ?? extractAssistantPlainText(record.content) ?? ''
const text = normalizeSearchablePlainText(stripNotifySummaryFooter(rawText))
return text ? { role: 'assistant', text } : null

Add an extraction/search test where ordinary prose is followed by a valid AGENT_NOTIFY_SUMMARY footer and assert that prose is searchable while footer-only terms are not.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Message ingestion performs an unindexed FTS delete for every row — message_id is UNINDEXED, so the delete in indexMessageContent() scans the growing virtual table even for newly generated IDs.
  • [Major] Search indexes assistant records that the chat renderer intentionally hides — isMeta and isCompactSummary output can appear in results, but the target is removed when its context window is normalized.
  • [Major] Reloading a search context loses the required latest-tail reset — the persisted window keeps the real head cursor but omits requiresLatestReset, so hydration can treat old context as a current tail.

Questions

  • None.

Summary

Review mode: follow-up after new commits

The new commits resolve the two findings from the prior HAPI Bot review. The full latest diff still has the three major issues above.

Testing

  • Not run (automation); PR code was not executed during review.
  • Add coverage for write-path query behavior, hidden meta/compact-summary extraction, and persistence/reload of a historical message window.

HAPI Bot


export function indexMessageContent(db: Database, message: IndexableMessage): void {
ensureMessageContentSearchTable(db)
removeMessageContentSearchIndex(db, message.id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] Avoid a full FTS scan on every message insert. message_id is declared UNINDEXED at line 58, but every call here first executes DELETE ... WHERE message_id = ?. FTS5 cannot look up that column through its inverted index, and all normal create/import paths call this function with a newly generated ID, so ingestion cost grows with the entire search corpus (cumulative O(N²) work). Remove the delete from the insert-only path and index only rows that actually transition from queued to invoked; reserve removal/replacement for real mutations.

Suggested fix:

export function indexMessageContent(db: Database, message: IndexableMessage): void {
    ensureMessageContentSearchTable(db)
    if (message.invokedAt === null) return

    const searchable = extractSearchableMessageText(message.content)
    if (!searchable) return
    insertMessageContentSearchIndex(db, {
        id: message.id,
        sessionId: message.sessionId,
        text: searchable.text,
        role: searchable.role,
        seq: message.seq,
        createdAt: message.createdAt
    })
}

For markMessagesInvoked, use UPDATE ... WHERE invoked_at IS NULL RETURNING ... and index only the returned rows, preventing duplicate inserts without this scan.

Comment thread shared/src/messages.ts Outdated
? record.content.text
: null
const text = normalizeSearchablePlainText(
stripNotifySummaryFooter(directText ?? extractAssistantPlainText(record.content) ?? '')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] Exclude assistant records that are not chat-visible. This extraction accepts output payloads with data.isMeta or data.isCompactSummary, while web/src/chat/normalizeAgent.ts:632-644 deliberately drops both. Their text can therefore be exposed by search, and selecting the result can fail because loadMessageContext() later filters the target out during normalization. Apply the same visibility rule before extracting assistant prose.

Suggested fix:

if (isObject(record.content) && record.content.type === 'output') {
    const data = isObject(record.content.data) ? record.content.data : null
    if (data && (Boolean(data.isMeta) || Boolean(data.isCompactSummary))) {
        return null
    }
}

const text = normalizeSearchablePlainText(
    stripNotifySummaryFooter(directText ?? extractAssistantPlainText(record.content) ?? '')
)

Add extraction/search tests for both flags.

oldestPositionSeq: response.page.nextBeforeSeq,
newestPositionAt: response.page.snapshotHeadAt,
newestPositionSeq: response.page.snapshotHeadSeq,
requiresLatestReset: true,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] Persist this reset marker with the historical window. The context response stores the real session head in newestPosition* and sets requiresLatestReset, but PersistedMessageWindowState/persistState() omit that flag. After a reload, hydration sees valid head/epoch values and reconstructs requiresLatestReset: false; runTailSync() then requests only rows after the real head, receives none, and leaves the bounded old context displayed as though it were the current tail.

Suggested fix:

type PersistedMessageWindowState = {
    // existing fields...
    requiresLatestReset: boolean
}

const persisted: PersistedMessageWindowState = {
    // existing fields...
    requiresLatestReset: state.requiresLatestReset
}

requiresLatestReset:
    parsed.requiresLatestReset === true
    || (parsed.messages.length > 0 && (newest === null || epoch === null))

Add a regression test that loads a search context, flushes storage, hydrates a fresh store, and verifies the next sync fetches/replaces with the latest window.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Global FTS results rejoin the virtual table through the UNINDEXED message_id column, so snippet retrieval can repeatedly scan the search corpus.
  • [Minor] Content-search results bypass the existing active-only/sidebar preparation path, allowing inactive sessions to reappear when the active-only preference is enabled.
  • [Minor] The new lookup table is required by every indexed write but is absent from the schema-required table list.

Questions

  • None.

Summary

Review mode: follow-up after new commits

The new commits address the three findings from the prior HAPI Bot pass. The full latest diff still has one major performance issue and two minor consistency issues, detailed inline.

Testing

  • Not run (automation); PR code was not executed during review.
  • git diff --check passed.
  • Add coverage for the global FTS query plan, content search with active-only enabled, and current-schema validation when the lookup table is absent.

HAPI Bot

Comment thread hub/src/store/messageContentSearch.ts Outdated
snippet(${MESSAGE_CONTENT_SEARCH_TABLE}, 0, '', '', '…', 24) AS snippet
FROM ranked_matches AS ranked
INNER JOIN ${MESSAGE_CONTENT_SEARCH_TABLE} AS f
ON f.message_id = ranked.message_id

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] Join snippets by FTS rowid, not the unindexed message id. message_id is declared UNINDEXED at line 94, but this second join probes the virtual table through that column after ranked_matches has already identified the matching rows. FTS5 cannot use its inverted index for this predicate, so returning up to 50 deduplicated sessions can devolve into repeated scans of the full virtual table and make the opt-in search slow on large histories.

Suggested fix:

WITH ranked_matches AS (
    SELECT
        f.rowid AS search_rowid,
        f.message_id,
        -- existing columns...
    FROM message_content_search AS f
    -- existing MATCH/ranking...
)
SELECT ranked.message_id, /* ... */,
       snippet(message_content_search, 0, '', '', '…', 24) AS snippet
FROM ranked_matches AS ranked
INNER JOIN message_content_search AS f
    ON f.rowid = ranked.search_rowid

Add a large-corpus/query-plan regression that verifies the outer lookup uses the FTS rowid.

[allSessions, isFiltering, normalizedQuery, timeRange?.start, timeRange?.end, machineLabelsById] // eslint-disable-line react-hooks/exhaustive-deps
() => {
if (contentSearchActive) {
const results = contentSearchResponse?.results.map((result) => result.session) ?? []

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] Apply the existing sidebar scope before rendering content results. The metadata path starts from allSessions, which already applies prepareSidebarSessions() and the active-only preference. This branch instead renders the API results directly, so switching to Content can make inactive sessions reappear even when “active sessions only” is enabled (and also skips the normal sidebar dedup/stub filtering).

Suggested fix:

const results = contentSearchResponse?.results.map((result) => result.session) ?? []
const prepared = prepareSidebarSessions(results, selectedSessionId)
const scoped = showActiveSessionsOnly
    ? filterActiveSessionsOnly(prepared, selectedSessionId)
    : prepared
return scoped.filter((session) => sessionMatchesTimeRange(session, timeRange))

Include selectedSessionId and showActiveSessionsOnly in this memo's dependencies, and add a content-search test with active-only enabled.

Comment thread hub/src/store/index.ts
'sessions',
'machines',
'messages',
'message_content_search',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] Treat the auxiliary lookup table as required schema. A database already marked schema v24 does not call createMessageContentSearchTable(); startup only runs assertRequiredTablesPresent(). Because message_content_search_lookup is omitted here, a partial/corrupt v24 database can pass startup and then fail every indexed message write with “no such table”.

Suggested fix:

const REQUIRED_TABLES = [
    'sessions',
    'machines',
    'messages',
    'message_content_search',
    'message_content_search_lookup',
    // ...
] as const

Add a current-version reopen test that drops only the lookup table and expects the schema mismatch to be detected at startup.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Current-schema databases rebuild the entire message-id lookup on first search/write after every hub restart.
  • [Major] The bulk session-cleanup script leaves deleted message text in the new FTS tables.
  • [Major] The new-message/send jump exits the visual search lock without reconciling the bounded history window to the real tail.

Questions

  • None.

Summary

Review mode: follow-up after new commits

The new commits address the three findings from the previous HAPI Bot review. The full latest diff still has three major correctness/performance/data-retention issues, detailed inline.

Testing

  • Not run (automation); PR code was not executed during review.
  • git diff --check passed.
  • Add regression coverage for a cold reopen of a populated v25 database, bulk cleanup with indexed messages, and leaving a search context through the new-message/send tail jump.

HAPI Bot

Comment thread hub/src/store/messageContentSearch.ts Outdated
// FTS5 UNINDEXED columns are intentionally not searchable, but SQLite
// still has to scan the virtual table when deleting by one of them. Keep
// an ordinary indexed message-id lookup so the write path stays bounded.
backfillMessageContentSearchLookup(db)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Avoid replaying the full lookup backfill on every cold start. initializedDatabases is process-local, while the current-schema path only calls assertRequiredTablesPresent() (hub/src/store/index.ts:352). Therefore the first content search or indexed message write after each hub restart enters ensureMessageContentSearchTable(), reaches this line, scans every FTS row, and performs an INSERT OR IGNORE for each already-populated lookup row. On a large history that makes the first search/message O(total indexed messages) and holds the synchronous SQLite path while it runs.

Suggested fix:

export function markMessageContentSearchReady(db: Database): void {
    initializedDatabases.add(db)
}

// Store.initSchema(), after assertRequiredTablesPresent()
markMessageContentSearchReady(this.db)

Keep backfillMessageContentSearchLookup() confined to the v24→v25 migration, and add a populated-v25 reopen regression that verifies the first search/write does not replay the backfill.

Comment thread hub/src/store/sessions.ts Outdated
'SELECT 1 FROM sessions WHERE id = ? AND namespace = ?'
).get(id, namespace)
if (!existing) return false
removeMessageContentSearchForSession(db, id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Purge the derived index from the bulk cleanup path too. This helper protects application deletes, but hub/scripts/cleanup-sessions.ts:310 still deletes sessions directly. Foreign-key cascading removes messages; it cannot remove rows from these standalone FTS/lookup tables. Running the documented cleanup script will therefore retain the deleted user/assistant text on disk and make future searches scan orphaned corpus rows, even though the API join hides them.

Suggested fix:

const messageIds = `SELECT id FROM messages WHERE session_id IN (${placeholders})`
const remove = db.transaction(() => {
    db.run(`DELETE FROM message_content_search
            WHERE rowid IN (SELECT search_rowid FROM message_content_search_lookup
                            WHERE message_id IN (${messageIds}))`, ids)
    db.run(`DELETE FROM message_content_search_lookup
            WHERE message_id IN (${messageIds})`, ids)
    db.run(`DELETE FROM sessions WHERE id IN (${placeholders})`, ids)
})
remove()

Add a cleanup-script regression that indexes a distinctive phrase, deletes its session, and asserts both derived tables no longer contain its message row.


// Scroll to bottom handler for the indicator button
const scrollToBottom = useCallback(() => {
releaseSearchTargetHistoryLock()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Reconcile the real tail when the search-history jump is exited. A targeted load replaces the server window with only the bounded context and sets requiresLatestReset: true (web/src/lib/message-window-store.ts:1033-1042). The retained target keeps skipInitialTailSync enabled (web/src/router.tsx:388-390). This handler—used both by the “new messages” button and by a successful send—only releases the visual lock and switches the existing 41-row context to tail; it never clears the retained target or starts syncTailMessages(). The UI can therefore show the ancient context followed by new replies while silently omitting every message between that context and the actual tail.

Suggested fix:

const scrollToBottom = useCallback(() => {
    if (searchTargetHistoryLockRef.current) {
        dismissSearchTarget() // clears the retained route target; useMessages performs the latest reset
    } else {
        releaseSearchTargetHistoryLock()
    }
    // existing scroll/tail-mode logic...
}, [dismissSearchTarget, releaseSearchTargetHistoryLock])

Add an integration test that applies a search context, triggers forceScrollToken or the new-message indicator, and verifies a latest-page request replaces the bounded history window before tail rendering continues.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Tail reconciliation still retains the search target, so the next render re-locks the viewport and can leave a sent/new reply off-screen. Evidence: web/src/components/AssistantChat/HappyThread.tsx:1317.
    Suggested fix:
    if (activeSearchTargetId) dismissSearchTarget()
    else releaseSearchTargetHistoryLock()
    props.onJumpToTail?.()
  • [Major] One- and two-character content queries use an unbounded %LIKE% scan over the FTS corpus on the synchronous hub SQLite path. Evidence: hub/src/store/messageContentSearch.ts:335.
    Suggested fix:
    const MIN_INDEXED_QUERY_LENGTH = 3
    if ([...normalizedQuery].length < MIN_INDEXED_QUERY_LENGTH) return []
    // Use MATCH for indexed searches.

Questions

  • None.

Summary

Review mode: follow-up after new commits

The startup lookup backfill and bulk-cleanup retention findings are resolved. The tail-exit finding remains partially unresolved, and the full latest diff also contains an unbounded short-query scan.

Testing

  • Not run (automation): bun is unavailable in this runner environment.
  • git diff --check passed.
  • SQLite diagnostic: the trigram %LIKE% path for one/two-character patterns uses virtual-table scan plan INDEX 0:L0; a simple 100,000-row synthetic no-match scan took about 22 ms before this PR's namespace join, window function, and ranking work.

HAPI Bot

// bottom after a clamped/programmatic scroll. Always use the
// explicit tail transition so the bounded window is reconciled
// with the server's latest messages before the next turn renders.
props.onJumpToTail()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Clear the retained search target before starting the tail transition. The follow-up now invokes onJumpToTail(), but releaseSearchTargetHistoryLock() only clears the local marker. SessionPage still supplies the retained initialTargetMessageId (web/src/router.tsx:352-362), so the search-target layout effect runs again when isSyncingTail/messagesVersion changes, sets searchTargetHistoryLockRef.current = true (web/src/components/AssistantChat/HappyThread.tsx:1429), and returns because the jump phase is already complete. After the latest page replaces the bounded context, the auto-scroll layout effect is therefore blocked; a successful send or “new messages” jump can leave the new reply off-screen while the search navigation remains active.

Suggested fix:

if (activeSearchTargetId) {
    dismissSearchTarget() // clears the retained route target as well as the marker
} else {
    releaseSearchTargetHistoryLock()
}
props.onJumpToTail?.()

Extend the new forced-scroll regression to start with a completed retained search target, rerender through the latest-page replacement, and assert the target/navigation is dismissed and the viewport follows the replacement tail.

Comment thread hub/src/store/messageContentSearch.ts Outdated
if (!normalizedQuery) return []

const safeLimit = Number.isFinite(limit) ? Math.max(1, Math.min(100, Math.floor(limit))) : 50
const useLike = [...normalizedQuery].length < 3

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Keep short queries off the unbounded synchronous scan path. For every query shorter than three Unicode code points this switches from trigram MATCH to searchable_text LIKE '%…%'. FTS5 cannot use the trigram index without a three-character literal, so the hub scans the entire content corpus; the LIMIT is applied only after the ROW_NUMBER() partition/ranking. The web activates content search for any non-empty input (web/src/components/SessionList.tsx:1284) after 180 ms, making a one- or two-character pause capable of blocking the hub's synchronous SQLite/event-loop path on a large history.

Suggested fix:

const MIN_INDEXED_QUERY_LENGTH = 3
const queryLength = [...normalizedQuery].length
if (queryLength < MIN_INDEXED_QUERY_LENGTH) return []

const rows = db.prepare(`
    -- existing MATCH query only
`).all(namespace, escapeFtsPhrase(normalizedQuery), safeLimit)

Apply the same guard to the session-scoped endpoint and surface a localized minimum-length hint in the web UI. If two-character CJK substring search is required, back it with a separately indexed bigram structure instead of %LIKE%.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Unigram searches still rank a corpus-scale candidate set — the short-query index removes the virtual-table scan, but a common one-character gram such as e still selects nearly every containing message, computes ROW_NUMBER() across all of them, and only then applies LIMIT. The web sends any non-empty content query after 180 ms, so this remains an event-loop blocking path on large histories. Evidence: hub/src/store/messageContentSearch.ts:409.
    Suggested fix:

    const queryLength = [...normalizedQuery].length
    if (queryLength < 2) return []
    const useShortIndex = queryLength === 2

    Also stop emitting unigram rows and gate the web request at the same two-character minimum.

  • [Minor] Cross-node Markdown matches receive an invisible fallback highlight — findConversationMessageTextRange intentionally returns ranges spanning rendered nodes, but Range.surroundContents() throws when the range partially contains an element such as <strong>. The catch inserts only a zero-width <mark>, so the navigation scrolls correctly while no matched text is visibly highlighted. Evidence: web/src/components/AssistantChat/HappyThread.tsx:268.
    Suggested fix:

    } catch {
        const fallback = range.commonAncestorContainer instanceof HTMLElement
            ? range.commonAncestorContainer
            : range.commonAncestorContainer.parentElement
        fallback?.classList.add(SEARCH_TARGET_HIGHLIGHT_CLASS)
        return fallback
    }

    Prefer wrapping each intersecting text-node segment for exact highlighting; at minimum, make the fallback visible and add a regression using the existing KV <strong>Cache</strong> fixture.

Questions

  • None.

Summary

Review mode: follow-up after new commits

Two issues remain in the latest full diff: one-character content searches can still perform work proportional to most of the message corpus, and formatted matches spanning Markdown nodes are not visibly highlighted. The prior tail-transition issue is not present at this head.

Testing

  • Not run locally (automation review; Bun is unavailable in this runner).
  • GitHub checks: test passed; integration passed.
  • Add regressions for a one-character query over many matching rows and a visible cross-node Markdown highlight.

HAPI Bot

Comment thread hub/src/store/messageContentSearch.ts Outdated
if (!normalizedQuery) return []

const safeLimit = Number.isFinite(limit) ? Math.max(1, Math.min(100, Math.floor(limit))) : 50
const useShortIndex = [...normalizedQuery].length < MIN_INDEXED_QUERY_LENGTH

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Reject unigram queries before ranking the short-index candidates. The new table avoids the FTS virtual-table scan, but a common one-character gram such as e still returns nearly every message containing that character. The ROW_NUMBER() CTE processes that full candidate set before the outer LIMIT, and the web issues any non-empty content query after 180 ms, so large histories can still block the synchronous SQLite/event-loop path.

Suggested fix:

const queryLength = [...normalizedQuery].length
if (queryLength < 2) return []
const useShortIndex = queryLength === 2

Apply the same guard to searchMessageContentInSession, stop generating unigram rows in getShortSearchGrams, and gate the web request at two characters with a localized hint. Add a regression for a common one-character query across many messages.

try {
range.surroundContents(marker)
} catch {
const collapsed = range.cloneRange()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Make the cross-node fallback visibly highlight the match. findConversationMessageTextRange deliberately supports phrases spanning rendered Markdown nodes. For a range such as KV <strong>Cache</strong>, surroundContents() throws because the range partially contains <strong>; this catch then inserts only a zero-width <mark>. The jump works, but the matched phrase has no visible persistent highlight.

Suggested fix:

} catch {
    const fallback = range.commonAncestorContainer instanceof HTMLElement
        ? range.commonAncestorContainer
        : range.commonAncestorContainer.parentElement
    fallback?.classList.add(SEARCH_TARGET_HIGHLIGHT_CLASS)
    return fallback
}

Prefer wrapping each intersecting text-node segment for exact highlighting. At minimum, use a visible fallback and extend the existing cross-node range test to assert a non-zero highlighted area.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] One-character searches still rank a corpus-scale candidate set — the unigram lookup avoids an FTS scan, but a common gram such as e can still select nearly every containing message; ROW_NUMBER() ranks that full set before the outer LIMIT, on the synchronous hub SQLite path. Evidence hub/src/store/messageContentSearch.ts:415.
    Suggested fix:

    const queryLength = [...normalizedQuery].length
    if (queryLength < 2) return []
    const useShortIndex = queryLength === 2

    Apply the equivalent { matches: [], total: 0 } guard to the session-scoped search, stop emitting unigram rows, gate the web request at two characters, and add a common-unigram corpus regression.

  • [Minor] Cross-node Markdown matches still receive an invisible fallback highlight — a range spanning elements such as KV <strong>Cache</strong> makes surroundContents() throw; the catch inserts only a zero-width marker, so navigation scrolls but the matched phrase is not visibly highlighted. Evidence web/src/components/AssistantChat/HappyThread.tsx:268.
    Suggested fix:

    } catch {
        const fallback = range.commonAncestorContainer instanceof HTMLElement
            ? range.commonAncestorContainer
            : range.commonAncestorContainer.parentElement
        fallback?.classList.add(SEARCH_TARGET_HIGHLIGHT_CLASS)
        return fallback
    }

    Prefer wrapping each intersecting text-node segment for exact highlighting; add a rendered cross-node regression that asserts a visible highlighted area.

Questions

  • None.

Summary

Review mode: follow-up after new commits

The latest commit changes short-index backfill mechanics, but both current-head issues remain: one-character content searches can still do corpus-scale ranking work, and formatted cross-node matches are not visibly highlighted.

Testing

  • Not run (automation review; PR content was not executed per security policy).
  • GitHub checks: test passed; integration passed.
  • Missing regressions: common one-character query volume; visible cross-node Markdown highlighting.

HAPI Bot

Comment thread hub/src/store/messageContentSearch.ts Outdated
if (!normalizedQuery) return []

const safeLimit = Number.isFinite(limit) ? Math.max(1, Math.min(100, Math.floor(limit))) : 50
const useShortIndex = [...normalizedQuery].length < MIN_INDEXED_QUERY_LENGTH

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Reject one-character queries before ranking short-index candidates. A common unigram such as e can match nearly every containing message. The ROW_NUMBER() CTE processes that entire candidate set before the outer LIMIT, and the web sends any non-empty content query after 180 ms, so this can still block the synchronous hub SQLite/event-loop path on large histories.

Suggested fix:

const queryLength = [...normalizedQuery].length
if (queryLength < 2) return []
const useShortIndex = queryLength === 2

Apply the equivalent { matches: [], total: 0 } guard to searchMessageContentInSession, stop generating unigram rows in getShortSearchGrams, gate the web request at two characters with a localized hint, and add a regression using a common one-character query across many messages.

try {
range.surroundContents(marker)
} catch {
const collapsed = range.cloneRange()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Make the cross-node fallback visibly highlight the match. findConversationMessageTextRange() deliberately returns ranges that can span Markdown nodes. For a range such as KV <strong>Cache</strong>, surroundContents() throws because the range partially contains an element; this catch then inserts only a zero-width <mark>. The jump works, but no matched text receives a visible persistent highlight.

Suggested fix:

} catch {
    const fallback = range.commonAncestorContainer instanceof HTMLElement
        ? range.commonAncestorContainer
        : range.commonAncestorContainer.parentElement
    fallback?.classList.add(SEARCH_TARGET_HIGHLIGHT_CLASS)
    return fallback
}

Prefer wrapping each intersecting text-node segment for exact highlighting. At minimum, add a rendered cross-node regression that asserts a non-zero visible highlighted area.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Apply sidebar filters before the global result cap - the content request is capped at 50 sessions before active-only/date/machine/unread filters run, so an eligible match beyond the first 50 is silently omitted and the UI can report no results. Evidence web/src/components/SessionList.tsx:1305.
    Suggested fix:

    const eligibleSessionIds = contentSearchCandidateSessions.map((session) => session.id)
    void api.searchSessionContent({
        query: normalizedQuery,
        limit: 50,
        sessionIds: eligibleSessionIds,
        signal: controller.signal,
    })

    Constrain those session ids in the hub query before ranking and limiting.

  • [Minor] Release the target lock after terminal context-load failure - opening a search result skips and locks the initial tail sync, but both exhausted-retry branches only clear the component-local lock. The route target remains, so a deleted or stale hit can leave a fresh page with an empty thread until the user manually closes search. Evidence web/src/components/AssistantChat/HappyThread.tsx:1639.
    Suggested fix:

    } else {
        current.phase = 'failed'
        dismissSearchTarget()
        props.onJumpToTail?.()
    }

    Apply the same fallback to the rejection branch.

Questions

  • None.

Summary

Review mode: follow-up after new commits

The previous bot findings are no longer present. The current head still has one result-scoping correctness issue and one terminal search-jump recovery issue.

Testing

  • Not run (automation review; PR content was not executed per security policy).
  • GitHub checks: test passed; integration passed.
  • Missing regressions: more than 50 content matches with a later match passing sidebar filters; terminal context failure returning to the latest transcript.

HAPI Bot

Comment thread web/src/components/SessionList.tsx Outdated
const controller = new AbortController()
const timer = window.setTimeout(() => {
setContentSearchLoading(true)
void api.searchSessionContent(normalizedQuery, 50, controller.signal)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Apply sidebar filters before the global result cap. This request hard-caps content matches at 50 sessions, but active-only/date filtering runs later in visibleSessions, and machine/unread filtering runs after that. If the newest 50 matching sessions are outside the selected scope, the UI reports no results even though an older eligible session matches.

Suggested fix:

const eligibleSessionIds = contentSearchCandidateSessions.map((session) => session.id)
void api.searchSessionContent({
    query: normalizedQuery,
    limit: 50,
    sessionIds: eligibleSessionIds,
    signal: controller.signal,
})

Constrain sessions.id in the hub query before ROW_NUMBER() and LIMIT. Add a regression with more than 50 matches where only a later session passes the active machine/date/unread filters.

current.phase = 'idle'
scheduleSearchTargetRetry(SEARCH_TARGET_QUERY_RETRY_DELAY_MS)
} else {
searchTargetHistoryLockRef.current = false

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Release the target lock when context loading is exhausted. skipInitialTailSync locks this session's message window before any latest-page request. Both terminal failure branches only clear the component-local history lock; the route target remains, so useMessages keeps the global target lock and never syncs the tail. A deleted or stale search hit on a fresh page can therefore leave an empty thread until the user manually closes search.

Suggested fix:

} else {
    current.phase = 'failed'
    dismissSearchTarget()
    props.onJumpToTail?.()
}

Use the same fallback in the rejection branch, and add a regression where getMessageContext() returns null.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Move the session scope out of the GET query string — the follow-up now appends every eligible session ID to the URL. Session IDs are UUID-sized, so roughly 350 sessions already produce a request target around 16 KB; no larger request-target limit is configured in the repo/docs. Users with a large history will get a transport-level failure before the scoped SQL runs. Evidence web/src/api/client.ts:266.
    Suggested fix:
    return await this.request<SessionContentSearchResponse>(
        '/api/sessions/content-search',
        {
            method: 'POST',
            signal,
            body: JSON.stringify({
                query: query.trim(),
                limit,
                sessionIds: [...new Set(sessionIds ?? [])]
            })
        }
    )
    Parse and validate that body in the hub route, then keep applying sessionIds before ranking and limiting.

Questions

  • None.

Summary

Review mode: follow-up after new commits

The two prior bot findings are resolved. One new scalability regression remains in the session-scoping transport.

Testing

  • Not run (automation review; PR content was not executed per security policy).
  • GitHub checks: test passed; integration passed.
  • Missing regression: several hundred eligible session IDs should not be serialized into the request URL.

HAPI Bot

Comment thread web/src/api/client.ts Outdated
const params = new URLSearchParams({ query: query.trim(), limit: String(limit) })
for (const sessionId of new Set(sessionIds ?? [])) {
const trimmed = sessionId.trim()
if (trimmed) params.append('sessionId', trimmed)

@github-actions github-actions Bot Aug 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Move the session scope out of the GET query string. This loop appends every eligible session ID to the URL. With UUID-sized IDs, roughly 350 sessions already create a request target around 16 KB, and no larger request-target limit is configured in the repo/docs. Large HAPI histories will therefore fail at the browser/proxy/server transport layer before the scoped SQL can run.

Suggested fix:

return await this.request<SessionContentSearchResponse>(
    '/api/sessions/content-search',
    {
        method: 'POST',
        signal,
        body: JSON.stringify({
            query: query.trim(),
            limit,
            sessionIds: [...new Set(sessionIds ?? [])]
        })
    }
)

Parse and validate the JSON body in the hub route, then pass sessionIds to the existing pre-limit query scope. Add a regression with several hundred IDs and assert the URL stays bounded.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Minor] Keep canonical message writes and derived search-index writes atomic — copyMessageToSession commits the messages insert before the new index writes, and the invoked-message helpers use the same split pattern. If an FTS/lookup/short-index statement fails after the canonical write, retries see an existing or already-invoked message and do not repair the missing index row, so that content remains permanently absent from search. Evidence hub/src/store/messages.ts:241.
    Suggested fix:
    return db.transaction(() => {
        db.prepare(/* INSERT or UPDATE messages */).run(/* ... */)
        indexMessageContent(db, indexableMessage)
        return result
    })()
    Apply the same transaction boundary to markMessagesInvoked and markUninvokedImmediateMessages.

Questions

  • None.

Summary

Review mode: follow-up after new commits

The previous URL-length finding is no longer present. One derived-index consistency issue remains.

Testing

  • Not run (automation review; PR content was not executed per security policy).
  • GitHub checks: test passed; integration passed.
  • Missing regression: inject an index-write failure and verify the canonical message transition rolls back, then succeeds and becomes searchable on retry.

HAPI Bot

Comment thread hub/src/store/messages.ts Outdated
invoked_at: invokedAt ?? null,
scheduled_at: message.scheduledAt ?? null
})
indexMessageContent(db, {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Keep canonical and derived search-index writes atomic.

At this line the messages insert has already autocommitted, while indexMessageContent performs several subsequent writes to the lookup, FTS, and short-query tables. If one of those statements fails, the request throws but the copied message remains; a retry sees that canonical message and can skip copying it, leaving it permanently absent from search. The same split transaction occurs in markMessagesInvoked and markUninvokedImmediateMessages, where a retry sees invoked_at IS NOT NULL and skips indexing.

Suggested fix:

return db.transaction(() => {
    db.prepare(/* INSERT or UPDATE messages */).run(/* ... */)
    indexMessageContent(db, indexableMessage)
    return result
})()

Use this transaction boundary for each canonical-message transition plus its derived-index maintenance, and add a fault-injection regression that verifies rollback and successful retry.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Bound scoped content-search IDs before constructing SQL placeholders — the POST endpoint accepts an unbounded sessionIds array, and searchMessageContent expands every entry into an IN (?, ...) parameter. A sufficiently large authenticated request therefore exceeds SQLite's variable limit and returns 500; before that point it can force large synchronous JSON parsing and statement preparation on the Hub event loop. Evidence hub/src/web/routes/sessions.ts:225, hub/src/store/messageContentSearch.ts:425.
    Suggested fix:

    const scopedSessionIds = sessionIds?.filter((id) => sessionsById.has(id))
    if (scopedSessionIds && scopedSessionIds.length > MAX_CONTENT_SEARCH_SESSION_IDS) {
        return c.json({ error: 'Too many sessionIds' }, 400)
    }
    // Or bind one JSON array and scope through json_each(?) instead of one variable per id.
  • [Minor] Make session deletion and derived-index cleanup atomic — removeMessageContentSearchForSession commits three index deletes before the canonical DELETE FROM sessions. If that delete fails, the session and messages remain but their content is permanently absent from search; retrying deletion does not repair the index. Evidence hub/src/store/sessions.ts:701.
    Suggested fix:

    return db.transaction(() => {
        const existing = db.prepare(
            'SELECT 1 FROM sessions WHERE id = ? AND namespace = ?'
        ).get(id, namespace)
        if (!existing) return false
    
        removeMessageContentSearchForSession(db, id)
        return db.prepare(
            'DELETE FROM sessions WHERE id = ? AND namespace = ?'
        ).run(id, namespace).changes > 0
    })()

Questions

  • None.

Summary

Review mode: follow-up after new commits

The previous message-transition transaction finding is no longer present. Two issues remain: scoped-search input is unbounded before dynamic SQL construction, and session deletion can desynchronize canonical data from the derived search index on failure.

Testing

  • Not run (automation review; PR content was not executed per security policy).
  • GitHub checks: test passed; integration passed.
  • Missing regressions: oversized/scoped sessionIds handling without SQLite variable overflow; injected session-delete failure preserving searchable content.

HAPI Bot

Comment thread hub/src/web/routes/sessions.ts Outdated
const sessionIds = body.sessionIds === undefined
? undefined
: [...new Set(
(body.sessionIds as string[])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Bound scoped content-search IDs before dynamic SQL construction.

This validation accepts any-length sessionIds array, while messageContentSearch.ts:425 creates one SQLite placeholder per distinct ID. A sufficiently large authenticated request exceeds SQLite's bind-variable limit and returns 500; smaller oversized requests still amplify synchronous JSON parsing and SQL preparation on the Hub event loop. Unknown IDs are currently forwarded too, so callers can trigger this without owning a large session corpus.

Suggested fix:

const scopedSessionIds = sessionIds?.filter((id) => sessionsById.has(id))
if (scopedSessionIds && scopedSessionIds.length > MAX_CONTENT_SEARCH_SESSION_IDS) {
    return c.json({ error: 'Too many sessionIds' }, 400)
}

Alternatively, bind one JSON array and use json_each(?) in the SQL so the scope consumes one variable, plus add an explicit request-body bound. Add a regression that sends a scope beyond the SQLite variable threshold and expects a controlled 4xx or successful bounded query.

Comment thread hub/src/store/sessions.ts Outdated
'SELECT 1 FROM sessions WHERE id = ? AND namespace = ?'
).get(id, namespace)
if (!existing) return false
removeMessageContentSearchForSession(db, id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Keep session deletion and derived-index cleanup atomic.

The three search-index deletes commit before the canonical DELETE FROM sessions. If the session delete fails (for example a trigger, I/O error, or later constraint), the session and messages remain but their content is permanently absent from search; a retry does not rebuild those rows.

Suggested fix:

return db.transaction(() => {
    const existing = db.prepare(
        'SELECT 1 FROM sessions WHERE id = ? AND namespace = ?'
    ).get(id, namespace)
    if (!existing) return false

    removeMessageContentSearchForSession(db, id)
    return db.prepare(
        'DELETE FROM sessions WHERE id = ? AND namespace = ?'
    ).run(id, namespace).changes > 0
})()

Add a fault-injection test that aborts the session delete and verifies the message remains searchable after rollback.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Preserve every raw message ID when stream updates are coalesced — the new DOM mapping derives one source ID from the surviving block ID, but the existing reducer mutates the first block for later records with the same streamId without retaining those later msg.id values. The search index returns those later message IDs (and global search prefers the newest matching seq), so the loaded text can render while the requested target ID never exists in the DOM; the jump then retries and dismisses, and match navigation can contain unlocatable entries. Evidence web/src/lib/assistant-runtime.ts:376, related context web/src/chat/reducerTimeline.ts:772, hub/src/store/messageContentSearch.ts:445.
    Suggested fix:

    if (existing) {
        existing.sourceMessageIds ??= [sourceMessageIdFromBlockId(existing.id)]
        if (!existing.sourceMessageIds.includes(msg.id)) {
            existing.sourceMessageIds.push(msg.id)
        }
        existing.text = c.text
        // existing metadata updates...
    }
    
    function getSourceMessageIdsForBlock(block: VisibleChatBlock): string[] {
        return block.sourceMessageIds?.length
            ? block.sourceMessageIds
            : [sourceMessageIdFromBlockId(block.id)]
    }
  • [Major] Keep scoped content search working above 500 sessions — the client submits every sidebar candidate ID, while the route rejects scopes larger than 500. There is no repository limit keeping accumulated sessions below that count, so a user with 501 eligible sessions gets 400 Too many sessionIds for every content query instead of filtered results. Evidence web/src/components/SessionList.tsx:1434, hub/src/web/routes/sessions.ts:148.
    Suggested fix:

    const scopeArgs = scopedSessionIds === undefined
        ? []
        : [JSON.stringify(scopedSessionIds)]
    const sessionScope = scopedSessionIds === undefined
        ? ''
        : ' AND f.session_id IN (SELECT value FROM json_each(?))'
    
    // Bind the scope as one SQLite variable, and apply a request-byte / ID-length
    // limit instead of rejecting legitimate candidate counts.
    statement.all(namespace, query, ...scopeArgs, safeLimit)
  • [Minor] Make bulk cleanup index maintenance atomic — the maintenance script commits the three derived-index deletes before its canonical session delete. If the latter statement fails, the sessions and messages remain but their content is no longer searchable. Evidence hub/scripts/cleanup-sessions.ts:315.
    Suggested fix:

    return db.transaction(() => {
        removeMessageContentSearchForSessions(db, ids)
        db.run(`DELETE FROM sessions WHERE id IN (${placeholders})`, ids)
        return ids.length
    })()

Questions

  • None.

Summary

Review mode: follow-up after new commits

The prior scoped-SQL overflow and normal session-deletion transaction findings were addressed, but the current head still has two major correctness regressions for streamed/large-session content search and one bulk-cleanup consistency gap.

Testing

  • Not run (automation review; PR content was not executed per security policy).
  • GitHub checks: test passed; integration passed.
  • Missing regressions: a matching later record in a coalesced streamId; a legitimate 501-session sidebar scope; injected bulk session-delete failure preserving searchable content.

HAPI Bot

|| block.kind === 'codex-review'
|| block.kind === 'generated-image'
) {
const separator = block.id.lastIndexOf(':')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Preserve every raw message ID across stream coalescing.

This derives only one source ID from the surviving block ID. However, reducerTimeline.ts:772-779 updates the first block when later stored messages share its streamId and discards each later msg.id. Content search indexes those stored messages separately and prefers the newest matching seq, so a result commonly points at a later streaming update whose ID is absent from the rendered card. The context loads, but target lookup keeps failing and eventually dismisses; previous/next navigation can also include these unlocatable IDs.

Suggested fix:

if (existing) {
    existing.sourceMessageIds ??= [sourceMessageIdFromBlockId(existing.id)]
    if (!existing.sourceMessageIds.includes(msg.id)) {
        existing.sourceMessageIds.push(msg.id)
    }
    existing.text = c.text
    // existing metadata updates...
}

function getSourceMessageIdsForBlock(block: VisibleChatBlock): string[] {
    return block.sourceMessageIds?.length
        ? block.sourceMessageIds
        : [sourceMessageIdFromBlockId(block.id)]
}

Add a regression with two stored text records sharing one streamId, then jump using the second record's message ID.

normalizedQuery,
limit,
controller.signal,
contentSearchSessionIds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Do not make content search fail for sidebars above 500 sessions.

This call always passes every eligible sidebar session ID. The route now returns 400 Too many sessionIds when that array has 501 entries, and there is no repository cap preventing users from accumulating that many sessions. Consequently every content query fails for those users; truncating the array would silently omit valid matches and violate the filter-before-limit contract.

Suggested fix:

const scopeArgs = scopedSessionIds === undefined
    ? []
    : [JSON.stringify(scopedSessionIds)]
const sessionScope = scopedSessionIds === undefined
    ? ''
    : ' AND f.session_id IN (SELECT value FROM json_each(?))'

// One SQLite binding for the complete scope; separately cap request bytes and ID length.
statement.all(namespace, query, ...scopeArgs, safeLimit)

Add a UI/API regression with 501 authorized candidate sessions and verify the request returns results rather than 400.

Comment thread hub/scripts/cleanup-sessions.ts Outdated
// Store wrapper. Remove derived message-content rows first, while the
// messages still exist for the session-id lookup, so deleted text cannot
// remain searchable in the FTS index.
removeMessageContentSearchForSessions(db, ids)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Keep bulk deletion and derived-index cleanup in one transaction.

The new index cleanup runs as separate autocommit statements before the canonical DELETE FROM sessions. If the session delete fails, the canonical rows remain while their searchable rows are permanently gone. The regular deleteSession path now avoids this, but this maintenance path still has the same consistency gap.

Suggested fix:

return db.transaction(() => {
    removeMessageContentSearchForSessions(db, ids)
    db.run(`DELETE FROM sessions WHERE id IN (${placeholders})`, ids)
    return ids.length
})()

Add fault injection around the bulk session delete and assert the retained messages remain searchable after rollback.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Collapse streamed snapshots before counting message matches — the index stores and counts each raw row, while the web reducer replaces rows sharing a stream ID with one visible message. A repeated phrase therefore produces duplicate navigation targets for the same card; an older snapshot can also match text absent from the final rendered snapshot. Evidence: hub/src/store/messageContentSearch.ts:540, with render behavior at web/src/chat/reducerTimeline.ts:771.
    Suggested fix:
    const renderKey = searchable.renderKey
        ? `${message.sessionId}:${searchable.renderKey}`
        : message.id
    upsertMessageContentSearchIndex(db, {
        ...message,
        id: renderKey,
        text: searchable.text
    })
    Extend extraction/index storage with the same stream identity used by the reducer, replacing the prior snapshot for that identity.

Summary

Review mode: follow-up after new commits

The three findings from the previous HAPI Bot review are addressed. One correctness issue remains in match counting/navigation for coalesced streaming snapshots.

Testing

Not run (automation review; PR content was not executed per security policy). Existing checks are passing, but coverage does not combine backend search with two raw rows sharing one rendered stream ID.

HAPI Bot

const useShortIndex = queryLength === MIN_INDEXED_QUERY_LENGTH
const countRow = useShortIndex
? db.prepare(`
SELECT COUNT(*) AS count

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Count the rendered message, not each stream snapshot.

COUNT(*) and the result query operate on raw indexed rows, but the chat reducer replaces text rows sharing a streamId with one visible block (web/src/chat/reducerTimeline.ts:771-783) and maps all source IDs to that one card. If a phrase appears in two snapshots, the UI reports two matches and previous/next repeatedly lands on the same card. Worse, a phrase removed by the final replacement can still match an older snapshot although it is no longer visible.

Suggested fix:

const renderKey = searchable.renderKey
    ? `${message.sessionId}:${searchable.renderKey}`
    : message.id
upsertMessageContentSearchIndex(db, {
    ...message,
    id: renderKey,
    text: searchable.text
})

Have extractSearchableMessageText expose the same stream identity used by the reducer, persist a unique render key, and replace its indexed row when a newer snapshot arrives. Add a regression with two matching rows sharing one stream ID; assert total === 1, one navigable match, and only the final snapshot text remains searchable.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Normalize Antigravity planner text before indexing — extractAssistantPlainText() returns raw agy_message content, and the new search extractor indexes it directly at shared/src/messages.ts:223. The chat renderer first removes the duplicated [Message] timestamp=... content=Task ... Output: ... block and converts an Inside the task-NNN log... marker into a tool chip at web/src/chat/normalizeAgent.ts:772 and web/src/chat/normalizeAgent.ts:778. As a result, sidebar search can return and snippet raw background-task/tool payload text that is absent from the rendered assistant message, violating the visible-prose-only indexing boundary.
    Suggested fix:
    // Move/reuse stripAgyEchoedTaskResult in shared code.
    const outputData = isObject(record.content) && record.content.type === 'output'
        ? (isObject(record.content.data) ? record.content.data : null)
        : null
    const assistantText = directText ?? extractAssistantPlainText(record.content) ?? ''
    const visibleText = outputData?.type === 'agy_message'
        ? stripAgyEchoedTaskResult(assistantText)
        : assistantText
    if (outputData?.type === 'agy_message' && /^Inside the task-\d+ log\b/.test(visibleText)) {
        return null
    }
    const text = normalizeSearchablePlainText(stripNotifySummaryFooter(visibleText))

Summary
Review mode: follow-up after new commits

The previous streamed-snapshot indexing finding is addressed by the new render-key replacement logic. One Major issue remains in the visible-text extraction boundary for Antigravity messages.

Testing
Not run (automation review; PR content was not executed per security policy). Add a shared extractor/index test proving echoed AGY task output is removed and a task-log-only marker is not searchable.

HAPI Bot

Comment thread shared/src/messages.ts Outdated
? record.content.text
: null
const text = normalizeSearchablePlainText(
stripNotifySummaryFooter(directText ?? extractAssistantPlainText(record.content) ?? '')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Normalize AGY planner output before adding it to the search index

For output.data.type === 'agy_message', extractAssistantPlainText() returns raw data.content, so this line indexes the duplicated [Message] ... Task ... Output: ... payload. The renderer removes that block with stripAgyEchoedTaskResult() and renders an Inside the task-NNN log... marker as a tool chip instead of prose (web/src/chat/normalizeAgent.ts:772-790). This can expose raw background-task/tool content in search snippets and create hits that cannot be found in the rendered message.

Move/reuse the renderer normalization in shared code before indexing:

const outputData = isObject(record.content) && record.content.type === 'output'
    ? (isObject(record.content.data) ? record.content.data : null)
    : null
const assistantText = directText ?? extractAssistantPlainText(record.content) ?? ''
const visibleText = outputData?.type === 'agy_message'
    ? stripAgyEchoedTaskResult(assistantText)
    : assistantText
if (outputData?.type === 'agy_message' && /^Inside the task-\d+ log\b/.test(visibleText)) {
    return null
}
const text = normalizeSearchablePlainText(stripNotifySummaryFooter(visibleText))

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Claude sidechain prose is indexed even though it has no standalone root-timeline message - extractSearchableMessageText excludes isMeta and isCompactSummary, but not data.isSidechain; the renderer moves those rows into a subagent group under the parent tool call. A search jump loads only 20 rows before the hit, so a long sidechain can omit that parent and leave the returned message ID with no rendered anchor. Evidence: shared/src/messages.ts:212, web/src/chat/normalizeAgent.ts:231, web/src/chat/reducer.ts:123.
    Suggested fix:
    function isHiddenAssistantOutput(content: unknown): boolean {
        if (!isObject(content) || content.type !== 'output') return false
        const data = isObject(content.data) ? content.data : null
        return Boolean(data?.isMeta)
            || Boolean(data?.isCompactSummary)
            || Boolean(data?.isSidechain)
    }
  • [Major] The POST scope limit is enforced only after parsing and processing an unbounded body - c.req.json() materializes the full payload, then every array entry is type-checked, trimmed, and deduplicated; the 256 KiB check runs later and only after IDs outside the namespace are discarded. An authenticated client can therefore send a very large duplicate/unknown-ID array, consume memory and CPU, and still pass the intended guard. Evidence: hub/src/web/routes/sessions.ts:207, hub/src/web/routes/sessions.ts:230, hub/src/web/routes/sessions.ts:145.
    Suggested fix:
    import { bodyLimit } from 'hono/body-limit'
    import {
        MAX_CONTENT_SEARCH_SESSION_SCOPE_BYTES,
        serializeContentSearchSessionIds
    } from '../../store/messageContentSearch'
    
    const contentSearchBodyLimit = bodyLimit({
        maxSize: MAX_CONTENT_SEARCH_SESSION_SCOPE_BYTES,
        onError: (c) => c.json({ error: 'Request body too large' }, 413)
    })
    
    app.post('/sessions/content-search', contentSearchBodyLimit, async (c) => {
        // existing handler
    })

Summary
Review mode: follow-up after new commits

The prior AGY planner-text finding is addressed. Two Major issues remain: Claude sidechain hits can be returned without a navigable chat anchor, and the new POST endpoint parses an unbounded request before applying its scope-size guard.

Testing
Not run (automation review; PR content was not executed per security policy). Add focused coverage that rejects isSidechain output from the search index and returns 413 for an oversized content-search POST before parsing.

HAPI Bot

Comment thread shared/src/messages.ts Outdated
function isHiddenAssistantOutput(content: unknown): boolean {
if (!isObject(content) || content.type !== 'output') return false
const data = isObject(content.data) ? content.data : null
return Boolean(data?.isMeta) || Boolean(data?.isCompactSummary)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Sidechain text can produce a search hit with no rendered anchor

This predicate excludes meta and compact-summary output, but Claude marks subagent-only output with data.isSidechain (web/src/chat/normalizeAgent.ts:231). The reducer removes those messages from the root timeline and emits them only through the parent subagent tool group (web/src/chat/reducer.ts:123). Because search navigation loads only 20 rows before the hit, a long sidechain can exclude the parent tool call, so this indexed message ID cannot be located in the rendered chat.

Suggested fix:

function isHiddenAssistantOutput(content: unknown): boolean {
    if (!isObject(content) || content.type !== 'output') return false
    const data = isObject(content.data) ? content.data : null
    return Boolean(data?.isMeta)
        || Boolean(data?.isCompactSummary)
        || Boolean(data?.isSidechain)
}

Comment thread hub/src/web/routes/sessions.ts Outdated
return engine
}

const body = await c.req.json().catch(() => null) as {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Enforce the scope limit before parsing the JSON body

c.req.json() materializes an unbounded payload before the route validates, trims, and deduplicates every sessionIds entry. The later 256 KiB check also runs after IDs outside the namespace are discarded, so a huge duplicate/unknown-ID array can consume memory and CPU while still passing the guard.

Suggested fix:

import { bodyLimit } from 'hono/body-limit'
import {
    MAX_CONTENT_SEARCH_SESSION_SCOPE_BYTES,
    serializeContentSearchSessionIds
} from '../../store/messageContentSearch'

const contentSearchBodyLimit = bodyLimit({
    maxSize: MAX_CONTENT_SEARCH_SESSION_SCOPE_BYTES,
    onError: (c) => c.json({ error: 'Request body too large' }, 413)
})

app.post('/sessions/content-search', contentSearchBodyLimit, async (c) => {
    // existing handler
})

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • None.

Summary
Review mode: follow-up after new commits

No new Blocker, Major, Minor, or Nit findings at the current head. The prior HAPI Bot findings on sidechain indexing and pre-parse request-size enforcement are addressed. Residual risk remains in the large SQLite FTS migration/backfill and timing-sensitive historical-scroll behavior; the PR includes focused coverage, but it was not independently executed in this automation review.

Testing
Not run (automation review; PR content was not executed per security policy).

HAPI Bot

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Bound short-query indexing work per message - each unique adjacent code-point pair causes a synchronous SQLite insert. User prompts are not truncated and the send schema has no text-size bound, so one large/high-entropy prompt or imported row can trigger O(n) database writes on the Hub event loop and during migration. Evidence hub/src/store/messageContentSearch.ts:279.

    Suggested fix:

    const MAX_SHORT_INDEX_CHARACTERS = 16_384
    
    function getBoundedShortSearchGrams(text: string): string[] {
        return getShortSearchGrams(
            Array.from(text).slice(0, MAX_SHORT_INDEX_CHARACTERS).join('')
        )
    }
    
    for (const gram of getBoundedShortSearchGrams(message.text)) {
        insertShortGram.run(gram, lookup.search_rowid)
    }

    Preferably store encoded grams as one FTS row per message rather than one ordinary-table row/write per gram; either approach must place a hard bound on per-message work.

  • [Minor] Normalize Markdown to rendered text before indexing - the index currently only collapses whitespace, so visible phrases split by Markdown syntax are absent from the trigram stream. For example, KV **Cache** renders as KV Cache, but searching for KV Cache will not match because the stored text contains the ** delimiters. Evidence shared/src/messages.ts:156.

    Suggested fix:

    import { fromMarkdown } from 'mdast-util-from-markdown'
    import { toString } from 'mdast-util-to-string'
    
    function normalizeSearchablePlainText(value: string): string | null {
        const text = toString(fromMarkdown(value)).trim().replace(/\s+/g, ' ')
        return text.length > 0 ? text : null
    }

Questions

  • None.

Summary

Review mode: follow-up after new commits

Two issues found at the current head: one Hub availability/storage risk in the two-character index write path, and one correctness gap between indexed Markdown source and visible rendered text. The previous HAPI Bot review and the compare from f9b670b... were used as context; the full latest PR diff was reviewed.

Testing

Not run (automation review; PR content was not executed). Add a bounded-work test with a large high-entropy user prompt, plus a backend search test proving KV **Cache** matches the visible query KV Cache.

HAPI Bot

Comment thread hub/src/store/messageContentSearch.ts Outdated
INSERT INTO ${MESSAGE_CONTENT_SEARCH_SHORT_TABLE} (gram, search_rowid)
VALUES (?, ?)
`)
for (const gram of getShortSearchGrams(message.text)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Bound short-query indexing work per message

This loop performs one synchronous SQLite insert for every unique adjacent code-point pair. Because user prompts are intentionally not truncated and the send schema accepts an unbounded string, one large/high-entropy prompt (or migration/import row) can trigger O(n) writes on the Hub event loop and inflate the derived table.

Suggested fix:

const MAX_SHORT_INDEX_CHARACTERS = 16_384
const boundedText = Array.from(message.text)
    .slice(0, MAX_SHORT_INDEX_CHARACTERS)
    .join('')

for (const gram of getShortSearchGrams(boundedText)) {
    insertShortGram.run(gram, lookup.search_rowid)
}

Preferably encode all grams into one FTS row per message; whichever representation is used, enforce a hard per-message work bound and cover it with a large high-entropy prompt test.

Comment thread shared/src/messages.ts
}

function normalizeSearchablePlainText(value: string): string | null {
const text = value.trim().replace(/\s+/g, ' ')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Normalize Markdown to rendered text before indexing

Whitespace normalization alone leaves Markdown delimiters in the trigram stream. A message such as KV **Cache** is visibly rendered as KV Cache, but the query KV Cache cannot match the indexed source because ** interrupts the phrase. This also undermines the new cross-Markdown-node highlighting path.

Suggested fix:

import { fromMarkdown } from 'mdast-util-from-markdown'
import { toString } from 'mdast-util-to-string'

function normalizeSearchablePlainText(value: string): string | null {
    const text = toString(fromMarkdown(value)).trim().replace(/\s+/g, ' ')
    return text.length > 0 ? text : null
}

Add a backend search test for KV **Cache** queried as KV Cache.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Minor] Index Claude user records that the renderer exposes as user messages — extractSearchableMessageText routes every outer role: agent/assistant envelope through assistant extraction, but extractAssistantPlainText rejects output/user. The web normalizer intentionally renders non-sidechain, all-text output/user arrays as ordinary user bubbles, so those visible prompts are omitted from both rebuilt and live content search. Evidence shared/src/messages.ts:237, web/src/chat/normalizeAgent.ts:371.

    Suggested fix:

    const data = isObject(record.content) && record.content.type === "output"
        && isObject(record.content.data) ? record.content.data : null
    const message = isObject(data?.message) ? data.message : null
    const blocks = Array.isArray(message?.content) ? message.content : null
    if (data?.type === "user" && !Boolean(data.isSidechain)
        && blocks?.length && blocks.every((block) =>
            isObject(block) && block.type === "text" && typeof block.text === "string"
        )) {
        const text = extractUserPlainText(blocks)
        return text ? { role: "user", text } : null
    }

Summary

Review mode: follow-up after new commits

One search-correctness issue remains at the current head. The previous HAPI Bot findings are no longer present; the full latest PR diff was reviewed, using the prior review and compare diff only as follow-up context.

Testing

Not run (automation review; PR content was not executed). Add shared extractor and Hub search regression tests for a non-sidechain output/user record whose content is an all-text array.

HAPI Bot

Comment thread shared/src/messages.ts
return text ? { role: 'user', text } : null
}

if (record.role === 'agent' || record.role === 'assistant') {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Index Claude user records rendered in the user lane

This branch treats every outer role: agent/assistant record as assistant prose. extractAssistantPlainText() returns null for content.type: output with data.type: user, while web/src/chat/normalizeAgent.ts:371-389 deliberately renders a non-sidechain, all-text array of that shape as a real user message. Consequently those visible prompts are absent from both index backfills and new-message search.

Suggested fix:

const data = isObject(record.content) && record.content.type === "output"
    && isObject(record.content.data) ? record.content.data : null
const message = isObject(data?.message) ? data.message : null
const blocks = Array.isArray(message?.content) ? message.content : null
if (data?.type === "user" && !Boolean(data.isSidechain)
    && blocks?.length && blocks.every((block) =>
        isObject(block) && block.type === "text" && typeof block.text === "string"
    )) {
    const text = extractUserPlainText(blocks)
    return text ? { role: "user", text } : null
}

Place this after the hidden-output check, before assistant extraction, and cover it with an extractor plus Hub search test.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Bound derived search text before FTS insertion - canonical user prompts are unbounded (shared/src/apiTypes.ts:556) and intentionally bypass agent-content truncation, but this added path inserts the full text into trigram FTS synchronously inside message writes and the startup backfill. A large pasted prompt can therefore stall the Hub and grow the derived DB index without a bound; MAX_SHORT_INDEX_CHARACTERS only limits the separate two-character table. Evidence hub/src/store/messageContentSearch.ts:280.

    Suggested fix:

    const MAX_INDEXED_MESSAGE_CHARACTERS = 16_384
    
    function boundSearchableText(text: string): string {
        if (text.length <= MAX_INDEXED_MESSAGE_CHARACTERS) return text
        const half = MAX_INDEXED_MESSAGE_CHARACTERS / 2
        return `${text.slice(0, half)} ${text.slice(-half)}`
    }
    
    const searchableText = boundSearchableText(message.text)
    // Insert searchableText into FTS and derive short grams from the same value.
  • [Minor] Clear the retained message target when the route session changes - SessionPage is reused for different $sessionId params, so this state survives A -> B navigation. Returning to A makes retainedMessageTarget valid again, re-enables skipInitialTailSync, and unexpectedly reopens the old historical match. Evidence web/src/router.tsx:367.

    Suggested fix:

    useEffect(() => {
        setConsumedMessageTarget(null)
    }, [sessionId])

Summary

Review mode: follow-up after new commits

Two issues remain at the current head. The previous HAPI Bot finding about visible Claude user records is resolved by the latest commit; the full latest PR diff was reviewed, with the prior review and compare diff used only as follow-up context.

Testing

Not run (automation review; PR content was not executed). Add a regression test with a multi-megabyte user prompt that asserts bounded FTS/short-index work, and a router test for content result A -> session B -> session A without restoring the consumed target.

HAPI Bot

Comment thread hub/src/store/messageContentSearch.ts Outdated
) VALUES (?, ?, ?, ?, ?, ?, ?)
`).run(
lookup.search_rowid,
message.text,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Bound the text stored in the trigram index

message.text can contain an arbitrarily large user prompt: SendMessageRequestSchema.text has no maximum, and the content codec deliberately leaves user messages untruncated. This line therefore tokenizes and stores the entire payload synchronously inside canonical message transactions and the migration backfill. The 16,384-character cap below only protects the two-character side table, not FTS.

Suggested fix:

const MAX_INDEXED_MESSAGE_CHARACTERS = 16_384

function boundSearchableText(text: string): string {
    if (text.length <= MAX_INDEXED_MESSAGE_CHARACTERS) return text
    const half = MAX_INDEXED_MESSAGE_CHARACTERS / 2
    return `${text.slice(0, half)} ${text.slice(-half)}`
}

const searchableText = boundSearchableText(message.text)
// Use searchableText for both the FTS row and short grams.

Comment thread web/src/router.tsx Outdated
messageId: string
messageQuery?: string
} | null>(null)
const retainedMessageTarget = consumedMessageTarget?.sessionId === sessionId

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Drop the consumed target when leaving the session

TanStack reuses SessionPage while only $sessionId changes. This retained state is merely ignored on session B, then becomes valid again when navigating back to session A, which reactivates skipInitialTailSync and reopens the old historical search hit.

Suggested fix:

useEffect(() => {
    setConsumedMessageTarget(null)
}, [sessionId])

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Bound derived search text before FTS insertion — user prompts are unbounded and this path synchronously tokenizes/stores the full text during message writes and migration backfill. Evidence hub/src/store/messageContentSearch.ts:280.
    Suggested fix:

    const MAX_INDEXED_MESSAGE_CHARACTERS = 16_384
    const searchableText = boundSearchableText(message.text)
    // Use searchableText for FTS and short grams.
  • [Minor] Clear the retained target when the route session changes — SessionPage survives parameter-only navigation, so returning to session A can reactivate its consumed historical target and skip the normal tail sync. Evidence web/src/router.tsx:367.
    Suggested fix:

    useEffect(() => {
        setConsumedMessageTarget(null)
    }, [sessionId])

Summary

Review mode: follow-up after new commits

Two unresolved issues remain at the latest head. Commits since the previous HAPI Bot review only changed version/build metadata; the full latest PR diff was reviewed, with the previous review and compare diff used only as follow-up context.

Testing

Not run (automation). Add a regression test with a multi-megabyte user prompt that asserts bounded FTS work, and a router test for content result A → session B → session A without restoring the consumed target.

HAPI Bot

Comment thread hub/src/store/messageContentSearch.ts Outdated
) VALUES (?, ?, ?, ?, ?, ?, ?)
`).run(
lookup.search_rowid,
message.text,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Bound the text stored in the trigram index

message.text can contain an arbitrarily large user prompt: the send schema has no maximum and user content is intentionally not truncated. This line therefore tokenizes and stores the entire payload synchronously inside canonical message transactions and the startup backfill. MAX_SHORT_INDEX_CHARACTERS only protects the separate two-character table.

Suggested fix:

const MAX_INDEXED_MESSAGE_CHARACTERS = 16_384

function boundSearchableText(text: string): string {
    if (text.length <= MAX_INDEXED_MESSAGE_CHARACTERS) return text
    const half = MAX_INDEXED_MESSAGE_CHARACTERS / 2
    return `${text.slice(0, half)} ${text.slice(-half)}`
}

const searchableText = boundSearchableText(message.text)
// Use searchableText for both the FTS row and short grams.

Comment thread web/src/router.tsx Outdated
messageId: string
messageQuery?: string
} | null>(null)
const retainedMessageTarget = consumedMessageTarget?.sessionId === sessionId

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Drop the consumed target when leaving the session

TanStack reuses SessionPage while only $sessionId changes. This state is ignored on session B but remains stored; navigating back to session A makes it valid again, re-enables skipInitialTailSync, and reopens the old historical search hit.

Suggested fix:

useEffect(() => {
    setConsumedMessageTarget(null)
}, [sessionId])

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Avoid rebuilding the FTS and short-query indexes for every cumulative live stream snapshot — Pi emits cumulative snapshots every 250 ms, while the new write path deletes and reinserts the full growing text synchronously for each one. See the inline comment at hub/src/store/messageContentSearch.ts:321.
    Suggested fix:

    if (isLiveStreamSnapshot(message.content)) return
    // Index the explicit terminal snapshot once.
  • [Minor] Preserve both ends when bounding indexed text — the current prefix-only slice makes visible text after character 16,384 permanently unsearchable, including the tail intentionally retained by the message codec. See the inline comment at hub/src/store/messageContentSearch.ts:120.
    Suggested fix:

    return `${safeHead(text, half)} ${safeTail(text, half)}`

Summary

Review mode: follow-up after new commits

The two findings from the prior HAPI Bot pass no longer reproduce at this head. The full latest diff still has one Major streaming-write amplification issue and one Minor long-message search gap.

Testing

Not run (automation; PR code was not executed). Add a regression test that feeds many cumulative snapshots for one render key and asserts only the terminal snapshot rebuilds the derived indexes, plus a long-message test with the search needle in the retained tail.

HAPI Bot

const renderKey = extractMessageRenderKey(message.content)
const searchKey = renderKey ? `${message.sessionId}:${renderKey}` : message.id
removeMessageContentSearchIndex(db, message.id)
if (searchKey !== message.id) removeMessageContentSearchIndexByKey(db, searchKey)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Avoid rebuilding both indexes for every live stream snapshot

indexMessageContent runs inside every canonical message insert. Pi emits cumulative text snapshots every 250 ms (cli/src/pi/piMessageAccumulator.ts:6,136-166). For one render key, this path deletes the previous FTS/gram rows and parses/reinserts the entire growing text on every snapshot. Coalescing prevents duplicate search results, but it does not prevent write amplification: a long or high-entropy answer can cause thousands of synchronous short-index writes every 250 ms and block other Hub SQLite work.

Suggested fix:

const stream = getMessageStreamState(message.content)
if (stream?.snapshot && !stream.final) {
    return
}
// Replace the derived row once for the terminal snapshot.

Carry an explicit terminal flag through the wire format, and make the accumulator emit a terminal snapshot even when its text equals the last live snapshot.

Comment thread hub/src/store/messageContentSearch.ts Outdated
// allocate an Array for its entire contents just to index its prefix. The
// cap is intentionally expressed in UTF-16 code units to keep this work
// bounded even for messages containing only astral characters.
let boundedText = text.slice(0, MAX_INDEXED_MESSAGE_CHARACTERS)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Keep the retained tail searchable when applying the size cap

This prefix-only slice makes every phrase after character 16,384 invisible to search. User prompts are intentionally unbounded, and agent strings are stored with a head+tail truncation (hub/src/store/contentCodec.ts:8-20), so text that is still visible at the end of a message cannot be found.

Suggested fix:

const half = Math.floor((MAX_INDEXED_MESSAGE_CHARACTERS - 1) / 2)
let head = text.slice(0, half)
let tail = text.slice(-half)
if (/^[\uDC00-\uDFFF]/.test(tail)) tail = tail.slice(1)
if (/[\uD800-\uDBFF]$/.test(head)) head = head.slice(0, -1)
return `${head} ${tail}`

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Pi answers can remain permanently absent from content search after normal completion — the new index guard skips all live snapshots, but PiMessageAccumulator still suppresses the terminal snapshot when its text equals the last live snapshot. See the inline comment at hub/src/store/messageContentSearch.ts:344; related context cli/src/pi/piMessageAccumulator.ts:159.
    Suggested fix:
    const unchanged = segment.text === segment.lastSnapshot
    if (!segment.text.trim() || (unchanged && (live || kind !== 'text'))) continue

Summary

Review mode: follow-up after new commits

The prior long-message tail gap no longer reproduces. The live-snapshot write-amplification change remains incomplete because the producer does not guarantee the non-live terminal row that the index now relies on. No other findings met the 80% confidence threshold in the full current diff.

Testing

Not run (automation; PR code was not executed). Add a regression where a live snapshot contains the complete final text, message_end arrives without another delta, and the accumulator still emits a non-live terminal snapshot; then assert that row becomes searchable.

HAPI Bot

// Pi emits cumulative live snapshots every 250 ms. They are deliberately
// not indexed; the explicit terminal snapshot carries the same render key
// and is indexed once when the stream finishes.
if (isLiveStreamSnapshot(message.content)) return

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] Ensure Pi emits the terminal row that this guard relies on

This now drops every live: true snapshot, but Pi does not always emit a later non-live copy. PiMessageAccumulator.addSnapshots skips unchanged text (cli/src/pi/piMessageAccumulator.ts:159), so when the last throttled live snapshot already contains the complete answer, message_end flushes nothing. The completed answer then has no indexed row and remains unsearchable indefinitely.

Suggested fix:

const unchanged = segment.text === segment.lastSnapshot
if (!segment.text.trim() || (unchanged && (live || kind !== 'text'))) continue

This preserves live deduplication and reasoning behavior while forcing one terminal text snapshot. Add a test where message_end follows an unchanged live snapshot.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Minor] Preserve sidebar scroll when clearing the content-search target — both new same-route replace navigations omit the repository navigation flag that disables router-wide scroll restoration, so consuming or dismissing a hit can reset the session sidebar and lose the result position. Evidence web/src/router.tsx:787; related context web/src/lib/sessionNavigation.ts:6.
    Suggested fix:
    navigate({
        to: "/sessions/$sessionId",
        params: { sessionId },
        replace: true,
        ...PRESERVE_SESSION_SIDEBAR_SCROLL,
    })
    Apply the same option in handleSearchTargetDismissed.

Summary

Review mode: follow-up after new commits

One Minor navigation regression found. The previous Pi terminal-snapshot indexing issue is resolved by the new non-live terminal emission and regression test. No other findings met the 80% confidence threshold in the full current diff.

Testing

Not run (automation; PR code was not executed). GitHub test and integration checks passed. Add a router/navigation test asserting resetScroll: false for both search-target consumption and dismissal.

HAPI Bot

Comment thread web/src/router.tsx
navigate({
to: '/sessions/$sessionId',
params: { sessionId },
replace: true,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Preserve the sidebar position during URL cleanup

The result-selection navigation already spreads PRESERVE_SESSION_SIDEBAR_SCROLL, and the helper documents why internal /sessions navigation must set resetScroll: false. These two new replace navigations omit it, so after the target is located (or dismissed), router scroll restoration can reset the still-mounted sidebar and lose the user position in the content-search results.

Suggested fix:

navigate({
    to: "/sessions/$sessionId",
    params: { sessionId },
    replace: true,
    ...PRESERVE_SESSION_SIDEBAR_SCROLL,
})

Apply the same addition in handleSearchTargetDismissed.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • None.

Questions

  • None.

Summary

Review mode: follow-up after new commits

No issues found at the current head. The prior Minor sidebar-scroll regression is fixed in web/src/router.tsx:789 and web/src/router.tsx:799. Residual risk is concentrated in production-scale index migration/storage and browser-specific historical-scroll behavior; no additional testing gap was identified beyond the PR’s existing coverage.

Testing

  • Not run (automation review; PR code was not executed).
  • GitHub test and integration checks passed.

HAPI Bot

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • None.

Questions

  • None.

Summary

Review mode: follow-up after new commits

No issues found at the current head. Residual risk remains around the schema-v27 FTS rebuild cost on unusually large production databases and browser-specific historical search jumps/scroll anchoring; the PR includes targeted migration, store, UI, and end-to-end coverage for these areas.

Testing

  • Not run (automation review; PR code was not executed).
  • GitHub checks observed: build, drift-gate, integration, and test passed; pr-review was pending during review.

HAPI Bot

…search

# Conflicts:
#	cli/src/agent/messageConverter.ts
#	hub/src/store/index.ts
#	hub/src/store/messages.test.ts
#	hub/src/store/messages.ts
#	hub/src/store/migration-v13.test.ts
#	hub/src/store/migration-v15.test.ts
#	hub/src/store/migration-v18.test.ts
#	hub/src/store/migration-v19.test.ts
#	hub/src/store/migration-v20.test.ts
#	hub/src/store/migration-v23.test.ts
#	hub/src/store/migration-v24.test.ts
#	shared/src/messages.test.ts
#	shared/src/messages.ts
#	web/src/components/SessionList.tsx

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • None.

Questions

  • None.

Summary

Review mode: follow-up after new commits

No issues found at the current head. Residual risk remains around the synchronous schema-v28 FTS/short-index backfill on unusually large databases and browser-specific historical jump/scroll behavior; the PR includes targeted migration, store, component, and end-to-end coverage.

Testing

  • Not run locally (automation review; Bun is unavailable in this runner).
  • GitHub checks observed: build, drift-gate, integration, and test passed.

HAPI Bot

…search

# Conflicts:
#	web/src/lib/assistant-runtime.ts

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Minor] Pi error turns can remain unindexed — when the last live snapshot already contains the final partial response, the error path suppresses the terminal snapshot. The live text remains visible in chat, but the content index deliberately ignores live snapshots, so that visible response cannot be found. Evidence: cli/src/pi/piMessageAccumulator.ts:92; related index guard: hub/src/store/messageContentSearch.ts:340.

    Suggested fix:

    const messages = this.flush()

Questions

  • None.

Summary

Review mode: follow-up after new commits

One minor correctness issue found: Pi responses that fail immediately after a final live snapshot are visible but absent from message-content search. Residual risk remains around synchronous large-database index migration and browser-specific historical scroll behavior.

Testing

  • Not run (automation review; PR code was not executed).
  • GitHub checks observed: build, drift-gate, integration, and test passed.
  • Missing regression coverage: Pi live snapshot → error boundary → terminal searchable content.

HAPI Bot

Comment thread cli/src/pi/piMessageAccumulator.ts Outdated
const wasActive = this.active;
const messages = this.flush();
const errorMessage = 'message' in event ? extractPiTurnError(event.message) : null;
const messages = this.flush(errorMessage === null);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Error-finalized Pi text is skipped by content search

When errorMessage is non-null, flush(false) suppresses an unchanged terminal text snapshot. That is the common case when the last throttled live snapshot already contains the partial answer. The live row is still rendered, but indexMessageContent explicitly skips live snapshots, so the visible failed response never becomes searchable.

Suggested fix:

const messages = this.flush()

Please update the error-boundary test to expect the terminal text followed by the error, and add coverage that this terminal row reaches content search.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • None.

Questions

  • None.

Summary

Review mode: follow-up after new commits

No issues found at the latest head. The prior Pi error-finalization indexing issue is addressed by the new terminal snapshot behavior and regression coverage. Residual risk remains around synchronous v25→v28 migration and common-query latency on very large databases.

Testing

  • Not run (automation review; PR code was not executed).
  • GitHub checks observed: build, drift-gate, integration, and test passed.
  • Large-database migration/search load test: Not found in repo/docs.

HAPI Bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(web,hub): add opt-in search for session message content

1 participant