test: identifier limit boundary + empty-identifier coverage (#1981/#1982 follow-up)#2003
Open
markmhendrickson wants to merge 1 commit into
Open
test: identifier limit boundary + empty-identifier coverage (#1981/#1982 follow-up)#2003markmhendrickson wants to merge 1 commit into
markmhendrickson wants to merge 1 commit into
Conversation
follow-up) Adds the two tests flagged NON-BLOCKING in the review of #1982, covering `retrieveEntityByIdentifierWithFallback`. Test A — limit boundary across the two snapshot passes. The exact pre-pass applies `.limit(limit)` per field query while the JS-scan fallback is separately capped at 500 and appends every further match; nothing re-applies `limit` to the merged `snapshotMatches` array, so the caller-facing bound rests on the final `queryEntities({limit})` call. Seeds 3 contacts sharing one email and pins that the result is bounded and deterministic (a stable ascending-entity_id prefix), not an arbitrary subset. Documents the observed `total` semantics: for the snapshot-field pass `total` is the size of the CAPPED set (`withObservations.length`), not the true match count — so `total === limit` is indistinguishable from "exactly `limit` matches exist" and callers cannot use it to detect truncation. Test B — empty / whitespace-only identifier. Currently FAILING, documenting a live bug rather than papering over it: an identifier that trims to empty makes the canonical/alias stage build `canonical_name.ilike.%%`, which matches every entity row for the user. The handler returns match_mode "direct" with all of the caller's entities instead of an empty result. The test asserts the correct contract (total 0 / match_mode "none") and also guards the related exact-pass false positive, `.eq(lower(snapshot->>field), "")` matching rows that store that field as an empty string. No production source changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
|
🤖 Lanius — Ateles swarm, PR gate inheritance PR Gate Inheritance CheckParent issue resolved from PR #1982's
Blocking gate(s)
Per PR gate inheritance, any pre-impl gate (pm/ux/arch) left unsigned on the parent issue blocks descendant PRs — including this test-only follow-up. Resolution paths
No other commenter can clear this gate. 📎 Neotoma: issue GATE_INHERITANCE: blocked |
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.
Follow-up to the review of #1982 (merged), adding the two tests flagged NON-BLOCKING. Both target
retrieveEntityByIdentifierWithFallbackinsrc/shared/action_handlers/entity_identifier_handler.ts.No production source is changed by this PR.
Test A —
limitboundary across the two snapshot passesThe identifier handler bounds its two snapshot passes independently: the exact pre-pass applies
.limit(limit)to each per-field query, while the JS-scan fallback is separately capped at a fixed.limit(500)and appends every further match it finds. Nothing in between re-applieslimitto the mergedsnapshotMatchesarray, so the caller-facing bound rests entirely on the finalqueryEntities({ limit })call.The test seeds 3 contacts sharing one
emailvalue (whosecanonical_namedeliberately omits the email, so resolution must go through the snapshot passes) and asserts:entities.length <= limitatlimit: 1, and exactly 2 atlimit: 2;entity_idprefix (queryEntities' default sort is.order("id", { ascending: true })), not an arbitrary subset;totalsemantics (caller-facing contract)Verified empirically rather than assumed. For the snapshot-field pass,
totalis the size of the capped, returned set — not the true number of matching entities. The handler returnstotal: withObservations.length, so against 3 genuine matches:limitentities.lengthtotalConsequence: callers cannot use
totalto detect truncation or to paginate —total === limitis indistinguishable from "exactlylimitmatches exist". This differs from thesemanticpass, which returns the search's owntotaland can exceedentities.length. Documented in a comment on the test.Test B — empty / whitespace-only identifier (FAILING — live bug)
This test currently fails, deliberately. Per the review instruction, it asserts the correct behavior rather than encoding the buggy behavior.
The bug is real but sits earlier in the pipeline than the review predicted. The review anticipated a false positive in the exact pre-pass, where
.eq(lower(snapshot->>field), "")would match snapshot rows storing that field as an empty string. That failure mode is masked by a broader one at the canonical/alias stage:With an identifier that trims to empty,
normalizedis""and the pattern degrades to%%— which matches every entity row for the user. Resolution never reaches the snapshot passes at all.Observed behavior for both
identifier: ""andidentifier: " ", with two seeded contacts:total: 2,match_mode: "direct", returning both contacts — i.e. an empty identifier acts as a wildcard that dumps the caller's entities.total: 0,match_mode: "none".Failing assertion (
tests/integration/entity_identifier_handler.test.ts:983):The test seeds a contact with
email: ""plus a normal contact, so it guards both failure modes: the wildcard%%dump and the.eq(lower(snapshot->>email), "")empty-string-field false positive (the latter additionally pinned via an explicitby: "email"call, which routes straight to that query).Isolating the stages confirms the mechanism — with an
entityTypethat has no matching entity rows, the empty identifier correctly returnstotal: 0/"none", so the wildcard is unambiguously the canonical/aliasilikestage.Suggested fix (not applied here)
Guard early in
retrieveEntityByIdentifierWithFallback: ifidentifier.trim()is empty, return{ entities: [], total: 0, match_mode: "none" }before the canonical/alias query. That fixes the wildcard dump and, being ahead of the pre-pass, the empty-string-field false positive too. Left to a follow-up so this PR stays test-only and the bug is reviewed on its own merits.Verification
npx tsc --noEmit— clean (exit 0, repo-wide)npx prettier --writeon the touched filenpx vitest run tests/integration/entity_identifier_handler.test.ts --testTimeout=60000— 20 passed, 1 failed (21 total); the single failure is Test B, documenting the bug above. All 19 pre-existing tests still pass, plus Test A.🤖 Generated with Claude Code