Skip to content

fix: preserve binary file content as base64 - #650

Open
quseijuro-design wants to merge 2 commits into
zereight:mainfrom
quseijuro-design:fix/preserve-binary-file-content
Open

fix: preserve binary file content as base64#650
quseijuro-design wants to merge 2 commits into
zereight:mainfrom
quseijuro-design:fix/preserve-binary-file-content

Conversation

@quseijuro-design

Copy link
Copy Markdown

Summary

  • preserve the original Base64 payload and base64 encoding for non-UTF-8 repository files
  • keep the existing decoded text response and utf8 encoding for valid UTF-8 files
  • add MCP-level regression coverage for both binary and text content

Root cause

getFileContents unconditionally decoded every GitLab Base64 payload with Buffer.toString("utf8"). Invalid UTF-8 bytes were replaced with the Unicode replacement character, so binary content could not be reconstructed from the response.

The updated read path only returns decoded text when converting the bytes to UTF-8 and back is lossless. Otherwise, it leaves GitLab's original Base64 response unchanged.

Impact

Clients can now retrieve non-UTF-8 files without data corruption, while text-file behavior remains unchanged. This does not change the write path or public tool arguments.

Validation

  • npm.cmd run build
  • node --import tsx/esm --test --experimental-test-isolation=none --test-concurrency=1 test/test-get-file-contents.ts test/streamable-http-concurrent-session.test.ts (4 tests passed)
  • npx.cmd prettier --check test/test-get-file-contents.ts
  • npm.cmd run check:runtime-deps
  • npx.cmd tsx test/oauth-tests.ts

Closes #648

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ce625fc6-b80e-47ff-998e-5eeff45e99c0

📥 Commits

Reviewing files that changed from the base of the PR and between 130305d and bbd9402.

📒 Files selected for processing (2)
  • index.ts
  • test/test-get-file-contents.ts
📜 Recent review details
🔇 Additional comments (3)
index.ts (1)

2056-2063: LGTM!

test/test-get-file-contents.ts (2)

56-67: LGTM!


149-149: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Remove the duplicate file declaration.

The supplied code declares const file twice in the same block. TypeScript rejects the second declaration, so the test suite cannot compile.

Proposed fix
       const file = JSON.parse(responseText) as { content: string; encoding: string };
-      const file = JSON.parse(responseText) as { content: string; encoding: string };
			> Likely an incorrect or invalid review comment.

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved file content handling by decoding valid UTF-8 files as text while preserving binary files in their original Base64 format.
    • Binary file encoding metadata is now retained for more reliable content retrieval.
  • Tests

    • Added coverage for text and binary files, authentication, timeouts, cleanup, and server communication.

Walkthrough

getFileContents now preserves non-UTF-8 Base64 content and decodes only lossless UTF-8 content. End-to-end tests cover binary, UTF-8, and empty responses.

Changes

File Content Decoding

Layer / File(s) Summary
Lossless content decoding
index.ts
The response path checks whether decoded bytes round-trip through UTF-8. Binary content remains Base64-encoded with base64 metadata.
End-to-end response validation
test/test-get-file-contents.ts
Mock GitLab and MCP servers test binary preservation, UTF-8 decoding, empty content handling, authentication, cleanup, and timeout handling.

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

Mergeability Score: ⚪ Minimal · up to bbd94

This localized change preserves binary file content while retaining existing UTF-8 behavior; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: zereight, vrajpal-jhala, raskad

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The implementation preserves non-UTF-8 content as Base64 and retains UTF-8 decoding for valid text, matching issue #648.
Out of Scope Changes check ✅ Passed The code and regression tests directly support the linked issue objectives without unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly and concisely describes the main change: preserving binary file content as Base64.
Description check ✅ Passed The description directly explains the binary-content fix, UTF-8 behavior, regression tests, root cause, impact, and validation.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

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.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@index.ts`:
- Around line 2055-2063: Update the content guard in the parsedData decoding
block to verify that parsedData.content is a string rather than truthy, so empty
Base64 content is decoded and marked with encoding "utf8". Preserve the existing
lossless UTF-8 validation and assignment behavior.
🪄 Autofix

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: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: be39e191-b063-4586-8dcf-a9fd923c07b6

📥 Commits

Reviewing files that changed from the base of the PR and between 926d42c and 130305d.

📒 Files selected for processing (2)
  • index.ts
  • test/test-get-file-contents.ts
📜 Review details
🧰 Additional context used
🪛 Betterleaks (1.7.3)
test/test-get-file-contents.ts

[high] 15-15: Identified a GitLab Personal Access Token, risking unauthorized access to GitLab repositories and codebase exposure.

(gitlab-pat)

🔇 Additional comments (1)
test/test-get-file-contents.ts (1)

1-125: LGTM!

Comment thread index.ts
@quseijuro-design
quseijuro-design marked this pull request as ready for review August 13, 2026 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

get_file_contents corrupts binary files (PDF, images, archives): Base64 content is force-decoded to UTF-8

1 participant