build: order the whole eql_v3 surface from one walk in the codegen; retire tsort - #382
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 50 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 (2)
📝 WalkthroughWalkthroughThe PR moves v3 SQL surface discovery and dependency ordering into ChangesV3 installer ordering and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Build as tasks/build.sh
participant Codegen as eql-codegen order
participant Validator as verify_symbol_order_v3.sh
participant Installer as release/cipherstash-encrypt.sql
Build->>Codegen: generate ordered src/v3 manifest
Codegen-->>Build: return ordered SQL paths
Build->>Validator: verify symbol definition order
Validator-->>Build: return validation status
Build->>Installer: concatenate filtered SQL and pin script
Build->>Installer: run completeness validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
freshtonic
reviewed
Jul 16, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Review — build-ordering refactor (deterministic topo-sort)
Reviewed the whole refactor: ordering.rs, build.sh, build/ordering.sh, the order CLI (main.rs), generate.rs/lib.rs, verify_symbol_order_v3.sh, verify_installer_complete.sh, the parity/cli tests, and the doc/gitignore/CI edits. Scoped out the pre-existing c8d50efb jsonb-docs commit (its src/v3/jsonb/operators.sql is the only SQL-body change on the branch, and it rides along).
Overall: excellent — this is exactly the right fix, and it's approve-worthy. It replaces the non-deterministic, macOS-hostile comment-parse → tsort → tac → cat bash pipeline with a deterministic Rust topo-sort, and hardens completeness with two DB-free gates. (For context: I personally hit this class of bug twice last week — BSD-vs-GNU tsort producing different valid orders, and | tac being absent on stock macOS — so I can vouch that this eliminates a real, recurring footgun.) One thing to fix before merge: the title/description are stale (details below); the code is not blocked.
Design — verified correct
ordering.rs(Kahn + min-heap keyed by path) is byte-reproducible. Name-sorted tie-breaking gives a canonical order, not just a valid one — the propertytsortnever had. Determinism, cycle detection, self-edge tolerance, external-edge tolerance are all unit-tested.- One walk, not two enumerations.
walk_v3_surface+surface_orderorder exactly the set they walk, with no generated/hand-written classifier — so "a file matching neither predicate is silently dropped from the installer" (the old two-block risk) is unrepresentable.install_order_contains_every_v3_sql_filepins it against an independent walk. This is the standout design decision. - Invariants moved into the sort.
OutsideSurface(self-containment) andUnknownTargets(dangling REQUIRE) subsume the oldverify_v3_self_contained/verify_deps_existshell gates; the prefix check is path-segment-exact (src/v3suffix/correctly rejected). Cycles are a hardCycleErrornaming the stuck files. - Reorder-only confirmed. The refactor commits touch no
src/v3/*.sqlbody — the only SQL change on the branch is in the out-of-scope jsonb-docs commit. Combined withcodegen:parity(generated files committed-in-place) andstrip_require_lines(fail-loud, anchored), the "no SQL body modified" claim holds.
Defense-in-depth gates — exemplary
verify_symbol_order_v3.sh: define-before-use across the ordered list, deliberately stricter than PG (treats plpgsql body callees as references), with the allowlist escape hatch documented. Fail-safe on empty order / unreadable allowlist / unreadable path. Correctly handles the SEM types' split DDL forms (DOMAIN/TYPE/OPERATOR CLASS) and the CIP-3442eql_v3.query_*domains.verify_installer_complete.sh: the reasoning here is the best part of the PR. Because 93/244 files are leaves, a dropped leaf body installs cleanly and passes the symbol checker (no dangling reference), surfacing only in a creds-gated DB test. Gate 3's line-count identity (Σ(lines − REQUIRE lines) + pin == installer) is pure arithmetic no leaf can hide from, and Gate 2 (trailing-newline) closes the>>-concat statement-gluing hole nothing else checks.
To fix before merge — stale title/description
The PR title ("order the generated surface from the codegen manifest; tsort as whole-surface verifier") and the body's Task 1/Task 3 describe an intermediate design — a two-block scheme (shell-globbed hand-written + codegen manifest of generated) with tsort demoted to a verifier. Commit 8b9a19a7 ("order the whole v3 surface from one walk, not two enumerations") superseded that: the final state has no tsort at all (fully replaced by the Rust Kahn sort — only 3 comment references remain, all explaining its removal) and no separate codegen manifest (a single walk orders everything). Please retitle to describe the final single-walk design so the squash-merge message isn't misleading to a future git log reader. Code-wise this is purely cosmetic.
Nits (non-blocking)
verify_symbol_order_v3.shopens each ordered file 3× (probe + definitions + references) — negligible at ~244 files, just noting.- Gate 3's REQUIRE regex must stay in lockstep with
strip_require_lines; the comment flags this, and a divergence fails the build loudly rather than silently, so it's fail-safe.
Great work — the single-walk invariant and the leaf-completeness arithmetic are the parts I'd hold up as the model for this kind of build-infra change.
Contributor
|
I've seen tsort produce output that differs between execution environments and can produce build flake. Probably comes down to the version of tsort shipped by macOS/Linux distros. I would not be averse to either removing the need for it or shipping a new binary crate in the workspace that replaces it (calling out to the toposort crate). |
Contributor
|
oh wait, you HAVE replaced it! Nice |
tobyhede
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/eql-codegen/src/ordering.rs`:
- Around line 184-190: The dependency graph construction around the files/reqs
loop currently skips self-dependencies, allowing malformed REQUIRE graphs
through cycle validation. Remove the dep != p.as_str() exception so every
dependency present in nodes, including self-dependencies, contributes to
indegree and dependents; update the related tests around the cycle-validation
logic to expect OrderError::Cycle for self-cycles.
In `@tasks/build.sh`:
- Line 4: Update the `#MISE` sources declaration in the build configuration to
include all relevant workspace and crate Cargo.toml manifests plus Cargo.lock,
alongside the existing SQL, Rust, and task inputs, so both cargo run steps
invalidate their cache when manifests or dependency versions change.
In `@tasks/test/verify_symbol_order_v3.sh`:
- Around line 81-85: The symbol-order validator currently keys overloaded
functions and aggregates only by schema and name, allowing one overload to
satisfy another’s ordering check. Update the definition and reference tracking
logic around the CREATE matching block and the validation paths near lines
135-163 to include each symbol’s argument list in its key, and add a regression
case covering two overloads.
🪄 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
Run ID: d371ad24-d428-4c8f-a0ce-d0cedcf61f84
📒 Files selected for processing (23)
.github/workflows/README.md.github/workflows/test-eql.yml.gitignoreCLAUDE.mdDEVELOPMENT.mdcrates/eql-codegen/src/generate.rscrates/eql-codegen/src/lib.rscrates/eql-codegen/src/main.rscrates/eql-codegen/src/ordering.rscrates/eql-codegen/tests/cli.rscrates/eql-codegen/tests/parity.rsdocs/reference/adding-a-scalar-encrypted-domain-type.mdmise.tomlsrc/v3/jsonb/operators.sqltasks/build.shtasks/build/ordering.shtasks/test/build_ordering_helpers_test.shtasks/test/self_contained_v3.shtasks/test/symbol_order_allowlist.txttasks/test/symbol_order_selftest.shtasks/test/verify_installer_complete.shtasks/test/verify_symbol_order_v3.shtests/sqlx/src/property.rs
freshtonic
approved these changes
Jul 17, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Re-review — approving
Re-reviewed after the retitle and 6fbc9079. All feedback from my earlier comment is addressed, and the new commit fixes a real issue I'd missed. No remaining concerns — this is excellent, ship it.
Resolved:
- Stale title/description → fixed. Now "order the whole eql_v3 surface from one walk in the codegen; retire tsort" — accurately describes the final single-walk design (no tsort, no separate manifest). And, re my tsort comment on the thread: yep, you'd already retired it.
New commit 6fbc9079 — correct and well-judged:
- The
#MISE sourcescache-invalidation fix is the important one, and I missed it.ordering.sh(shaping the installer viastrip_require_lines) and the two gate scripts + allowlist weren't in the task's sources, so an edit to any of them left mise considering the build fresh — re-serving a stale installer and skipping the gates (a cache hit skips the whole script). Adding all four is exactly right, and verifying the cache "discriminates rather than always rebuilds" is the right way to confirm it. count_linesfail-loud mirrors thestrip_require_linesrc <= 1idiom — a swallowed grep error no longer reads as0and silently drops a file from the line-count identity. The note on keeping callers as barevar=$(...)(soset -esees the failed assignment) is the correct subtlety.- Dropping the redundant
sort_unstableintopo_order— good call; the min-heap already owns ordering, so removing it makes the determinism source unambiguous (I'd noted the same privately). - Documenting the symbol checker's cross-file-only scope, and making the two gate scripts executable, are both worthwhile.
- Confirmed the commit touches no SQL body — the reorder-only guarantee holds.
The single-walk invariant and the leaf-completeness line-count arithmetic remain the standout parts. Nice work.
tobyhede
added a commit
that referenced
this pull request
Jul 17, 2026
… widen build cache inputs Addresses the three CodeRabbit threads on #382. The symbol checker keyed definitions and references by schema+name with no argument list, and kept the minimum index. eql_v3.eq has 186 definitions across files #55..#242, so every eq reference from #55 on passed for free — and the gate still printed "OK (244 files)". Proven: swapping text_eq_operators ahead of text_eq_functions in the real order passes this gate, and fails a real install with `function eql_v3.eq(text_eq, text_eq) does not exist` (psql exit 3). Overloads cannot be resolved here: a call site is a bare eql_v3.eq(a, b) with no types, and CREATE OPERATOR supplies LEFTARG/RIGHTARG on other lines — that needs a type checker, not a line scan. Nor do they need to be: Postgres resolves them exactly at CREATE time via test:clean_install_v3, which runs on every relevant PR across PG 14-17, needs no credentials, and is not skipped on forks. So the defect was the silence, not the blindness — the same vacuous pass this script already refuses for the empty-list case ("a pass that means 'I checked nothing', indistinguishable from 'I checked everything'"). Track defcount/defmax alongside the min index and report the unresolvable set: 41 names on the current surface. Two catches are deliberately preserved rather than skipped wholesale: a reference preceding EVERY overload is still an error (wrong whichever was meant), and overloads whose definitions all precede the use stay soundly checked — that keeps 8 of 49 multi-def names, including the same-file eql_v3.ste_vec_contains and the OPERATOR FAMILY+CLASS pair sharing eql_v3_internal.ore_cllw_ops. Self-edges are now rejected. The tolerance was deliberate but its rationale died with the shell build: that build emitted a self-edge per file because tsort only prints tokens appearing in an edge. The walk enumerates nodes directly, so the only source now is a typo — and a typo'd edge meant to name another file, so the real dependency is missing and the order can be silently wrong. Reported as a dedicated SelfEdge, not folded into Cycle: "dependency cycle" would send the reader hunting a loop when the fix is one line in one file. Cargo.toml/Cargo.lock join #MISE sources for the same reason ordering.sh did: the cargo run steps render this artefact and eql-codegen's deps decide what they render (minijinja templates the SQL, prettyplease pins bindings formatting), so a dep bump changes output with no .rs file touched. No CHANGELOG entry: build tooling only, nothing observable to a caller.
Contributor
Author
CodeRabbit autofix — all three threads addressed (
|
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 17, 2026
freshtonic
added a commit
that referenced
this pull request
Jul 17, 2026
The merge queue ejects PRs when the sqlx shards fail with `53200 out of
shared memory`. Despite the wording this is not RAM exhaustion — it is
the shared lock table running out of slots, exactly as the error's own
hint ("You might need to increase max_locks_per_transaction") says.
max_locks_per_transaction is not a per-transaction cap; it sizes the
CLUSTER-WIDE lock table as max_locks_per_transaction * max_connections.
The shipped uninstaller drops both eql_v3 schemas with DROP ... CASCADE,
which locks every one of the ~5,682 objects it removes and holds them all
until commit (sqlx::raw_sql sends the script as one implicit
transaction). Measured: 5,689 locks for a single uninstall against a
stock table of 64 * 100 = 6,400 slots — 89% of the entire cluster's
budget in one transaction. Overlapping uninstalls exhaust it, and the
backends that then fail are mostly innocent tests that were merely
installing EQL, which is why this surfaces as `failed to apply
migrations` in unrelated tests.
Sharding is what tipped it over: the queue runs 2 shards where PRs run 4,
doubling test density per shard so the uninstall-heavy tests overlap.
Partitioning is a deterministic hash, so they land in the same shard on
every run — hence shard 2/2 failing on all four PG versions while the
same commit passes on the PR. This is not specific to one PR: #382, #401
and #402 ejected with the identical signature.
Raise max_locks_per_transaction to 1024 (102,400 slots), sized for the
worst case of nextest's 16 concurrent tests each uninstalling (~91k
locks). Verified on postgres:17: 6 concurrent uninstalls fail 5/6 at the
default and pass 6/6 at 1024. Costs ~42MB of shared memory (145MB ->
187MB), well inside the container's 2GB limit.
Add lock_capacity_tests.rs as a preflight gate so an undersized lock
table fails once, immediately, with a message naming the cause and the
fix — instead of six unrelated tests dying 20 minutes into CI with what
reads like an OOM. NB: a Postgres container created before this change
keeps the old setting, so local devs must recreate it
(mise run postgres:down && mise run postgres:up postgres-17).
Deliberately not "fixed" by giving the queue 4 shards: that would only
hide an undersized lock table until the catalog grew again.
…te, drop tac, anchored strip)
…le-surface cycle+linearization verifier
…-emitted ordering
… errors) and unreadable-path guard in symbol checker
…ubstrate Reformat generate.rs/ordering.rs to satisfy `cargo fmt --check`, and add an EqRow type alias alongside the existing OrdRow for the equality oracle's result tuple.
The cross-check recognised `CREATE DOMAIN` only in `eql_v3_internal` and `public`, but its reference scanner matches every `eql_v3.<name>`. Since CIP-3442 the query-operand twins are `CREATE DOMAIN eql_v3.query_<T>_<cap>` (plus the hand-written `eql_v3.query_jsonb`) — a query operand is never a column type, so it lives in `eql_v3` rather than `public`. None of those definitions were recorded, so every reference to them, including the `CREATE DOMAIN` line itself, reported "defined nowhere". This branch passes on its own, where the operands are still `public.<name>_query`. It only fails once merged with `eql_v3`, which is the tree CI builds for a pull_request — a semantic merge conflict that git resolves cleanly and the checker then rejects. Add the eql_v3 arm, ordered after the eql_v3_internal test since `eql_v3.` is a prefix of `eql_v3_internal.`. `isdomain[]` stays public-only: it gates which bare `public.*` tokens are checked, while `eql_v3*.*` references are checked unconditionally. Self-test gains accept and reject cases for the new form, so recognising the schema cannot silently blunt the ordering check.
`tasks/build.sh` enumerated the SQL surface twice and nothing reconciled the two. The shell glob classified a file as generated by its first line (`-- AUTOMATICALLY GENERATED FILE.`) and skipped it; `generated_manifest()` classified a file as generated by re-running `render_type()` over `scalar_families()`. `src/v3/scalars/ore_fallback.sql` — rendered outside `render_type` — matched the first predicate and not the second, so it landed in neither block and was dropped from `release/cipherstash-encrypt.sql`. The ORE poison constraints never installed and `v3_ore_fallback_tests` failed on the merge with eql_v3, while the build itself reported success. Nothing could have caught it. `verify_deps_exist` checked that every *listed* file exists on disk, never that every file on disk is listed. `verify_linearization` skipped edges whose endpoints were absent from the order, commented "absence is caught by verify_deps_exist" — it wasn't. And the manifest test asserted a hardcoded per-family count of 219, structurally blind to cross-family files. Replace both enumerations with one. `eql-codegen order` walks src/v3 once, parses `-- REQUIRE:` from every file (generated and hand-written alike), and linearizes via the existing tested `topo_order`. You order exactly the set you walk, so a dropped file is unrepresentable rather than merely detectable. The new `surface_order` wrapper hard-errors on a dangling REQUIRE target and on any edge leaving src/v3, subsuming `verify_deps_exist` and `verify_v3_self_contained`. Ordering is name-sorted in Rust rather than left to `tsort`'s platform-dependent tie-break, which is what the manifest was introduced to dodge in the first place. Verified against CI's merge commit: 241 files on disk, 241 ordered (was 240), 38 hits for `eql_ore_unavailable` in the installer (was 0). ore_fallback.sql sorts immediately after the operator_class.sql whose outcome it reads. Every SQL statement in the merged installer is identical to the base branch's own build; only statement order among independent files changes. Removed: the marker classifier, the two-phase concat, verify_deps_exist, verify_v3_self_contained, run_tsort_or_die, verify_linearization, generated_manifest(), and five gitignored intermediates. strip_require_lines stays in tasks/build/ordering.sh. Tests: install_order_contains_every_v3_sql_file (parity.rs) compares the order against an independent walk by set equality, replacing the 219-count assertion; surface_order unit tests cover dangling targets, out-of-surface edges, cycles and determinism; order_subcommand_fails_on_a_dangling_require pins that the build aborts with no partial order on stdout.
Review follow-ups on the build-ordering refactor. The installer is byte-identical
before and after; every change is to a gate, a diagnostic, or a comment.
The symbol-order cross-check runs inside `mise run build`, and the release
workflow reaches it via release-eql.yml -> _build-sql.yml -> `mise run build`.
That makes it the sole symbol-order gate on the release path — the separate
`test:symbol_order_v3` CI step only guards PRs. Two consequences, both fixed:
- The allowlist was read with an unguarded `getline < file`, which cannot
distinguish EOF from an unreadable file. A bad ALLOW path silently yielded an
empty allowlist. That fails safe today (nothing to suppress), but the moment a
real entry lands, a typo'd path resurrects the false positive the entry exists
to suppress — during a release. Guard with `test -r`, and make the path
overridable via SYMBOL_ORDER_ALLOWLIST so the self-test can exercise it.
- The gate is stricter than PostgreSQL: a `LANGUAGE plpgsql` body resolves its
callees at execution time, so Postgres accepts a forward reference the gate
rejects. Unfixable by a line-oriented scan, which cannot tell a plpgsql body
from a `LANGUAGE sql` one (whose callees Postgres DOES resolve at CREATE time,
which is what the ordering is for). Pin the trade instead: tests for both the
rejection and the allowlist escape hatch, and document the divergence in the
script header and the allowlist, warning never to allowlist a `LANGUAGE sql`
body — that yields a green build and a failed install.
Diagnostics and dead code:
- CycleError said "dependency cycle among generated files", inherited from the
two-block design this module replaced. The sort draws no generated/hand-written
distinction, so a cycle through a hand-authored `-- REQUIRE:` edge sent readers
into the codegen. Now names the stuck files; a test asserts the word "generated"
never appears. topo_order's doc described the same dead two-block scheme.
- topo_order is pub(crate). It tolerates edges to non-nodes; on the real surface a
non-node target IS the bug surface_order exists to catch. No external callers.
- Drop the never-constructed WriteError::Codegen variant. Clippy does not flag an
unused pub enum variant in a library, so it would have rotted silently.
- Four comments narrated `scalars/ore_fallback.sql` as a file this build once
dropped. It is not in the tree and its adding commit is unreachable from this
branch. The bug class is real; the file is another lineage's. Describe the class,
and rename the synthetic tempdir fixture to cross_family.sql.
Also hoist the per-edge format!("{SURFACE_ROOT}/") out of surface_order's inner loop.
…t disk The ordering refactor pinned the wrong half of the invariant. `eql-codegen order` guarantees the ORDER LIST names every .sql file on disk, and install_order_contains_every_v3_sql_file proves it against an independent walk. Nothing proved that build.sh's concat loop then emitted each ordered file's BODY into the installer. That is the same bug class one layer down, and it is the layer where a dropped file actually costs you something. It matters because 93 of the ~244 files under src/v3 are leaves: no other file `-- REQUIRE:`s them. Several define no object any inventory test enumerates — a bare DO block, functions in eql_v3_internal, a CREATE OPERATOR CLASS. Drop one and the monolith still applies cleanly and every symbol still resolves, so an install smoke test passes. verify_symbol_order_v3.sh is blind by construction: a dropped leaf removes its definition and, being a leaf, leaves no reference dangling to trip on. The only backstop is a DB behavioural test, which needs CipherStash credentials and is skipped on fork PRs. Add tasks/test/verify_installer_complete.sh — DB-free, creds-free, wired into the self-contained-v3 job (which runs on forks) and into `mise run build` itself: - Non-vacuity: the order names exactly the files find(1) sees under src/v3. - Trailing newline: no ordered file may lack one. build.sh assembles with `>>`, so a missing newline glues one file's last statement onto the next file's first line — different SQL, not a syntax error. Nothing checked this. - Line-count identity: Σ(lines − anchored REQUIRE lines) + pin script == installer. Exact today (43339). A dropped, truncated, or twice-emitted body breaks it. Each gate was proven to fail before being wired in: full order with two leaf bodies missing from the installer, a file dropped from the order, an empty order, a duplicated emit, and a file without a trailing newline. Also, from the same audit: - Restore the no-duplicate assertion lost when generated_manifest's test was deleted. parity.rs collapsed the order into a BTreeSet before comparing, so a repeated path was absorbed; build.sh concatenates with no `uniq`, so it would emit that file's DDL twice. Verified the assertion fires by temporarily pushing a duplicate. - verify_symbol_order_v3.sh reported "OK (0 files)" and exited 0 on an empty order — a pass meaning "I checked nothing", indistinguishable in CI from one meaning "I checked everything". Refuse the vacuous case; self-test both empty and whitespace-only. - walk_v3_surface's read error said "stream did not contain valid UTF-8" without naming which of 244 files. Name it. - Pin that a self-require is tolerated (topo_order's `dep != p` guard makes it reachable; the old shell build emitted one per file for tsort's benefit). - #MISE outputs omitted src/deps-ordered-v3.txt, so a cached build could skip while a consumed product was absent. Add it, and trap the order's temp file. Docs: CLAUDE.md and DEVELOPMENT.md still described verify_v3_self_contained, src/deps-v3.txt and tsort, all deleted. self_contained_v3.sh's file gate is now vacuous (the walk is rooted at src/v3, so every node is under it by construction) — kept as belt-and-braces, comment corrected.
verify_monolith_reorder_only.sh proved the build-ordering refactor changed only statement order: build the installer at HEAD and at a pre-refactor baseline, sort both, demand byte-identical. It served that purpose and is now dead weight. It is referenced by no CI job, no mise task wiring, and no doc. It cannot run unattended — the baseline ref is a mandatory positional arg with no default, because `git merge-base HEAD main` sits 594 commits back on this branch and would diff on content rather than order. And its premise expires on merge: once eql_v3 lands and new files appear past c8d50ef, "a line was added" is the correct answer, and the script can only report failure. verify_installer_complete.sh is the standing form of the same intent. Rather than comparing against a baseline that goes stale, it asserts on every build that the installer's line count equals the sum of its ordered inputs — so a dropped, truncated, or duplicated file body fails immediately, with no baseline to pin. It also carried a live hazard. It builds in a second git worktree sharing CARGO_TARGET_DIR, which is the configuration that lets cargo serve a stale eql-domains rlib built from another worktree's catalog — silently rewriting src/v3/scalars in the tree you are standing in.
…l loud on unreadable inputs The build sources tasks/build/ordering.sh and shells out to two gate scripts under tasks/test/, but none were in the task's #MISE sources. Editing strip_require_lines left mise considering the build fresh, so it re-served an installer assembled by the old logic — and skipped the gates that would have caught it, since a cache hit skips the whole script. Add all four (the two gates and the allowlist included: they decide whether the build passes) so any edit invalidates. Verified the mechanism discriminates rather than always rebuilding: untouched reports "sources up-to-date, skipping", each new source re-runs, and a tasks/test file left out of sources still skips. count_lines in verify_installer_complete.sh swallowed every grep error, and the caller's arithmetic read the resulting empty string as 0 — a file could contribute nothing to the line-count identity while the gate stayed green. Gate 1 makes that unreachable today, but this script exists to fail loudly. Mirror the rc <= 1 idiom already in strip_require_lines: exit 2 naming the file on a real fault, tolerate grep's exit 1 on an empty file. Also: document that the symbol checker enforces cross-file order only (the opclass files name their own class in a RAISE NOTICE, and would otherwise need allowlisting); drop the redundant sort_unstable in topo_order, which read as load-bearing for determinism when the min-heap already owns ordering; make the two runnable gate scripts executable, matching verify_symbol_order_v3.sh. No CHANGELOG entry: build tooling only, nothing observable to a caller.
… widen build cache inputs Addresses the three CodeRabbit threads on #382. The symbol checker keyed definitions and references by schema+name with no argument list, and kept the minimum index. eql_v3.eq has 186 definitions across files #55..#242, so every eq reference from #55 on passed for free — and the gate still printed "OK (244 files)". Proven: swapping text_eq_operators ahead of text_eq_functions in the real order passes this gate, and fails a real install with `function eql_v3.eq(text_eq, text_eq) does not exist` (psql exit 3). Overloads cannot be resolved here: a call site is a bare eql_v3.eq(a, b) with no types, and CREATE OPERATOR supplies LEFTARG/RIGHTARG on other lines — that needs a type checker, not a line scan. Nor do they need to be: Postgres resolves them exactly at CREATE time via test:clean_install_v3, which runs on every relevant PR across PG 14-17, needs no credentials, and is not skipped on forks. So the defect was the silence, not the blindness — the same vacuous pass this script already refuses for the empty-list case ("a pass that means 'I checked nothing', indistinguishable from 'I checked everything'"). Track defcount/defmax alongside the min index and report the unresolvable set: 41 names on the current surface. Two catches are deliberately preserved rather than skipped wholesale: a reference preceding EVERY overload is still an error (wrong whichever was meant), and overloads whose definitions all precede the use stay soundly checked — that keeps 8 of 49 multi-def names, including the same-file eql_v3.ste_vec_contains and the OPERATOR FAMILY+CLASS pair sharing eql_v3_internal.ore_cllw_ops. Self-edges are now rejected. The tolerance was deliberate but its rationale died with the shell build: that build emitted a self-edge per file because tsort only prints tokens appearing in an edge. The walk enumerates nodes directly, so the only source now is a typo — and a typo'd edge meant to name another file, so the real dependency is missing and the order can be silently wrong. Reported as a dedicated SelfEdge, not folded into Cycle: "dependency cycle" would send the reader hunting a loop when the fix is one line in one file. Cargo.toml/Cargo.lock join #MISE sources for the same reason ordering.sh did: the cargo run steps render this artefact and eql-codegen's deps decide what they render (minijinja templates the SQL, prettyplease pins bindings formatting), so a dep bump changes output with no .rs file touched. No CHANGELOG entry: build tooling only, nothing observable to a caller.
tobyhede
force-pushed
the
build-ordering-refactor
branch
from
July 24, 2026 03:31
e879fa7 to
2fd6bd3
Compare
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/eql-codegen/src/ordering.rs (1)
161-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDirectory-walk errors don't name the offending path (mirrors the file-read fix above).
fs::read_dir(&dir)?(Line 165) andentry.file_type()?(Line 168) propagate bareio::Errors with no path context, the same failure mode the code explicitly fixed forfs::read_to_stringa few lines below with a comment explaining why ("A bare?here surfaces as ... with no indication of which of ~244 files is at fault"). A permission-denied or missing subdirectory deep in the walk will produce an unqualified OS error instead of naming the directory.♻️ Proposed fix
while let Some(dir) = stack.pop() { - for entry in fs::read_dir(&dir)? { + for entry in fs::read_dir(&dir).map_err(|e| { + io::Error::new(e.kind(), format!("reading directory {}: {e}", dir.display())) + })? { let entry = entry?; let path = entry.path(); - if entry.file_type()?.is_dir() { + if entry.file_type().map_err(|e| { + io::Error::new(e.kind(), format!("stat {}: {e}", path.display())) + })?.is_dir() { stack.push(path); continue; }🤖 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 `@crates/eql-codegen/src/ordering.rs` around lines 161 - 191, Update walk_v3_surface to add path context to errors from fs::read_dir(&dir) and entry.file_type(), matching the existing contextual handling for fs::read_to_string. Preserve each original error kind while including the offending directory or entry path in the message.
🤖 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.
Nitpick comments:
In `@crates/eql-codegen/src/ordering.rs`:
- Around line 161-191: Update walk_v3_surface to add path context to errors from
fs::read_dir(&dir) and entry.file_type(), matching the existing contextual
handling for fs::read_to_string. Preserve each original error kind while
including the offending directory or entry path in the message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c374ceb6-4154-4914-a32c-44d169cc5904
📒 Files selected for processing (12)
.github/workflows/README.md.github/workflows/test-eql.yml.gitignoreCLAUDE.mdDEVELOPMENT.mdcrates/eql-codegen/src/generate.rscrates/eql-codegen/src/lib.rscrates/eql-codegen/src/main.rscrates/eql-codegen/src/ordering.rscrates/eql-codegen/tests/cli.rscrates/eql-codegen/tests/parity.rsdocs/reference/adding-a-scalar-encrypted-domain-type.md
🚧 Files skipped from review as they are similar to previous changes (9)
- .gitignore
- crates/eql-codegen/src/lib.rs
- crates/eql-codegen/src/generate.rs
- crates/eql-codegen/src/main.rs
- .github/workflows/README.md
- docs/reference/adding-a-scalar-encrypted-domain-type.md
- .github/workflows/test-eql.yml
- crates/eql-codegen/tests/parity.rs
- crates/eql-codegen/tests/cli.rs
…er id The symbol-order gate's comments cited a private issue identifier, which the test:public_identifiers gate on main now rejects. State the reason directly instead: query operands live in eql_v3 rather than public because a query operand is never a column type. Same pass corrects two staleness bugs in those comments: the containment needle is eql_v3.query_json (renamed with src/v3/jsonb -> src/v3/json), and the hardcoded "39 query domains" is now 40 — replaced with a count-free phrasing so it cannot rot again.
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
2 participants
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.
Build-ordering refactor
Moves SQL dependency-ordering for the whole
src/v3surface out of the bashcomment-parse → tsort → tac → catpipeline and into the Rust codegen.eql-codegen orderwalkssrc/v3once, parses the-- REQUIRE:edges every file declares (hand-written and generated alike), and topologically sorts them;tasks/build.shconcatenates in that order.tsortis gone from the build entirely.This is a pure reordering + verification change: no SQL body is modified.
Why one walk
The intermediate design here ordered the surface as two blocks — hand-written files (shell-globbed, minus the
-- AUTOMATICALLY GENERATED FILE.marker) plus a codegen-emitted manifest ofrender_typeoutput — withtsortdemoted to a verifier over the union. Two enumerations means two predicates, and a file matching neither (a generated file rendered outsiderender_type, say) is silently dropped from the installer while every check stays green.Ordering exactly the set you walk makes that unrepresentable, and it removes the marker classifier, the codegen manifest, and the shell
tsortwrapper along with it.install_order_contains_every_v3_sql_file(parity tests) pins the order against an independent walk — set equality, not a count.What changed
crates/eql-codegen/src/ordering.rs(new).walk_v3_surface+surface_order: Kahn with a name-keyed min-heap ⇒ byte-reproducible order. Dangling-- REQUIRE:targets (UnknownTargets), edges leavingsrc/v3(OutsideSurface), self-requires (SelfEdge), and cycles (Cycle) are hard errors that name every offender. These subsume the oldverify_deps_existandverify_v3_self_containedshell gates — the invariants now travel with the sort instead of with whoever remembers to call the checker. 18 unit tests.eql-codegen ordersubcommand. Prints the install order, one repo-relative path per line. Stdout contract pinned intests/cli.rs.tasks/build.sh. Concatenates fromeql-codegen order; no dep-list construction, notsort, no| tac(absent on stock macOS anyway). The order is written via a temp file so an aborted run leaves no truncated list for downstream tasks to read.tasks/build/ordering.sh(new). Onlystrip_require_lines— an anchored-- REQUIRE:strip replacing the unanchoredgrep -v REQUIRE, which would eat any body line containing the substring. Propagates real grep failures (exit ≥ 2) rather than|| true-ing them into a truncated monolith.verify_symbol_order_v3.sh(new). DB-free pre-flight: aneql_v3./eql_v3_internal./public.<domain>reference must be defined by a file ordered earlier. Comment-stripped; recognises theCREATE DOMAIN/CREATE TYPE/CREATE OPERATOR CLASS|FAMILYdefinition forms. Deliberately stricter than Postgres forLANGUAGE plpgsqlforward references — the allowlist is the documented escape hatch, and is currently empty (no false positives on the real surface).Scope, stated honestly: it is sound for singleton symbols (
hmac_256, the ~39eql_v3.query_*domains, the opclasses,version()) and for plpgsql forward references. It does not resolve overloads — a call site is a bareeql_v3.eq(a, b)carrying no types, andCREATE OPERATORsuppliesLEFTARG/RIGHTARGon other lines, so a line-oriented scan cannot pick the right one.eql_v3.eqalone has 186 definitions spanning files Add support forGROUP BYwithcs_grouped_value_v1#55–feat(numeric): encrypted-domain scalar type for numeric/Decimal #242. Rather than imply otherwise, the gate reports the count it cannot resolve (41 names today) and names the authority. It still rejects a reference that precedes every overload, and still checks overloads whose definitions all precede the use.Overload define-before-use is proven exactly by
test:clean_install_v3— Postgres resolves atCREATEtime when the monolith installs, across PG 14–17, on every relevant PR, with no CipherStash credentials and no fork gating. Verified both ways: swappingtext_eq_operators.sqlahead oftext_eq_functions.sqlin the real order passes the DB-free gate and fails the install withfunction eql_v3.eq(text_eq, text_eq) does not exist(psql exit 3).verify_installer_complete.sh(new). Closes the layer below the order: that the concat loop actually emitted each ordered file's body. 93 of ~244 v3 files are leaves, so a dropped one still installs cleanly and still passes the symbol checker — this gate does arithmetic on lines, which no leaf can hide from.CI + docs.
test:symbol_order_v3,test:installer_complete,test:build_ordering_helperswired intotest-eql.yml;CLAUDE.md,DEVELOPMENT.md, and the scalar-adding reference updated to describe codegen-emitted ordering.Verification
cargo test -p eql-codegen,mise run clean && mise run build,codegen:parity,types:check,test:self_contained_v3,test:symbol_order_v3,test:installer_complete,test:build_ordering_helpers— all green.REQUIRE:gates driven end-to-end (injected cycle / typo'd target fails the build).LC_ALL=C-sorted the pre-refactor baseline monolith against the current one and required byte-identity — it passed. That script was a one-shot and is not committed (53db3eab), so treat the claim as a development observation, not a standing gate. The standing guarantees areinstall_order_contains_every_v3_sql_file(order ⊇ disk) andverify_installer_complete.sh(installer == Σ ordered bodies + pin script).e879fa70): build-cache inputs widened to the Cargo manifests + lockfile (a dep bump changes rendered output with no.rstouched); self--- REQUIRE:now rejected via a dedicatedSelfEdgeerror rather than misreported as a cycle — its tolerance was justified by the oldtsortself-edge-per-file workaround, which died with the shell build; symbol-gate overload blindness made visible rather than silent.#MISE sourcesnow lists the build's shell inputs (an edit toordering.shor either gate previously left mise serving a cached installer built by the old logic),count_linesfails loud instead of yielding a silent zero, and the symbol checker documents its cross-file-only scope.Note
The first commit (
c8d50efb, jsonb docs) is pre-existing work tracked onjsonb-hash-merge-clarification; it rides along here and drops out of the diff if that merges first. The refactor commits are58ca3188..e879fa70.Executed from
.work/eql-v3-build-ordering-refactor-plan.md.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests