The local grep service: one newest-first file walk, sidecar-or-scan per file (LLP 0265 T4)

[bgmcmullen]

What this is

LLP 0265 T4, the wave-2 task: the local grep-search service, built on T1 (hypgrep, #877) and T3 (the shared core/search modules, #876). After this, the engine hyp query grep needs exists and is fully tested; T5 (the verb) is what makes it user-visible.

The shape

executeGrepSearch in src/core/search/grep_service.js mirrors the server's src/search/grep-search.js tier for tier, folded onto the client's single cache:

1. Freshness first. The dataset's pending spool is flushed through settlePendingCacheForQuery, now exported from sql.js so the two read surfaces share one flush policy. The settle list includes the gateway's label spool table (proxy_messages_v5), which partition discovery cannot see before its first flush - without that, a row captured seconds ago would be unsearchable.
2. One walk, newest message-day first, across every live data file of every source partition, day taken from each file's identity-partition value, so a truncated answer keeps the newest matches whichever client wrote them.
3. Per file, sidecar or scan. A file with a hypgrep .index.parquet beside it is served through parquetFind (the index proposes candidate blocks, the shared matcher confirms); a file without one is brute-scanned under the narrow SCAN_COLUMNS projection. Sidecar existence is the marker, no ledger (LLP 0264 #lifecycle). No sidecar exists anywhere until T6 lands, so today every file takes the scan tier: slower, never wrong.
4. Budget and shape as the server. Collect limit+1, report truncated/exhausted, sort newest-first with the same tiebreaks, project hits through the shared allowlist so content_text leads the snippets.

Unlike the server there is no cross-tier day-exclusion rule: a client row lives in exactly one file and each file is served by exactly one tier, so double-counting is impossible by construction.

Two row gates the server does not need

• Purge (LLP 0104). A raw file read does not apply Iceberg position deletes. The walk carries each file's committed delete positions (new listLiveDataFiles export on the iceberg store, reusing the purge path's own delete-map loader) and filters both tiers by row position - parquetFind yields __index__ for exactly this. A stale sidecar built before a purge cannot resurrect the purged row.
• Visibility (LLP 0105). Every surfaced row passes the lattice check through cwdWithheldFromCaller, hoisted out of withLocalOnlyVisibility so the SQL path and this walk share one predicate instead of two drifting copies. The check runs after the match predicate, so localOnly.withheldRows counts hits the caller was not allowed to see (the number T5 will render as guidance) and a withheld row consumes no result budget.

One deliberate deviation from the plan's sketch

The plan sketched "per-partition source from storage.dataSourceForTable wrapped in withLocalOnlyVisibility". A table-level AsyncDataSource cannot deliver two things the same sentence also requires: the newest-day-first walk (a table scan yields rows in file order, so a truncated answer would keep the oldest matches) and per-file tiering (an indexed file must be excluded from the table scan or its rows are counted twice). So the service walks files directly and carries the two guarantees the wrapper would have provided - position deletes and the visibility lattice - explicitly, each through the same shared machinery the wrapped path uses (loadDeletedPositions, cwdWithheldFromCaller). What the plan's verification note was really pinning (purge-correct, visibility-correct rows, no reimplemented policy) still holds, and the tests prove it row for row.

The test that proves it

test/core/search-grep-service.test.js, 14 tests over a real Iceberg cache built with the gateway's own partition declaration:

• hit locators, snippets, newest-first order, truncation keeping the newest match, exhausted semantics
• from/to pruning at the file walk, session/chain scoping, case-insensitive literal and regex modes
• the JSON tool_args column producing a hit through cellText (the drift T3's shared matcher exists to prevent)
• local-only withholding at every caller rank: withheld from full, visible to an equal-rank caller, fail-closed with no derivable cwd, surfaced by the override
• purge on both tiers, including a sidecar built before the purge
• the indexed tier answering identically to the scan tier, and a sub-ngram query (dle) still answering exactly through the index (the T7 "literal cliff" is performance, not truth)
• spool-captured rows surfacing after the service's own freshness flush
• empty-cache and refused-query edges

Verification

• node --test test/core/search-grep-service.test.js: 14/14.
• npm run typecheck: clean.
• npm test: green except two failures that reproduce identically on a clean origin/master worktree on this machine and are unrelated: command-dispatch help-text (a graph command registers locally but not in CI) and the LLP duplicate-number lint tripping on an untracked local draft (llp/0266-local-inference-capture-lane.decision.md, not part of any commit).

🤖 Generated with Claude Code

[philcunliffe]

neutral review: changes requested (fixes pushed)

Reviewed git diff origin/master...f49824b0 (store.js, sql.js, visibility.js, grep_service.js, types.d.ts, the new test). Every finding below was reproduced with a throwaway probe against a real Iceberg cache before it was written down. Six of seven are fixed and pushed as 8b694e5b; the seventh is a design question the T6 author has to answer.

Fixed

1. HIGH - src/core/search/grep_service.js:286 - --limit returned the OLDEST matches. if (truncated) hits.length = limit ran before sortHits(hits), so the cut fell in walk order rather than sort order. Files are walked newest-day-first, but rows inside a file are in write order (LLP 0022 clusters by session, so a session's rows run oldest to newest) and one day is many files. Reproduced: ten m1..m10 rows in one file, limit: 100 answers m10..m1, limit: 3 answers m3,m2,m1. That is the common case for hyp query grep (today's session, capped limit) and it contradicts the module docstring ("a truncated answer keeps the newest matches") and sortHits' own comment. Fixed: hits are trimmed in sort order through an amortized trimHits (buffer stays bounded by the budget), and the file walk now stops only once every file still ahead is strictly older than the oldest kept hit, instead of at the first file that filled the budget.

2. MEDIUM - grep_service.js:403 - a deadline abort threw instead of returning a partial answer. isAbort accepted only err.name === 'AbortError'. ABORT_CHECK_ROWS' own docstring says the signal exists so "the deadline has to be able to land in the middle of a file", and the natural deadline is AbortSignal.timeout(ms), whose reason is a DOMException named TimeoutError. Reproduced: AbortSignal.timeout() threw TimeoutError out of executeGrepSearch, while a plain AbortController.abort() returned correctly. Fixed: isAbort(err, signal) also accepts identity against signal.reason, which covers every abort shape (including a custom abort(reason)) without swallowing an unrelated failure that races the deadline.

3. MEDIUM - grep_service.js:377 - chainId was silently ignored without sessionId. compileChainPredicate returned () => true whenever sessionId === undefined, discarding a caller-supplied chainId. GrepSearchParams declares both independently optional, so {query, chainId: 'a2'} returned hits across every session. Reproduced. Fixed: each predicate is applied independently.

4. MEDIUM - src/core/cache/iceberg/store.js:473 - corrupt table metadata degraded to "no matches". listLiveDataFiles wrapped loadLatestFileCatalogMetadata in a bare catch { return [] }, and the docstring claimed parity with dataSourceForTable. It is not parity: dataSourceForTable (same file, ~line 598) does not catch that call and returns null only for a snapshot-less table. So a truncated .metadata.json made hyp query sql fail loudly while hyp query grep reported zero hits over that partition, the fail-open polarity this codebase rejects elsewhere (cf. the LocalOnlyListUnreadableError note added in visibility.js in this same PR). Fixed: the load error propagates; the missing/snapshot-less cases still return [].

5. LOW - grep_service.js:109 - limit was unvalidated while query was. limit: -1 reached hits.length = -1 and threw a bare RangeError: Invalid array length from the result assembly; limit: undefined made budget NaN, so the walk never stopped and collected every match in the cache. Since GrepSearchParams is the wire shape the grep_search tool hands straight through, this is validated beside the query now.

6. LOW (follow-on) - sortHits never returned 0. Its tiebreak returned -1 for fully equal keys. Harmless when the array was sorted once, an inconsistent comparator now that the trim sorts the buffer repeatedly. Fixed.

Five regression tests were added to test/core/search-grep-service.test.js, and each was verified to fail against the pre-fix tree (tests 3, 4, 5, 18, 19) and pass after. npm test 4624 pass / 0 fail and npm run typecheck are green in a clean worktree install.

Not fixed: needs a decision before T6

HIGH - grep_service.js:224 - the indexed tier cannot match tool_args, so the two tiers give different answers. hypgrep's createIndex indexes only leaf columns whose parquet type is converted_type: UTF8 / logical_type.type === 'STRING' (hypgrep/src/utils.js:getTextColumnsFromSchema). Confirmed against a real cache file: tool_args writes as a VARIANT group with two BYTE_ARRAY leaves (metadata, value) and no string annotation, so it is never in the sidecar. A block whose only match is in tool_args is never proposed, and rowFilter: accept never sees the row. Reproduced: a row with tool_args: {file_path: '/repo/hidden_needle_path.js'} is found by the scan tier and returns zero once a sidecar exists. It only appears to work for queries the index cannot prune (a sub-ngram literal, which proposes every block).

This breaks LLP 0264 #lifecycle's stated invariant ("index state is never a correctness input") and matcher.js' own claim that "the indexed tier, which reads the column's own text, answers otherwise". It is dormant today because nothing writes sidecars, but LLP 0265 T6 says the build runs "over the allowlist columns", and createIndex will in fact throw on textColumns: [...SEARCHABLE_COLUMNS] (textColumns not found as string columns in source: tool_args). So T6 cannot avoid confronting it, and the failure mode if it is papered over is silent: grepping for a file path or a shell command inside a tool call answers nothing.

I did not push a fix because the three available resolutions have materially different performance contracts and the choice belongs in the LLP, not in a reviewer's patch:

• fall back to the scan tier whenever the sidecar does not cover every searchable column, which is exact but makes the indexed tier dead code as long as tool_args is in the allowlist;
• keep the index for block pruning and add a supplementary narrow scan of the uncovered columns per indexed file, which is exact and keeps most of the win (content_text is the bulk) at the cost of a second projected decode and a dedupe;
• drop tool_args from SEARCHABLE_COLUMNS, which is a shared-constant change the server has to agree to.

LOW (noted, not fixed) - grep_service.js:233 - the sidecar buys decode savings only. asyncBufferFactory: async ({url}) => await io.reader(url) resolves to createLocalIcebergIO().resolver.reader, which is fs.readFileSync of the whole file (src/core/cache/iceberg/resolver.js:26). Both the data file and the sidecar are slurped in full, synchronously, before any block pruning, so the index's IO win is zero and the daemon's event loop stalls once per walked file. A range-reading buffer belongs with the T7 performance work rather than in this PR.

[philcunliffe]

Review: PR #951, the local grep service (LLP 0265 T4)

Verdict: request changes. The tier structure, purge gate, visibility gate, sort-order truncation and abort handling are all correct and well argued. Two defects in how the indexed tier is entered are fixed and pushed. One HIGH finding is real, reproduced, and needs a human decision (it cannot be fixed inside T4 without killing the indexed tier outright). Three perf/robustness findings are deferred with reasons.

Baseline: npm test green (4624 pass) at 8b694e5b, and green (4626 pass) plus npm run typecheck clean after the fixes.

---

HIGH, DEFERRED, needs a decision: the indexed tier silently loses every tool_args match

src/core/search/grep_service.js:259 (parquetFind call), against src/core/search/searchable_columns.js:36 (tool_args in SEARCHABLE_COLUMNS).

tool_args is declared JSON, which src/core/cache/iceberg/schema.js:194 maps to Iceberg variant, a binary physical column. hypgrep's getTextColumnsFromSchema (hypgrep/src/utils.js:25) only indexes leaves whose converted_type is UTF8 or whose logical_type.type is STRING, so a sidecar over an ai_gateway_messages data file never carries tool_args n-grams. queryIndex then prunes to zero candidate blocks and rowFilter: accept is never consulted, so matcher.rowTest's cellText coercion (the entire reason cellText exists, matcher.js:141) cannot rescue the row.

Reproduced on this branch, one row with tool_args: { file_path: '/repo/hidden_needle_path.js' }:

• sidecar hypgrep.text_columns = session_id,conversation_id,agent_id,model,cwd,git_branch,git_remote,tool_name,content_text,date,part_id,message_id,client_name (no tool_args)
• scan tier: 1 hit; same cache with sidecars built: 0 hits, indexedFiles: 1

This directly violates LLP 0264 #lifecycle ("index state is never a correctness input") and LLP 0264 #shared ("a column in this set that cannot produce a hit is worse than one that is absent from it"). The PR's own coverage does not catch it: 'the JSON column matches through cellText...' (test/core/search-grep-service.test.js:241) only runs the scan tier, and 'sidecars serve every file with identical hits' (line 306) matches on content_text.

Why I did not fix it here. The mechanical fix is a coverage gate: if the sidecar's hypgrep.text_columns does not cover SEARCHABLE_COLUMNS, brute-scan that file. But tool_args is unindexable by hypgrep 0.5.1 (createIndex throws textColumns not found as string columns in source: tool_args when handed the allowlist explicitly, createIndex.js:60). So a full-coverage gate rejects every sidecar this schema can ever produce, permanently disabling the indexed tier and making T6 pointless. That is a design choice, not a cleanup.

Note this is latent, not live: T6 does not exist yet, no sidecars are written, and today every file takes the scan tier. It becomes live the moment T6 lands, and it fails silently (null results, not an error), which is the worst failure shape.

The decision a human owes T6 (any one of these closes it):

1. tool_args moves out of SEARCHABLE_COLUMNS for the indexed tier, and the coverage caveat is documented in the verb summary and SKILL.md (T7 already owns a coverage clause). Cheapest; makes the shared-allowlist promise conditional, which LLP 0264 #shared explicitly refuses.
2. The client stores tool_args as STRING on ai_gateway_messages rather than JSON, making it indexable. A schema change with its own migration cost, and it changes what cellText is for.
3. hypgrep gains variant/JSON column support upstream, and T1's pin moves. Durable, out of tree, slowest.
4. T4 gates on coverage and the indexed tier is accepted as dead for this dataset. Correct, but then T6 should not be built.

Whichever is chosen, T4 should end up with the coverage gate so the invariant is enforced rather than assumed.

---

MEDIUM, FIXED: a poisoned sidecar failed the whole search instead of degrading one file

src/core/search/grep_service.js:246. The comment promised "degrade this one file to the scan tier if the read races a delete", but the try/catch wrapped only io.reader(sidecarUrl) (a readFileSync, so effectively only the ENOENT race) and swallowed only ENOENT. A sidecar that exists but is unparseable (a partial write from a killed T6 build, a disk-full truncation, an index version the installed hypgrep refuses) throws from inside parquetFind -> queryIndex -> parquetMetadataAsync, outside any handler.

Reproduced: writing PAR1 truncated garbage over one sidecar made executeGrepSearch reject with parquet file invalid (footer != PAR1). One bad sidecar took down every query over the entire cache, including partitions the walk never reached, which is exactly index state acting as a correctness input.

Fix (b590b110): the indexed read is hoisted into searchIndexed, which returns false when the index proves unusable before it produced a row, handing the file to the scan tier. A failure after the first row still propagates, because retrying it as a scan would double-count the hits already collected. The reader catch no longer special-cases ENOENT: any unreadable sidecar is an unindexed file. Aborts are re-thrown in both places via the existing isAbort. indexedFiles is now incremented on success, so a degraded file counts as scanned.

Two tests added (test/core/search-grep-service.test.js), both verified red before the fix and green after:

• indexed tier: a poisoned sidecar degrades that file, it does not fail the search (truncated footer)
• indexed tier: an unreadable sidecar degrades that file rather than throwing (EISDIR, not the ENOENT race)

---

LOW, FIXED: file-walk comparator never returned 0

src/core/search/grep_service.js:207. files.sort((a, b) => (a.day < b.day ? 1 : -1)) returns -1 for both cmp(a,b) and cmp(b,a) when days are equal. Its sibling sortHits guards exactly this case with an explicit comment (line 373). One message-day is many files and the early-break optimization at line 304 reads the walk as strictly day-descending, so the invariant rested on implementation-defined sort behavior rather than on the comparator. I fuzzed V8's sort and found no ordering violation today, so this was latent, not broken. Fixed to return 0 on equal days.

---

MEDIUM, DEFERRED: the scan tier materializes a whole data file with no heap guard

src/core/search/grep_service.js:279. await parquetReadObjects({ file: sourceFile, columns: SCAN_COLUMNS }) decodes the entire file into one JS array. src/core/cache/maintenance.js:50 sets target_file_bytes: 128 * 1024 * 1024 and the same file notes text compressing ~70x, so one compacted file can decode to multiple GB of strings at once. The SQL read path deliberately avoids this (scanRowsFromTable exists "so callers ... never materialize the full table in memory") and executeQuerySql additionally runs a heap-growth watchdog. The docstring's "the request's memory bound is one data file plus its index" is literally true but the bound is far larger than it reads.

Deferred: the fix is chunked reads over row-group boundaries (rowStart/rowEnd), which has to keep the absolute row index intact for position-delete filtering and keep the ABORT_CHECK_ROWS cadence. That is a perf refactor with real regression surface on a branch that has #952 and #953 stacked on it, not a review-rung cleanup. Worth its own task; LLP 0265 #verified already flags "the walk cost of a 90-day cache under the narrow projection (T4 measures it)" as unverified.

LOW, DEFERRED: the indexed tier still reads the whole source parquet

src/core/search/grep_service.js:263. asyncBufferFactory: async ({ url }) => await io.reader(url) resolves to createLocalIcebergIO's reader, which is fs.readFileSync(urlToPath(url)): it ignores the byteLength hint parquetFind passes and reads the whole file synchronously. So block pruning saves decode but not IO, and in the daemon each file read stalls the event loop. Given T4's stated purpose is that a sidecar-indexed file is faster in wall clock, a range-capable reader is what makes the tier worth having. Deferred: it belongs with the chunked-read work above and touches resolver.js, shared with the SQL path.

LOW, DEFERRED (arguably not a defect): ENOENT on a data file fails the search

src/core/search/grep_service.js:278. listLiveDataFiles snapshots the file list up front; if maintenance.js's generation sweep deletes a retired table-<seq> dir mid-walk, io.reader(file.filePath) throws ENOENT and the whole grep fails. I am not treating this as a bug: src/core/cache/iceberg/store.js documents the polarity deliberately ("a reader that swallows it reports 'no matches' over a partition it never read ... grep search must not answer zero where SQL raises"), and the SQL path races the same sweep with the same loud failure. Silently skipping would break that parity. Recording it so the next reviewer does not re-find it.

---

Checked and found correct

• __index__ from parquetFind is range.rowStart + i, the absolute source row position, so it lines up with Iceberg position deletes; the purge and stale-sidecar tests are genuine, not self-agreeing.
• cwd is in SEARCHABLE_COLUMNS and therefore in SCAN_COLUMNS, and parquetFind reads full rows, so the LLP 0105 gate cannot fail open on either tier. Withholding runs after the match predicate, so withheldRows counts real hits and an out-of-rank row consumes no budget, as the docstring claims.
• isAbort identity-checks signal.reason, which correctly handles AbortSignal.timeout's TimeoutError (a name check alone would have turned the documented partial answer into a thrown error for the one abort shape the service exists to serve).
• Sort-order truncation with amortized trimming, and the day-descending early break, are right.
• Case-insensitivity survives index pruning (hypgrep lowercases regex literals under i).
• The settle list (spool label tables plus committed partitions) does not double-count, and the label table lands in only one settle list, so no duplicate freshness messages.
• settlePendingCacheForQuery extraction, cwdWithheldFromCaller extraction, and listLiveDataFiles' degradation contract all match their SQL-path counterparts.

Pushed

b590b110 on grep/t4-grep-service. npm test 4626 pass / 0 fail, npm run typecheck clean.

[philcunliffe]

neutral triage: residual findings deferred, PR clear to merge

Both review rounds are exhausted with findings still open, so triage classified each residual at head b590b1102ca0a7d78ea452fc11b915cac239bfd6 against the committed tree.

All nine fixed findings from the two review rounds were verified present at this head (sort-order truncation via trimHits, isAbort accepting signal.reason, independent chainId/sessionId predicates, listLiveDataFiles propagating metadata errors, limit validation, both comparators returning 0 on equal keys, and the poisoned-sidecar degradation in searchIndexed), and test/core/search-grep-service.test.js passes 21/21.

Four findings remain open. None blocks this merge: executeGrepSearch has no caller outside its own module at this head (T5 wires the verb), and the HIGH tool_args indexed-tier gap additionally requires T6-built sidecars to exist before it can misbehave, with T6 forced to confront it because createIndex throws on the explicit allowlist. The MEDIUM heap-guard and LOW range-reader findings are perf work already pointed at T7 or their own task, and the ENOENT polarity is correct by design.

The residuals are enumerated with file:line evidence in #971, including the decision the HIGH item needs from a human before T6.

[bgmcmullen]

Superseded by #984, which carries this work (and every review fix from this PR) on a single branch cut from current master.

The stack could not be merged in sequence: this repo squash-merges, so merging one PR put a commit on master sharing no history with the branches behind it, and each remaining PR then re-proposed its parent's changes. Verified by simulation: merging #951 and then #953 produced add/add conflicts in grep_service.js and search-grep-service.test.js.

Integrating on current master also surfaced two problems invisible here: a sidecarPathFor import left dangling when #953's review moved the function, and master's new D1 short-flag gate (LLP 0293), which query grep trips as the second verb with a greedy positional. Both are fixed in #984.

The branch is retained, so this can be reopened if needed.

This PR's task: T4, the local grep service.