Lex restore-reachable SQL function calls - #1159
Conversation
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning Your free Security trial is over. An organization admin can activate billing to continue. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6541cd65ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if bytes[cursor] == b'\\' { | ||
| cursor = (cursor + 2).min(bytes.len()); |
There was a problem hiding this comment.
With PostgreSQL's default standard_conforming_strings=on, a plain literal such as '\' ends at the quote after the backslash, but this branch always treats that quote as escaped. A valid body such as SELECT '\', restore_probe_tail() is therefore consumed through EOF and the real call is omitted from the reachability closure, allowing an unpinned reachable function to pass INV-070. Only E'...' strings should apply backslash escaping.
AGENTS.md reference: AGENTS.md:L6-L10
Useful? React with 👍 / 👎.
| b'\'' => { | ||
| cursor = skip_single_quoted(bytes, cursor); | ||
| preceding_identifier = None; |
There was a problem hiding this comment.
When a check-reachable PL/pgSQL function executes a literal query such as EXECUTE 'SELECT helper()', helper is genuinely reachable during restore, but this unconditional string skip removes it from the closure. The previous conservative regex did discover that call, so this change lets an unpinned dynamic callee pass the invariant; either recognize call-shaped names in EXECUTE query literals or explicitly reject dynamic application-function calls from reachable bodies.
AGENTS.md reference: AGENTS.md:L6-L10
Useful? React with 👍 / 👎.
| const fn is_identifier_start(byte: u8) -> bool { | ||
| byte.is_ascii_alphabetic() || byte == b'_' | ||
| } |
There was a problem hiding this comment.
PostgreSQL permits letters with diacritics and non-Latin letters in unquoted identifiers, but this byte classifier accepts only ASCII. A valid call such as réachable() is tokenized as separate fragments and never compared with the catalogue name, so a migration using such a function can leave it unpinned while INV-070 still passes; scan identifiers as Unicode characters according to PostgreSQL's identifier rules rather than rejecting every non-ASCII byte.
AGENTS.md reference: AGENTS.md:L6-L10
Useful? React with 👍 / 👎.
| #[test] | ||
| fn quoted_function_identifier_is_a_call_edge() { |
There was a problem hiding this comment.
These seven new focused tests directly enforce the lexical clauses added to INV-070, but none of their names or attached doc comments carries the invariant identifier. The repository's invariant discovery intentionally recognizes only declaration-local test names and doc comments, so these enforcement points are undiscoverable as INV-070 coverage until the tag is added.
AGENTS.md reference: AGENTS.md:L231-L235
Useful? React with 👍 / 👎.
| b'"' => { | ||
| let (identifier, after) = quoted_identifier(source, cursor); | ||
| cursor = after; | ||
| preceding_identifier = identifier; |
There was a problem hiding this comment.
Quoted identifiers followed by ( are not necessarily calls: PostgreSQL permits constructs such as WITH "helper"(value) AS (...) and FROM table AS "helper"(value), where the parentheses introduce a column-alias list. If an unrelated schema function is also named helper, this lexer adds it to the reachable set and can fail the restore invariant solely because that unrelated function is unpinned; the classifier needs enough syntactic context to distinguish these aliases from quoted function invocations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6541cd65ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cursor += 1; | ||
| continue; | ||
| } | ||
| identifier.push_str(&source[segment_start..cursor]); |
There was a problem hiding this comment.
When a function body calls a catalogue function through PostgreSQL's Unicode-escaped identifier syntax, such as U&"restore\005fprobe\005ftail"(), PostgreSQL resolves the decoded name restore_probe_tail, but quoted_identifier records the raw escape spelling. The name therefore never matches the catalogue entry, allowing that reachable function to remain unpinned without failing INV-070; recognize the U& prefix and decode its escapes, including an optional UESCAPE character.
AGENTS.md reference: AGENTS.md:L8-L10
Useful? React with 👍 / 👎.
Rust coverage (report only)Report only. This measurement has no threshold, gates no merge, and
What this number does not measure
Per crate, least-covered first
25 files with the most uncovered lines
Measured at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Outcome
Restore-reachability validation now lexes SQL function bodies instead of relying on one regular expression. It recognizes quoted function identifiers and comments between a function name and its opening parenthesis, while excluding aliases and call-shaped text inside comments or string literals. Fixed-point closure still finds functions reached through multiple body hops.
The persistence contract's verified-against reference advances with the implementation, and focused straight-line tests cover each lexical boundary.
Meaningfully changed lines: 392 (325 additions, 67 deletions; no lockfiles).
Validation
cargo fmt --all -- --checksignalboxdis byte-identical to the deployed parent artifactNumeric-bound ceilings added by this slice: none.
Absorption ledger
eeee1c5b5.4cdc5dfbc.546600143; reconstruction chain: Inventory numeric-bound deployment policy #1069 → Require deployment numeric-bound configuration #1072 → Route protocol and application bounds through required configuration #1077 → dogfood → gate.