Repository navigation
spec: define the v0.2 query corpus format - #5
Conversation
Empty commit to open the PR before the work lands. Co-Authored-By: Claude Opus 5 <[email protected]>
Adds §7, the contract between capture (v0.2) and the coverage check (v0.3), written before either exists because both read it. The format records shape only — collection, scope, filter tree, sort order — and omits values, project/database, limit/offset/cursors/select, and occurrence counts. Values are customer data in a committed file and do not affect index selection; counts would churn the diff on every run without changing what must be indexed. Vocabulary is aligned to the canonical index key of §5, so a corpus entry and an index key can be read against each other without translation. Two honesty constraints are written into the format rather than left implicit: implicit `__name__` is not materialised (the v0.3 oracle is Firestore, which applies the real rule), and RunAggregationQuery and find_nearest are counted as skipped rather than dropped silently — a discarded query would look like coverage. Sections 7-10 shift to 8-11. Co-Authored-By: Claude Opus 5 <[email protected]>
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthrough
ChangesQuery corpus specification
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SPEC.md`:
- Around line 373-375: Add the text language identifier to both fenced code
examples in SPEC.md, including the
`<collectionGroup>::<queryScope>::<where>::<orderBy>` example and the second
affected fence, by changing each opening fence to ```text while preserving their
contents.
- Around line 334-338: Update the privacy statement near “Values are not
recorded” to clarify that query values are excluded from the corpus, while
collectionGroup and fieldPath identifiers are still stored and may contain
tenant or schema-sensitive data. State that corpus files must therefore use
normal repository access controls.
- Around line 443-453: Expand the “Replay without values” specification to
define deterministic synthetic values: the scalar type, prohibited values, and
exact handling for IN, NOT_IN, NULL, ARRAY_CONTAINS_ANY, and composite filters.
Require value-independence validation before coverage can become reportable;
otherwise mark it inconclusive and do not report missing indexes when the
synthesized query may differ from the captured query.
- Around line 373-390: The canonical query-key format must safely encode
fieldPath identifiers containing structural delimiters such as :, |, ::, and
parentheses. Define a reversible escaping/encoding scheme in the SPEC and
implement matching encode/decode behavior in the key-generation and parsing
logic around src/key.ts, ensuring collectionGroup and field paths round-trip
without collisions. Add tests covering identifiers containing each reserved
delimiter while preserving existing canonical ordering behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
The format as written left a conforming implementation too much room, and three of its factual premises were wrong. Corrections. `Order.direction` is documented to default to ASCENDING, so an unspecified direction is normalised rather than skipped; only values with no published meaning are counted out. The claim that implicit ordering never reaches the wire is false for the Firebase Web SDK, which serialises the normalised orderBy including `__name__` — the corpus still records what was sent, but the section no longer explains that choice with a premise that does not hold. And a `StructuredQuery` reaches the server through `Listen` and `PartitionQuery` as well as `RunQuery`, so a suite whose only exercise of a collection is `onSnapshot` produced a corpus with no entry for it and no skip reason — coverage that was never measured, reported as coverage. §3 names that and the WebChannel transport as implementation gaps rather than folding them into the inherent limit. Ambiguities. The canonical key now escapes its own delimiters and control characters, so two distinct queries cannot share an entry; composites keep their own root operator, flatten same-operator children, and sort by code point; the stored `where` is the normalised tree the key was computed from. The file shape states total presence, key derivation and uniqueness, and that a reader refuses a corpus it cannot read rather than repairing it. `skipped` draws from a closed vocabulary, so no text decoded from the wire reaches a committed file. Replay. `__name__` takes a document reference, an empty root composite replays with `where` omitted, and a non-FAILED_PRECONDITION status is never read as a missing index. Also: the privacy claim no longer implies that stripping values makes a corpus publishable — field paths are recorded verbatim and are not always static. §10 gives the corpus the compatibility standing it was already being used with. And the section renumbering broke seven `SPEC §N` references in src/ and test/. Co-Authored-By: Claude Opus 5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
SPEC.md (1)
530-551: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDuplicate: define deterministic replay values and the coverage gate.
The rule still specifies only “one-element array” and “single scalar”. It does not define scalar types, prohibited values, or exact
IN,NOT_IN,ARRAY_CONTAINS_ANY,NULL/NaN, composite, and__name__handling. The phrase “under the collection being queried” also does not define a deterministic document path for collection-group queries. Firestore uses full document resource names forreferenceValueand requires non-empty arrays for these array operators. (docs.cloud.google.com)The section says v0.3 must test value independence, but it does not require
inconclusivecoverage or suppress missing-index findings until validation passes. Complete these rules before v0.3 reports coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SPEC.md` around lines 530 - 551, Expand the v0.3 replay specification to define deterministic scalar types and prohibited values, non-empty array contents for IN, NOT_IN, and ARRAY_CONTAINS_ANY, NULL/NaN behavior, composite handling, and exact __name__ reference paths including collection-group queries. Require value-independence validation to produce an explicit inconclusive result when coverage is insufficient, and prevent missing-index findings until that validation passes; update the replay and coverage rules consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SPEC.md`:
- Around line 431-436: Specify that control characters must be escaped as
exactly four uppercase hexadecimal digits in the \uXXXX form, ensuring canonical
output across writers. Update the relevant escaping rule in SPEC.md and add a
cross-writer test covering equivalent control-character escapes and verifying
identical generated keys.
- Around line 336-339: Update the status-independent capture rule so only
normalized, supported RunQuery requests enter queries regardless of response
status; ensure unsupported RunQuery shapes and all other observed RPC traffic
are routed to skipped, consistent with the behavior described near the
unsupported-shape and RPC handling sections.
---
Duplicate comments:
In `@SPEC.md`:
- Around line 530-551: Expand the v0.3 replay specification to define
deterministic scalar types and prohibited values, non-empty array contents for
IN, NOT_IN, and ARRAY_CONTAINS_ANY, NULL/NaN behavior, composite handling, and
exact __name__ reference paths including collection-group queries. Require
value-independence validation to produce an explicit inconclusive result when
coverage is insufficient, and prevent missing-index findings until that
validation passes; update the replay and coverage rules consistently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2279ce30-1d63-49e8-bffb-0c4a2be9d2ce
📒 Files selected for processing (7)
SPEC.mdsrc/args.tssrc/collections.tssrc/index.tssrc/types.tstest/helpers.jstest/package.test.js
| A query enters the corpus when its request is observed, whatever the server answers next. One that | ||
| failed still describes something the application issues — and a query that failed *because an index | ||
| was missing* is precisely the case v0.3 exists to find, so waiting for a successful status would | ||
| drop the most interesting entries in the file. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Limit the status-independent capture rule to supported RunQuery shapes.
Lines 336-339 say that an observed query enters queries before status handling. Lines 408-411 and 555-577 say unsupported shapes and RPCs go to skipped. State that only a normalized, supported RunQuery enters queries regardless of response status. Route all other observed traffic to skipped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SPEC.md` around lines 336 - 339, Update the status-independent capture rule
so only normalized, supported RunQuery requests enter queries regardless of
response status; ensure unsupported RunQuery shapes and all other observed RPC
traffic are routed to skipped, consistent with the behavior described near the
unsupported-shape and RPC handling sections.
| `<collectionGroup>` and `<fieldPath>` are escaped before they are joined: a backslash becomes `\\`, | ||
| each of `:`, `|`, `(`, `)` is prefixed with one, and every character below `U+0020` becomes `\uXXXX`. | ||
| Enum names need no escaping but are subject to the same rule, so that decoding is uniform. The | ||
| control-character clause is not about ambiguity but about §6's requirement that a value read off an | ||
| untrusted input cannot forge a line of output: a key holding a raw newline would end the line it was | ||
| printed on. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Specify the hexadecimal case for control-character escapes.
\uXXXX does not define the exact canonical spelling. One writer could emit \u000a and another \u000A, producing different keys for the same query shape. Specify exactly four hexadecimal digits and their case, then add a cross-writer test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SPEC.md` around lines 431 - 436, Specify that control characters must be
escaped as exactly four uppercase hexadecimal digits in the \uXXXX form,
ensuring canonical output across writers. Update the relevant escaping rule in
SPEC.md and add a cross-writer test covering equivalent control-character
escapes and verifying identical generated keys.
The comment credited §4 for findings having no line number, but §4 only says that input which is not valid JSON is malformed. §5 is where a finding's fields are fixed — rule id, file, key, reason — and the absence of a line number there is the reason the annotations carry none. Co-Authored-By: Claude Opus 5 <[email protected]>
1 participant
Opens the v0.2 work. First commit is empty; the corpus format lands next.
Why now
A throwaway spike confirmed the assumption SPEC §3 rests on but nobody had tested: the Firestore
emulator's gRPC traffic can be intercepted and
RunQuerydecoded..level=ALL, the only trace of a query isnetty's
Http2FrameLoggerhex dump, truncated at a hardcoded 64 bytes. No collection or fieldname appears in plaintext. A proxy is not avoidable.
node:http2h2c pass-through proxy, withFIRESTORE_EMULATOR_HOSTpointed atit, decoded every shape tried: composite filters,
collectionGroup(allDescendants),OR,array-contains,in,!=, and== null(aUnaryFilter).The spike is throwaway and is not part of this branch.
Scope
SPEC only — the corpus format is a contract that
@indexwright/recordand the v0.3 coverage checkboth read, so it is written before either exists (§7 design principles).
🤖 Generated with Claude Code
Summary by CodeRabbit