fix(mapper): inference hardening — LIMIT/FETCH, UPDATE targets, unmatched statements (CIP-3700)

[freshtonic]

Implements CIP-3700: hardening for the likely-benign loose ends found in the InferType survey (companion to CIP-3699).

1. Query's limit_clause / fetch pinned to Native

infer_type_impls/query_statement.rs only handled body and order_by; expressions in LIMIT/OFFSET/FETCH were left as unconstrained type variables. In practice a bare LIMIT $1 placeholder was quietly mopped up by the late unresolved-value fallback (Unifier::resolve_unresolved_value_nodes), so it did not fail — but the constraint now lives where the clause is inferred instead of relying on that catch-all, and it is stronger: an encrypted column used as a row count (LIMIT enc_col) is now rejected by the mapper instead of being forwarded. All LimitClause shapes are covered (LIMIT/OFFSET, LIMIT BY, MySQL LIMIT o, l) plus Fetch::quantity. Query::locks (FOR UPDATE/SHARE) carries no expressions in the AST, so there is nothing to constrain there — noted in a comment.

2. Update assignment targets resolve against table (the FIXME at statement.rs:23)

Assignment targets used to resolve through the lexical scope, which also contains the FROM relations, so UPDATE t1 SET x = $1 FROM t2 with a same-named column on t2 made the target spuriously ambiguous (AmbiguousMatch) — the shadowing trap the FIXME warned about. Targets now resolve via table_resolver.resolve_table_column against the table being updated, mirroring how INSERT resolves its columns. A non-table UPDATE target (or a joined target table, which PostgreSQL cannot produce) is rejected with UnsupportedSqlFeature rather than guessed at.

5. The fail-open _ => {} in InferType<Statement> now fails closed

All seven variants admitted by requires_type_check are matched explicitly, so the wildcard was dead code — but a future widening of requires_type_check would have traversed the statement without constraining its top-level type. The arm now returns a TypeError::InternalError naming the statement and stating the invariant (every variant requires_type_check admits must have an explicit arm). A truly exhaustive match over sqltk's ~100 Statement variants would break on every parser bump, so the invariant is stated and enforced at the wildcard instead; rejection (rather than unreachable!) keeps an invariant break from panicking a proxy worker.

Surveyed and fine — no change

• 3. Delete's using / selection: covered by ordinary Expr traversal.
• 4. Aggregate filter / null_treatment: covered by ordinary Expr traversal.

Tests

Five new mapper tests in packages/eql-mapper/src/lib.rs:

• limit_and_offset_placeholders_infer_native, fetch_first_placeholder_infers_native — $n in LIMIT/OFFSET/FETCH resolves to Native (regression pins; these passed pre-change via the late fallback).
• encrypted_column_in_limit_is_rejected — fails without the fix.
• update_assignment_resolves_against_target_table_not_from_relation — same-named column on the FROM table; the assignment gets the target table's (encrypted) type. Fails without the fix.
• statement_without_inference_rule_fails_closed — TRUNCATE through type_check directly, asserting the invariant-stating error. Fails without the fix.

Verification

• mise run check — clean.
• cargo test -p eql-mapper — 139 passed, 0 failed.
• cargo test -p cipherstash-proxy — all 121 unit tests pass single-threaded; the config::tandem env-var races and the three doc-test failures reproduce identically on a clean origin/main checkout (pre-existing, unrelated).
• Integration tests were not run: this is mapper-only hardening per the ticket.

Coordination

A concurrent branch for CIP-3699 touches other parts of query_statement.rs (ORDER BY handling) and the infer_type_impls; this diff is kept minimal and does not incorporate it, so a small textual overlap in query_statement.rs is expected at rebase time.

Summary by CodeRabbit

• Bug Fixes

  • Fixed UPDATE assignments so target-table columns resolve correctly.
  • Enforced native numeric values for LIMIT, OFFSET, and FETCH row counts.
  • Encrypted expressions and unsupported LIMIT ... BY clauses are now rejected.
  • Statements without supported inference rules now fail with a clear error.
  • Resolved table and column names now use canonical schema identifiers.

• Tests

  • Added regression coverage for update assignments, row-count validation, identifier resolution, and unsupported statements.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 459c953d-e9b2-48ac-813e-60fedbf91bbd

📥 Commits

Reviewing files that changed from the base of the PR and between 7c6342f and 561e9c6.

📒 Files selected for processing (5)

• CHANGELOG.md
• packages/eql-mapper/src/inference/infer_type_impls/query_statement.rs
• packages/eql-mapper/src/inference/infer_type_impls/statement.rs
• packages/eql-mapper/src/lib.rs
• packages/eql-mapper/src/model/schema_delta.rs

🚧 Files skipped from review as they are similar to previous changes (4)

• packages/eql-mapper/src/model/schema_delta.rs
• packages/eql-mapper/src/inference/infer_type_impls/statement.rs
• CHANGELOG.md
• packages/eql-mapper/src/lib.rs

---

📝 Walkthrough

Walkthrough

The EQL Mapper now resolves UPDATE assignments against target tables, enforces native types for row-count clauses, canonicalizes resolved identifiers, and reports missing inference rules immediately. Regression tests cover these behaviors.

Changes

EQL Mapper inference

Layer / File(s)	Summary
UPDATE assignment resolution packages/eql-mapper/src/inference/infer_type_impls/statement.rs, packages/eql-mapper/src/model/schema_delta.rs, packages/eql-mapper/src/lib.rs, CHANGELOG.md	UPDATE assignments resolve columns from the updated plain table. Resolved identifiers use canonical schema names. Joined or non-table targets return UnsupportedSqlFeature.
Row-count clause inference packages/eql-mapper/src/inference/infer_type_impls/query_statement.rs, packages/eql-mapper/src/lib.rs, CHANGELOG.md	LIMIT, OFFSET, and FETCH quantities use native types. Encrypted row-count expressions are rejected.
Fail-closed statement inference packages/eql-mapper/src/inference/infer_type_impls/statement.rs, packages/eql-mapper/src/lib.rs, CHANGELOG.md	Unhandled statements now return a statement-specific InternalError instead of proceeding without inference.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: tobyhede

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the mapper inference hardening changes, including LIMIT/FETCH handling, UPDATE targets, and unmatched statements.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch james/cip-3700-eql-mapper-inference-hardening-unconstrained-limitfetch

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 `@CHANGELOG.md`:
- Around line 45-49: Rewrite the three changelog entries to describe observable
SQL behavior and Proxy responses rather than resolver scope, inference timing,
or internal invariants. Mention that same-named UPDATE/FROM columns now resolve
correctly, encrypted LIMIT/OFFSET/FETCH expressions are rejected with a type
error, and unsupported statements fail immediately with an error naming the
statement.

In `@packages/eql-mapper/src/inference/infer_type_impls/query_statement.rs`:
- Around line 79-117: Move the row-count type-inference logic currently added
around the query statement handling into the appropriate module under
transformation_rules, or make the query_statement implementation delegate to
that module. Preserve the Native unification for LIMIT, OFFSET, LIMIT BY, and
FETCH quantities while keeping infer_type_impls/query_statement.rs as a thin
adapter.
- Around line 99-101: Remove the loop that calls unify_node_with_type for each
expr in limit_by within the LimitClause::LimitOffset handling. Constrain only
the global LIMIT count, leaving limit_by expressions to the normal
type-inference path so encrypted grouping keys remain valid.

🪄 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: a3798ef5-55b3-4e90-9884-ac36adcb4c14

📥 Commits

Reviewing files that changed from the base of the PR and between 15b7f99 and 5abfdf5.

📒 Files selected for processing (4)

• CHANGELOG.md
• packages/eql-mapper/src/inference/infer_type_impls/query_statement.rs
• packages/eql-mapper/src/inference/infer_type_impls/statement.rs
• packages/eql-mapper/src/lib.rs

[freshtonic]

Comment-only review.\n\nThe LIMIT/FETCH native constraints, target-table resolution for UPDATE assignments, and fail-closed statement fallback look coherent.\n\nFinding: every PostgreSQL matrix job is currently failing in . The client attempts to bind a Rust where the prepared statement reports , producing . This may be an existing/base-branch issue rather than caused by the mapper hardening here, but the current head is red across all supported PostgreSQL versions and needs to be updated or the failure accounted for before merge.

[freshtonic]

Correction to my preceding review (shell formatting stripped identifiers): all matrix jobs fail in update_with_param_reused_for_storage_and_query because a Rust String is bound where the prepared statement reports eql_v3_text_search, producing WrongType.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.