Skip to content

fix(*): find a homebrew libcairo and refuse an svg the ppt engine cannot draw - #719

Merged
ZuyiZhou merged 2 commits into
mainfrom
fix/ppt_cairo_discovery
Sep 23, 2026
Merged

ZuyiZhou merged 2 commits into
mainfrom
fix/ppt_cairo_discovery

Conversation

@ZuyiZhou

@ZuyiZhou ZuyiZhou commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

On an Apple silicon Mac with Homebrew, ctypes.util.find_library('cairo') returns None and import cairosvg raises OSError; it only worked when the process was started with DYLD_FALLBACK_LIBRARY_PATH=/opt/homebrew/lib. ppt_fetch swallowed that failure in _rasterised_svg, so a fetched SVG logo was saved as a .md full of path data, with no figure_id and no log line. raven doctor did not mention cairo at all. The design engine draws SVG through Chromium and is not affected.

Head: b84764f, on top of main at 8ee5ae0.

  • Finding libcairo. New raven/utils/cairo.py. libcairo_path() tries the default lookup and then /opt/homebrew/lib, /usr/local/lib and /opt/local/lib. The libcairo_reachable() context manager adds that directory to DYLD_FALLBACK_LIBRARY_PATH only while the import runs, holding a lock, and puts back any value the user had set. This works because find_library on macOS reads the variable from os.environ at call time and then hands cairocffi an absolute path. Loading the dylib by absolute path beforehand does not work (tried and measured): dyld does not match a bare leaf name against an image that is already loaded. The variable is set only around the import, so later subprocesses do not inherit it. The cairosvg import itself lives in the ppt engine (_load_cairosvg), because deptry refuses a core import of a plugin-only dependency.
  • Clear failure instead of silent downgrade. A download whose root element is <svg> (after any XML declaration, comment or doctype) now either becomes a PNG or is refused. If cairo cannot load, the fetch fails with the first line of cairocffi's error and the install command. If the SVG will not draw, the fetch fails with the parser's error. Both cases log a warning and write nothing to the deck's sources. An HTML page with inline <svg> icons stays an .html document and no longer goes to the rasteriser; before, any text with <svg in its first 4 KB did.
  • Doctor. External tools gains a Cairo: row that shows the library path, or not found with what that costs and the install command. Like the LibreOffice row, it does not change the exit code. --json carries external_tools.cairo.

Behaviour change: an SVG that cannot be drawn was kept as a text document and is now refused. This is on purpose; path data was never usable as a source.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

Head b84764f, rebased onto origin/main 8ee5ae0; the rebase replayed both commits unchanged (git range-diff shows = for each) and the focused command below reran at this head. The full-suite line is from 5f4051e on afdc04f. The second commit answers the two review findings: _SVG_ROOT allows a UTF-8 byte-order mark before the root element (_sniff(b"\xef\xbb\xbf<svg ...") returned a document before, and now returns a PNG, or a refusal without cairo), and the two LibreOffice doctor tests read the LibreOffice row alone, so a runner without libcairo no longer fails the healthy case on the Cairo row's "not found".

uv run --frozen --python 3.12 --all-extras pytest -q tests/test_ppt_engine_fetch_tool.py tests/test_cli_doctor_commands.py tests/test_utils_cairo.py
  142 passed
COLORTERM=truecolor uv run --frozen --python 3.12 --all-extras pytest -q -p no:cacheprovider
  25916 passed, 108 skipped, 1 failed: tests/test_rpc_files.py::test_a_host_without_libreoffice_says_so
  (local only and pre-existing: it fails the same way on origin/main afdc04f89 on this machine, which has
  LibreOffice installed; this branch does not touch rpc/files)

Revert-to-red for the second commit: without \ufeff? the new BOM test fails; with libcairo_path forced to None across the doctor file (the CI shape), the old LibreOffice assertion fails and the new one passes.

The numbers below are from the first head and are kept for the record.

uv run --frozen --all-extras pytest -q tests/test_utils_cairo.py tests/test_cli_doctor_commands.py tests/test_ppt_engine_fetch_tool.py
  141 passed

test_a_brand_mark_arrives_as_svg_and_is_kept_as_a_picture used to skip on this machine (no DYLD variable set). It now runs and passes. test_cairosvg_imports_with_no_dyld_variable_where_brew_installed_cairo repeats the reported failure in a fresh interpreter with every DYLD_* variable removed.

Revert-to-red: I restored the parent's fetch.py and doctor_commands.py, kept the new tests, and 6 of them failed (both doctor cairo tests, the refused-SVG tests, the declaration-prefixed SVG test, and the end-to-end no-file-written test).

COLORTERM=truecolor uv run --frozen --python 3.12 --all-extras pytest -q
  25872 passed, 108 skipped   (before the rebase and the install-hint test)
make coverage
  25894 passed, 108 skipped   (at this head)
COVERAGE_BASE_REF=origin/main make coverage-diff
  Diff coverage: 100.00% (53/53 executable changed lines)
uv run --frozen --python 3.12 --extra dev ruff check raven evolver agents plugins-dist tests scripts
uv run --frozen --python 3.12 --extra dev ruff format --check raven evolver agents plugins-dist tests scripts
make lint-imports lint-deps lint-types
PYTHONPATH=. uv run --frozen --python 3.12 --extra dev python scripts/check_source_language.py origin/main...HEAD
make check-commits
COMMIT_RANGE=origin/main...HEAD make check-large-files
git diff --check origin/main...HEAD
  all clean

I have not run it on Linux or Windows locally. Off macOS libcairo_reachable() does nothing and libcairo_path() is plain find_library. The per-platform install hints are unit-tested.

  • Relevant tests pass locally
  • Relevant lint / type checks pass locally
  • User-facing docs or screenshots are updated when needed

Risk

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

DYLD_FALLBACK_LIBRARY_PATH affects the whole process, but it is only changed inside the import block, under a lock, and is restored afterwards. A fetched SVG that cannot be rasterised now returns ok: false instead of landing as a document. Roll back by reverting this commit.

Related Issues

N/A

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: handle valid BOM-prefixed SVGs and restore the focused doctor suite to green.

I reviewed git diff github/main...HEAD, the surrounding fetch/write and doctor-reporting callers, relevant file history, dependency placement, and the repository rules in AGENTS.md and CONTEXT-MAP.md. I also checked backward compatibility (including document-vs-image sniffing), looked for weakened tests, and checked the Runtime/plugin boundary; I found no additional issue there.

Verification: after uv sync --extra dev, uv run pytest tests/test_utils_cairo.py tests/test_cli_doctor_commands.py tests/test_ppt_engine_fetch_tool.py -q completed with 1 failed, 137 passed, and 3 skipped. The failure is called out inline. A direct _sniff probe also reproduced the BOM-prefixed SVG as ('.md', 'document').

Comment thread plugins-dist/ppt-engine/raven_ppt/tools/fetch.py Outdated
Comment thread raven/cli/doctor_commands.py
@ZuyiZhou
ZuyiZhou force-pushed the fix/ppt_cairo_discovery branch from a3029c4 to 5f4051e Compare September 23, 2026 11:51

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

The two prior blockers are fixed: BOM-prefixed SVGs now enter the SVG refusal/rasterization path, and the doctor tests inspect the LibreOffice row without suppressing the new Cairo result. Both threads are resolved.

For this revision I reviewed the full github/main...HEAD diff and the fix delta, surrounding fetch and doctor callers, relevant history, backward compatibility, test-strength changes, dependency placement, and the repository architecture and naming rules in AGENTS.md and CONTEXT-MAP.md. I found no additional issue.

Verification: uv run pytest tests/test_utils_cairo.py tests/test_cli_doctor_commands.py tests/test_ppt_engine_fetch_tool.py -q completed with 139 passed and 3 platform/dependency skips.

ZuyiZhou added a commit that referenced this pull request Sep 23, 2026
…al groups (#735)

## Summary

Two tests from #712 fail on `main` and on every PR rebased onto it
(#703, #714, #719 and #723 all fail the same `unit` shard). They fail
for two unrelated reasons, one of which is in the production code.

- **`_stop_child` spent its SIGTERM round on a group it had already
SIGKILLed** (`raven/cli/serve_commands.py`). When the gateway ignored
SIGTERM past `_CHILD_STOP_S`, the whole group was SIGKILLed, and then
the code still sent the group SIGTERM and polled `_group_gone` against
the original deadline, which by then had passed. A member the gateway
orphaned stays in the group as a zombie until init reaps it, so that
first probe found the group present, gave up at once, printed "the
gateway's own processes ignored SIGTERM; killing them" and sent a second
SIGKILL. On a CI runner the reap is slow enough to lose that race every
time, which is what
`test_a_stubborn_gateway_is_killed_with_its_group_in_one_step` saw. A
group killed here now gets the `_KILL_WAIT_S` wait and returns; the
SIGTERM round remains for a gateway that stopped on its own or was
already gone.
- **`TestStopping` could signal a real process group**
(`tests/test_cli_serve_commands.py`). Those tests use made-up pids 111
and 222 and fake `os.kill`, but `_force_kill` still called the real
`os.getpgid` and `os.killpg`. On the runner that failed `main`, one of
those pids was a live process leading its own group, so `killpg` raised
`PermissionError`, `_stop_resident` reported it as a warning and skipped
that process's kill wait: `45.05 >= 2 * 25.0` in
`test_both_processes_it_names_waited_the_time_it_reports`. Had the pid
belonged to the test's own user, the test would have SIGKILLed that
group. An autouse fixture on the class now fakes `getpgid` as "no such
process" and fails the test on any `killpg` the test did not arrange;
the tests that exercise groups already install their own fakes over it.

## Type

- [x] Fix
- [ ] Feature
- [ ] Docs
- [ ] CI / tooling
- [ ] Refactor
- [ ] Other

## Verification

Head f221828, on origin/main afdc04f.

```
COLORTERM=truecolor uv run --frozen --python 3.12 --all-extras pytest -q -p no:cacheprovider tests/test_cli_serve_commands.py
  122 passed
COLORTERM=truecolor uv run --frozen --python 3.12 --all-extras pytest -q -p no:cacheprovider
  25901 passed, 109 skipped, 1 failed: tests/test_rpc_files.py::test_a_host_without_libreoffice_says_so, which fails on origin/main on this machine too (LibreOffice is installed here; CI has none)
uv run --frozen --python 3.12 --extra dev ruff check raven tests / ruff format --check
make lint-imports lint-deps lint-types / git diff --check origin/main...HEAD
PYTHONPATH=. uv run --frozen --python 3.12 --extra dev python scripts/check_source_language.py origin/main...HEAD
make check-commits / COMMIT_RANGE=origin/main...HEAD make check-large-files
  all clean
```

The zombie cannot be produced on macOS (launchd reaps an orphan at
once), so the new test
`test_a_killed_group_still_being_reaped_is_waited_out_not_reported`
stands in for it: a real gateway that ignores SIGTERM, with the first
group probe after the SIGKILL reporting the group present. With
`raven/cli/serve_commands.py` restored to origin/main it fails (the "own
processes" line is printed); with this change it passes.

The fixture's case was reproduced by making the fake pid 111 behave as a
foreign group leader (`getpgid` returns the pid, `killpg` raises
`PermissionError`):
`test_both_processes_it_names_waited_the_time_it_reports` then fails
with `assert 45.1 >= 50.0`, the shape of the `main` failure. With the
fixture as committed it passes.

- [x] Relevant tests pass locally
- [x] Relevant lint / type checks pass locally
- [ ] User-facing docs or screenshots are updated when needed

No user-facing docs: the stop's interface and budgets are unchanged.

## Risk

User-visible: a stopping supervisor whose gateway had to be killed no
longer prints the second "own processes ignored SIGTERM" line or sends a
second SIGKILL; it waits up to `_KILL_WAIT_S` (5s) for the killed group
to be reaped, where it used to wait up to the same 5s after the second
SIGKILL. The stop's total budget is unchanged. The test fixture changes
no production behaviour. Rollback: revert the squash commit.

- [x] Security impact considered
- [x] Backward compatibility considered
- [x] Rollback path is clear for risky changes

## Related Issues

N/A

Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
@ZuyiZhou
ZuyiZhou force-pushed the fix/ppt_cairo_discovery branch from 5f4051e to 8195db5 Compare September 23, 2026 13:29

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

This revision is a patch-identical rebase of the previously clean two commits; git range-diff reports both commits unchanged. After refreshing github/main, I rechecked the full six-file PR diff, surrounding callers, relevant history, backward compatibility, test strength, dependency and architecture boundaries, and the applicable AGENTS.md and CONTEXT-MAP.md rules. No new issue was introduced, and both earlier threads remain resolved.

Verification: git diff --check github/main...HEAD passed, and uv run pytest tests/test_utils_cairo.py tests/test_cli_doctor_commands.py tests/test_ppt_engine_fetch_tool.py -q completed with 139 passed and 3 platform/dependency skips.

@ZuyiZhou
ZuyiZhou force-pushed the fix/ppt_cairo_discovery branch from 8195db5 to 80d652c Compare September 23, 2026 14:09

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

This revision is another patch-identical rebase of the two previously clean commits; git range-diff reports both unchanged. After refreshing github/main, I rechecked the full six-file diff, callers and history, backward compatibility, test strength, dependency and architecture boundaries, and the applicable repository rules. Both earlier threads remain resolved and I found no new issue.

Verification: git diff --check github/main...HEAD passed, and uv run pytest tests/test_utils_cairo.py tests/test_cli_doctor_commands.py tests/test_ppt_engine_fetch_tool.py -q completed with 139 passed and 3 platform/dependency skips.

ZuyiZhou and others added 2 commits September 23, 2026 22:35
…not draw

On an Apple silicon Mac cairocffi never looks in /opt/homebrew/lib, so
import cairosvg raised OSError unless the process was started with
DYLD_FALLBACK_LIBRARY_PATH set. ppt_fetch swallowed that and saved a
fetched SVG logo as a .md of path data: no figure id, no log line.

raven/utils/cairo.py finds libcairo under the Homebrew and MacPorts lib
directories and holds that directory in dyld's fallback path only for the
import. ppt_fetch now treats a document whose root element is svg as a
picture or a refusal: when cairo cannot load, or the SVG will not draw,
the fetch fails with the reason and the install command, and logs a
warning. An HTML page with inline svg icons stays a page. raven doctor
gains a Cairo row under External tools.

Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
…ows stay apart

A UTF-8 BOM before the root element failed the SVG root check, so a
BOM-prefixed SVG was saved as a document of path data: the silent
missing-logo outcome this change exists to close. The root pattern now
allows an optional BOM.

The two LibreOffice doctor tests searched the whole External tools section
for "not found", which the new Cairo row prints on a machine without
libcairo (the CI runners). They now read the LibreOffice row alone, so the
healthy case holds on such a machine and the missing case cannot pass on
Cairo's words.

Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
@ZuyiZhou
ZuyiZhou force-pushed the fix/ppt_cairo_discovery branch from 80d652c to b84764f Compare September 23, 2026 14:38

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

This revision is a patch-identical rebase of the two previously clean commits, now directly on the current target; git range-diff reports both unchanged. I rechecked the six-file diff, its callers and history, backward compatibility, test strength, dependency and architecture boundaries, and the applicable repository rules. Both earlier threads remain resolved and no new issue was introduced.

Verification: git diff --check github/main...HEAD passed, and uv run pytest tests/test_utils_cairo.py tests/test_cli_doctor_commands.py tests/test_ppt_engine_fetch_tool.py -q completed with 139 passed and 3 platform/dependency skips.

@ZuyiZhou
ZuyiZhou merged commit 72fe7e4 into main Sep 23, 2026
28 of 29 checks passed
@ZuyiZhou
ZuyiZhou deleted the fix/ppt_cairo_discovery branch September 23, 2026 14:54
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.

3 participants