Fix HIGH/MEDIUM code-review findings across build, pull, ddl, and deploy#53
Merged
Conversation
Addresses HIGH-severity review findings H1/H2/H3: - H1: LIKE include options rendered `INCLUDING include_comments` and the LIKE branch never emitted its closing `)`. Emit the bare upper-cased option name and always close the paren. - H2: table-level CHECK constraints were dropped from CREATE TABLE. They are now rendered as `CONSTRAINT name CHECK (expr)` entries. - H3: database identifiers flowed raw into filesystem paths, allowing path traversal. A `safe_component` helper now gates every nested and top-level path join and rejects empty/`.`/`..`/slash components. Adds build tests for the LIKE and CHECK paths and writer tests for the path sanitizer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
quote_ident's unquoted fast-path accepted any all-lowercase [a-z0-9_]+ string, letting reserved keywords (order, user, ...) and digit-leading identifiers (2fa) through unquoted, which is invalid SQL. Add a sorted RESERVED_KEYWORDS table (pg_dump's fmtId set) and reject leading digits, quoting both cases like everything else. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MANAGED_DIRS treated every YAML under roles/, users/, and the schema-scoped directories as stale whenever the render didn't reproduce it, regardless of why: --dump replays skip pg_dumpall entirely (roles = None in pull::pull), a live pull's role extraction is best-effort and can fail silently, and --exclude-schema/ --exclude-table deliberately omit objects that still exist in the database. Any of these left --prune free to delete the entire roles/users inventory or excluded objects' files. stale_files now skips roles/ and users/ when pg_dumpall did not run for this update (mirroring the --no-roles/--dump gate in pull::pull), and skips any file whose schema or table matches an active --exclude-schema/--exclude-table pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…replace Type aliases (int4, varchar, timestamptz, ...) were spelled differently between the repo and the server for function return types, domain base types, and foreign-table columns, causing deploy to report a change and fall back to drop+recreate on every run. Route these comparisons through the existing canonical_type() helper, matching what table() already does for ordinary columns: - diff.rs normalize(): canonicalize the "returns" JSON field, not just "data_type", so function-return aliasing no longer flags a permanent Changed diff. - alter.rs resolve() for functions: compare repo/db returns via canonical_type instead of a raw string compare. - alter.rs domain(): compare data_type via canonical_type before deciding to rebuild. - alter.rs foreign_table(): canonicalize column data types before comparing, via a new canonicalize_columns() helper. Also fix a correctness bug: a function whose OUT/TABLE parameter set changed stayed in the "OrReplace" path even though CREATE OR REPLACE FUNCTION cannot alter the output signature and Postgres rejects it at apply time. resolve() now falls back to the gated Resolution::Replace when out_parameters() differs (canonicalized, so an aliased OUT type still uses OrReplace). Finally, close two unescaped-string-interpolation gaps: ALTER EXTENSION ... UPDATE TO and ALTER TYPE ... ADD VALUE now go through the existing string_literal() helper instead of ad hoc quoting/escaping. Skipped: the LOW-priority function drop-ordering key mismatch (mod.rs entry_key vs diff.rs function_key_name) is not touched. entry_key's name comes verbatim from the pg_dump archive tag, which (per pg_dump's own TOC format) omits argument names, while function_key_name intentionally keeps them for identity/ACL matching elsewhere. Unifying the two cleanly would mean parsing the archive tag's signature back into structured parameters or dropping names from function_key_name and risking regressions in other callers -- neither is a small, safe change within src/deploy/. The fallback path this leaves (unordered append) was already documented as safe, just not topologically ordered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- COMMENT ON TRIGGER now qualifies with its owning table (a trigger name is not schema-qualifiable), and COMMENT ON AGGREGATE/OPERATOR now append the (argtypes) signature Postgres requires to disambiguate overloads. COMMENT ON CAST uses the cast's quoted (src AS tgt) signature instead of an erroneous schema-qualified name. - Plain-identifier COMMENT ON targets (tables, views, indexes, etc.) are now quote_ident'd on both namespace and tag; FUNCTION is exempted since its tag already carries a parenthesized signature. - Column comments were silently dropped by build (data loss): dump_table now emits a COMMENT ON COLUMN child entry per commented column, mirroring the existing index-comment pattern. - A view with both check_option and security_barrier emitted two WITH (...) clauses, which CREATE VIEW rejects; they're now merged into a single WITH clause. tests/build_parity.rs is updated to match the corrected CAST/OPERATOR comment output and to assert the newly-recovered column comments, since the Python fixture reflects the old broken/lossy behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…traction Extracts several clauses that the tree-sitter DDL parser was silently dropping during pull, so the round trip through the project model no longer loses schema structure: - Table partitioning: `PARTITION BY` populates `Table.partition`, and `PARTITION OF ... FOR VALUES` children are parsed into a new `Statement::CreateTablePartition` and folded into the parent's `partitions` during pull assembly (deferred/retried the same way indexes are, since pg_dump can order the child before its parent). - `CREATE TABLE ... OF type` and `CREATE TABLE (LIKE ...)` now populate `from_type` and `like_table` (including its INCLUDING/EXCLUDING options). - Foreign key `DEFERRABLE`/`INITIALLY DEFERRED` now read from `ConstraintAttributeSpec`, mirroring the existing trigger extraction. - Table `USING <access_method>`, `WITH (...)` storage parameters, `TABLESPACE`, and a primary/unique constraint's `USING INDEX TABLESPACE` now populate their respective fields via a new shared `ddl::object::reloptions` helper. - Index `WITH (...)` storage parameters, view `security_barrier`, and materialized view storage parameters use the same helper. - C-language functions (`AS 'obj_file', 'link_symbol'`) now populate `object_file`/`link_symbol` instead of only `definition`; `TRANSFORM FOR TYPE ...` populates `transform_types`. Added unit tests in ddl/table.rs, ddl/view.rs, ddl/function.rs, and a pull/mod.rs assembly test covering the deferred-partition merge path. `from_type`/`partitions` are still not rendered by src/build/mod.rs (out of scope here — build/ was off-limits for this change), so a full pull → build round trip for those two forms remains incomplete; deploy already diffs on `partitions`/`from_type` so pulling now at least surfaces the drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…vergence', 'fix/build-comments' and 'fix/ddl-extraction' into fix/review-findings
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds explicit comment targets, partition and reloptions parsing, deploy alias canonicalization, deferred partition merging, stale-file filtering, safe path validation, and reserved-keyword identifier quoting. ChangesComment Target Emission and Table Rendering
Table Partition Support
Storage Parameters, Constraint, and Reloptions Parsing
Function AS/TRANSFORM Parsing
Type Alias Canonicalization in Deploy Reconciliation
Pull Stale File Filtering
Safe Path Writer Helpers
Reserved Keyword Quoting
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 6
🤖 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 `@src/build/mod.rs`:
- Around line 1178-1184: The CHECK constraint rendering in `src/build/mod.rs` is
emitting `check.name` directly, which can produce invalid DDL for reserved words
or non-simple identifiers. Update the `check_constraints` loop in the table DDL
builder to quote the constraint name with `quote_ident(&check.name)`, matching
the identifier handling used elsewhere in this module, while leaving the check
expression unchanged.
In `@src/ddl/object.rs`:
- Around line 418-457: The reloptions parser in reloptions currently skips bare
options because it requires a def_arg, which causes shorthand boolean reloptions
like WITH (security_barrier) to be dropped. Update reloptions so that when a
reloption_elem has a ColLabel but no def_arg, it is recorded with the string
value "true" instead of being continued over, while preserving the existing
handling for explicit Sconst and other values. Add or update a test covering a
bare boolean reloption to verify it round-trips through reloptions.
In `@src/ddl/table.rs`:
- Around line 611-616: The partition_value helper currently collapses composite
partition bounds into a single string, which loses the structure needed for
multi-column bounds. Update partition_value in the table.rs parsing path so that
the _ => branch preserves multiple a_expr entries as a serde_json::Value array
instead of using list.text(src), while keeping single_expr_value for the
one-item case. Use the existing partition parsing symbols like partition_value,
single_expr_value, and TablePartition.for_values_from/for_values_to to ensure
single-value bounds stay scalars and multi-value bounds remain structured JSON.
In `@src/ddl/view.rs`:
- Around line 37-44: The `security_barrier` reloption handling in `view.rs` is
too strict and only parses string booleans, so valid PostgreSQL inputs can be
lost during round-trip. Update the `security_barrier` extraction in the view
parsing path to accept PostgreSQL boolean reloptions case-insensitively
(including bare-key forms handled by `reloptions()` in `object.rs`, as well as
on/off/yes/no/1/0), instead of only relying on `s.parse::<bool>()`. Make the
logic resilient in the `security_barrier` branch so valid `CREATE VIEW ... WITH
(security_barrier)` values are preserved.
In `@src/utils.rs`:
- Around line 5-108: The `RESERVED_KEYWORDS` list in `quote_ident` is incomplete
for pg_dump-style quoting, so identifiers that are keywords but not currently
listed can slip through unquoted. Update the keyword set in `utils.rs` to
include all non-UNRESERVED entries used by pg_dump, including missing values
like `all` and the COL_NAME_KEYWORD group, and keep the list sorted so
`binary_search` continues to work.
In `@tests/build_parity.rs`:
- Around line 187-188: The build parity test expectation includes accidental
trailing commas in the `users.last_modified_at` tag and its comment text, so the
assertion is matching the wrong strings. Update the string literals in the
affected test case in `tests/build_parity.rs` to use the exact tag and comment
emitted by the builder, and verify the surrounding expectations in the same test
data remain consistent with the `<table>.<column>` format used by the relevant
assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bc38c95b-72d6-4e5e-ae60-d2ac2725ebfd
📒 Files selected for processing (13)
src/build/mod.rssrc/ddl/function.rssrc/ddl/mod.rssrc/ddl/object.rssrc/ddl/table.rssrc/ddl/view.rssrc/deploy/alter.rssrc/deploy/diff.rssrc/pull/mod.rssrc/pull/update.rssrc/pull/writer.rssrc/utils.rstests/build_parity.rs
- Parse bare reloptions (WITH (security_barrier)) as `true` in object::reloptions, so shorthand boolean options round-trip instead of being dropped. - Accept PostgreSQL boolean reloption spellings (on/off/yes/no/1/0) case-insensitively for view security_barrier via a pg_bool helper. - Preserve multi-column RANGE partition bounds as a JSON array in partition_value instead of collapsing them to a flat string. - Add the missing RESERVED keyword `all` to quote_ident's keyword set. The broader COL_NAME_KEYWORD set is intentionally not added: those keywords double as type names and quote_ident is also applied to type names (e.g. CAST target types), where quoting them would diverge from pg_dump/Python parity. Guard list sortedness with a test. - Fix trailing-comma typos in test-project users.yaml (column name, comment, and data_type) that were the real source of the commas CodeRabbit flagged in build_parity; update the test expectation to match the corrected fixture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
🤖 This comment was posted by Claude on behalf of @gmr Addressed the latest CodeRabbit review (commit Fixed
Declined (with reasoning)
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ddl/table.rs (1)
590-636: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse direct children for partition bounds
find_all("expr_list")/find_all("a_expr")walks descendants, so nested row/composite expressions can get flattened into the top-level FROM/TO or IN lists. Restrict these lookups to the immediate partition-bound lists to avoid mis-parsing composite bounds.🤖 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 `@src/ddl/table.rs` around lines 590 - 636, The partition-bound parsing in table.rs is flattening nested composite expressions because partition_bound, partition_value, and partition_values rely on find_all("expr_list") and find_all("a_expr"), which searches descendants instead of only the direct bound items. Update the parsing to use immediate children for the FROM/TO and IN lists so nested row/composite expressions stay grouped correctly, and keep the existing single_expr_value handling unchanged.
🧹 Nitpick comments (1)
src/ddl/view.rs (1)
103-113: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
pg_booldoesn't accept single-letter prefixes.PostgreSQL's boolean parser also accepts unique prefixes like
t,f,y,n(case-insensitive) in addition to the full words already handled here.pg_boolwould returnNoneforsecurity_barrier=t, silently dropping a valid value.♻️ Proposed fix
fn pg_bool(value: &str) -> Option<bool> { match value.trim().to_ascii_lowercase().as_str() { - "true" | "on" | "yes" | "1" => Some(true), - "false" | "off" | "no" | "0" => Some(false), + "true" | "t" | "on" | "yes" | "y" | "1" => Some(true), + "false" | "f" | "off" | "no" | "n" | "0" => Some(false), _ => None, } }🤖 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 `@src/ddl/view.rs` around lines 103 - 113, pg_bool in view.rs only handles the full boolean words, so it incorrectly rejects PostgreSQL’s accepted single-letter prefixes like t, f, y, and n. Update pg_bool to recognize those case-insensitive prefixes alongside the existing true/false/on/off/yes/no/1/0 handling, and keep the same trimming/lowercasing behavior so reloption parsing for values like security_barrier=t returns the correct bool instead of None.
🤖 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.
Outside diff comments:
In `@src/ddl/table.rs`:
- Around line 590-636: The partition-bound parsing in table.rs is flattening
nested composite expressions because partition_bound, partition_value, and
partition_values rely on find_all("expr_list") and find_all("a_expr"), which
searches descendants instead of only the direct bound items. Update the parsing
to use immediate children for the FROM/TO and IN lists so nested row/composite
expressions stay grouped correctly, and keep the existing single_expr_value
handling unchanged.
---
Nitpick comments:
In `@src/ddl/view.rs`:
- Around line 103-113: pg_bool in view.rs only handles the full boolean words,
so it incorrectly rejects PostgreSQL’s accepted single-letter prefixes like t,
f, y, and n. Update pg_bool to recognize those case-insensitive prefixes
alongside the existing true/false/on/off/yes/no/1/0 handling, and keep the same
trimming/lowercasing behavior so reloption parsing for values like
security_barrier=t returns the correct bool instead of None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e29eb66-3a67-4014-9034-a70a54243f4d
📒 Files selected for processing (6)
src/ddl/object.rssrc/ddl/table.rssrc/ddl/view.rssrc/utils.rstest-project/tables/test/users.yamltests/build_parity.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/utils.rs
- src/ddl/object.rs
- tests/build_parity.rs
Owner
Author
|
PR monitoring complete — this PR is ready to merge.
Fixes applied: bare reloptions, view Not merging — left for manual merge as requested. |
pg_bool recognized the full boolean words (true/false/on/off/yes/no) and 1/0 but rejected PostgreSQL's accepted single-letter forms, so a reloption like security_barrier=t parsed as None. Add the case- insensitive t/f/y/n forms alongside the existing set, keeping the same trim/lowercase behavior. Addresses a CodeRabbit review finding on PR #53. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Addresses the HIGH-severity and highest-priority MEDIUM findings from the
2026-07-06 five-domain code review (
CODE-REVIEW-REPORT.md). Work wassplit across five isolated worktrees by disjoint file ownership, then
integrated; the full
just checkand both Postgres-backed gate suites(
just gates) pass on the merged tree.Problem
The review surfaced several correctness bugs that produced unrestorable
archives, silent data loss, non-converging deploys, and a destructive
--prunepath:buildemitted invalidCOMMENT ONSQL for triggers/aggregates/operators/casts and unquoted comment targets; dropped column comments
and table CHECK constraints entirely; emitted two
WITHclauses on aview; mangled
LIKEinclude options.pullsilently discarded table partitioning, typed/LIKEtables, FKdeferrability, storage parameters, tablespaces, view security_barrier,
and C-function link symbols.
deploynever converged when types were spelled with aliases(
int4vsinteger) and could emit an invalidOR REPLACEon anOUT-parameter change.
quote_identfailed to quote reserved keywords and digit-leadingidentifiers.
pull --update --prunedeletedroles//users/and exclude-filteredfiles it had never re-derived — data loss.
Solution
src/build/mod.rs): per-object-typeCOMMENT ONtargets(trigger
ON table, aggregate/operator(argtypes), cast(src AS tgt)), quoted plain-identifier targets,COMMENT ON COLUMNchild entries, table CHECK constraints, single merged view
WITH,corrected
LIKEoptions + closing paren.src/ddl/*,src/pull/mod.rs): extract partitioning(parent
PARTITION BY+PARTITION OFchildren via a deferred-retryqueue),
OF type/LIKE, FK deferrable, storage params, access method,tablespaces, security_barrier, and C-function object_file/link_symbol/
transform_types.
src/deploy/*): comparereturns/domain/foreign-tabletypes through
canonical_type; fall to gatedReplaceon OUT/TABLEparam changes; route version/enum literals through
string_literal.src/utils.rs):quote_identquotes reserved keywords anddigit-leading identifiers.
src/pull/update.rs): scope--prunestaleness to theclasses the run actually covered — skip roles/users when pg_dumpall
didn't run, and never prune exclude-filtered paths.
Each fix ships with unit tests;
tests/build_parity.rswas updated forthe corrected/newly-recovered comment output (64→76 entries).
Verification
just check— fmt,clippy -D warnings, full test suite: green.just gates— round-trip diff empty; deploy equivalence + convergenceempty; safety gate excludes drops without
--allow-drop.Deferred follow-ups (not in this PR)
N-L2 (build does not yet render
from_type/partitions, so a fullround-trip for typed/partitioned tables is still incomplete), L5 (drop
ordering), M6 (duplicate-object loader guard), N-M2 (role comments on
pull), and the remaining low/efficiency/duplication items.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Summary by CodeRabbit
CREATE TABLE ... PARTITION OF ...ingestion and deferred attachment to parent tables.TRANSFORMtypes, and richerCOMMENTtargeting (includingCOMMENT ON COLUMN).LIKE ... INCLUDING/EXCLUDING ..., tableCHECKconstraints, and viewWITH (...)parsing/formatting (includingsecurity_barrier).