Skip to content

hyp query grep: the verb that makes grep a surface (LLP 0265 T5) - #952

Closed
bgmcmullen wants to merge 3 commits into
grep/t4-grep-servicefrom
grep/t5-query-grep-verb
Closed

hyp query grep: the verb that makes grep a surface (LLP 0265 T5)#952
bgmcmullen wants to merge 3 commits into
grep/t4-grep-servicefrom
grep/t5-query-grep-verb

Conversation

@bgmcmullen

@bgmcmullen bgmcmullen commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

What this is

LLP 0265 T5, stacked on T4 (#951) - retarget to master when that merges. After this PR, grep is a user-facing surface: hyp query grep <pattern> works locally on the scan tier, the grep_search MCP tool exists on every host, and --remote <target> reaches any current server's archive-backed search unchanged.

The shape

queryGrepVerb joins CORE_VERBS beside query sql: one declaration, both adapters (LLP 0034). Three contract points worth review attention:

  • Wire compatibility. The tool name and inputSchema (query, regex, session_id, chain_id, from, to, limit) match the server's grep_search field for field, so --remote sends exactly what the server validates. The plan's open question about argv spelling resolved itself: the codec already maps --session-id onto session_id, no alias needed. A stubbed-MCP test asserts the exact arguments object that travels.
  • include-local-only carries no schema default, deliberately. argvToParams auto-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 sql never hit this because the server's query_sql IS the kernel verb, same schema; grep's remote twin is the server's own registration.)
  • The coverage clause rides the summary verbatim (server LLP 0157's reasoning): an MCP caller sees only that text, so it must be able to explain zero hits - only the ten allowlisted columns are searched, and query_sql is 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 into query sql or --session-id), then delegate to the shared query formatter. That single choice buys the whole LLP 0225 contract: table/markdown escape captured bytes (an ESC byte in a recorded message renders as the six ASCII characters \^[, never drives the terminal), json/jsonl stay byte-exact, and --max-cell/--max-bytes/--output behave exactly as query 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 5 on this machine finds this very work session's own captured messages - including a tool_args match 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-line command-dispatch update: the query group's children now include grep.

npm run typecheck clean; npm test green 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 head b590b110, which also brings the --chain-id fix into this branch's own ancestry. Verified at this head: typecheck clean, npm test 4641/0, grep suite 15/15, CI green. Residuals (all in #973): the dayBound remote gap (MEDIUM, server-side fix is cheapest), the doubled --remote truncation notice (LOW, LLP 0033 / T7), the 518-char hyp query --help row (LOW cosmetic, T7), and the server #364 deployment-ordering release note. The #953 tool_args drop does not affect this PR: the coverage clause derives its column list from SEARCHABLE_COLUMNS dynamically. The PR now waits on: T4 (#951) merging, retarget to master, then human-held merge.

@bgmcmullen bgmcmullen added the neutral:adopt Foreign PR adopted into neutral's reconcile scope label Aug 19, 2026
@philcunliffe philcunliffe added the neutral:adopted Adoption completion record: merged while carrying neutral:adopt (LLP 0031) label Aug 20, 2026
@philcunliffe

Copy link
Copy Markdown
Contributor

neutral review round 1: changes requested (fixes pushed as f0b1ad67)

Reviewed git diff origin/grep/t4-grep-service...101d237d only: core_verbs.js, the new grep_verb.js, and the two tests. Findings that belong to the T4 service are called out as such and left to #951.

Baseline at 101d237d: npm test 4627 pass / 1 fail, npm run typecheck clean. The one failure is the known-environmental test/core/hyparquet-floor-pin.test.js. Same after the fixes (4630 pass, same single failure), and test/core/query-grep-verb.test.js is 12/12.

Every finding below was reproduced with a probe harness against a real Iceberg cache (61 matching rows) before it was written down, and each fix was re-run against the same probe afterwards.

Verdict: the shape is right. One declaration, both adapters; the wire-compatibility argument holds (I confirmed on the stubbed MCP call that only query, session_id, limit travel, and that include-local-only carrying no schema default is what keeps it off the wire); the LLP 0225 delegation to the shared formatter is correct in both directions. Three defects in the verb's own edges are fixed and pushed. Three residuals are reported and not fixed, with reasons.


Fixed and pushed (f0b1ad67)

1. MEDIUM. src/core/search/grep_verb.js:87 (pre-fix) - an over-max --limit collapsed to 50, then the output told you to raise it.

The guard read rawLimit <= MAX_LIMIT ? rawLimit : DEFAULT_LIMIT, so "too large" and "absent" were the same case. Reproduced over 61 matching rows:

$ hyp query grep needle --limit 5000
... 50 rows ...
grep: more matches exist beyond the limit - narrow with --from/--to or --session-id, or raise --limit

A request for more rows was answered with fewer than the default, followed by advice the caller had already taken and could not take further: raising --limit again keeps returning 50 forever. And the collapse is local-only, since --remote sends the raw 5000 to the server, so one flag meant two things.

Fixed: above the ceiling clamps to MAX_LIMIT, the number the flag's own description advertises (Max hits (default 50, max 1000)); only an unusable value (absent, fractional, 0) still falls back to the default. Verified after: --limit 9999 now returns all 60 rows with an empty stderr; --limit 0 still falls back to 50 with the truncation notice. Pinned by two tests (the clamp needs 60 rows to be distinguishable from the fallback, so it builds them).

2. MEDIUM. src/core/search/grep_verb.js:70 (pre-fix) - a mistyped --from/--to forged a silent zero-hit answer.

Both bounds are compared lexicographically against the row's day in grep_service.js, and prune whole files the same way, so anything that is not YYYY-MM-DD sorts outside every real day. Reproduced:

$ hyp query grep needle --from 2026-8-1
date  session_id  column  snippet  message_id  part_id
(no rows)
$ echo $?
0

Nothing on stderr, exit 0, over a cache that plainly contains matches. --from yesterday behaved identically. This verb spends 518 characters of summary making "zero hits" honest for an MCP caller; a typo must not be able to counterfeit one.

Fixed: a dayBound() helper refuses a bound outside ^\d{4}-\d{2}-\d{2}$ and names the flag. Verified after: hyp query grep: --from expects a day as YYYY-MM-DD (got 2026-8-1), exit 1, empty stdout. Remote is unaffected (the operation does not run on --remote; the server validates its own).

3. LOW. src/core/search/grep_verb.js:150 (pre-fix) - the column order contradicted its own comment and guaranteed misaligned tables.

The render comment says "the locator columns lead, the snippet trails", but columns was ['date','session_id','column','snippet','message_id','part_id'], with snippet ahead of two locators. renderTable (src/core/query/format.js:213) caps a column's width at 80 but never truncates the cell, so any snippet longer than 80 characters (the common case: the window runs to the --max-cell budget of 200) shoves message_id and part_id out of their columns on precisely the rows a reader wants to scan. Reproduced, then verified fixed:

date        session_id  column        message_id  part_id  snippet
----------  ----------  ------------  ----------  -------  ----------------------------------------
2026-08-12  s2          content_text  m61         m61#0    alpha needle xxxxxxxx...
2026-08-10  s1          content_text  m60         m60#0    needle body 59

Fixed by moving snippet last, as the comment always claimed. The row objects are now built with their keys inserted in that same order too: --format json serializes the row objects themselves (renderResult, format.js:159), so insertion order is the key order a pipeline sees, and the table and the json render of one answer should not disagree about where the snippet sits. Pinned by a jsonl key-order test.

Note: the PR body's "Hits flatten to one row per matched column (date, session_id, column, snippet, message_id, part_id)" is now stale; the order is date, session_id, column, message_id, part_id, snippet.

Reported, not fixed

4. NOT THIS PR (already fixed on the base branch). --chain-id alone returns unfiltered results.

At this PR's merge base (f49824b0), compileChainPredicate in grep_service.js returns () => true whenever sessionId === undefined, never consulting chainId. Reproduced through the verb: hyp query grep needle --chain-id NOPE (a chain id in no row) returned every match. The verb's schema advertises chain_id as an independent filter with no such caveat.

I did not fix it: #951 already did, in 8b694e5b ("honor a chainId alone"), and the current origin/grep/t4-grep-service head b590b110 has the standalone predicate. This branch is stacked on the older f49824b0, so the defect is only visible here until T4 lands. Action for whoever retargets this PR: rebase onto the current T4 head (or master once T4 merges) rather than merging as stacked, so the verb ships with the corrected predicate under it.

5. LOW. src/core/search/grep_verb.js:128 - --remote prints two truncation notices, and the generic one points at the wrong escape hatch.

runRemoteVerb calls serverCapNotices (src/core/mcp/remote_verb.js:162) for any structured.truncated, and that helper reads rows/row_count/server_cap.rows - fields a grep result does not carry (it has hits). So the count degrades away and the line becomes the bare form, beside the verb's own:

remote: result truncated - narrow the query, or read the Iceberg archive directly for bulk
grep: more matches exist beyond the limit - narrow with --from/--to or --session-id, or raise --limit

Under LLP 0033 #two-truncations those two lines are meant to be server cap versus client display budget; here both describe the server cap, and "read the Iceberg archive directly" is not grep's escape hatch (query_sql is). Not fixed here on purpose: every clean fix edits shared remote machinery (either teaching serverCapNotices about non-row-shaped results, or suppressing it for them), which changes the remote stderr contract for every verb including graph neighbors, and that is an LLP 0033 question rather than a T5 one. Worth a line in T7.

6. LOW, cosmetic. src/core/search/grep_verb.js:46 - the 518-character summary is also the hyp query --help subcommand row.

renderGroupHelp writes summary verbatim into the table (src/core/cli/group_help.js:104), with no wrapping or truncation, so grep renders as one 518-character line where the longest existing summary in the tree is ~120. Alignment of the neighbouring rows survives (each row is padded independently), so this is ugly, not broken. Not fixed: LLP 0264 #verb requires the coverage clause in the summary verbatim, and that is the right call for the MCP surface, so the fix belongs in the CLI renderer (truncate a long summary in the group table; the full text is still one hyp query grep --help away). T7 territory.

7. VERIFY BEFORE MERGE (cross-repo, unverifiable from this repo). src/core/cli/core_verbs.js:22.

LLP 0264 #verb names this hazard explicitly: the server's daemon registers its own grep_search and defers if the tool already exists, so "shipping the kernel half alone is a regression on every server host" - the server would silently serve local-cache grep instead of archive-backed search, with no error anywhere. The new doc comment asserts the server now displaces the kernel twin via unregister (hypaware-server LLP 0178 / #364). The kernel-side affordance is present and correct (isVerbProjection retracts only kernel-projected commands), but the server half is out of this repo. Confirm #364 has actually shipped, and that no server host can pick up this kernel ahead of it, before release.

Checked and clear

include-local-only correctly carries no schema default, verified on the wire, not just in the schema. The truncated / exhausted else-branch is right against the service's exhausted: exhausted && !truncated, so the two notices can never both fire. LLP 0225 escaping is delegated, not reimplemented (table escapes ESC, json stays byte-exact). --json shorthand matches query sql exactly. No import cycle through renderLocalOnlyNotice. Notice ordering (freshness, then withholding, then completeness, then the display budget) matches query sql. A server-shaped result with none of the local fields renders without throwing. verbToCommand needs no signal, so not threading one is correct rather than an omission.

philcunliffe pushed a commit that referenced this pull request Aug 20, 2026
…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>
@philcunliffe

Copy link
Copy Markdown
Contributor

neutral review round 2: changes requested (fixes pushed as b25c9cf4)

Reviewed git diff origin/grep/t4-grep-service...f0b1ad67, i.e. round 1's own fix commit included. Baseline at f0b1ad67: npm test 4630 pass / 1 fail (the known-environmental test/core/hyparquet-floor-pin.test.js), npm run typecheck clean, test/core/query-grep-verb.test.js 12/12. After the fixes: 4633 pass, same single failure, 15/15 on the grep suite, typecheck clean.

Verdict: round 1's three fixes all hold under re-examination (clamp, dayBound, snippet-last with matching JSON key order). Three further defects in edges this verb owns are fixed and pushed. Three residuals are reported and not fixed, with reasons; two of round 1's residuals are re-confirmed and one is now resolved by the base branch.


Fixed and pushed (b25c9cf4)

1. MEDIUM. src/core/search/grep_verb.js:5 (pre-fix) - the verb put hypgrep, hyparquet and the Iceberg store on the front door of every hyp invocation.

core_commands.js:80 projects every CORE_VERBS entry pre-boot so hyp --help can render before the kernel starts. Adding queryGrepVerb to CORE_VERBS therefore made its top-level import { executeGrepSearch } from './grep_service.js' a boot-path import, and grep_service.js pulls hypgrep, hyparquet, cache/iceberg/resolver, cache/iceberg/store, cache/spool and query/sql with it. Every hyp status, hyp --help, hyp session ignore and daemon start paid for a module set only hyp query grep uses.

Measured, medians of 7 interleaved runs of node bin/hypaware.js --help:

eager import (f0b1ad67):   176ms, 173ms
lazy  import (this fix):   158ms, 159ms
no grep verb at all:       157ms

Module-load alone: import('./src/core/cli/core_commands.js') was 145ms eager, 120ms lazy, 124ms with no grep verb. So the whole regression is the import, and deferring it recovers all of it.

Fixed by loading grep_service.js inside operation with await import(...), which is the idiom verb_command.js:112 already uses for the remote stack and for the same stated reason ("so a local hyp <verb> never pays for it"). SEARCHABLE_COLUMNS stays a top-level import: it is 2ms and the summary needs it at declaration time. A module-load failure now surfaces as hyp query grep: <msg> exit 1 through runVerbCommand's existing catch, rather than taking down the CLI at boot.

2. LOW. src/core/search/grep_verb.js:134 (pre-fix) - the truncation notice still said "raise --limit" in exactly the case round 1's clamp made that impossible.

f0b1ad67's own rationale (lines 87-92) is that falling back for an over-max limit would "print 'raise --limit' at a caller who just did - advice that cannot be followed". But the notice was unconditional, so --limit 9999 over a cache with more than 1000 matches returned 1000 rows and then said ... or raise --limit. The clamp moved the unfollowable advice one flag along instead of removing it.

Fixed: operation returns limitCeilingReached: limit >= MAX_LIMIT beside the existing local-only fields, and the notice becomes grep: more matches exist beyond the 1000-hit ceiling - narrow with --from/--to or --session-id when the ceiling was the binding limit. Below the ceiling the wording is unchanged, and a server result (which carries no such field) falls back to the general wording. Pinned by a render-level test asserting both branches.

3. LOW. src/core/search/grep_verb.js:112 (pre-fix) - zero hits over zero searched files was indistinguishable from zero hits over the whole cache.

Against a cache root with no ai_gateway_messages table the command exits 0 with (no rows) on stdout and empty stderr: byte-identical to a full search that found nothing. This verb spends ~500 characters of summary making "zero hits" mean something specific for an MCP caller, and on the MCP surface an agent sees only the rows. executeGrepSearch already returns indexedFiles and scannedFiles and render was discarding both.

Fixed: when there are no hits and both counts are 0, stderr carries grep: no ai_gateway_messages data files were searched - nothing is recorded on this machine yet, or --from/--to excluded every file. Quiet on --remote (a server result carries no file counts) and quiet whenever any file was actually walked. Pinned by two tests: the empty-cache case emits it, a populated cache with no match stays silent.

Reported, not fixed

4. MEDIUM, open. src/core/search/grep_verb.js:102 - dayBound guards only the local path, so --remote can still forge the silent zero-hit answer the guard was added to prevent.

runVerbCommand never calls operation when --remote is set (src/core/cli/verb_command.js:100-127); params go to the server verbatim. So hyp query grep needle --from 2026-8-1 --remote prod ships the malformed day, and round 1's fix does not reach it. The JSDoc's claim that "the server validates its own" is unverified: LLP 0265 #verified records the server's grep_search params but says nothing about day validation, and nothing in this repo can settle it.

Not fixed here because every clean fix leaves this verb. The idiomatic home is the codec: verb_codec.js:210 states that shape rules live in argvToParams and only "mutual exclusions, conditional requirements, value cross-checks" stay in the command, and a day format is shape. That means adding a pattern (plus a human-readable noun for the error text) to VerbInputProperty, which hypaware-plugin-kernel-types.d.ts:1622 calls "a deliberately small JSON-Schema subset" - a published-contract change that wants an LLP, not a review round. It would land cleanly otherwise: the MCP projection strips only greedy via rest-spread (verb_codec.js:282), so a pattern would ride to the tool schema and help there too. The alternative is a verb-level pre-dispatch validate hook. Either way it fixes the class, not just --from.

5. LOW, carried from round 1 (#5). --remote prints two overlapping truncation notices, one of them SQL-shaped.

Re-confirmed. serverCapNotices (src/core/mcp/remote_verb.js:162) fires on any structured.truncated and reads rows/row_count/server_cap.rows, which a grep result does not carry, so it degrades to the bare remote: result truncated - narrow the query, or read the Iceberg archive directly for bulk beside the verb's own line. Under LLP 0033 #two-truncations those two lines are meant to be server cap versus client display budget; here both describe the server cap and the generic one names an escape hatch (bulk Iceberg reads) that is not grep's (query_sql is). Still not fixed for round 1's reason: the fix edits shared remote machinery and changes the stderr contract for every verb including graph neighbors. LLP 0033 question, T7 territory.

6. LOW, cosmetic, carried from round 1 (#6). The 518-character summary is also the hyp query --help subcommand row.

Re-confirmed against the built CLI: renderGroupHelp (src/core/cli/group_help.js:104) writes summary verbatim with no wrap or truncation, so grep renders as one 518-character line next to overview/schema/sql at ~40-70. Padding is per-row so alignment survives; it is ugly, not broken. LLP 0264 #verb requires the coverage clause in the summary verbatim and that is right for the MCP surface, so the fix belongs in the CLI renderer. T7.

Resolved since round 1

Round 1 #4, --chain-id alone returning unfiltered results, is no longer reachable through a merge of this PR. Verified directly: this branch's merge base is f49824b0, whose compileChainPredicate short-circuits if (sessionId === undefined) return () => true, but the base branch head origin/grep/t4-grep-service = b590b110 has the corrected standalone predicate (grep_service.js:462-472, sessionId === undefined && chainId === undefined). GitHub merges into the branch head, not the merge base, so #952 lands on top of the fix. The stale predicate is visible only when checking this branch out in isolation. No action needed beyond not resetting the base.

Round 1 #7 stands unchanged (cross-repo, VERIFY BEFORE MERGE). LLP 0264 #verb: the server's daemon registers its own grep_search and defers if the tool already exists, so a server host booting this kernel without hypaware-server #364 silently serves local-cache grep in place of archive-backed search, with no error anywhere. The kernel-side affordance is present and correct (isVerbProjection retracts only kernel-projected commands; test/core/verb-registry.test.js:113-133 pins both directions). Confirm #364 has shipped and no server host can pick up this kernel ahead of it.

Checked and clear this round

hypgrep is a hard dependency, not optional, so the (now-deferred) import cannot fail to resolve. SEARCHABLE_COLUMNS is a frozen constant-Set and spreads correctly into the summary; compileMatcher refuses an empty pattern, so String(params.query ?? '') cannot become a match-everything scan. resolveFlag (verb_codec.js:327) tries snake-case first and then the literal name, which is what makes both --session-id and the dashed --include-local-only resolve. MAX_QUERY_LENGTH is enforced in the matcher, not left to the verb. All four @ref anchors exist (llp/0264#verb, #shared, llp/0105#override, llp/0105). No em dashes, no semicolons, @import at the top with repo-root-anchored .js specifiers. T7 owns the hypaware-query SKILL.md update and the hyp query status coverage line, so their absence here is on-plan, not an omission.

@philcunliffe

Copy link
Copy Markdown
Contributor

neutral triage: defer remaining findings, PR can ship

The review rounds are exhausted with findings still open, so this is the triage rung's disposition at head b25c9cf4. Every unresolved finding was re-verified against the code (and, where it mattered, against the hypaware-server repo) and classified. None is a merge blocker; all are deferred to #973.

Verified and deferred:

  1. MEDIUM, the dayBound remote gap. Confirmed real end to end, beyond the review's claim: the remote path never calls operation (src/core/cli/verb_command.js:100-127), and the server's MCP grep_search operation does not validate day shape (hypaware-server src/daemon.js:1213 passes from/to through on a typeof check into a lexicographic comparison), even though the HTTP /v1/search route does (routes-search.js:64-68). So --from 2026-8-1 --remote <target> really is exit 0 with zero rows and empty stderr. Judged non-blocking because: it needs both a malformed input and the opt-in remote path; the outcome is a bounded misleading-empty result, not data loss or a crash; and the server tool was already reachable with the same laxity by any MCP client, so this PR exposes a preexisting server-side validation asymmetry rather than introducing it. The cheapest complete fix is server-side (mirror isValidPartitionDate in the MCP operation); the kernel-side class fix (a pattern keyword on VerbInputProperty, or a pre-dispatch validate hook) is a published-contract change that wants an LLP. Both routes are written up in Follow-up: deferred review findings from PR #952 #973.

  2. LOW, doubled truncation notice on --remote (serverCapNotices, src/core/mcp/remote_verb.js:162, reads row-shaped fields a grep result lacks). Confirmed. Fix changes the shared remote stderr contract for every verb, an LLP 0033 question. Deferred to Follow-up: deferred review findings from PR #952 #973, flagged for T7 (Surfaces and proof: status coverage, the skill learns grep, and the roundtrip smoke (LLP 0265 T7) #954).

  3. LOW cosmetic, the 518-character summary as one hyp query --help row (src/core/cli/group_help.js:103). Confirmed; per-row padding keeps neighbours aligned, so ugly not broken, and the verbatim clause is required by LLP 0264 #verb. Renderer fix deferred to Follow-up: deferred review findings from PR #952 #973, T7 territory.

  4. Cross-repo VERIFY BEFORE MERGE item: resolved. hypaware-server T7: Wizard fork phase + returning-gate amendment #364 (LLP 0178, displace the kernel twin) is verified MERGED, 2026-08-19. The residual is upgrade ordering only (a pre-T7: Wizard fork phase + returning-gate amendment #364 server build must not pick up a post-T5 kernel), which is a release-note item recorded in Follow-up: deferred review findings from PR #952 #973, not a merge blocker.

Dropped as already resolved: round 2's three fixes are the current head b25c9cf4 (boot-path lazy import, ceiling-aware truncation wording, empty-cache notice), round 1's three fixes are in f0b1ad67, and round 1 #4 (--chain-id alone) is fixed on the base branch head b590b110, which is what GitHub merges against.

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.

@philcunliffe philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 20, 2026
bgmcmullen and others added 3 commits August 20, 2026 21:20
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>
@bgmcmullen
bgmcmullen force-pushed the grep/t5-query-grep-verb branch from b25c9cf to 260e9b4 Compare August 21, 2026 04:21
@philcunliffe philcunliffe removed the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 21, 2026
@philcunliffe

Copy link
Copy Markdown
Contributor

neutral triage: re-triage at rebased head 260e9b48, disposition unchanged, PR can ship

The branch was force-updated after the previous triage (which covered b25c9cf4), so this is a fresh triage at head 260e9b48. Verified directly: the new head is the same three T5 commits rebased onto the current T4 head b590b110; the diff between the two heads touches only T4-owned files (grep_service.js, cache/iceberg/store.js, T4's own tests). No T5-owned file changed, and the rebase strictly improves the picture: round 1 finding 4 (--chain-id alone returning unfiltered results) is now fixed in this branch's own ancestry (8b694e5b, standalone chain predicate confirmed at src/core/search/grep_service.js:462), not just on the base branch head.

Verified at this head in a clean worktree: npm run typecheck clean, npm test 4641 pass / 0 fail / 1 skipped, test/core/query-grep-verb.test.js 15/15, and CI on the PR is fully green. All six review-round fixes are present in the tree (limit clamp, dayBound on the local path, snippet-last with matching JSON key order, lazy grep_service import off the boot path, ceiling-aware truncation wording, empty-cache stderr notice).

Residual findings, re-verified live at this head, all non-blocking, all already tracked in #973:

  1. MEDIUM, dayBound remote gap (Follow-up: deferred review findings from PR #952 #973 item 1): dayBound runs only in operation (src/core/search/grep_verb.js:109-110), which --remote never calls, so a malformed --from still travels verbatim and yields a silent zero-hit from the server. Non-blocking for the same reasons as before: needs both a malformed input and the opt-in remote path, the outcome is a bounded misleading-empty result, and the server-side laxity predates this PR. Cheapest complete fix is server-side; the kernel-side class fix wants an LLP.
  2. LOW, doubled truncation notice on --remote (Follow-up: deferred review findings from PR #952 #973 item 2): serverCapNotices (src/core/mcp/remote_verb.js:162) still reads row-shaped fields a grep result lacks. Fix changes shared remote stderr for every verb, an LLP 0033 question, T7 territory.
  3. LOW cosmetic, 518-character summary as one hyp query --help row (Follow-up: deferred review findings from PR #952 #973 item 3): renderGroupHelp (src/core/cli/group_help.js:103) still writes summaries verbatim. Ugly, not broken; renderer fix is T7 territory.
  4. Release-note ordering (Follow-up: deferred review findings from PR #952 #973 item 4): server hosts must be on a hypaware-server T7: Wizard fork phase + returning-gate amendment #364-or-later build before picking up a kernel containing this verb.

Cross-PR check: the maintainer's decision on the #954 thread (T6 #953 drops tool_args from SEARCHABLE_COLUMNS, sidecarPathFor moves to search/searchable_columns.js) does not invalidate anything here: the verb's coverage clause builds its column list dynamically from SEARCHABLE_COLUMNS (src/core/search/grep_verb.js:48), so the drop flows through with no edit to this PR.

Disposition: no new findings at this head, every residual is a preference or a known gap with an owner, follow-up tracked in #973. The PR now waits on its base: retarget to master when T4 (#951) merges, then merge under hold-for-human rules.

@philcunliffe philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 21, 2026
@bgmcmullen

Copy link
Copy Markdown
Contributor Author

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: T5, the hyp query grep verb.

@bgmcmullen bgmcmullen closed this Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:adopt Foreign PR adopted into neutral's reconcile scope neutral:adopted Adoption completion record: merged while carrying neutral:adopt (LLP 0031) neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants