Skip to content

Pin element-scoping semantics of queries over array-valued properties - #2437

Merged
kriszyp merged 4 commits into
mainfrom
test/query-array-element-scoping
Sep 1, 2026
Merged

Pin element-scoping semantics of queries over array-valued properties#2437
kriszyp merged 4 commits into
mainfrom
test/query-array-element-scoping

Conversation

@kriszyp

@kriszyp kriszyp commented Sep 1, 2026

Copy link
Copy Markdown
Member

Pins the element-scoping semantics of queries over array-valued (elements) properties, verifying the two open questions from the RQL 2.0 element-scoped-matching assessment. Tests plus a DESIGN.md row only — no production-code changes.

Chained conditions are same-element scoped on both execution paths. prepareConditions collapses a chained condition (a=ge=X&=le=Y) into a single range comparator before execution, so the indexed path (one range scan over per-element index entries) and the unindexed path (per-element some over the collapsed predicate in attributeComparator) agree: for {skiLengths: [172, 174, 181]}, ge=175&=le=180 correctly does NOT match (no single element is inside the range), while the independent form skiLengths=ge=175&skiLengths=le=180 correctly DOES (181 and 172 witness the legs separately). filterByType never sees chainedConditions — there is no path that silently drops a leg; unsupported chain shapes (|=, non-range comparators, a second &= leg) are rejected loudly.

contains over an array is a per-element toString() substring test for string and numeric elements alike, and never scans by range (full-scan filter on both paths), so indexed and unindexed attributes agree on membership.

Queries run through a real RequestTarget (the object REST constructs from the query string), so the error-path assertions pin the SyntaxViolation messages REST clients actually receive, and fixtures sit on both range bounds so ge/le vs gt/lt inclusivity is exercised exactly.

Three defects were found during verification and filed; each is pinned at current behavior (the raw duplicated index-scan output doubles as the observable proof that the per-element index served the query) with a skipped twin asserting the correct behavior:

Also adds a Q&A row to resources/DESIGN.md documenting the chain-collapse invariant and pointing at this suite.

For the human reviewer

  • Tests titled pins harper#NNNN assert today's wrong results on purpose so the suite stays green until the defect is fixed — at that point they fail loudly and their it.skip twins take over. If you'd rather not pin wrong behavior at all, deleting the pins and keeping only the skipped twins is a small change, at the cost of losing the only green signal that the per-element index (not a table scan) serves these queries.
  • The review's decision ledger flags that this PR freezes three behaviors as documented contract: same-element chaining, independently-existential repeated conditions, and numeric contains matching by decimal string. All three match the RQL 2.0 direction, but a human should confirm they are meant to be durable API surface.
  • The independent-conditions indexed test uses unique ids (not raw dup counts) because which leg leads the scan is estimate-dependent; the deterministic dup pins live on the single-condition tests.

Verification

  • npx mocha unitTests/resources/query-array-scoping.test.js — 17 passing, 4 pending (the skipped defect twins).
  • npm run test:unit:resources — 1892 passing, 27 pending, 0 failures.
  • End-to-end route: the tests drive RequestTargetTable.search, the same parse/plan/execute layers REST requests traverse; behavior was additionally probed with programmatic condition shapes, scalar and array attributes, indexed and unindexed.
  • Prettier + lint:required clean.

Refs #2433, #2434, #2435

—Claude Fable

🤖 Generated with Claude Code

Review-Coverage: authored=claude; ran=codex; blocked=gemini(auth); declined=cursor-grok,cursor-composer,domain; rounds=3 @ 91c8232

Human-Review-Need: 3 @ 91c8232

kriszyp and others added 2 commits August 31, 2026 22:50
Verifies (with unit tests) that chained conditions (a=ge=X&=le=Y) are
same-element scoped on both the indexed and unindexed execution paths —
the planner collapses the chain into one range comparator, the index
holds one entry per element, and the unindexed filter applies the
collapsed predicate per element — while repeating the attribute as
independent conditions stays independently existential. Also pins
contains over arrays as a per-element toString() substring test for
string and numeric elements, consistent across indexed/unindexed.

Documents three defects found during verification, each pinned at
current behavior with a skipped test asserting the correct behavior:
- #2433 REST chained-leg values are never type-coerced (silent
  superset/empty results on numeric attributes)
- #2434 index scans over elements attributes return one result per
  matching element (duplicate records)
- #2435 chained-condition error paths (second &= leg unparseable,
  |= rejected as 500 with the wrong message)

Refs #2433, #2434, #2435

Co-Authored-By: Claude Fable <noreply@anthropic.com>
…bservability

- Drive queries through a real RequestTarget so the error-path tests pin
  the SyntaxViolation messages REST clients actually receive
- Add a fixture on the exact upper bound (180) so ge/le vs gt/lt
  inclusivity is exercised on both sides
- Pin the raw duplicated output of single-condition indexed scans (the
  duplicate ids are the observable proof the per-element index served
  the query) and mirror a two-match fixture on the string column
- Trim narration comments; reuse collectIds in the skip twins

Co-Authored-By: Claude Fable <noreply@anthropic.com>
@kriszyp kriszyp added this to the v5.3 milestone Sep 1, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request documents the behavior of chained conditions over array values (SAME-ELEMENT scoping) in DESIGN.md and adds a comprehensive test suite in query-array-scoping.test.js. The review feedback correctly identifies style guide violations in the new test file, specifically recommending the use of the 'node:' prefix for the assert module import and advising the use of explicit strict assertions (assert.strictEqual and assert.deepStrictEqual) instead of loose ones.

Comment thread unitTests/resources/query-array-scoping.test.js Outdated
Comment thread unitTests/resources/query-array-scoping.test.js Outdated
kriszyp and others added 2 commits September 1, 2026 00:53
Co-Authored-By: Claude Fable <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kriszyp
kriszyp marked this pull request as ready for review September 1, 2026 21:25
@kriszyp
kriszyp merged commit eeadbdd into main Sep 1, 2026
46 of 47 checks passed
@kriszyp
kriszyp deleted the test/query-array-element-scoping branch September 1, 2026 21:26
@claude

claude Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

kriszyp pushed a commit that referenced this pull request Sep 2, 2026
Pre-existing drift from #2437 under the current prettier 3.9.x; formatting only,
no behavior change. Format Check fails identically on main without it.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Sep 2, 2026
`main` has been red on Format Check since #2437 landed
unitTests/resources/query-array-scoping.test.js unformatted, so every branch that
merges main inherits the failure. Unrelated to this PR; here only so this branch's
Format Check is green.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Sep 2, 2026
* fix: keep Rocks audit retention cleanup scheduled

* Reset Rocks audit cleanup backoff after purging

* Keep Rocks cleanup cadence independent of retention

* Tie Rocks retention cadence to the retention window and retire the loop on close

The Rocks cleanup delay is now a pure function of the pressure-adjusted retention
window rather than the LMDB per-entry backoff: segment eligibility only changes on
rotation/flush, so reacting to a delete count just rescanned the same files. Rocks
re-arms on the last worker only, so a reclamation signal received elsewhere stays
one-shot instead of starting a second store-wide purge loop.

stopAuditCleanup() retires the loop from the database close, drop, and branch-close
paths. The in-pass status check already declined to re-arm on a closing store, but a
pass already on the timer had to fire first to reach it.

Adds a live end-to-end regression that fails on main with purgeRuns stuck at 0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Deregister storage reclamation when a database closes, and cover the pressure cadence

Only closeBranchHandles dropped its reclamation registrations; closeDatabase and
dropDatabase never did, so a closed or dropped store left a handler — and the closure
pinning the store — registered for the life of the process, with every re-open of the
same path appending another. That hazard is what removeStorageReclamation's own
docstring describes; the close and drop paths now match the branch path.

Adds the pressure-cadence regression the earlier commit left to inspection: it drives a
real reclamation signal through a path-scoped ratio getter and pins the shortened delay,
so dropping the priority term from the cadence fails rather than passing silently.

Widens the live regression's waits, whose cadence floor makes two passes ~20s.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Correct the single-loop ownership claim and isolate the cadence test's store

The re-arm's last-worker conjunct cannot be reached through either arming path:
onStorageReclamation registers its handler only on the last worker (it takes no
skipThreadCheck) and the store-open arm gates on the same index. The comment and
DESIGN.md said a reclamation signal arrives on an arbitrary worker and this guard
absorbs it, which is false — ownership sits at the arming sites, and the conjunct is
a backstop for a direct caller of the exported scheduleAuditCleanup. The guard stays;
only the claim about why changes.

The cadence test now opens its own store. Patching global.setTimeout process-wide while
the shared fixture's Rocks loop is live on a real timer let a stray pass land in
`scheduled` and break the per-pass assertions.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Stop cleanup before table stores close, and scope the cadence test's timer capture

Round-2 review findings. closeDatabase closed each table's primaryStore and indices
before retiring the audit loop, so an LMDB pass inside removeAuditEntry could fire a
delete-type tombstone callback against an already-closed primary store; the stop pass is
now hoisted above the table closes.

The cadence test's own store did not actually isolate it: the shared fixture's audit loop
stays armed on a real 10s timer, and its re-arm landed in `scheduled` through the patched
global, so `scheduled.length` could exceed 1. The capture now takes only this test's own
sub-second delays, and the comment claiming isolation is gone.

Covers the drop path's deregistration, names rocksdb-js#805 as the reader-gap tracker,
and records that both re-arm guards are Rocks-only rather than engine-independent.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Pin rocksdb-js 2.8.0 for the purge extent-resolution fix

Continuous retention needs 2.8.0, not the unreleased #799. The pinned 2.7.1 already
refuses to purge above the durable flush floor, but at the boundary segment it compares
logFile->size without resolving the extent: a lazily-registered segment reads 0, so
`0 > positionInLogFile` is false, the guard passes, and the segment is deleted with a tail
that never reached RocksDB. 2.8.0 calls ensureExtent() there and refuses to purge when the
extent cannot be resolved; its new `should not purge a log file whose tail is past the
flushed position` regression covers exactly that case and does not exist in 2.7.1.

One purge per boot rarely lands on whichever segment sits at the floor. A retention-derived
cadence takes a fresh shot at it every pass, which is what makes this a prerequisite for
the cadence change rather than routine dependency hygiene.

rocksdb-js #799 remains a safety improvement on top: it restricts deletion to a contiguous
oldest prefix, so a non-contiguous purge cannot leave a hole that silently truncates a
reader (HarperFast/rocksdb-js#805).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Keep msgpackr deduped across the rocksdb-js bump

rocksdb-js 2.7.1 declared msgpackr 2.0.5, matching this package's exact root pin, so npm
deduped them. 2.8.0 declares 2.0.6, which the exact 2.0.5 pin cannot satisfy, so the install
nested a second copy under node_modules/@harperfast/rocksdb-js — two module instances, each
with its own extension registry and its own msgpackr-extract native addon.

Moving the root pin to 2.0.6 restores a single instance. The release is one commit,
"Reject array and map lengths that exceed the remaining source data", which is a decode
bounds fix worth having on its own.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Retire audit cleanup on the legacy per-table drop path too

The three teardown paths in databases.ts stop the cleanup loop and deregister reclamation
before releasing a root store, but Table.ts's legacy per-table drop branch closed and
unlinked its primaryStore with neither call, so an armed pass could fire against a store
that is closed and deleted. Reachable on LMDB only; RocksDB never takes this branch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Break the LMDB cleanup loop on stop, and stop handing foreign timers a dead stub

stopAuditCleanup() retired a scheduled pass but not one already suspended at
`await removeAuditEntry` or `await new Promise(setImmediate)`. closeDatabase sets the flag
and then closes the primary and root stores in the same tick, so the resumed iteration drove
a live lmdb-js cursor and getEntry()/remove() against a closed env. The loop now re-checks
the flag each iteration, which is what the teardown comment already claimed.

Resolving definedRoot before the hoisted stop pass collapses closeDatabase's two stop loops
into one.

The cadence and pressure tests replaced global.setTimeout wholesale, so any timer another
subsystem armed inside the window got a stub that never fires and whose clearTimeout was
swallowed — a dead replication retry surfacing as an unrelated flake in a later file. They
now delegate anything above their own sub-second delays to the real timer.

DESIGN.md's mapped-segment reasoning is POSIX-only; on Windows deleting a mapped segment
raises a sharing violation, so the purge throws, re-arms and makes no progress while a
consumer holds the mapping. The continuous cadence makes that a steady state, and the Rocks
retention test skips win32, so nothing covers it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Check for retirement before advancing the cursor, not after

The per-iteration guard sat in a for-of body, which runs after the iterator has already
produced its record — so a close landing mid-pass still advanced an lmdb-js cursor over a
closed env before the check could stop it. The loop is now driven explicitly so the guard
precedes every next(), and it releases the cursor on the way out, which for-of had been
doing for it.

The trailing updateLastRemoved() is skipped for the same reason: it writes to the audit
store, which closeDatabase has already closed by the time a stopped pass reaches it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Make audit-cleanup retirement a drain barrier, and retire the store the legacy drop actually holds

Review follow-ups on #2338.

The legacy per-table drop retired `primaryStore.auditStore`, which nothing
assigns: `openAuditStore()` assigns `rootStore.auditStore` and `makeTable()`
captures the store it was handed. The drop now retires that capture.

`stopAuditCleanup()` only latched the loop closed; it could not say anything
about a pass already suspended inside `await removeAuditEntry()`. lmdb-js
stamps the DBI number into its write instruction synchronously and the native
writer consumes it later, so that pass still has a delete pending against the
primary and audit DBIs while teardown closes them. It now returns a drain
barrier, awaited by `dropDatabase()` and the legacy drop. `closeDatabase()`
and branch `close()` stay synchronous; every environment touch left in a
resumed pass — cursor advance, cursor release, marker write, re-arm — re-checks
the root's status instead, which is also what covers `resetDatabases()`,
where roots close with no retirement call at all.

The last-removed marker is now awaited inside error containment and retained
until it commits: a pass that deletes nothing never reaches the write again, so
one transient failure would leave the recorded boundary permanently behind the
entries already removed. The pass's own timer callback gets an outermost
backstop so a failing log sink cannot reject it outward or retire the loop.

Co-Authored-By: Claude Opus <noreply@anthropic.com>

* Read the cleanup cadence off the store instead of replacing the global setTimeout

Two audit-retention regressions asserted the cadence by swapping `global.setTimeout`
for a stub that captured every timer armed with a delay under a second. That global
is shared by every audit store in the process: under `HARPER_STORAGE_ENGINE=lmdb` the
shared fixture's own cleanup loop is still re-arming at a sub-second cadence by the
time these tests run, so its timer landed in the stub's queue, the "a pressure signal
should arm a cleanup pass" assertion saw two timers, and the failing test then hung
forever in its own `finally` — `runReclamationHandlers()` was still awaiting a pass
whose timer the stub had swallowed and would never fire. `.mocharc.json` sets
`timeout: 0`, so that wedged the whole LMDB unit run rather than reporting a failure.
It reproduces on the branch as pushed; it stays hidden on RocksDB, where the shared
store's cadence never drops below a second.

The cadence now lives on the audit store, which is the loop's own observable state,
and both tests drive real passes and read it. No fake timers, no global patching, and
the pressure test is a real 100ms pass rather than a synthetic one. Both still fail
when their formula term is removed.

Also adds the regression for the marker write that initializes a new audit store:
`openAuditStore()` is synchronous and returns the store, so nothing downstream can
own that write's rejection, and only the cleanup-pass caller was covered.

Co-Authored-By: Claude Opus <noreply@anthropic.com>

* Contain the initializing marker write against its own log sink, and trim narration

Pre-push review follow-up. The `.catch` on the audit store's initializing last-removed
write called the logger directly, so a failing sink turned one contained rejection into
an uncontained one — the same double failure the cleanup pass's outermost backstop
already guarded. Both now go through one `warnContained()` helper, and the
initialization regression runs against a throwing sink as well as a working one.

Also drops comment narration the review flagged: review history and restatements of
the statements below them.

Co-Authored-By: Claude Opus <noreply@anthropic.com>

* Drop the last cadence comment the review flagged as narration

Co-Authored-By: Claude Opus <noreply@anthropic.com>

* Format the test file main landed unformatted

`main` has been red on Format Check since #2437 landed
unitTests/resources/query-array-scoping.test.js unformatted, so every branch that
merges main inherits the failure. Unrelated to this PR; here only so this branch's
Format Check is green.

Co-Authored-By: Claude Opus <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Chris Barber <chris@harperdb.io>
kriszyp added a commit that referenced this pull request Sep 2, 2026
…et (#2459)

* Bound a request-path commit's conflict retries to its queue-time budget

rocksdb-js 2.8 returns control from a commit parked on a conflicting write
intent every ROCKSDB_JS_PARK_TIMEOUT_MS even when the holder never releases, so
Harper sees a stream of transient conflicts rather than one hung commit. The
40-attempt cap alone then keeps the initiating request pending for roughly 40
park timeouts - minutes past the queue limit an operator configured, which is
what left a control-plane node rejecting every write for 12 minutes.

Carry one elapsed clock per logical commit on its chain root, stamped at the
first native submission, and read it at both retry decisions. Past
max(storage.maxTransactionQueueTime, timeoutBudget) the commit takes the
existing retry-exhaustion abort path, logs the stuck-commit identity through the
same rate limiter checkOverloaded() uses, and throws a 503
TRANSACTION_COMMIT_CONFLICT_TIMEOUT. Only a chain root that has not rotated
through a mid-scope commit reports retryable: anywhere else an earlier store
already landed durable audit entries a replayed request would repeat.

Outstanding-commit tracking stays per native attempt - it measures native
liveness and drives thread-wide load shedding, so back-dating it would let one
uncapped source-apply retry shed every unrelated request on the thread.
Source-applied writes stay exempt from the deadline as they are from the cap.

Refs #2450

Co-Authored-By: Claude Opus <noreply@anthropic.com>

* Address pre-push review: split log cooldowns, pin the production stamp

- Give the abandonment log its own cooldown slot. It shared checkOverloaded()'s,
  and that one fires on every bystander write during a wedge, so the line naming
  the abandoned transaction - the only server-side record of why a request was
  failed - lost the slot to a line naming a different commit.
- Add a test that observes production arming the clock at the first native
  submission and releasing it at settle. Every other case plants the clock, so
  the one line that arms it could have been deleted with the suite still green.
- Pin the snapshotFree half of the retryable decision: a scope that landed an
  earlier mid-handler segment must report retryable: false.
- Trim two comments that restated the adjacent code.

Co-Authored-By: Claude Opus <noreply@anthropic.com>

* Document the commit-conflict deadline in DESIGN.md

Co-Authored-By: Claude Opus <noreply@anthropic.com>

* Reformat query-array-scoping.test.js to unblock the format check

Pre-existing drift from #2437 under the current prettier 3.9.x; formatting only,
no behavior change. Format Check fails identically on main without it.

Co-Authored-By: Claude Opus <noreply@anthropic.com>

* Apply repo style: loose null checks, strict test assertions

Co-Authored-By: Claude Opus <noreply@anthropic.com>

---------

Co-authored-by: Kris Zyp <kris@harperdb.io>
Co-authored-by: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Sep 3, 2026
…ng (#2493)

* test: pin one-result-per-record for indexed elements-array scans (harper#2434)

Enables the three harper#2434 it.skip twins from #2437 and converts the
assertions that pinned the duplicating output. Adds coverage the fix has to
satisfy: a limit/offset page sweep across a multiplicity>1 boundary, a
GraphQL-declared [Int]/[String] @indexed table (type 'array', not the
programmatic elements-only shape that takes coerceType's autoCast branch),
array primary keys whose repeated entries decode to distinct instances,
descending scan order, count: 'exact', equality comparator aliases, and the
scope boundary for an undeclared indexed attribute holding arrays.

Extends the REST integration suite with an unpaged range and a paged sweep
over the existing [String] @indexed fixture.

17 of these fail on origin/main; the 4 no-regression and pin assertions pass
there and must keep passing.

Refs #2434

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KCwRQH1DhJmPMrX6frsSoh

* Collapse indexed elements-array scans to one result per record

A record with an array attribute gets one index entry per element, and the
secondary-index scan forwarded every entry as its own result. Anything counting
positions downstream — most visibly the limit/offset page slice — was therefore
counting index entries, not records, so a record with several matching elements
came back several times and consumed several page slots.

searchByIndex now collapses the scan to distinct primary keys, behind two gates
that keep it off everything that cannot repeat a record: the attribute must be
declared multi-valued (`elements`), and the scan must span more than one indexed
value. Element equality is exempt because `[indexedValue, primaryKey]` is unique,
so a point scan already reaches each record once — verified on both engines,
including an array that repeats an element.

The collapse cannot be a run-collapse: the composite key sorts on the indexed
value first, so a record's entries are interleaved with other records'. It keeps
a set of the primary keys already yielded, canonicalized through ordered-binary
so an array primary key compares by value rather than by decoded instance.
flattenKey is not usable for that: it joins with a NUL byte, so the key ['t', 7]
and a scalar string id with those same bytes fold together and one record is
lost. A test covers exactly that pair.

Fixes #2434
Refs #1063

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KCwRQH1DhJmPMrX6frsSoh

* chore: realign the DESIGN.md contract table after the widened row

The new sentence widens the column, so prettier realigns every row of the
table. No other row changed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KCwRQH1DhJmPMrX6frsSoh

* Track scalar and encoded primary keys in separate dedup sets

Encoding a non-scalar key to a latin1 string and putting it in the same set as
scalar ids reintroduces the collision the encoding exists to avoid: a string id
may legitimately carry exactly the bytes an array key encodes to, and the two
would fold together, dropping a record. Keeping the two key spaces apart makes
that impossible and leaves scalar ids unencoded, which is the common case.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KCwRQH1DhJmPMrX6frsSoh

* Address pre-push review: reuse writeKeyId, preserve error continuation

- Drop the local key-encoding buffer for writeKeyId, which already centralizes
  exactly this canonicalization with the same rationale and sizes its own
  allocation, removing both the fixed-buffer truncation hazard and a duplicate
  implementation of store key equality.
- Collapse through map + SKIP rather than filter. filter clears
  continueOnRecoverableError, so inserting one would have silently broken the
  recoverable-error continuation the response path installs with mapError.
- Cover starts_with, where two elements of one record share a prefix, and two
  scans interleaved a step at a time.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KCwRQH1DhJmPMrX6frsSoh

* Pin the all-string array-vs-scalar key collision and trim narrating comments

The existing identity pair used a numeric element, which carries a type tag and
so dodges the collision that actually exists: an all-string array key joins with
a bare NUL while a scalar string escapes one, so ["acme","west"] and the scalar
of the same bytes are two records the store keeps apart and one key to any
canonicalization with a single key space.

Comments trimmed to the three that carry a why the code cannot.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KCwRQH1DhJmPMrX6frsSoh

* Pin why the collapse runs after the condition filter, not before

The condition filter tests one array element at a time, so a record whose
lowest-sorting entry fails it must still be reached by a later entry that
passes. Collapsing first would settle that record on the failing entry and drop
it from the result. A review leg proposed exactly that reordering as an
optimization, so the invariant is now stated in code and pinned by a test.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KCwRQH1DhJmPMrX6frsSoh

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant