Skip to content

Conversation

@dylan-conway
Copy link
Member

Summary

  • libarchive.zig:110: Fix self-assignment bug where this.pos was assigned to itself instead of new_pos
  • s3/credentials.zig:165,176,199: Fix impossible range checks - and should be or for pageSize, partSize, and retry validation (a value cannot be both less than MIN and greater than MAX simultaneously)
  • postgres.zig:14: Fix copy-paste error where createConnection function was internally named "createQuery"

Test plan

  • Verify S3 credential validation now properly rejects out-of-range values for pageSize, partSize, and retry
  • Verify libarchive seek operations work correctly
  • Verify postgres createConnection function has correct internal name

🤖 Generated with Claude Code

…indings

- libarchive.zig: Fix self-assignment bug where `this.pos` was assigned
  to itself instead of `new_pos`
- s3/credentials.zig: Fix impossible range checks - `and` should be `or`
  for pageSize, partSize, and retry validation (a value cannot be both
  less than MIN and greater than MAX simultaneously)
- postgres.zig: Fix copy-paste error where createConnection function
  was named "createQuery" internally

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@github-actions github-actions bot added the claude label Jan 8, 2026
@robobun
Copy link
Collaborator

robobun commented Jan 8, 2026

Updated 7:18 PM PT - Jan 8th, 2026

❌ Your commit ab448170 has 2 failures in Build #34338 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 25905

That installs a local version of the PR into your bun-25905 executable, so you can run:

bun-25905 --bun

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 8, 2026

Walkthrough

Archive buffer position update to use clamped target, S3 credential range checks switched from AND to OR to catch out-of-range values, PostgreSQL binding fixed to expose "createConnection" instead of "createQuery".

Changes

Cohort / File(s) Summary
Archive Buffer Management
src/libarchive/libarchive.zig
BufferReadStream.archive_skip_callback now updates this.pos with the clamped target new_pos rather than the old pos.
Credential Validation Logic
src/s3/credentials.zig
Replaced logical ANDs with ORs in range checks for pageSize, partSize, and retry, causing out-of-range values to throw range errors as intended.
PostgreSQL Binding API
src/sql/postgres.zig
Binding property "createConnection" now creates a JS function named "createConnection" (was "createQuery"), aligning the exposed function name with the property.
Tests (S3)
test/js/bun/s3/s3-storage-class.test.ts
Increased test writer partSize from 5 * 1024 to 5 * 1024 * 1024 (5MB minimum) in two locations.

Suggested reviewers

  • cirospaciari
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: fixing logic bugs across three files (libarchive, s3 credentials, and postgres bindings).
Description check ✅ Passed The description covers the required template sections with specific details about each bug fix, but the test plan items are left incomplete (unchecked checkboxes).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8ce07b0 and ab44817.

📒 Files selected for processing (1)
  • test/js/bun/s3/s3-storage-class.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.test.ts?(x): Never use bun test directly - always use bun bd test to run tests with debug build changes
For single-file tests, prefer -e flag over tempDir
For multi-file tests, prefer tempDir and Bun.spawn over single-file tests
Use normalizeBunSnapshot to normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
Use tempDir from harness to create temporary directories - do not use tmpdirSync or fs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met
Verify tests fail with USE_SYSTEM_BUN=1 bun test <file> and pass with bun bd test <file> - tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with .test.ts or .test.tsx
Avoid shell commands like find or grep in tests - use Bun's Glob and built-in tools instead

Files:

  • test/js/bun/s3/s3-storage-class.test.ts
test/**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

Always use port: 0 in tests - do not hardcode ports or use custom random port number functions

Files:

  • test/js/bun/s3/s3-storage-class.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun bd test <...test file> to run tests with compiled code changes. Do not use bun test as it will not include your changes.
Use bun:test for files ending in *.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use bun bd <file> instead of bun bd test <file> since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.

Files:

  • test/js/bun/s3/s3-storage-class.test.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
🔇 Additional comments (1)
test/js/bun/s3/s3-storage-class.test.ts (1)

246-246: Test correctly updated to comply with S3 minimum partSize of 5MB.

The change is valid. The partSize was increased from 5KB to 5MB (5,242,880 bytes), which matches the AWS S3 minimum requirement. With a 100MB test file and 5MB parts, this will generate approximately 20 parts as intended. Other S3 tests using custom partSize values (lines 621, 765 use mediumPayload.length = 6MB; line 504 uses parameterized values) all meet the minimum threshold and are unaffected.


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

Copy link
Member

@cirospaciari cirospaciari left a comment

Choose a reason for hiding this comment

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

we need to fix the tests to not use an invalid partSize but changes LGTM

The test was using partSize of 5KB but the minimum is 5MB. This was
exposed after fixing the range validation logic (and -> or).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@Jarred-Sumner Jarred-Sumner merged commit 596e83c into main Jan 9, 2026
53 of 55 checks passed
@Jarred-Sumner Jarred-Sumner deleted the claude/fix-logic-bugs branch January 9, 2026 03:46
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.

5 participants