Repository navigation
Add full-text boosts and optional scoring conditions - #3847
Draft
kylebernhardy wants to merge 11 commits into
Draft
kylebernhardy wants to merge 11 commits into
kylebernhardy wants to merge 11 commits into
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces query relevance controls for full-text search, allowing users to scale required clause scores using boost and add optional scoring clauses using should. These features require native ABI 14 and query API 8, while maintaining backward compatibility with ABI 13 and query API 7 for ordinary searches. The changes span across query compilation, validation, and execution paths, alongside comprehensive integration and unit tests. A review comment suggests refactoring duplicated logic in fullTextQueryIndex.ts that ensures native bindings are loaded and validates the scoring runtime capabilities into a private helper method.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
⊙ Problem
Applications cannot require a broad full-text match while awarding a per-request phrase bonus. Index field weights apply a shared ranking policy. Closes #3843 — Expose full-text query boosts and optional scoring clauses through Table.search and REST.
🤔 Does this belong in Harper's query contract?
Yes: Harper must authorize the fields that affect ranking. The wrapper owns native scoring; Harper exposes a bounded same-index contract alongside existing comparators.
💡 Solution
Full-text conditions accept
boostand positiveshouldclauses. Required matches remain eligible without an optional match; optional-only matches stay excluded. Applications can favor title phrases without changing the index definition.Draft and merge blocked: release the compatible wrapper and update the production dependency pin before merging. With this Harper change, the published fulltext 0.8.0 wrapper causes scoring requests to return 400. The live feature tests deliberately require the new runtime.
⚖️ Alternatives
🔧 Changes
Required eligibility and optional relevance
resources/ResourceInterface.ts adds the public clause types. resources/indexes/fullTextQueryProtocol.ts validates float32-rounded boosts in 0–100, rejects negative zero, bounds nonempty optional lists, and rejects unsupported descriptor properties or option locations.
resources/search.ts compiles the required leaf into a native boost only when supplied, and builds
optional(required, bonuses)only whenshouldexists. Positive optional leaves also reach the existing current-source highlight path. The absence of controls retains the prior expression and avoids optional-clause arrays or per-leaf helper closures.From request to result
The request keeps its required membership while optional matches contribute to ranking.
Runtime compatibility and request paths
resources/indexes/fullTextNativeBinding.ts accepts exactly ABI 13/query API 7 and ABI 14/query API 8, keeping lifecycle and mutation requirements intact. Shared named constants distinguish the legacy pair from scoring support. The parallel wrapper PR Expose fulltext operation telemetry and latency histograms claims 14/7 plus telemetry API 1, which this validator rejects. Reconcile the unreleased contracts before releasing either artifact; if telemetry ABI 14 ships first, scoring needs a fresh ABI and this pair/pin must change together.
resources/indexes/fullTextQueryIndex.ts rejects scoring on the older runtime and traverses optional/boost nodes for fuzzy and autocomplete policy. Non-waiting searches certify coverage synchronously before capability discovery yields, preserving
Harper-Index-Coverage; ordinary queries retain their prior scheduling and reuse the existing fuzzy-policy traversal to detect scoring. Cached capabilities do not yield; cancellation rejects without waiting for cold capability discovery, without cancelling shared initialization or opening a reader. Direct index callers must provide a compiled expression for scoring controls. Closed checks after capability loading also prevent a zero-result scoring request from succeeding after index shutdown.dataLayer/harperBridge/ResourceBridge.ts rejects request-root scoring controls before operations normalization, preserves valid leaf controls and rejects unsupported negated controls before that mapping. A method-entry shared-validator call also rejects misplaced operation-root
boostandshouldbefore normalization can discard them. validation/searchValidator.ts admits the narrow optional-leaf shape. Existing HTTPQUERYaccepts the structured target; FIQL syntax is unchanged..github/workflows/unit-test.yml uses the built shared runtime validator for its native smoke check, so both supported runtime pairs follow the same compatibility contract.
Documentation and compatibility boundary
resources/FULL_TEXT.md documents the API, numeric policy, source permissions, shared budgets and older-runtime failure. resources/DESIGN.md records the required-membership/source-authorization invariant and its named regressions; DESIGN.md updates the existing full-text navigation entry.
resources/indexes/DESIGN.md records the legacy and scoring runtime pairs and the shared CI validator.
No persisted index identity or production dependency pin changes. A real-addon compatibility check passed ABI 13 seed → ABI 14 reopen/write → ABI 13 reopen/read. Companions: Add query boosts and optional full-text scoring and Document full-text boosts and optional scoring.
✅ Verification
Claude and Composer completed the requested cross-layer review at
6587206, including the prior cancellation/error-precedence fixes. The new Table validator allocation is removed; the wrapper release/pin remains blocking.End-to-end route: deployed two-worker HTTP
QUERY, operations and Table fixture against the final deadline-aware optimized local ABI 14 wrapper addon (SHA-256652d1aaafd045641c1880a6c08e1a52099df5aa599de91fabf64f1ec6191cd6b). The final artifact rerun passed 113/113 focused reader/native/scoring tests and 20/20 deployed tests at Harpera53592d. Logs:query-relevance-harper-deadline-final-focused.logandquery-relevance-harper-deadline-final-deployed.log. 20 tests passed, including inversion of baseline ranking, boost scaling, optional bonus scores, source permissions, pagination and non-waiting coverage headers.Focused regression coverage
unitTests/resources/fullTextQueryIndex.test.js checks rejection before a pending cold capability gate is released, without opening a reader. Cold and warm API-7 scoring must return permanent 400 before coverage 503; removing only the warm check makes the strengthened test fail. The real-native test also proves the compiled clause boundary: 127 single-field optional terms compile to 256 clauses and succeed; 128 compile to 258 and return 400, while the coarse Harper precheck accepts either shape. The reader/native/scoring files passed 111 tests against the rebuilt paired addon; deployed verification passed 20/20 after the native-order repair. The final warm-cache response separately passed both API cases. The later bridge response passed the native file 11/11 and deployed suite 20/20; the final close/client-error response passed 102 reader/policy tests. All scoped builds, typechecks, lint and formatting checks passed. Exact built CI smoke accepted ABI 14/query API 8.
Crash recovery: 3 passed with the paired local artifact. Build, type, lint/format and design-index checks passed.
Broader verification limitations
Canonical resources completed: 6,274 passed, 235 pending, 7 failed. Three crash fixtures selected the installed ABI 13 platform addon beneath the paired ABI 14 facade; after staging the matching local artifact, all three exact crash cases passed. The remaining unchanged tests reported a fractional future tombstone version, a missing RocksDB flush-policy config value, and two in-flight subscription replay delivery failures. The tombstone case passed in isolation; the broad result remains non-green.
Canonical main stopped at an unchanged Darwin zombie-process cleanup hang. The rerun excluded exactly that test and removed the inherited npm allow-scripts variable. It was stopped as incomplete after a second unchanged preparation-lock hang: no progress for several minutes, roughly 217% CPU and a 4.7 GiB process footprint, with repeated filesystem-stat callbacks in a stack sample. Earlier failures included the untouched npm 12 bundle fixture, the configured lifecycle-script allowlist, and a preparation-lock deadline assertion. No global configuration was changed.
The full integration selection completed non-green: 2,699 passed, 68 failed, 349 cancelled, 19 skipped across 3,137 tests. It began before the final review-fix commit and is not an immutable final-head gate. GitHub component-template setup fails in unchanged
components/Application.ts, whose npm-pack parser assumes array output while npm 12 emits an object. Authentication and direct tar/tar.gz component routes passed. These failures are outside the scoring path; the full gate is not claimed green.One initial mixed run observed a native Tantivy phrase assertion. It did not reproduce in fresh isolated, final combined or published-wrapper baseline checks. No workaround was added.
Hosted native CI on Linux, macOS and Windows each reports 49 passed and one API-7 scoring failure. Four resource jobs fail that same case; Bun integration also hits the gate. Native CI run.
Review tracking: native boundary tests qualify the budget notes; permissions overlap remains merge coordination. The owning feature issue carries all three exact review markers. The review receipt now records those issue URLs. The issue also records the verified telemetry ABI collision and requires the metadata merge to preserve scoring-aware walkers and lifecycle fixes.
Layer review response and remaining limits
Requested repeat review
The fifth whole review completed at
2ff0661, with late Grok feedback folded into adjudication. A local domain pass found Operations silently discarding request-rootboost/should. Commit19d5cd9adds one shared-validator call before normalization and real bridge/public Operations regressions (34 added lines in three files). Both malformed requests fulfilled before the fix and reject with 400 afterward. The native file passed 11/11; deployed tests passed 20/20; build, typecheck, lint and formatting passed. This 34-line bridge response was not independently re-reviewed as code. Commita53592dadds the subsequent 49-line close/test response: two scoring-path closed checks, waiting/non-waiting zero-result close regressions, and an explicit 400 matcher for malformed optional clauses. Both close regressions returned an empty result before the fix and reject with 503 afterward; the reader/policy files passed 102 tests. This response was also not independently re-reviewed as code.🤔 Shared validation stays in Table
The framing recheck cleared the factory-scope validator: Harper has the principal and current schema; the wrapper has neither. Required and optional leaves reuse the same authorization logic. No schema cache or custom scorer is added. The existing definition lookup and opt-in field-list checks still allocate; no latency or GC improvement is claimed.
🤔 Bounded final response
The earlier 128-line validator/additive-score/warm-cache response was included in the fifth whole comparison at
2ff0661. Only the subsequent 34-line bridge and 49-line close/client-error responses remain outside that immutable review snapshot; their fail-before/pass-after checks are recorded above. No further whole-code review is claimed.The earlier fourth whole pass found the validator allocation after cancellation/precedence fixes. The fifth whole pass found the bridge-root omission and zero-result close response above. Subsequent work stayed within those ingress/lifecycle guards and their regressions; no further whole pass is claimed. Native version-pair tests deliberately permit both supported pairs during transition; this is local compatibility qualification, not proof that the new artifact is published. Concurrent cold configured-binding initialization remains a small opt-in cost, with no in-flight memo added without evidence of material impact.
The review harness flagged its marker-table parser despite completed manual and adjudicator traces. Capability state publishes after validation, cold awaits recheck cancellation, abandoned binding failures remain observed, listeners are removed and closed scoring requests reject before zero-result success or reader use, including requests resumed after capability loading. This trace adds no outside-family coverage. Historical Grok findings below are superseded by these fixes and the recorded issue qualifications.
PR QA: skipped @ a53592d — dispatch unavailable (CLI is not installed).
Comment generated by kAIle (GPT-6)
Release target: main — new public search controls.
Related PRs: #3651 overlaps (Isolate permissions when reusing nested relationship selectors), #3846 overlaps (Allow stored-field sorting and exact counts for full-text search; query reader, compiler and integration fixtures), 2 others independent
Complexity: complicated
Deferred findings
1806ea835e950478nit maintainability —unitTests/resources/nativeFullTextTableSearch.test.js— Open PR Isolate permissions when reusing nested relationship selectors #3651 overlaps the real-native full-text test — filed Expose full-text query boosts and optional scoring clauses through Table.search and REST #38437eefabe7d730624enit concurrency —resources/indexes/fullTextQueryIndex.ts— Configured binding can resume reader creation after close — filed Expose full-text query boosts and optional scoring clauses through Table.search and REST #38434550e3d8d6a7afc1none test-coverage —unitTests/resources/fullTextScoring.test.js— No host/native maximum-clause boundary regression test — filed Expose full-text query boosts and optional scoring clauses through Table.search and REST #3843Late Grok findings
5 surviving (highest major) — Grok reviewed 4de87cb, ruled against 5696976
unitTests/resources/nativeFullTextTableSearch.test.js:112— the pinned API 7 wrapper rejects the new scoring calls, so the real-native unit and deployed integration tests cannot passresources/search.ts:368— every ordinary full-text query now allocates a closure and empty optional-clause arrays without any measurement of the hot-path costresources/indexes/fullTextQueryIndex.ts:293— aborting during the scoring capability await still lets reader acquisition begin before cancellation is observedresources/indexes/fullTextQueryProtocol.ts:122— the host precheck counts optional entries but not their compiled wrappers, so a boundary request performs reader/native work before receiving the less-specific native 400unitTests/resources/nativeFullTextTableSearch.test.js:66— the real-native test overlaps an open related PR, leaving merge-order and conflict riskNot standing: 4 resolved, 1 separate-issue. Grok ran 20m (20m from launch to report); the fold adjudication (
cursor-grok@4de87cb64305-316861fa, codex) took 2m.Late Grok findings
8 surviving (highest blocker) — Grok reviewed 2ff0661, ruled against 19d5cd9
package.json:298— pinned query API 7 makes the unconditional API-8 scoring suites failresources/indexes/fullTextQueryProtocol.ts:129— scoring validation allocates an object-key array and allowlist per optional-clause key without measurementresources/indexes/fullTextQueryProtocol.ts:118— the host accepts optional-clause counts that exceed the native compiled-node budget once wrappers are addedunitTests/resources/nativeFullTextDerivedIndexLifecycle.test.js:209— the published-package test no longer pins its exact ABI/API pairresources/indexes/fullTextQueryIndex.ts:260— close can finish while a cold scoring search continues binding initialization and mutates the closed indexunitTests/resources/fullTextScoring.test.js:38— malformed-clause tests accept any thrown error, including an internal TypeError instead of the required 400integrationTests/resources/full-text-search.test.ts:348— the scoring test fails if the correct baseline already has the promoted orderresources/indexes/fullTextQueryIndex.ts:1137— concurrent first scoring requests independently initialize the configured bindingNot standing: 1 merged, 2 resolved, 1 separate-issue. Grok ran 15m (15m from launch to report); the fold adjudication (
cursor-grok@2ff0661e1fb0-e3d64535, codex) took 3m.Review-Coverage: authored=codex; ran=cursor-composer,claude,cursor-grok; late=cursor-grok:adjudicated; adjudicated=domain; blocked=gemini(auth); declined=muse; rounds=6; full=5 @ a53592d
Review-Attention: deep ~45m (critical: ResourceBridge.ts, Table.ts +4; decisions: compatibility-rollout, relevance-api-shape, do-less-alternative) @ a53592d