Follow-up: deferred review findings from PR #951

[philcunliffe]

Deferred review findings from PR #951 (grep/t4-grep-service, head b590b1102ca0a7d78ea452fc11b915cac239bfd6, LLP 0265 T4). Both neutral review rounds are recorded in that PR's thread (markers f49824b0 and 8b694e5b); nine findings were fixed and verified on the branch. The findings below remain open at the merged head. Triage verified each against the committed tree, and verified that executeGrepSearch has no production caller at this head (T5 wires the verb), so none of these can misbehave in a shipped flow yet. They must be closed before or alongside the tasks named on each.

1. HIGH, decision required before T6: the indexed tier silently loses every tool_args match

• src/core/search/grep_service.js:273 (parquetFind call) against src/core/search/searchable_columns.js:31 (tool_args in SEARCHABLE_COLUMNS).
• tool_args maps to Iceberg variant (src/core/cache/iceberg/schema.js:194), a binary column hypgrep 0.5.1 never indexes (hypgrep/src/utils.js:getTextColumnsFromSchema takes only UTF8/STRING leaves). A sidecar therefore never carries tool_args n-grams; a block whose only match is in tool_args is never proposed, and the row filter never sees the row. Reproduced in review: 1 hit on the scan tier becomes 0 hits once a sidecar exists.
• Violates LLP 0264 #lifecycle ("index state is never a correctness input") and LLP 0264 #shared. Latent today: no sidecar producer exists until T6, and createIndex throws loudly if handed the allowlist explicitly, so T6 cannot land without confronting this.
• The decision (any one closes it): drop tool_args from the indexed tier's coverage and document the caveat; store tool_args as STRING instead of JSON; add variant/JSON support to hypgrep upstream and move T1's pin; or gate T4 on sidecar coverage and accept the indexed tier as dead for this dataset. Whichever is chosen, grep_service.js should end up with a coverage gate (compare the sidecar's hypgrep.text_columns against SEARCHABLE_COLUMNS, degrade uncovered files to the scan tier) so the invariant is enforced rather than assumed.

2. MEDIUM, perf work with T7 or its own task: the scan tier materializes a whole data file with no heap guard

• src/core/search/grep_service.js:322-323: parquetReadObjects({ file: sourceFile, columns: SCAN_COLUMNS }) decodes an entire data file into one array. src/core/cache/maintenance.js:50 targets 128 MiB files and notes ~70x text compression, so one compacted file can decode to multiple GB. The SQL path deliberately streams (scanRowsFromTable) and runs a heap watchdog; this path does neither.
• Fix is chunked reads over row-group boundaries keeping absolute row positions intact for position-delete filtering and keeping the ABORT_CHECK_ROWS cadence. LLP 0265 #verified already flags the 90-day walk cost as unmeasured.

3. LOW, belongs with the same range-reader work: the indexed tier reads the whole source parquet synchronously

• src/core/search/grep_service.js:277: asyncBufferFactory resolves to createLocalIcebergIO's reader, a readFileSync of the full file (src/core/cache/iceberg/resolver.js:26) that ignores the byteLength hint. Block pruning saves decode but not IO, and each file read stalls the daemon's event loop. A range-capable reader is what makes the indexed tier worth having (T7).

4. LOW, recorded as correct-by-design, do not re-find: ENOENT on a data file mid-walk fails the search

• src/core/search/grep_service.js:322: if the maintenance generation sweep deletes a retired table dir mid-walk, the grep fails loudly. This matches the documented polarity in src/core/cache/iceberg/store.js (grep must not answer zero where SQL raises) and the SQL path races the same sweep the same way. No action unless that polarity changes.

Backlink: PR #951. Review records: the two neutral-review marker comments on that PR (rounds at f49824b0 and 8b694e5b).