Skip to content

test/js/sql: drop duplicate pgParameterStatus export in wire-frames.ts - #35753

Open
robobun wants to merge 2 commits into
mainfrom
farm/3ba2fea1/dedup-pgParameterStatus
Open

robobun wants to merge 2 commits into
mainfrom
farm/3ba2fea1/dedup-pgParameterStatus

Conversation

@robobun

@robobun robobun commented Jul 25, 2026 •

Copy link
Copy Markdown
Collaborator

What

test/js/sql/wire-frames.ts exported pgParameterStatus twice with identical bodies (lines 92 and 126 on main). Bun's transpiler silently accepts the second declaration so no test failed, but tsc flags it as TS2393 Duplicate function implementation, and anyone editing just one copy would be surprised.

Cause

#35112 added pgParameterStatus to the shared wire-frames helper; #35114 then landed with its own copy of the same helper a few lines lower. Both merged and the file ended up with two.

Fix

Delete the second copy (the one between pgErrorResponse and pgNotificationResponse). The first copy stays next to pgAuthenticationOk / pgReadyForQuery where the other startup-sequence builders live.

Verification

bun bd test test/js/sql/postgres-datestyle.test.ts \
            test/js/sql/postgres-infinity-date.test.ts \
            test/js/sql/postgres-frame-boundary.test.ts
# 21 pass, 0 fail

All three importers of wire-frames.ts (including the two that call pgParameterStatus directly) still pass.

This is a test-helper cleanup with no src/ change, so there is no fail-before state to demonstrate: the duplicate was silently accepted at runtime and the fix is purely removing dead duplication.


no test proof · iteration 1 · docs-only change; test-proof not applicable

#35112 and #35114 each added pgParameterStatus to the shared wire-frames
helper; when both merged the file ended up with two identical
'export function pgParameterStatus' declarations. Bun's transpiler
silently takes the second one so nothing failed at runtime, but tsc
flags TS2393 and anyone editing only one copy would be confused.

Keep the first copy (next to the other startup-sequence builders),
drop the one between pgErrorResponse and pgNotificationResponse.
@coderabbitai

coderabbitai Bot commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 13 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ee128eab-ed6e-4c81-94df-4a42aab283d1

📥 Commits

Reviewing files that changed from the base of the PR and between df6c7ee and 822749e.

📒 Files selected for processing (1)
  • test/js/sql/wire-frames.ts

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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — pure dedup of a byte-identical test helper.

What was reviewed:

  • Confirmed the surviving pgParameterStatus (next to pgAuthenticationOk) is byte-identical to the deleted copy.
  • Checked both importers (postgres-datestyle.test.ts, postgres-frame-boundary.test.ts) — they import the name, not a position, so nothing changes for them.
  • No src/ changes; test-helper only.
Extended reasoning...

Overview

This PR deletes 5 lines from test/js/sql/wire-frames.ts: a second export function pgParameterStatus declaration that was byte-for-byte identical to the one ~30 lines above it. The duplicate arrived via two PRs (#35112, #35114) each adding the same helper. Bun's transpiler let the redeclaration through silently; tsc flags it as TS2393. The remaining copy sits with the other startup-sequence builders (pgAuthenticationOk, pgReadyForQuery), which is the more natural home.

Security risks

None. This touches only a test-fixture helper file that builds mock PostgreSQL wire frames for fault-injection tests. No runtime code, no user-facing surface, no auth/crypto/permissions.

Level of scrutiny

Minimal. The change is a mechanical deletion of dead duplication in test infrastructure. I verified the two bodies were identical (same signature, same pgRaw("S", Buffer.concat([pgCString(name), pgCString(value)])) body, same comment), so removing either one cannot change behavior. Function declarations are hoisted, so the surviving copy calling pgRaw (defined later in the file) is unaffected — and it already worked that way before this PR.

Other factors

Grepped for callers: only postgres-datestyle.test.ts and postgres-frame-boundary.test.ts import pgParameterStatus, matching the PR's verification run. No prior reviewer comments to address. This is exactly the "delete dead code in the same PR that makes it dead" pattern the repo review guide asks for, applied retroactively.

@robobun

robobun commented Jul 25, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 2:23 PM PT - Jul 25th, 2026

⏳ @robobun, your commit 822749e is still building in Build #81173, but has 1 failures so far (All Failures):

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — pure dedup of an identical test-helper export.

What was reviewed:

  • Confirmed the remaining pgParameterStatus at wire-frames.ts:92 has the same signature and body as the deleted copy.
  • Grepped importers: postgres-datestyle.test.ts and postgres-frame-boundary.test.ts both still resolve the export; no other call sites.
  • No src/ changes; test-only, zero behavioral impact.
Extended reasoning...

Overview

This PR deletes 5 lines from test/js/sql/wire-frames.ts: a second, byte-identical export function pgParameterStatus(name, value) that landed via two overlapping PRs (#35112 and #35114). The surviving copy at line 92 is unchanged and sits next to the other startup-sequence builders (pgAuthenticationOk, pgReadyForQuery).

Security risks

None. This is a test-only helper file for SQL wire-protocol fault-injection mocks; no runtime, auth, or crypto code is touched.

Level of scrutiny

Minimal. The change is a mechanical deletion of dead duplication in test infrastructure. Bun's transpiler silently accepted the duplicate function declaration (last one wins), so removing either copy is behavior-preserving; the two bodies were identical anyway. I verified via grep that the two importers of pgParameterStatus still resolve against the remaining export, and that no third call site exists.

Other factors

The PR description documents that all three importing test files (21 tests) still pass under bun bd test. No CODEOWNER paths, no outstanding reviewer comments, and the bug-hunting system found nothing. This is exactly the kind of trivial cleanup that does not need human review.

@robobun

robobun commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: both builds (#81069 and #81173) failed on Buildkite agent scheduling, not on this diff. In each build the *-build-cpp job sat in scheduled with started_at: null (no agent ever picked it up), so the dependent *-build-bun job timed out after an hour waiting on it and every downstream test lane went waiting_failed without running. Zero test jobs executed.

This diff is a 5-line deletion in test/js/sql/wire-frames.ts (a test helper); it is not part of any compiled artifact. Locally all three importers pass:

bun bd test test/js/sql/postgres-datestyle.test.ts \
            test/js/sql/postgres-infinity-date.test.ts \
            test/js/sql/postgres-frame-boundary.test.ts
# 21 pass, 0 fail

Ready to merge once the Buildkite queue recovers.

@robobun

robobun commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

The duplicate is still on main at 0d34bf2 (test/js/sql/wire-frames.ts:108 and :142). It came up again while resolving conflicts for #33740, which imports other helpers from this file.

This branch (822749e) still merges into main without a conflict. The merged file has one pgParameterStatus declaration. #35784 carries the same 5-line deletion as part of the parser change, so whichever of the two lands first fixes this. No second PR is needed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant