agentic-forges/forgejo-mcp

Watch

Fork 0

get_file_content returns base64-encoded content instead of plain text #114

New issue

Closed

opened 2026-05-05 08:53:14 +00:00 by BrilliantKahn · 4 comments

BrilliantKahn commented

2026-05-05 08:53:14 +00:00

(Migrated from codeberg.org)

Problem

get_file_content calls GetContents from the Forgejo SDK, which fetches file content via the /repos/{owner}/{repo}/contents/{filepath} endpoint. That API returns a ContentsResponse with the file content base64-encoded. The MCP tool forwards this as-is, so callers receive a JSON blob containing base64 rather than the file's plain text.

For LLM/MCP usage this is the wrong default — the natural form for reading a text file is plain text, not base64. The caller then has to decode it themselves, which is awkward in an agentic context.

This is the read-side counterpart to #72 (create_file / update_file base64 fix, merged in v2.18.0). Same SDK family, opposite direction.

Proposed fix

Switch GetFileContentFn from GetContents to GetFile (SDK method, hits /repos/{owner}/{repo}/raw/{filepath}), which returns []byte — raw bytes, no base64 layer.

To preserve backward compatibility for clients that depend on the current base64 response, add an optional boolean parameter as_binary (default false):

Behavior:

WHEN get_file_content is called with owner, repo, ref, filePath (no as_binary)
→ THEN returns plain-text content via GetFile
WHEN get_file_content is called with the same arguments and as_binary=true
→ THEN returns the base64-encoded ContentsResponse via GetContents (current behavior)

Breaking change note: the default behavior changes. Clients that currently parse the raw encoding: base64 JSON response without setting as_binary=true will receive plain text instead. It is not clear how widespread that dependency is. If this is unacceptable, the fallback design is as_text=true as an opt-in parameter (base64 remains the default), but that forces every new caller to know about the flag — which seems like the wrong default for an MCP tool used by LLMs.

I am happy to defer to your judgment on which approach fits best, including a potential major version bump if you prefer to treat this as a breaking API change.

Working implementation

A working implementation is available at BrilliantKahn/forgejo-mcp, branch feat/get-file-plain-text.

Changes are confined to operation/repo/file.go and operation/repo/file_test.go. The branch has been running in production on an internal MCP deployment since 2026-05-05. All tests pass (5 tests total, 3 new: TestGetFileContentFn_ReturnsPlainText, TestGetFileContentFn_AsBinaryReturnsContentsResponse, TestGetFileContentFn_DefaultIsPlainText).

This is a request for feedback on the approach — not a PR announcement. I want to confirm the parameter name, default choice, and breaking-change posture before opening a PR.

SDK choice rationale

GetFile over GetFileReader: GetFile returns []byte directly, converting cleanly to string for to.TextResult(). GetFileReader returns io.ReadCloser and requires io.ReadAll + Close, adding complexity without benefit for non-streaming file reads.

No binary-file detection is implemented: the caller is responsible for interpreting raw bytes when as_binary is omitted for binary files. This keeps the implementation simple and consistent with how other tools handle output format.

## Problem `get_file_content` calls `GetContents` from the Forgejo SDK, which fetches file content via the `/repos/{owner}/{repo}/contents/{filepath}` endpoint. That API returns a `ContentsResponse` with the file content **base64-encoded**. The MCP tool forwards this as-is, so callers receive a JSON blob containing base64 rather than the file's plain text. For LLM/MCP usage this is the wrong default — the natural form for reading a text file is plain text, not base64. The caller then has to decode it themselves, which is awkward in an agentic context. This is the **read-side counterpart** to #72 (`create_file` / `update_file` base64 fix, merged in v2.18.0). Same SDK family, opposite direction. ## Proposed fix Switch `GetFileContentFn` from `GetContents` to `GetFile` (SDK method, hits `/repos/{owner}/{repo}/raw/{filepath}`), which returns `[]byte` — raw bytes, no base64 layer. To preserve backward compatibility for clients that depend on the current base64 response, add an optional boolean parameter `as_binary` (default `false`): **Behavior:** - WHEN `get_file_content` is called with `owner`, `repo`, `ref`, `filePath` (no `as_binary`) → THEN returns plain-text content via `GetFile` - WHEN `get_file_content` is called with the same arguments and `as_binary=true` → THEN returns the base64-encoded `ContentsResponse` via `GetContents` (current behavior) **Breaking change note:** the default behavior changes. Clients that currently parse the raw `encoding: base64` JSON response without setting `as_binary=true` will receive plain text instead. It is not clear how widespread that dependency is. If this is unacceptable, the fallback design is `as_text=true` as an opt-in parameter (base64 remains the default), but that forces every new caller to know about the flag — which seems like the wrong default for an MCP tool used by LLMs. I am happy to defer to your judgment on which approach fits best, including a potential major version bump if you prefer to treat this as a breaking API change. ## Working implementation A working implementation is available at [`BrilliantKahn/forgejo-mcp`](https://codeberg.org/BrilliantKahn/forgejo-mcp/src/branch/feat/get-file-plain-text), branch `feat/get-file-plain-text`. Changes are confined to `operation/repo/file.go` and `operation/repo/file_test.go`. The branch has been running in production on an internal MCP deployment since 2026-05-05. All tests pass (5 tests total, 3 new: `TestGetFileContentFn_ReturnsPlainText`, `TestGetFileContentFn_AsBinaryReturnsContentsResponse`, `TestGetFileContentFn_DefaultIsPlainText`). This is a request for feedback on the approach — not a PR announcement. I want to confirm the parameter name, default choice, and breaking-change posture before opening a PR. ## SDK choice rationale `GetFile` over `GetFileReader`: `GetFile` returns `[]byte` directly, converting cleanly to `string` for `to.TextResult()`. `GetFileReader` returns `io.ReadCloser` and requires `io.ReadAll` + `Close`, adding complexity without benefit for non-streaming file reads. No binary-file detection is implemented: the caller is responsible for interpreting raw bytes when `as_binary` is omitted for binary files. This keeps the implementation simple and consistent with how other tools handle output format.

goern commented

2026-05-05 14:08:16 +00:00

(Migrated from codeberg.org)

Hey @BrilliantKahn, thanks for opening the issues. Would you like to open a PR too, so we can have others chime in?!

goern commented

2026-05-05 15:36:30 +00:00

(Migrated from codeberg.org)

Thanks for the writeup, and double thanks for running it in production before opening the issue — that's the kind of legwork that makes this easy to say yes to.

Default plain text is the right call. Mirrors the v2.18.0 fix on the write side (#72). For an MCP tool with an LLM on the other end, base64 is the wrong default — and as_text=true opt-in punishes every new caller for a legacy choice we'd rather not preserve.

One pushback on the parameter name: I'd not go with as_binary. The flag isn't really about binary files. GetContents returns sha, encoding, links — metadata a caller might want before a follow-up update_file, regardless of whether the file is binary. as_binary=true on a UTF-8 source file reads as a lie. I lean with_metadata, with raw=false as a fallback. Name should describe the response shape, not guess at file content. Open to your counter if you see a reason to keep it.

Treating this as a minor bump (v2.20.0), same posture as #72. Release notes will flag the default flip. No v3 over a single tool.

Skipping binary-file detection is the right move too — caller's responsibility, consistent with how the rest of the tools handle output.

Open the PR against main from your branch when you're ready. feat: commit, squash. Please update the tool description so the new param surfaces in the MCP schema.

Thanks for the writeup, and double thanks for running it in production before opening the issue — that's the kind of legwork that makes this easy to say yes to. Default plain text is the right call. Mirrors the v2.18.0 fix on the write side (#72). For an MCP tool with an LLM on the other end, base64 is the wrong default — and `as_text=true` opt-in punishes every new caller for a legacy choice we'd rather not preserve. One pushback on the parameter name: I'd not go with `as_binary`. The flag isn't really about binary files. `GetContents` returns sha, encoding, links — metadata a caller might want before a follow-up `update_file`, regardless of whether the file is binary. `as_binary=true` on a UTF-8 source file reads as a lie. I lean `with_metadata`, with `raw=false` as a fallback. Name should describe the response shape, not guess at file content. Open to your counter if you see a reason to keep it. Treating this as a minor bump (v2.20.0), same posture as #72. Release notes will flag the default flip. No v3 over a single tool. Skipping binary-file detection is the right move too — caller's responsibility, consistent with how the rest of the tools handle output. Open the PR against `main` from your branch when you're ready. `feat:` commit, squash. Please update the tool description so the new param surfaces in the MCP schema.

BrilliantKahn commented

2026-05-05 18:29:36 +00:00

(Migrated from codeberg.org)

Now a personal note :) I'm quite novice on developing and this was actually my first PR ever, not done more than forking before, worrying about breaking some conventions. Some developers can be harsh :)
No surprise I rely on AI for this, as you do as well I reckon. Thanks for your response anyways and your great work, really enjoying it! Take care.

Now a personal note :) I'm quite novice on developing and this was actually my first PR ever, not done more than forking before, worrying about breaking some conventions. Some developers can be harsh :) No surprise I rely on AI for this, as you do as well I reckon. Thanks for your response anyways and your great work, really enjoying it! Take care.

goern commented

2026-05-05 20:22:24 +00:00

(Migrated from codeberg.org)

Hey @BrilliantKahn, thanks for your first PR ever! Congrats on your first PR ever! I hope you are enjoying, as I want this to be an easy, not-at-all-harsh project!

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

agentic-forges/forgejo-mcp#114

No description provided.

Rows
Columns