Skip to content

fix: write gallery persona manifests as UTF-8 - #251

Open
Dhevenddra wants to merge 1 commit into
andrewyng:mainfrom
Dhevenddra:fix/gallery-install-utf8
Open

fix: write gallery persona manifests as UTF-8#251
Dhevenddra wants to merge 1 commit into
andrewyng:mainfrom
Dhevenddra:fix/gallery-install-utf8

Conversation

@Dhevenddra

Copy link
Copy Markdown

Problem

The gallery persona install path uses three encodings for one piece of text, and only two of
them agree.

# coworker/server/app.py
markdown = manifest.get("manifest_markdown", "")
digest = "sha256:" + hashlib.sha256(markdown.encode()).hexdigest()   # UTF-8
...
with tempfile.TemporaryDirectory() as td:
    (Path(td) / f"{slug}.md").write_text(markdown)                   # locale encoding
    summaries = reg.install_from_dir(td)
# coworker/personas/manifest.py:261, via registry.install_from_dir -> load_manifest_file
p.read_text(encoding="utf-8")                                        # UTF-8

Path.write_text() with no encoding falls back to locale.getpreferredencoding(). On Linux
and macOS that is UTF-8, so all three agree and CI passes. On Windows it is cp1252.

Impact

Two different failures, depending on which characters the gallery copy contains.

Characters cp1252 cannot represent (CJK, emoji) raise UnicodeEncodeError during the write. The
blanket except Exception a few lines below turns it into a response the user sees as:

{"ok": false, "error": "'charmap' codec can't encode characters in position 39-40: character maps to <undefined>"}

Characters cp1252 can represent but UTF-8 encodes differently are worse, because the write
succeeds. An em-dash becomes the single byte 0x97, a right curly quote becomes 0x92. The file
lands on disk looking fine, and then load_manifest_file() fails reading it back:

UnicodeDecodeError: 'utf-8' codec can't decode byte 0x92 in position 17: invalid start byte

Persona marketing copy almost always contains em-dashes and curly quotes, so in practice gallery
install is broken on Windows for anything but a pure-ASCII manifest.

registry._snapshot() copies the manifest with shutil.copy2, a byte copy, so bad bytes also
propagate into the installed snapshot under installed_dir/<id>/manifest.md.

Fix

Pass encoding="utf-8" on the write. I also made the .encode() above it explicit rather than
relying on its UTF-8 default, since the whole point is that these sites must visibly agree.

Tests

Added test_gallery_install_handles_non_ascii_manifest to tests/test_cloud_server.py, reusing
the existing _stub_gallery harness. The manifest carries an em-dash, curly quotes, CJK and an
emoji, and the test asserts name and tagline survive the round trip rather than just checking
the call did not raise. Because it reads those back through list_all(), it also covers the
shutil.copy2 snapshot, not only the initial write.

Before the fix, on Windows:

AssertionError: {'ok': False, 'error': "'charmap' codec can't encode characters in position 39-40: character maps to <undefined>"}

Verification

Linux, Python 3.12, matching CI, run in Docker: 890 passed, 1 skipped, 0 failed (full suite).

Windows 11, Python 3.11.9: the new test goes from failing to passing. Full suite moves from
9 failed, 880 passed to 8 failed, 882 passed.

To be precise about that delta, since it is +2 rather than +1: one is the new test. The other is
test_ui_refresh_e2e, which is flaky on Windows and happened to pass on this run. This change did
not fix it, and #210 already addresses that flakiness.

The 8 remaining Windows failures are all pre-existing and unrelated to this change: four in
test_slack_relay plus one in test_github_installs (deterministic 2s timeouts),
test_workspace_command_trust_controls_live_engine (WinError 32 on a directory rename), and the
two user-only file permission tests that #250 covers.

Note

Found by running the suite on Windows. CI is ubuntu-latest only, where the locale is UTF-8, so
this class of defect cannot currently surface upstream.

The gallery install path hashes the manifest markdown as UTF-8, and
personas/manifest.py reads it back as UTF-8, but the temp file written
between them used Path.write_text() with no encoding. That falls back to the
locale encoding. On Linux and macOS the locale is UTF-8 and all three agree.
On Windows it is cp1252, and gallery copy routinely contains characters
cp1252 cannot represent.

Two distinct failures follow:

  * CJK and emoji raise UnicodeEncodeError during the write, so the install
    returns {"ok": false, "error": "'charmap' codec can't encode characters
    in position ..."}.
  * An em-dash or a curly quote encodes fine as a single cp1252 byte, so the
    write succeeds and load_manifest_file() then fails reading it back as
    UTF-8. Persona copy almost always contains these.

Pass encoding="utf-8" on the write, and make the .encode() above it explicit
so all three sites visibly agree on one encoding.

@rajpratham1 rajpratham1 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.

This pull request improves cross-platform reliability for gallery persona installation by explicitly using UTF-8 when hashing and writing temporary manifest files. This keeps the encoding consistent with the manifest loader and avoids failures on systems whose default locale encoding is not UTF-8, particularly for manifests containing CJK characters, emoji, curly quotes, or em dashes. The added regression test verifies successful installation of a manifest containing diverse Unicode characters, while the existing hash mismatch test continues to validate integrity checking. Based on the visible changes, the implementation is focused, addresses the reported encoding issue cleanly, and no blocking issues are apparent.

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.

2 participants