perf(core): reduce redundant formatter traversal work#960
Conversation
|
Warning Review limit reached
Next review available in: 23 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 Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis change adds AST phase benchmarking and a staged paired-runner workflow with persisted evidence, while optimizing SQL parameter collection and comment traversal. It also adds validation tests, benchmark records, reference data, documentation, and release changesets. ChangesAST benchmark workflow
Formatter traversal optimizations
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant PairedRunner
participant PhaseBenchmark
participant JSONLRecord
CLI->>PairedRunner: provide benchmark manifest
PairedRunner->>PhaseBenchmark: execute baseline and candidate runs
PhaseBenchmark-->>PairedRunner: return reports and semantic sinks
PairedRunner->>PairedRunner: classify deltas and mismatches
PairedRunner->>JSONLRecord: write summary and append candidate record
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
benchmarks/ast-analysis-phase-benchmark.ts (1)
875-893: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExclusive-write flag (
wx) is only applied to the explicit--output-filepath.The
--output-filebranch usesflag: 'wx'to prevent silently clobbering an existing raw artifact (matching the append-only evidence requirement described in the paired-runner docs), but the default auto-generated path (Line 891) writes without that flag. Collision risk is low given the timestamp+pid naming, but adding the same exclusivity guard would make the two branches consistent.🤖 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 `@benchmarks/ast-analysis-phase-benchmark.ts` around lines 875 - 893, Update writeReport so the default generated output path uses the same exclusive-write mode as the explicit --output-file branch. Preserve the existing generated filename and serialization behavior while passing the equivalent wx flag to its fs.writeFileSync call.
🤖 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 `@benchmarks/ast-analysis-paired-runner.ts`:
- Around line 803-823: Update the commandsComplete validation in the
confirmation command check to verify the complete invocation, matching the
screen-evidence validation: require the expected command and cwdRole, and ensure
command.args exactly matches the expected --profile, --phases, --condition,
--candidate-id, --pair-index, and --output-file values rather than checking only
selected arguments.
- Around line 560-571: Update the full-profile predecessor-screen validation
around validateConfirmationRecord to validate the complete Stage 1 evidence
contract, including the required two commands, artifacts, and rows, before
admitting Stage 3. Reuse the existing Stage 1 validation path or equivalent
checks rather than only validating inherited metadata, while preserving the
missing-screen error behavior.
- Around line 924-930: Add a stage-appropriate timeout to the spawnSync
invocation in the paired-run execution flow, and handle timeout expiry as a
failed run so the loop continues to produce summary and failure artifacts.
Preserve the existing command, arguments, environment, and normal success/error
handling.
In `@scripts/ast-analysis-paired-runner.test.ts`:
- Line 1150: Update the failure-retention tests around runPairedBenchmark to
stub the benchmark subprocess via spawnSync, returning a nonzero result instead
of executing the real benchmark. Keep the tests’ assertions and
incomplete-checkout setup focused on retention behavior while making them
independent of benchmark layout changes.
---
Nitpick comments:
In `@benchmarks/ast-analysis-phase-benchmark.ts`:
- Around line 875-893: Update writeReport so the default generated output path
uses the same exclusive-write mode as the explicit --output-file branch.
Preserve the existing generated filename and serialization behavior while
passing the equivalent wx flag to its fs.writeFileSync call.
🪄 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: f31c865e-0442-4db9-ad2d-eead3d9af524
📒 Files selected for processing (16)
.changeset/calm-formatters-collect-once.md.changeset/reduce-printer-comment-traversal.mdbenchmarks/ast-analysis-paired-runner.tsbenchmarks/ast-analysis-phase-benchmark.tsdocs/bench/ast-analysis-benchmark-candidate-records.jsonldocs/bench/ast-analysis-benchmark-measurement-efficiency.mddocs/bench/ast-analysis-paired-runner.mddocs/bench/ast-analysis-phase-benchmark.mddocs/bench/ast-analysis-phase-p0-reference.jsondocs/bench/ast-analysis-rejected-candidate-findings.mdpackage.jsonpackages/core/src/parsers/SqlPrintTokenParser.tspackages/core/src/transformers/SqlPrinter.tspackages/core/tests/transformers/CommentStyle.comprehensive.test.tspackages/core/tests/transformers/SqlFormatter.parameter-collection-fast-path.test.tsscripts/ast-analysis-paired-runner.test.ts
1 participant
Summary
rawsql-tspatch changesets for the adopted R1 and R2 improvements.Verification
pnpm --filter rawsql-ts test— 242 files / 2,467 tests passed / 1 skipped for R2.pnpm --filter rawsql-ts buildandpnpm --filter rawsql-ts lint— passed.pnpm --filter rawsql-ts benchmarkandpnpm demo:complex-sql— passed for formatter behavior changes.pnpm test:ast-paired-runner— 80 tests passed, including independently named malformed Stage 1 screen-evidence cases that reject before any benchmark spawn and leave the candidate record unchanged.pnpm changeset status— tworawsql-tspatch changesets.git diff --check origin/main...HEAD— passed.Merge Readiness
Tracking issue: not required; performance maintenance work has no linked issue.
Scoped checks run: core test/build/lint, formatter benchmark, complex SQL demo, paired-runner tests, paired Stage 1–3 semantic comparisons, changeset status, and diff check.
Why full baseline is not required: full workspace typecheck/test/build/lint ran through commit hooks; no baseline exception is requested.
Self Review
Self-review workflow: two-cycle self-review plus parent acceptance review for each candidate.
Self-review result: no merge blocker remains for R1/R2; R3/R4/R5 adverse or inconclusive evidence is retained and excluded from adopted product changes.
Concept-review workflow: packages/core DBMS-neutral analyzer and formatter boundary review.
Concept-review result: no public API, SQL grammar, driver, QuerySpec, ZTD, CLI, or scaffold boundary violation found.
CLI Surface Migration
No-migration rationale: no CLI-facing file or user command contract changed.
Upgrade note: none; this is a backward-compatible internal formatter/analyzer performance maintenance change.
Deprecation/removal plan or issue: none; no public API or CLI surface is removed.
Docs/help/examples updated: benchmark protocol documentation and rejected-candidate findings were added; CLI help and examples are unaffected.
Release/changeset wording: two rawsql-ts patch changesets describe reduced rendering traversal and redundant parameter-collection work without a universal speed claim.
Scaffold Contract Proof
No-proof rationale: no scaffold source, template, or generated artifact behavior changed.
Non-edit assertion: scaffold-related paths are absent from the PR diff.
Fail-fast input-contract proof: not applicable because no scaffold input contract changed.
Generated-output viability proof: not applicable because no scaffold output is generated by this PR.
Summary by CodeRabbit
Performance
Bug Fixes
Documentation