Fix silent statement/data loss in root render/parse path

[gmr]

Summary

Three bugs in the root render/parse path (src/formatter/mod.rs, src/lib.rs) caused statements, whole broken statements, and comments to be silently dropped rather than formatted or reported as errors.

Problem

• BEGIN transaction statements are silently dropped from formatted output #15 format_root only emitted a statement when its toplevel_stmt had a stmt child. tree-sitter-postgres parses BEGIN as (toplevel_stmt (TransactionStmtLegacy ...)) — a direct child, not wrapped in stmt — so BEGIN; returned Ok("") and multi-statement input like BEGIN; INSERT ...; COMMIT; silently lost the BEGIN.
• format() returns Ok and silently drops unparseable statements in multi-statement input #16 has_structural_error returned false as soon as any toplevel_stmt existed, so a genuinely broken sibling statement never triggered FormatError::Syntax. SELECT 1; THIS IS NOT SQL @@@; returned Ok("SELECT 1;"), silently discarding the broken input instead of erroring.
• SQL comments are silently discarded from formatted output #17 Standalone SQL comments between/around top-level statements were discarded.

Solution

• BEGIN transaction statements are silently dropped from formatted output #15 When a toplevel_stmt has no stmt child, fall back to the toplevel_stmt node so the inner statement (e.g. TransactionStmtLegacy) still routes through the statement formatter. New river fixtures begin and begin_insert_commit.
• format() returns Ok and silently drops unparseable statements in multi-statement input #16 Once at least one statement has parsed, recursively scan for MISSING nodes or ERROR nodes >= 5 bytes and return Err(FormatError::Syntax(..)). Small (< 5 byte) benign ERROR leaves remain tolerated per the documented grammar-limitation behavior; all 70 existing fixtures still pass. New smoke test broken_statement_errors_instead_of_dropping.
• SQL comments are silently discarded from formatted output #17 Standalone comments that are direct children of source_file are now preserved verbatim in source order. Note: comments embedded within a statement's clauses are still dropped — faithful intra-statement comment preservation is a larger feature requiring comment handling threaded through every clause formatter, out of scope for a surgical fix. New river fixture comment_between_statements. This partially addresses SQL comments are silently discarded from formatted output #17.

Fixes #15
Fixes #16

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved formatting of top-level SQL so comments between statements are preserved in the output.
  • More reliably handles transaction blocks like BEGIN / COMMIT during formatting.
  • Invalid SQL at the end of an input now returns a clear syntax error instead of being skipped.
• Tests
  • Added coverage for comments between statements and transaction-style SQL examples.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 6 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5b05f23b-cbeb-4625-bd93-c0d458f270bf

📥 Commits

Reviewing files that changed from the base of the PR and between 50a6998 and 22486df.

📒 Files selected for processing (1)

• src/formatter/mod.rs

📝 Walkthrough

Walkthrough

This PR fixes two formatter bugs: format_root now preserves top-level BEGIN-style transaction statements lacking a stmt child and retains standalone comments between statements; has_structural_error gains a has_significant_error helper detecting MISSING/large ERROR nodes, causing FormatError::Syntax for broken statements instead of silent omission. New fixtures and a regression test validate both fixes.

Changes

Preserve BEGIN statements and top-level comments

Layer / File(s)	Summary
format_root child handling src/formatter/mod.rs	Formats the nested stmt when present, otherwise the toplevel_stmt itself, and adds a branch to preserve standalone comment nodes verbatim in source order.
BEGIN/transaction fixtures tests/fixtures/river/begin.sql, tests/fixtures/river/begin.expected, tests/fixtures/river/begin_insert_commit.sql, tests/fixtures/river/begin_insert_commit.expected	New/updated fixtures verify BEGIN; alone and BEGIN;/INSERT/COMMIT; sequences are formatted correctly rather than dropped.
Comment-between-statements fixtures tests/fixtures/river/comment_between_statements.sql, tests/fixtures/river/comment_between_statements.expected	New fixtures verify a -- note comment between two SELECT statements is preserved in expected output.

Detect significant parse errors instead of silently dropping statements

Layer / File(s)	Summary
has_structural_error / has_significant_error logic src/lib.rs	has_structural_error now also invokes new has_significant_error, which recursively flags MISSING nodes or ERROR nodes with byte range ≥ 5, returning FormatError::Syntax when found.
Regression test for broken statements tests/smoke_test.rs	Imports FormatError and adds a test asserting format returns Err(FormatError::Syntax(_)) for input with an invalid trailing statement.

Estimated code review effort: 2 (Simple) | ~12 minutes

Possibly related PRs

• gmr/libpgfmt#1: Both PRs modify the core format_root and has_structural_error logic in the formatter entrypoints.
• gmr/libpgfmt#3: Both PRs adjust has_structural_error's error-tolerance logic based on toplevel_stmt/ERROR/MISSING node detection.

Poem

A rabbit hopped through parse trees deep,
Found BEGIN statements the code let sleep 🐇
Now comments linger, transactions stay whole,
And broken SQL can't sneak past the goal.
Thump-thump — errors surface, fixtures cheer,
Another clean burrow of code this year! 🥕

🚥 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 main fix: preventing silent data loss in the formatter's root path.
Linked Issues check	✅ Passed	The code and tests address both linked issues: BEGIN nodes are preserved and broken multi-statement input now errors.
Out of Scope Changes check	✅ Passed	The added fixtures and tests support the stated formatter/error-handling fixes and don't introduce unrelated scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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

[coderabbitai]

🧹 Nitpick comments (1)

src/formatter/mod.rs (1)

190-194: 📐 Maintainability & Code Quality | 🔵 Trivial

Matching on Style enum variant violates path coding guideline.

if self.style == Style::PgDump { ... } else { ... } matches directly on the Style enum. As per coding guidelines, src/formatter/**/*.rs should "Use StyleConfig flags (boolean/string) in formatter logic instead of matching on Style enum variants." Consider adding a pg_dump-style boolean to StyleConfig and branching on that instead, consistent with how the rest of the formatter dispatches on self.config.* flags (e.g. self.config.river, self.config.leading_commas seen elsewhere in the file).

♻️ Suggested direction

-if self.style == Style::PgDump {
+if self.config.pg_dump {
     results.push(self.format_pgdump_stmt(stmt)?);
 } else {
     results.push(self.format_stmt(stmt)?);
 }

🤖 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/formatter/mod.rs` around lines 190 - 194, The formatter dispatch in
`format_stmt` is branching directly on `Style::PgDump`, which violates the
formatter coding guideline. Update this logic to use a boolean flag on
`StyleConfig` (for example a `pg_dump`-style flag) and branch on `self.config.*`
instead of matching the `Style` enum, following the same pattern used for other
formatter options like `self.config.river` and `self.config.leading_commas`.

Source: Coding guidelines

🤖 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 `@src/formatter/mod.rs`:
- Around line 190-194: The formatter dispatch in `format_stmt` is branching
directly on `Style::PgDump`, which violates the formatter coding guideline.
Update this logic to use a boolean flag on `StyleConfig` (for example a
`pg_dump`-style flag) and branch on `self.config.*` instead of matching the
`Style` enum, following the same pattern used for other formatter options like
`self.config.river` and `self.config.leading_commas`.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: de63b472-7e4e-44e3-8792-ddf7f7c97b24

📥 Commits

Reviewing files that changed from the base of the PR and between c949e2b and 50a6998.

📒 Files selected for processing (9)

• src/formatter/mod.rs
• src/lib.rs
• tests/fixtures/river/begin.expected
• tests/fixtures/river/begin.sql
• tests/fixtures/river/begin_insert_commit.expected
• tests/fixtures/river/begin_insert_commit.sql
• tests/fixtures/river/comment_between_statements.expected
• tests/fixtures/river/comment_between_statements.sql
• tests/smoke_test.rs

[gmr]

Addressed in 22486df: added a pg_dump boolean to StyleConfig and changed the root statement loop to branch on self.config.pg_dump instead of matching Style::PgDump, per the formatter coding guideline.