fix(docs): recover symbols the manifest extraction was silently dropping - #427
Conversation
Four independent extraction bugs, each of which lost or misdescribed part of the API surface without failing the docs build. `@example` deleted the symbol it documented. Doxygen's \example takes a filename; used as an inline snippet marker it pulls the block out of the member's documentation. The only two symbols using it were the only two missing from the manifest: eql_v3.grouped_value lost its whole memberdef and eql_v3.lints() was left with an empty description, then dropped by the no-documentation guard. Both now use @code{.sql}/@Endcode. CREATE AGGREGATE bodies broke the C++ parse. The trailing (sfunc = ...) is a second parenthesised group with no C++ analogue, so Doxygen misread the declaration: a bare argument type made it name the member after the TYPE (grouped_value(jsonb) was extracted as a function called `jsonb`), and a schema-qualified one truncated the argument list mid-body. Strip aggregate bodies in the input filter, as it already does for dollar-quoted function bodies, and read an aggregate's argument types from its declaration and its return type from @return rather than from the C++ parse. The `match` capability was derived from the pre-3.0.1 operator spelling. Fuzzy match became @@ in 3.0.1 but the mapping still tested @>/<@, so NO domain could be assigned `match`: text_match matched no branch and fell through to the storage-only default, and text_search silently lost it. Containment on encrypted JSON keeps @>/<@ and becomes its own capability instead of being conflated with fuzzy match. public.eql_v3_json was in no list at all. dump_catalog filtered the json family's bare storage domain out of `stevec` as scalar, while `types` iterates scalar_families(), which excludes the mixed json family wholesale. The domain the SQL materializer creates therefore appeared in neither and never reached the manifest. Carry it in the json-family inventory behind a `scalar` flag so consumers do not describe it with the SteVec query surface. Also renders @code blocks readably: Doxygen encodes each space as an <sp/> element and the generic walk cleans every nested fragment, welding the SQL together ("SELECTeql_v3.grouped_value(...)"). Verified against the 3.0.3 tree: 1988 -> 1989 functions, gaining grouped_value and lints and dropping the phantom `jsonb` function; 52 -> 53 domains, gaining public.eql_v3_json; the `match` capability returns to text_match and both text_search variants. Diffing every shared symbol against the released 3.0.3 manifest shows no other description changes. The stale test fixture in test_xml_to_json.py (which predated `typname` and the 3.0.1 renames, and had been failing on main) is updated to the current catalog shape and now pins the capability mapping and the json family. Claude-Session: https://claude.ai/code/session_01NkuQNMvw9BpB4BWWeKtfV8
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes correct generated manifest extraction for JSON-family domains, fuzzy matching, containment, aggregate declarations, and SQL documentation snippets. Catalog serialization now preserves scalar JSON storage entries with explicit metadata, while Doxygen preprocessing and XML conversion handle aggregate signatures and code blocks. ChangesManifest extraction fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tasks/docs/generate/xml-to-json.py`:
- Around line 81-92: Update the JSON-domain manifest generation for
public.eql_v3_json_search and eql_v3.query_json to derive capabilities and
supportedOperators from the catalog through _capabilities_from_ops instead of
static storage/json values and empty operator lists. Ensure catalog entries for
@>, <@, ->, and ->> are reflected in the emitted manifest while preserving the
existing @@ match and containment handling.
🪄 Autofix (Beta)
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: 2ae43972-6d71-435f-ae95-67b784231e19
📒 Files selected for processing (8)
.changeset/manifest-extraction-fixes.mdcrates/eql-codegen/src/dump.rssrc/v3/aggregates.sqlsrc/v3/lint/lints.sqltasks/docs/doxygen-filter.shtasks/docs/generate/test_xml_to_json.pytasks/docs/generate/xml-to-json.pytasks/docs/generate/xml-to-markdown.py
|
Tracked in #428, which also records the one gap left open here (SteVec domains reporting no operators) and some suggested build-time guards. Not using |
|
Review performed by Codex (GPT-5.6-sol). The recovered documentation contains incorrect source-line metadata and publishes a malformed return type for
|
…uncated return type
Addresses both findings from the Codex review.
Skipping CREATE AGGREGATE bodies dropped rows outright. Doxygen reports source
positions against the FILTERED stream, so every symbol after an aggregate was
reported five lines early — max_sfunc at line 41 came through as 36. Emit a
blank line per skipped row, which is how the dollar-quoted body stripper has
always kept its 1:1 mapping. Verified across every SQL file in src/: zero
line-count drift, and all 1926 symbols shared with the released 3.0.3 manifest
now report its exact source lines.
lints() was published with the return type "TABLE(severity". Doxygen reads a
`RETURNS TABLE (col type, ...)` list as a C++ argument list and truncates it at
the first comma, and the return-type regex then matched to the next space.
Joining the declaration onto one line does not help — the truncation is at the
comma, not the line break — so the column list is unrecoverable from the XML.
Stop the match at "(" instead, giving a clean `TABLE`; the full column list is
already carried by the @return tag in returns.description. No other return type
in the corpus contains a paren, and none changed.
Both are regressions this branch introduced by recovering the symbols in the
first place: lints() was absent from the manifest before, so its malformed
return type could not surface, and nothing skipped aggregate bodies.
Adds a regression test for each, including one that runs the filter and asserts
input and output line counts match (confirmed failing without the fix).
Claude-Session: https://claude.ai/code/session_01NkuQNMvw9BpB4BWWeKtfV8
|
Both Codex findings were valid and are fixed in 8653d06. Both were regressions this branch introduced by recovering the symbols in the first place — [P2] Line numbers — correct, and the mechanism is exactly as described. The existing dollar-quoted body stripper prints one output line per input line (possibly empty); my aggregate rule used Verified across every SQL file under [P2] Fixed instead by stopping the return-type match at "returns": {
"type": "TABLE",
"description": "SETOF record (severity text, category text, object_name text, message text)"
}
Tests added for both, per the review. The line-numbering one runs the filter and asserts input and output line counts match; I confirmed it fails when the fix is reverted. Generator suite is 11 passed. |
EQL 3.0.4 ships the manifest-extraction fixes from cipherstash/encrypt-query-language#427, so `eql_v3.grouped_value` and `public.eql_v3_json` now resolve from the catalog and no longer need drift-check exemptions. Verified against the released manifest: grouped_value(jsonb) and lints() present, 53 domains (was 52) including public.eql_v3_json, and the `match` capability restored to text_match and both text_search variants. Only `eql_v3.query_text_eq` stays on the allowlist — the 40 scalar query-operand domains created in a `DO ... EXECUTE` block are a separate gap that 3.0.4 does not address. Removing the other two means a regression in either now fails the drift check instead of staying quiet. Also always stamp the pinned tag as the manifest's version rather than only substituting when it reads "DEV". EQL's release workflow derives that field from a tag input that can arrive empty, in which case json.sh falls back to "DEV" — which 3.0.4 shipped, and which would otherwise render as "EQL DEV" in the banner on every reference page. The existing guard already covered that case; this extends it to warn when a manifest is stamped with a DIFFERENT release, which would be a packaging mix-up worth seeing rather than a cosmetic default worth silencing. bun run build passes: drift check clean against 3.0.4, 1990 functions, banner reads EQL 3.0.4. Claude-Session: https://claude.ai/code/session_01NkuQNMvw9BpB4BWWeKtfV8
2 participants
Four independent bugs in the Doxygen → manifest pipeline, each of which lost or misdescribed part of the API surface without failing the docs build. Found while pinning the docs site to EQL 3.0.3.
1.
@exampledeletes the symbol it documentsDoxygen's
\exampletakes a filename — it documents an example source file. Used as an inline snippet marker, it pulls the block out of the member's documentation.There are exactly two
@exampleblocks in the 3.0.3 tree, and they are exactly the two symbols missing from the manifest:@exampleeql_v3.grouped_value(src/v3/aggregates.sql)eql_v3.lints()(src/v3/lint/lints.sql)eql_v3_internal.grouped_value_sfunc(same file)The failure mode inverts the usual one: it penalises the best-documented symbols.
grouped_valuehas the richest doc block of any aggregate in the codebase — brief, params, return, a note on parallel-aggregation determinism, a worked example — and that is why it vanished. Both blocks now use@code{.sql}/@endcode.2.
CREATE AGGREGATEbodies break the C++ parseThe trailing
(sfunc = …, stype = …)is a second parenthesised group, which C++ has no form for, so Doxygen misreads the declaration — differently depending on the argument type:So
grouped_valuewas doubly lost — the@examplefix alone does not recover it. The input filter now strips aggregate bodies (exactly as it already does for dollar-quoted function bodies — they carry no documentation either), and the extractor reads an aggregate's argument types from its declaration and its return type from@returninstead of trusting the C++ parse.This also fixes the 60
min/maxaggregates, which were present but garbled — and it was reaching the published page:min(eql_v3_bigint_ord)min(public.eql_v3_bigint_ord){name: "public.", type: "eql_v3_bigint_ord"}{name: "input", type: "public.eql_v3_bigint_ord"}"CREATE AGGREGATE eql_v3""public.eql_v3_bigint_ord"3. The
matchcapability is computed from the pre-3.0.1 operator_capabilities_from_opsstill tested("@>", "<@"). Fuzzy match was renamed to@@in 3.0.1, so no domain could be assignedmatchat all — across all 52 domains the vocabulary was{storage, equality, order, json}with zeromatch.For
public.eql_v3_text_matchthe consequence is worse than a missing label: its only operator is@@, so it matched no branch and fell through to theor ["storage"]default. The manifest described the fuzzy-match domain as storage-only, contradicting thesupportedOperators: ["@@"]andtermFunctions: ["eql_v3.match_term"]beside it.text_searchsilently lostmatchtoo.Containment on encrypted JSON legitimately keeps
@>/<@, so it is now reported as its owncontainmentcapability rather than conflated with fuzzy match.4.
public.eql_v3_jsonis in no list at alldump_catalogfiltered the json family's bare storage domain out ofstevecas scalar, on the assumption it would surface viatypes— buttypesiteratesscalar_families(), which excludes the mixed json family wholesale. The SQL materializer creates the domain (families_with_scalar_domains(),generate.rs:1024); the dump listed it nowhere. The stale comment gave the history away, still naming the pre-3.0.1json/jsonb_entry.It is now carried in the json-family inventory behind a
scalarflag, so consumers do not describe it with the SteVec query surface (it stores and decrypts, nothing more).I deliberately did not add it to
types: that list is the scalar-matrix surface, and thetest:matrix:inventorycoverage task requires ascalars::<token>::matrix_*suite for every(type, domain)it lists. The json family has no such matrix (its coverage lives in the separatev3_json_storage_testsbinary), so adding it there would have broken that check.Verification
Ran the real pipeline (
dump-catalog→doxygen→xml-to-json) against the 3.0.3 tree:grouped_valueandlints, drops the phantomjsonbfunction the old aggregate mis-parse invented.public.eql_v3_jsonwithcapabilities: ["storage"].matchrestored totext_match,text_search,text_search_ore;text_matchno longer reports storage-only.lints, and the only signature changes aremin/maxbecoming schema-qualified. No collateral.cargo test -p eql-codegen -p eql-domainsand the Python generator tests pass;cargo fmt --checkandclippyclean.CREATE AGGREGATE x(y) (sfunc = z);cannot latch the body-skip and swallow following declarations.Two notes for the reviewer:
test_xml_to_json.py::test_load_domainswas already failing onmain— its fixture predatedtypnameand the 3.0.1 renames, so it raisedKeyError: 'typname'. It is updated to the current catalog shape and now pins the capability mapping and the json family. A current fixture would have caught bug 3.supportedOperators: [](hardcoded), sopublic.eql_v3_json_searchclaims no operators despite being the domain that supports@>/<@. Populating that needs operator data added to the Rust catalog's SteVec entries, and I did not want to assert an authoritative operator set I could not verify from the catalog.https://claude.ai/code/session_01NkuQNMvw9BpB4BWWeKtfV8
Summary by CodeRabbit
Bug Fixes
eql_v3.grouped_valueandeql_v3.lints().public.eql_v3_jsoncatalog entry and identified it as storage-only.Documentation