Use an options object for searchByIndex and the custom-index search() contract - #2187
Use an options object for searchByIndex and the custom-index search() contract#2187maurice-harper wants to merge 2 commits into
Conversation
…ject (#2165) searchByIndex had grown to eight positional parameters, five optional, so callers padded with undefined or stopped early and inherited defaults they never chose. The same shape reached HierarchicalNavigableSmallWorld.search(), which is the contract an index implemented outside this repo has to satisfy — every future capability there would be another positional argument and a breaking change. The four required leading parameters stay positional on both; the optional tail becomes a named options object: searchByIndex(condition, txn, reverse, Table, { allowFullScan, filtered, context, minResults }) customIndex.search(condition, context, { filter, minResults }) No behaviour change. The plain-JS benchmark caller is updated too, since the TypeScript build cannot flag it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the searchByIndex and HNSW search methods to accept optional trailing parameters as a named options object instead of positional arguments, updating all call sites and adding a unit test to verify this behavior. The reviewer identified three locations in resources/Table.ts and resources/search.ts where the context parameter was omitted from the new options object, which would bypass the active transaction and potentially cause transaction isolation issues.
Dispatch durable state — QA PASS at
|
| Acceptance criterion | Verdict | Evidence |
|---|---|---|
| AC1: searchByIndex leading params stay positional, optional tail becomes a named options object with allowFullScan/filtered/context/minResults | PASS | Unchanged since 44711b8 (file byte-identical); unitTests/resources/searchByIndexOptions.test.js both cases pass at ab88257 |
| AC2: relationship-recursive searches (search.ts) and Table.ts:4960 relationship call preserve existing defaults, no positional-undefined padding | PASS | Unchanged since 44711b8 (file byte-identical); prior run's manual revert of this exact file breaks searchByIndexOptions.test.js as designed |
| AC3: customIndex.search() keeps context positional, filter/minResults via third options object | PASS | Unchanged since 44711b8 (file byte-identical); vectorIndex.test.js call sites pass at ab88257 |
| AC4: HierarchicalNavigableSmallWorld.search's options object preserves predicate-aware filtering, minResults widening, per-query ef, threshold comparators | PASS | 94 passing across vectorIndex.test.js + searchByIndexOptions.test.js at ab88257, including the #1241 predicate-aware-traversal and minResults/ef-widening describe blocks |
| AC5: custom-index search contract documentation and direct test calls updated to options-object shape, no speculative cursor/interface/shim added | PASS | resources/DESIGN.md:96 now reads HierarchicalNavigableSmallWorld.search(cond, ctx, { filter }), matching the real signature; repo-wide grep of *.md finds no other stale positional-contract statement; no cursor field/typed interface/shim present (unchanged production diff) |
| AC6: npm run build, npm run test:unit:resources, and existing HNSW/vector-index tests pass without new dependencies | PASS | build clean at ab88257; focused HNSW/vector-index suite 94 passing; matched base-vs-head unitTests/resources/** comparison from the prior run (unaffected by this docs-only delta) showed no new failures; no package.json changes in this delta |
Commands:
~/.dispatch-dev-team/scripts/sprint.sh spec-path 2165~/.dispatch-dev-team/scripts/sprint.sh branch-of 2165gh pr list --head fix/issue-2165-search-options --json number,headRefOid,isDraftgit fetch origin fix/issue-2165-search-optionsgit diff 44711b8ef..ab882571e (confirms exactly one line changed, resources/DESIGN.md)git diff 44711b8ef..ab882571e --stat -- resources/search.ts resources/Table.ts resources/indexes/HierarchicalNavigableSmallWorld.ts (empty -> byte-identical)git checkout ab882571egrep -rn 'customIndex\.search(' / 'searchByIndex(' / 'HierarchicalNavigableSmallWorld\.search(' --include=*.md .sed -n '971,1000p' resources/indexes/HierarchicalNavigableSmallWorld.ts (confirm real signature)npm ci --no-audit --no-fundnpm run buildnpx mocha unitTests/resources/vectorIndex.test.js unitTests/resources/searchByIndexOptions.test.js (94 passing)npx prettier --check resources/DESIGN.md~/.dispatch-dev-team/scripts/qa-revert-verify.sh 2165 2187 (reproduced documented infra limitation, tests-fail-before-revert)
Dismissed concerns:
- qa-revert-verify.sh reports tests-fail-before-revert instead of exercising an actual revert — Justification: pre-existing - Identical script/repo-build-step mismatch documented at the prior QA head 44711b8 (disposable worktree never runs npm run build, tests load dist/ not source). Not new to this delta, and the production files it would revert are byte-identical to that prior head, so that head's manual revert evidence stands.
- unitTests/resources/schemaMigrationFragility.test.js SIGABRTs mocha (exit 134) when the full resources/** suite is run — Justification: pre-existing - Reproduced identically on base commit 11a1c48 in the prior QA run, before this PR's changes existed; this docs-only delta does not touch that file or anything it depends on.
- 12 pre-existing failures remain in unitTests/resources/** at head — Justification: pre-existing - Same 12 test names fail on base commit 11a1c48 per the prior QA run's matched comparison; this delta only touches resources/DESIGN.md and cannot affect them.
Provenance: stage qa-check, head ab882571e4b050f7afda417773cd97a9d632a6a3, recorded 2026-08-17T03:57:52Z.
Prior heads:
44711b8— FAIL, 1 blocking finding(s), 2026-08-17T03:45:54Z
Dispatch status — head
|
| Signal | Source | State |
|---|---|---|
| CI | GitHub checks | passing |
| QA | Dispatch qa-check stage |
PASS (evidence) |
| Independent review | Dispatch reviewer stage — Gemini REST API (degraded) (gemini-flash-latest) |
RECOMMEND_MERGE — low risk, 2 lens(es), degraded evidence (review) |
| Other review | gemini-code-assist[bot] — automatic GitHub app | commented (open) |
| Unresolved blocking findings | QA + independent review | 0 |
Checked at this head
- QA exercised resources/DESIGN.md's 'How is a filter applied during a vector search?' row now reads…, HierarchicalNavigableSmallWorld.search's third parameter is a destructured { filter,…, searchByIndex options-object contract (allowFullScan/filtered/context/minResults) still…; tried to break it with Diffed 44711b8..ab88257, Grepped every *.md in the repo for customIndex.search(, searchByIndex(, and…, Re-ran the exact HierarchicalNavigableSmallWorld.search signature at head, and 2 more.
- Independent review inspected searchByIndex options object refactoring, HierarchicalNavigableSmallWorld search signature and options handling, relationship recursive search call sites, vector index unit tests and benchmarks.
Recommended human action: Review and merge if you agree.
Basis: QA PASS at ab88257 (Dispatch qa-check stage); independent review RECOMMEND_MERGE at ab88257 (Dispatch reviewer stage, Gemini REST API (degraded) gemini-flash-latest); CI passing; gemini-code-assist[bot] commented.
Prior heads and machine state
44711b8— CI failing, QA FAIL, review missing, 2026-08-17T03:46:11Z
…hape (#2165) resources/DESIGN.md still described HierarchicalNavigableSmallWorld.search as (cond, ctx, filter). The third argument is now an options object. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Dispatch durable state — independent review at
|
|
Follow-up filed for the one deliberately-declined review theme: #2188 tracks threading |
|
Reviewed — |
| { | ||
| allowFullScan, | ||
| filtered, | ||
| context, | ||
| minResults, | ||
| }: { | ||
| allowFullScan?: boolean; | ||
| filtered?: any; | ||
| context?: any; | ||
| // How many rows the query will ultimately consume (offset + limit), when it is bounded. An | ||
| // approximate index returns a fixed-size candidate list, so without this a query asking for more | ||
| // rows than that list holds silently gets a short result set. Only custom indexes read it. | ||
| minResults?: number; | ||
| } = {} |
There was a problem hiding this comment.
Low: a stale positional call silently permits a full scan — the one hazard the PR flags has no guard
allowFullScan is tested with a strict === false (lines 446 and 448), so destructuring a non-object 5th argument yields undefined and neither guard fires. I confirmed this at head rather than reasoning about it — searchByIndex({ attribute: 'unindexedName', value: 'x' }, undefined, false, T, false), the exact pre-PR call shape from Table.ts on main, returns the unindexed row instead of throwing the 404 "is not indexed" ClientError.
tsc rejects that in .ts (verified: npm run typecheck is clean), so nothing is broken today. But it does not cover the repo's plain-JS callers under unitTests/ and benchmarks/, and the PR body accepts "a missed positional call in plain JS fails silently, not at build time" as residual risk. Since the typed-index-interface mitigation was scoped out by the team-lead decision, a runtime guard is the only remaining one — and it is what makes the hazard this PR's new test file describes in prose actually assertable.
Suggested fix — name the parameter, reject a non-object, then destructure (structural, so not a one-click suggestion):
Table: any,
options: {
allowFullScan?: boolean;
filtered?: any;
context?: any;
minResults?: number;
} = {}
): AsyncIterable<Id | { key: Id; value: any }> {
if (typeof options !== 'object' || options === null)
throw new TypeError('searchByIndex: the 5th argument is an options object (#2165), not a positional value');
const { allowFullScan, filtered, context, minResults } = options;That also lets searchByIndexOptions.test.js add the case its header comment already describes: assert the stale positional form throws instead of quietly full-scanning.
—
Generated by Barber AI
| * @param distance | ||
| * @param comparator | ||
| * @param context | ||
| * @param options |
There was a problem hiding this comment.
Nit: options now names two different things in search()
This new @param options documents the third parameter, but the body already uses options for something unrelated — const options = context.transaction (line 1038), the nested RocksDB transaction handed to getEntryPoint(options) and searchLayer(..., options, ...).
Nothing breaks today, because the third parameter is destructured inline and never bound to a name. The cost is on the next edit: giving that parameter a name — which is exactly what adding a runtime type guard would require — collides with the local.
Renaming the local to txnOptions (3 uses) frees the name and makes the JSDoc unambiguous.
—
Generated by Barber AI
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Automated gate — not yet queued for human review.
This PR's AI review found issues, and the PR description reports no cross-model reviews.
Per team policy, a substantive PR with AI-review findings is queued for human review only after at least 2 cross-model reviews have been run, their findings addressed, and the coverage reported in the PR description (## Review coverage naming each model — see harper-engineering-guidelines).
The findings below count as one of the two: address them, run a second outside-model review, update the description, and the gate lifts automatically on the next pass.
TL;DR
This refactors growing optional positional parameters into named options objects.
The approach addresses the root cause, preserves in-repo behavior, and adds negligible hot-path overhead.
One external-contract migration concern remains.
Independent review and tests were unavailable because this read-only checkout has no installed dependencies; syntax and whitespace checks passed.
verdict: CHANGES
merge: rebase
Human-Review-Need: 4 @ ab88257
Change tour
resources/search.ts:143-148,resources/search.ts:241-259— query execution andsearchByIndexnow use named options with permissive omission defaults.resources/search.ts:281-309,resources/Table.ts:4960-4966— recursive relationship searches preserveallowFullScanand join-filter propagation.resources/search.ts:509-536,resources/indexes/HierarchicalNavigableSmallWorld.ts:960-1009— the custom-index boundary and HNSW implementation adopt{ filter, minResults }; traversal behavior remains unchanged atresources/indexes/HierarchicalNavigableSmallWorld.ts:1058-1095.benchmarks/hnsw-search.js:226,resources/DESIGN.md:96— benchmark and design documentation reflect the new shape.unitTests/resources/searchByIndexOptions.test.js:35-52,unitTests/resources/vectorIndex.test.js:809-813,unitTests/resources/vectorIndex.test.js:1955-2045— coverage checks omitted defaults,allowFullScan, filtering, and minimum-result behavior.
Findings
major — resources/search.ts:516 — the unversioned argument-shape change breaks existing out-of-repo custom indexes and legacy direct HNSW callers
Verification
- Passed
node --checkon every changed TypeScript/JavaScript file. - Passed
git diff --check origin/main...HEAD. - Unit tests and lint were not run: dependencies are absent and the sandbox prevented installation.
- Cross-model review setup failed before sharing the diff because its artifact directory is read-only.
Review coverage
| lens | outcome |
|---|---|
| gemini | pruned — pruned (policy minimal) |
| cursor-grok | pruned — pruned (policy minimal) |
| cursor-composer | pruned — pruned (policy minimal) |
| codex | ok — graded leg — produced review.md + comments.json |
| domain | pruned — pruned (policy minimal) |
Pre-push review of fix/issue-2165-search-options (ab88257) vs origin/main by codex.
Review emphasis: Dispatch-configured.
— codex review, submitted by the dispatch review gate
| return entry; | ||
| }); | ||
| const loaded = index.customIndex | ||
| .search(searchCondition, context, { filter: recordFilter, minResults }) |
There was a problem hiding this comment.
This is a breaking change to the external custom-index contract. An existing search(condition, context, filter, minResults) implementation now receives an options object as filter; invoking that truthy value fails with TypeError. Conversely, a legacy direct HNSW caller silently loses its filter because resources/indexes/HierarchicalNavigableSmallWorld.ts:995-1009 destructures the function as an object with no filter field. Please add a version/feature gate or transitional normalization for the old signature, or confirm and document that every out-of-repo implementation and caller will be updated atomically.
— KrAIs (Codex)
cross-model coverage reported — released to human review
3 participants
Closes #2165
Summary
searchByIndexhad eight positional parameters, five optional, so callers padded withundefinedor inherited unintended defaults. The same shape reachedHierarchicalNavigableSmallWorld.search(), the contract an external index implements. Both now name the optional tail. No behaviour change.Important changes
(condition, txn, reverse, Table, { allowFullScan, filtered, context, minResults }), defaulting to{}. All four call sites updated.searchByIndexOptions.test.js.contextstays second positional;benchmarks/hnsw-search.jsupdated too.resources/DESIGN.mdsaidsearch(cond, ctx, filter); nowsearch(cond, ctx, { filter }).*.mdstates the positional form now.Where to focus
allowFullScanwas a barefalse; destructured,falseyieldsundefined, so a missed conversion fails open — permitting a full scan.Risks and boundaries
contextstill do not.DESIGN.mdstates no signature, so it did not change.test:unit:mainandschemaMigrationFragility.test.js.Validation
resources/DESIGN.mdchanged since head 44711b8, where these ran:build,typecheck,lint,prettierclean; resources suite 1534 → 1536 passing, identical 12 pre-existing failures.resources/search.tsfails the new test and makestscrejectTable.ts.Dispatch evidence and stage history
Current head
ab88257· size classsmall(114 changed production lines) · visible body 300 words (guardrail 150-300).Evidence:
contextstays positional; no cursor field, typed interface, or shim; PR stays draft until Maurice's go-ahead.minResultsargument came from Remove linear-in-corpus-size work from HNSW vector search, and honor limit past the candidate list #2125 (Remove linear-in-corpus-size work from HNSW vector search, and honor limit past the candidate list #2125), kept positional there to avoid reshaping a contract inside a correctness fix.resources/indexes/customIndexes.tsregistersHierarchicalNavigableSmallWorldas the only custom index, so it is the whole in-tree surface of the reshaped contract.benchmarks/hnsw-search.jsat 44711b8: filtered recall 100% at every selectivity, so the namedfilterreaches traversal.sqlEngine: 103/103.benchmarks/hnsw-scale.jscallshnsw.search()with two arguments only and needed no change.minResultsprose at DESIGN.md:800 and theisIndexingnote at DESIGN.md:117 namesearchByIndexwithout a parameter list, so both stay accurate unchanged.resources/DESIGN.mdafter the edit; it rebalanced only that table row's trailing pad.Prior heads:
44711b8— small, 299 words, 2 important change(s), 2026-08-17T02:24:49Z