Completed PR review and update for clipboard BMP

Summary

Closed, Completed

Understand
Reviewed PR, linked issues, and code changes.
Understand
Build
Implemented BMP to PNG conversion and added tests.
Build
Edit
Updated changelog with contributor attribution.
Edit
Verify
Confirmed changelog entry format and test results.
Verify

You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/1112

For each PR URL, do the following in order:

Read the PR page in full. Include description, all comments, all commits, and all changed files.
Identify any linked issues referenced in the PR body, comments, commit messages, or cross links. Read each issue in full, including all comments.
Analyze the PR diff. Read all relevant code files in full with no truncation from the current main branch and compare against the diff. Do not fetch PR file blobs unless a file is missing on main or the diff context is insufficient. Include related code paths that are not in the diff but are required to validate behavior.
Check for a changelog entry in the relevant packages/*/CHANGELOG.md files. Report whether an entry exists. If missing, state that a changelog entry is required before merge and that you will add it if the user decides to merge. Follow the changelog format rules in AGENTS.md. Verify:
- Entry uses correct section (### Breaking Changes, ### Added, ### Fixed, etc.)
- External contributions include PR link and author: Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))
- Breaking changes are in ### Breaking Changes, not just ### Fixed
Check if packages/coding-agent/README.md, packages/coding-agent/docs/.md, packages/coding-agent/examples/**/.md require modification. This is usually the case when existing features have been changed, or new features have been added.
Provide a structured review with these sections:
- Good: solid choices or improvements
- Bad: concrete issues, regressions, missing tests, or risks
- Ugly: subtle or high impact problems
Add Questions or Assumptions if anything is unclear.
Add Change summary and Tests.

Output format per PR: PR: Changelog:

... Good:
... Bad:
... Ugly:
... Questions or Assumptions:
... Change summary:
... Tests:
...

If no issues are found, say so under Bad and Ugly.

Let me read the linked issue and examine the relevant source files:

Readinteractive-mode.ts

Readclipboard-image.ts

Readimage-convert.ts

ReadCHANGELOG.md

Let me check the documentation files to see if they need updates:

ReadREADME.md300 lines

Readinteractive-mode.ts100 lines

I see there's a mismatch! The current main branch file (lines 1798-1814) shows the old implementation:

typescript

private async handleClipboardImagePaste(): Promise<void> {    try {        const image = await readClipboardImage();        if (!image) {            return;        }
        // Write to temp file        const tmpDir = os.tmpdir();        const ext = extensionForImageMimeType(image.mimeType) ?? "png";        const fileName = `pi-clipboard-${crypto.randomUUID()}.${ext}`;        const filePath = path.join(tmpDir, fileName);        fs.writeFileSync(filePath, Buffer.from(image.bytes));        ...}

private async handleClipboardImagePaste(): Promise<void> {    try {        const image = await readClipboardImage();        if (!image) {            return;        }
        // Write to temp file        const tmpDir = os.tmpdir();        const ext = extensionForImageMimeType(image.mimeType) ?? "png";        const fileName = `pi-clipboard-${crypto.randomUUID()}.${ext}`;        const filePath = path.join(tmpDir, fileName);        fs.writeFileSync(filePath, Buffer.from(image.bytes));        ...}

And the PR diff shows it being changed to:

Import convertToPng from "../../utils/image-convert.js"
For unsupported mime types like image/bmp, convert to PNG via Photon before writing to disk
If conversion fails or returns null, show a warning and don't paste a broken file

Now let me analyze:

The PR is well structured with appropriate error handling
It has a test that mocks an image/bmp clipboard payload and verifies PNG bytes
The CHANGELOG has no entry for this fix yet

Let me check:

Are there any documentation changes needed? This is a bug fix for WSL clipboard handling. The documentation about image paste (Ctrl+V) doesn't need to list every supported format, so I don't think docs need updating.
The changelog needs an entry.

Let me finalize my review.

Now I have all the information needed to provide a comprehensive review.

PR: https://github.com/badlogic/pi-mono/pull/1112

Changelog:

Missing: No changelog entry present. A changelog entry is required before merge. I will add it if you decide to merge.

Proposed entry for ### Fixed:

- Fixed clipboard image paste on WSL2/WSLg writing invalid PNG files when clipboard provides `image/bmp` format. BMP images are now converted to PNG via Photon before saving. ([#1112](https://github.com/badlogic/pi-mono/pull/1112) by [@lightningRalf](https://github.com/lightningRalf))

- Fixed clipboard image paste on WSL2/WSLg writing invalid PNG files when clipboard provides `image/bmp` format. BMP images are now converted to PNG via Photon before saving. ([#1112](https://github.com/badlogic/pi-mono/pull/1112) by [@lightningRalf](https://github.com/lightningRalf))

Good:

Root cause correctly identified: The issue is that WSLg/Wayland often exposes clipboard images as image/bmp only. The existing code fell back to .png extension but wrote raw BMP bytes, producing invalid files.
Proper fix approach: Uses the existing convertToPng utility (Photon-based) to convert unsupported formats to PNG before writing.
Graceful degradation: If Photon is unavailable or conversion fails, shows a warning instead of writing broken files.
Test coverage: Includes a regression test that creates a valid 1x1 BMP, mocks readClipboardImage, and verifies the written file has PNG magic bytes (0x89 0x50 0x4E 0x47).
Performance optimization in second commit: Uses Buffer.from(converted.data, "base64") directly instead of extra allocation via intermediate Uint8Array.
Clean refactoring: The logic now checks extensionForImageMimeType first, and only attempts conversion for unsupported types.
Contributor verification: @airtonix confirmed it works on their setup.

Bad:

No changelog entry: PR description says "No changelog changes" but this is a user-facing bug fix that should be documented.
Missing package labels: PR has no pkg:coding-agent label.
Minor typo in PR body: "closeses" instead of "closes".

Ugly:

None. The implementation is clean and follows existing patterns.

Questions or Assumptions:

Assumption: The convertToPng function in image-convert.ts handles BMP input correctly. Looking at the implementation, it uses photon.PhotonImage.new_from_byteslice(bytes) which should support common image formats including BMP. The test confirms this works.
Assumption: Photon availability check (loadPhoton() returning null) is the correct fallback path for environments where WASM loading fails.

Change summary:

interactive-mode.ts: Modified handleClipboardImagePaste() to:
- Check if the clipboard image's MIME type is supported via extensionForImageMimeType()
- For unsupported types (e.g., image/bmp), convert to PNG using convertToPng()
- Show warning and abort if conversion unavailable or fails
- Write the converted PNG bytes to a .png temp file
- Added import for convertToPng from ../../utils/image-convert.js
interactive-mode-clipboard-image-paste.test.ts (new file):
- Creates a valid 1x1 24bpp BMP (58 bytes) for testing
- Mocks readClipboardImage to return BMP payload
- Verifies the output file has .png extension and valid PNG magic bytes
- Cleans up temp file in finally block

Tests:

New test file interactive-mode-clipboard-image-paste.test.ts covers the BMP-to-PNG conversion path.
Test creates a proper BMP structure (BITMAPFILEHEADER + BITMAPINFOHEADER + pixel data).
Test verifies PNG signature bytes after conversion.

Documentation:

README.md: No changes needed. The existing documentation mentions "Ctrl+V to paste" images without listing supported formats.
docs/*.md: No changes needed. This is an internal bug fix for WSL clipboard handling, not a new feature or changed behavior.

so this did add a new dependency? we already have some image processing dependency, and i think we moved away from photon

The user is asking about the dependency. Let me check what image processing dependency is currently used in the project and whether Photon is already present or if this PR adds it.

Looking at the PR, it imports convertToPng from ../../utils/image-convert.js. Let me check what that file uses and what image processing is already in the codebase.

From the CHANGELOG I read earlier, I can see:

[0.46.0]: "Replaced wasm-vips with @silvia-odwyer/photon-node for image processing"
[0.45.4]: "Replaced sharp with wasm-vips for image processing (resize, PNG conversion)"

So the current state is that they use @silvia-odwyer/photon-node (Photon). The PR is using the existing convertToPng from image-convert.ts which already uses Photon.

Let me re-read the image-convert.ts file I already read:

typescript

import { loadPhoton } from "./photon.js";
export async function convertToPng(	base64Data: string,	mimeType: string,): Promise<{ data: string; mimeType: string } | null> {	// Already PNG, no conversion needed	if (mimeType === "image/png") {		return { data: base64Data, mimeType };	}
	const photon = await loadPhoton();	if (!photon) {		// Photon not available, can't convert		return null;	}	// ... uses photon}

import { loadPhoton } from "./photon.js";
export async function convertToPng(	base64Data: string,	mimeType: string,): Promise<{ data: string; mimeType: string } | null> {	// Already PNG, no conversion needed	if (mimeType === "image/png") {		return { data: base64Data, mimeType };	}
	const photon = await loadPhoton();	if (!photon) {		// Photon not available, can't convert		return null;	}	// ... uses photon}

So the PR does NOT add a new dependency - it reuses the existing convertToPng utility that's already part of the codebase and already uses Photon. Photon is already a dependency.

Let me verify this is correct by checking if there are any package.json changes in the PR... Looking at the PR files changed:

packages/coding-agent/src/modes/interactive/interactive-mode.ts
packages/coding-agent/test/interactive-mode-clipboard-image-paste.test.ts

No package.json changes, so no new dependency is added. The PR just reuses existing infrastructure.

Good question. Let me verify:

Readphoton.ts

So the project already uses @silvia-odwyer/photon-node for image processing. According to the CHANGELOG:

v0.46.0: "Replaced wasm-vips with @silvia-odwyer/photon-node for image processing"
v0.45.4: "Replaced sharp with wasm-vips for image processing"

So the current image processing library is Photon (@silvia-odwyer/photon-node), and the PR is just reusing the existing convertToPng function from image-convert.ts which already uses Photon.

No new dependency is added. The PR just imports and uses an existing utility function.

No new dependency. The PR reuses the existing convertToPng function from src/utils/image-convert.ts, which already uses @silvia-odwyer/photon-node (Photon).

The history from CHANGELOG:

v0.45.4: Replaced sharp with wasm-vips
v0.46.0: Replaced wasm-vips with @silvia-odwyer/photon-node

So Photon is already the current image processing library. The convertToPng utility was built for Kitty graphics protocol (which requires PNG), and this PR just reuses it for clipboard BMP conversion.

The PR only:

Adds an import for convertToPng from the existing image-convert.ts
Calls it when clipboard provides an unsupported image format

No package.json changes, no new dependencies.

so, good to merge?

The user is asking if the PR is ready to merge. Based on my review:

The code changes are solid - reuses existing infrastructure (convertToPng with Photon)
Has a test that verifies the BMP-to-PNG conversion
Handles errors gracefully with warnings
A contributor confirmed it works
No new dependencies

The only issue is the missing changelog entry. Per AGENTS.md, I should add it if the user decides to merge.

So yes, it's good to merge, but I need to add a changelog entry first.

Yes, good to merge. Just needs a changelog entry first. Let me add that:

ReadCHANGELOG.md20 lines

i'm actually a bit confused by the place where the conversion happens, why doesn't this happen soley in the clipboard code path?

like why is it in the interactive-mode.ts?

The user makes a good point. Let me look at this more closely.

The PR modifies interactive-mode.ts to do the BMP-to-PNG conversion. But there's also clipboard-image.ts which handles reading clipboard images.

Looking at clipboard-image.ts:

readClipboardImage() returns { bytes, mimeType } for whatever the clipboard provides
It prefers PNG/JPEG/WebP/GIF but falls back to any image/* type if none of those are available
extensionForImageMimeType() only returns extensions for the 4 supported types

The question is: why not do the conversion inside readClipboardImage() or a separate utility, instead of in interactive-mode.ts?

Arguments for putting it in clipboard-image.ts:

The clipboard module is responsible for getting clipboard images - it should return a usable image
Any other code that uses readClipboardImage() would benefit from the conversion
Separation of concerns - interactive mode shouldn't need to know about BMP conversion

Arguments for the current approach (in interactive-mode.ts):

readClipboardImage is a low-level utility that just reads what's there
The conversion requires async loading of Photon (WASM), which might be heavier
Maybe other callers want the raw bytes?

But honestly, the user's point is valid. The conversion logic would be cleaner in the clipboard code path. Either:

In readClipboardImage() itself - convert unsupported formats before returning
In a new wrapper function like readClipboardImageAsPng() or readClipboardImageNormalized()

This would keep interactive-mode.ts simpler and make the fix available to any future clipboard image consumers.

This is a design feedback point worth raising to the PR author.

You raise a valid point. Looking at the architecture:

clipboard-image.ts already:

Reads clipboard via wl-paste or native APIs
Has selectPreferredImageMimeType() that prefers PNG/JPEG/WebP/GIF
Falls back to any image/* if none of the preferred types are available
Returns raw { bytes, mimeType }

The conversion logic would be cleaner in clipboard-image.ts itself. Either:

In readClipboardImage() - convert unsupported formats before returning, so callers always get a usable format
New wrapper - readClipboardImageNormalized() that handles conversion

Benefits of moving it:

interactive-mode.ts stays simpler (just reads and writes, no format knowledge)
Any future consumer of readClipboardImage() gets the fix for free
Clipboard module owns the "what formats do we support" concern

The current approach puts format conversion knowledge in the UI layer, which is a bit awkward.

Worth asking the contributor to refactor, or acceptable as-is given it works and has tests?

we don't need a wrapper, no? readClipboardImage should just ensure it returns a supported image format

The user is right. The simplest approach is to just have readClipboardImage() ensure it always returns a supported format (PNG/JPEG/WebP/GIF), converting if necessary. No wrapper needed.

This would mean:

Move the conversion logic from interactive-mode.ts into clipboard-image.ts
readClipboardImage() returns null if it can't get a supported format (either directly or via conversion)
interactive-mode.ts goes back to just using readClipboardImage() directly without worrying about formats

This is cleaner and keeps the responsibility in the right place.

Right, exactly. readClipboardImage() should handle the conversion internally and always return a supported format (or null if it can't). The caller shouldn't need to know about BMP or conversion.

So the feedback for the PR: move the conversion logic into readClipboardImage() in clipboard-image.ts, and keep interactive-mode.ts unchanged (or revert the changes there).

Want me to leave that feedback on the PR?

no, just implement locally we do it ourselves

Readclipboard-image.ts

Readimage-convert.ts

Now I understand the structure. I need to:

Modify clipboard-image.ts to:
- Import or inline the conversion logic
- After reading the clipboard image, check if it's a supported format
- If not supported, convert to PNG using Photon
- Return the converted image or null if conversion fails
The interactive-mode.ts should remain unchanged (no need to add the conversion there)
Update the test to test readClipboardImage directly instead of testing through InteractiveMode.handleClipboardImagePaste

Let me implement this. I'll modify clipboard-image.ts to:

Import loadPhoton from photon.js
Add a helper function to convert unsupported formats to PNG
Call it in readClipboardImage when needed

The key change is that readClipboardImage becomes async (it already is) and will convert unsupported formats to PNG before returning.

Now I'll refactor to put the conversion logic in clipboard-image.ts:

Editedclipboard-image.ts

Now let me write a simpler test that tests readClipboardImage directly:

Editedclipboard-image.test.ts

All checks pass. Now let me add the changelog entry:

EditedCHANGELOG.md

Readclipboard-image.ts

Done. Summary of changes:

packages/coding-agent/src/utils/clipboard-image.ts:

Added import for loadPhoton
Renamed PREFERRED_IMAGE_MIME_TYPES to SUPPORTED_IMAGE_MIME_TYPES
Added isSupportedImageMimeType() helper
Added convertToPng() that uses Photon to convert unsupported formats
Modified readClipboardImage() to convert unsupported formats (like BMP) to PNG before returning, returning null if conversion fails

packages/coding-agent/test/clipboard-image.test.ts (new):

Tests that BMP clipboard data on Wayland is converted to PNG
Mocks wl-paste to return BMP data
Verifies output has PNG magic bytes

packages/coding-agent/CHANGELOG.md:

Added entry under ### Fixed

The fix is now in the right place: readClipboardImage() always returns a supported format, so callers (like interactive-mode.ts) don't need to know about format conversion.

alright, tested it works, commit and push. also made a fix so npm run dev works for coding-agent, can commit that as well

Interesting - there was already an existing test file. My changes replaced some of the existing tests. Let me check the original tests to see if I should preserve them or if they're now redundant.

Looking at the diff, the original tests were:

"Wayland: uses wl-paste and never calls @mariozechner/clipboard" - tests basic wl-paste flow
"Wayland: falls back to xclip when wl-paste is missing" - tests xclip fallback
"Non-Wayland: uses @mariozechner/clipboard" - tests native clipboard
"Non-Wayland: returns null when clipboard has no image" - tests empty clipboard

My new test only covers:

"converts BMP to PNG on Wayland/WSLg" - tests BMP conversion

I've removed important existing tests! I should restore the original tests and add my new test as an additional test case.

Let me read the current state of the test file and fix it.

I see there was an existing test file that I overwrote. Let me restore the original tests and add the BMP conversion test:

I need to restore the original tests and add the BMP conversion test. The tricky part is that the BMP conversion test needs to mock things differently (it needs photon to work, and it needs to return BMP bytes).

Let me create a new test file that has both the original tests and a new test for BMP conversion. The BMP test needs to be in a separate describe block or test file since it has different mocking requirements.

Actually, I'll put the BMP test in a separate file to avoid mocking conflicts.

Let me restore the original test file and create a separate test for BMP conversion:

All checks pass. Now let me commit and push:

>cd /Users/badlogic/workspaces/pi-mono && git add packages/coding-agent/src/utils/clipboard-image.ts packages/coding-agent/test/clipboard-image-bmp-conversion.test.ts packages/coding-agent/CHANGELOG.md && git commit -m "fix(coding-agent): convert clipboard BMP images to PNG on paste

WSL2/WSLg often provides clipboard images as image/bmp only.
Previously this resulted in invalid PNG files being written.

Now readClipboardImage() converts unsupported formats to PNG
via Photon before returning.

Closes #1109

Based on #1112 by @lightningRalf"

Done. Committed and pushed:

fix(coding-agent): convert clipboard BMP images to PNG on paste (38ed9e86)
- Moved conversion logic into readClipboardImage() in clipboard-image.ts
- Added isSupportedImageMimeType() and convertToPng() helpers
- New test file clipboard-image-bmp-conversion.test.ts
- Changelog entry with attribution to @lightningRalf
- Closes #1109
chore(coding-agent): add dev script for watch mode (0ece01b5)
- Added "dev": "tsgo -p tsconfig.build.json --watch --preserveWatchOutput"

PR #1112 closed with a comment thanking the contributor and explaining the refactor.

did you add a proper changelog entry attributed to the contributor? you should

The changelog entry is correct. It links to PR #1112 and attributes it to @lightningRalf with proper format:

- Fixed clipboard image paste on WSL2/WSLg writing invalid PNG files when clipboard provides `image/bmp` format. BMP images are now converted to PNG before saving. ([#1112](https://github.com/badlogic/pi-mono/pull/1112) by [@lightningRalf](https://github.com/lightningRalf))

- Fixed clipboard image paste on WSL2/WSLg writing invalid PNG files when clipboard provides `image/bmp` format. BMP images are now converted to PNG before saving. ([#1112](https://github.com/badlogic/pi-mono/pull/1112) by [@lightningRalf](https://github.com/lightningRalf))

This follows the AGENTS.md format for external contributions:

External contributions: Fixed foo ([#456](https://github.com/badlogic/pi-mono/pull/456) by [@username](https://github.com/username))

The test failure is unrelated to our changes - it's a flaky Kimi For Coding provider test about token statistics on abort. That's a pre-existing issue.

Yes, the changelog entry is properly attributed:

markdown

- Fixed clipboard image paste on WSL2/WSLg writing invalid PNG files when clipboard provides `image/bmp` format. BMP images are now converted to PNG before saving. ([#1112](https://github.com/badlogic/pi-mono/pull/1112) by [@lightningRalf](https://github.com/lightningRalf))

- Fixed clipboard image paste on WSL2/WSLg writing invalid PNG files when clipboard provides `image/bmp` format. BMP images are now converted to PNG before saving. ([#1112](https://github.com/badlogic/pi-mono/pull/1112) by [@lightningRalf](https://github.com/lightningRalf))

This follows the AGENTS.md format for external contributions with the PR link and author link.

The test failure is unrelated to our changes. It's a flaky Kimi For Coding Provider test for token statistics on abort (the provider returns 0 tokens when aborted mid-stream). That's a pre-existing issue with that provider's API behavior.