hyp query grep: the verb that makes grep a surface (LLP 0265 T5) - #952
hyp query grep: the verb that makes grep a surface (LLP 0265 T5)#952bgmcmullen wants to merge 3 commits into
Conversation
neutral review round 1: changes requested (fixes pushed as
|
…s from lying (#952 review 2) Three edges the verb owned and got wrong: - `core_commands.js` projects every CORE_VERBS entry pre-boot so `hyp --help` can render, so the top-level `grep_service.js` import pulled hypgrep, hyparquet and the Iceberg store into the front door of every `hyp` invocation. Measured on `hyp --help`: 173-176ms with the eager import, 158-159ms with it deferred, 157ms on a tree with no grep verb at all. Now loaded inside `operation`, the way `verb_command.js` already defers the remote stack. - At the 1000-hit ceiling the truncation notice still said "raise --limit", which is the exact advice the clamp exists to avoid printing at a caller who cannot follow it. The operation now reports whether the ceiling was the binding limit and the notice names the ceiling instead. - Zero hits over zero searched files rendered identically to zero hits over the whole cache: empty stdout, empty stderr, exit 0. The summary spends 500 characters making "zero hits" honest for an MCP caller, and the un-searched case counterfeited one. The service already returns `indexedFiles`/`scannedFiles`; the render now says when both are zero. Quiet on `--remote`, which carries no file counts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
neutral review round 2: changes requested (fixes pushed as
|
neutral triage: defer remaining findings, PR can shipThe review rounds are exhausted with findings still open, so this is the triage rung's disposition at head Verified and deferred:
Dropped as already resolved: round 2's three fixes are the current head Disposition: all remaining findings are preferences or known gaps with owners; follow-up work tracked in #973. The PR is safe to ship from the triage rung's standpoint. |
queryGrepVerb in src/core/search/grep_verb.js joins CORE_VERBS beside query sql: one declaration projects the `hyp query grep` CLI command and the `grep_search` MCP tool, and because the tool name and inputSchema match the server's own grep_search (query, regex, session_id, chain_id, from, to, limit), `--remote <target>` reaches the server's archive-backed search with no server-side feature work. A server host displaces the kernel verb with its own via unregister (T2's affordance, server LLP 0178), so shipping it is safe on every host. The plan's open argv question resolves itself: the codec already maps `--session-id` onto the snake_case wire name, so no alias was needed. `include-local-only` is the one local-only parameter and deliberately carries NO schema default: argvToParams sends every defaulted property over the wire on --remote, the server's schema does not know the name, and a default would therefore fail validation on every remote call. A test pins the absence. The render flattens hits to one row per matched column (locators lead, snippet trails, part_id ready to pivot into query sql) and delegates to the shared query formatter, which is what gives grep the LLP 0225 contract for free: table/markdown escape captured bytes for a human reader, json/jsonl stay byte-exact for a pipeline, and the context budgets and --output spill behave exactly as query sql. Truncation, an uncovered walk, freshness, and local-only withholding each get their own stderr line; stdout stays a valid render. Tests: wire-schema pinning (property set, required, the no-default hazard, the coverage clause), codec flag mapping, end-to-end CLI runs over a real cache (hit render, newest-first, escape-vs-byte-exact, truncation notice, out-of-range limit fallback), a bare server-shaped result rendering without the local fields, and a stubbed remote MCP server proving the wire params travel exactly and the same render draws the server's answer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…he snippet last Review fixes on top of the T5 verb, each reproduced against a real Iceberg cache before the change: - `--limit 5000` answered with 50 rows (the default) and then advised "raise --limit", advice the caller had already followed and could not follow further. Above the ceiling now clamps to the ceiling the flag's own help advertises; only an unusable value (absent, fractional, zero) still falls back to the default. - `--from 2026-8-1` rendered an empty answer, exit 0, nothing on stderr: the window is compared lexicographically, so a mistyped day prunes every real one. A verb whose summary works this hard to make "zero hits" mean something must not let a typo forge one, so a day bound outside YYYY-MM-DD is refused with the flag named. - The snippet sat between the locators, and `renderTable` bounds a column's width but never truncates a cell, so any snippet past 80 characters pushed `message_id` and `part_id` out of column on exactly the rows a reader scans. Locators now lead and the snippet trails, as the render comment always claimed, with the row keys inserted in the same order so `--format json` and the table agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s from lying (#952 review 2) Three edges the verb owned and got wrong: - `core_commands.js` projects every CORE_VERBS entry pre-boot so `hyp --help` can render, so the top-level `grep_service.js` import pulled hypgrep, hyparquet and the Iceberg store into the front door of every `hyp` invocation. Measured on `hyp --help`: 173-176ms with the eager import, 158-159ms with it deferred, 157ms on a tree with no grep verb at all. Now loaded inside `operation`, the way `verb_command.js` already defers the remote stack. - At the 1000-hit ceiling the truncation notice still said "raise --limit", which is the exact advice the clamp exists to avoid printing at a caller who cannot follow it. The operation now reports whether the ceiling was the binding limit and the notice names the ceiling instead. - Zero hits over zero searched files rendered identically to zero hits over the whole cache: empty stdout, empty stderr, exit 0. The summary spends 500 characters making "zero hits" honest for an MCP caller, and the un-searched case counterfeited one. The service already returns `indexedFiles`/`scannedFiles`; the render now says when both are zero. Quiet on `--remote`, which carries no file counts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b25c9cf to
260e9b4
Compare
neutral triage: re-triage at rebased head
|
|
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 Integrating on current master also surfaced two problems invisible here: a The branch is retained, so this can be reopened if needed. This PR's task: T5, the |
What this is
LLP 0265 T5, stacked on T4 (#951) - retarget to
masterwhen that merges. After this PR, grep is a user-facing surface:hyp query grep <pattern>works locally on the scan tier, thegrep_searchMCP tool exists on every host, and--remote <target>reaches any current server's archive-backed search unchanged.The shape
queryGrepVerbjoinsCORE_VERBSbesidequery sql: one declaration, both adapters (LLP 0034). Three contract points worth review attention:inputSchema(query,regex,session_id,chain_id,from,to,limit) match the server'sgrep_searchfield for field, so--remotesends exactly what the server validates. The plan's open question about argv spelling resolved itself: the codec already maps--session-idontosession_id, no alias needed. A stubbed-MCP test asserts the exact arguments object that travels.include-local-onlycarries no schema default, deliberately.argvToParamsauto-fills every defaulted property into params, params travel verbatim on--remote, and the server's schema does not know this name - a default would fail validation on every remote call. Absent-unless-passed keeps the wire clean; a test pins the absence so a well-meaning cleanup can't reintroduce the hazard. (query sqlnever hit this because the server'squery_sqlIS the kernel verb, same schema; grep's remote twin is the server's own registration.)query_sqlis named as the escape hatch for the rest.Registering the verb is safe on server hosts: the server displaces it with its archive-backed twin via
unregister(T2, #875; server LLP 0178 / #364, merged).The render
Hits flatten to one row per matched column (
date, session_id, column, snippet, message_id, part_id- locators lead so any line pivots intoquery sqlor--session-id), then delegate to the shared query formatter. That single choice buys the whole LLP 0225 contract:table/markdownescape captured bytes (an ESC byte in a recorded message renders as the six ASCII characters\^[, never drives the terminal),json/jsonlstay byte-exact, and--max-cell/--max-bytes/--outputbehave exactly asquery sql. Truncation, an uncovered walk, cache freshness, and local-only withholding each get their own stderr line; stdout stays a valid render.Verified against the real cache
hyp query grep hypgrep --limit 5on this machine finds this very work session's own captured messages - including atool_argsmatch on the Write call that created the verb file - with the truncation notice on stderr. The dogfood loop closes.Tests
test/core/query-grep-verb.test.js, 9 tests: schema pinning (property set, required, no-default hazard, coverage clause), codec mapping, end-to-end CLI runs over a real Iceberg cache (render, newest-first order, escape-vs-byte-exact per LLP 0225, truncation notice, out-of-range limit fallback), a bare server-shaped result rendering without local fields, and the remote wire assertion above. Plus the one-linecommand-dispatchupdate: thequerygroup's children now includegrep.npm run typecheckclean;npm testgreen modulo the two known environment-only failures noted on #951.🤖 Generated with Claude Code
Triage at rebased head
260e9b48(2026-08-21): all residual findings non-blocking, deferred to #973; no new findings. This head is the same three T5 commits rebased onto T4 headb590b110, which also brings the--chain-idfix into this branch's own ancestry. Verified at this head: typecheck clean,npm test4641/0, grep suite 15/15, CI green. Residuals (all in #973): thedayBoundremote gap (MEDIUM, server-side fix is cheapest), the doubled--remotetruncation notice (LOW, LLP 0033 / T7), the 518-charhyp query --helprow (LOW cosmetic, T7), and the server #364 deployment-ordering release note. The #953tool_argsdrop does not affect this PR: the coverage clause derives its column list fromSEARCHABLE_COLUMNSdynamically. The PR now waits on: T4 (#951) merging, retarget tomaster, then human-held merge.