Skip to content

fix(skills): open PDFs on Windows by converting MSYS paths to native - #70315

Open
valicen-davidsaunders wants to merge 6 commits into
NousResearch:mainfrom
Valicen:fix/pdf-windows-paths
Open

fix(skills): open PDFs on Windows by converting MSYS paths to native#70315
valicen-davidsaunders wants to merge 6 commits into
NousResearch:mainfrom
Valicen:fix/pdf-windows-paths

Conversation

@valicen-davidsaunders

Copy link
Copy Markdown

What does this PR do?

On Windows, Hermes's shell reports paths in MSYS form (/c/Users/...), but the
pdf and ocr-and-documents skills passed those paths straight to native tools
(pdftotext, pymupdf, pdfplumber, Python's open()), which cannot open
/c/... and fail with I/O Error: Couldn't open file / FileNotFoundError.
The agent tends to misread this as a sandbox restriction and give up, asking the
user to copy-paste the PDF text by hand.

This PR fixes the guidance in both skills so PDF reads work on Windows with no
change in how the user prompts: convert MSYS paths to native C:\... before
handing them to a native tool, fall back to parsing read_file bytes via
io.BytesIO, and clarify which libraries to expect in the venv. Linux/macOS
behavior is unchanged.

Same MSYS-path class as #66524 (shell lint) and #67914 (rg search), applied here
to the PDF skills. Consistent with the terminal backend's existing
_msys_to_windows_path handling — surfaced as skill guidance rather than code.

Related Issue

N/A — no existing issue; documentation/skill-content fix.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • skills/productivity/pdf/SKILL.md — added a "Windows paths — critical" section
    (MSYS /c/... → native C:\..., read_file-bytes fallback); noted
    pypdf/pdfplumber/pdfminer.six/pymupdf are available; fixed the
    extract-text example to use a native path.
  • skills/productivity/ocr-and-documents/SKILL.md — added a "Hermes on Windows"
    section (same path rule + fallback), noted the expected libs and how to detect
    the wrong interpreter, fixed the inline pymupdf example path.

How to Test

  1. On a Windows Hermes venv, confirm the libraries import:
    venv\Scripts\python -c "import fitz, pymupdf, pdfplumber, pypdf, pdfminer; print('ok')"
  2. Extract a local PDF using a native path (not /c/...):
    venv\Scripts\python -c "import pymupdf; d=pymupdf.open(r'C:\Users\<you>\Downloads\some.pdf'); print(len(''.join(p.get_text() for p in d)))"
  3. Confirm a non-zero character count (verified: a 30-page PDF returned 11,791 chars).
    Passing /c/Users/... instead reproduces the original I/O Error.

Checklist

Code

Documentation & Housekeeping

  • I've updated relevant documentation — the change is the skill documentation
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) — Linux/macOS unchanged; adds Windows-only guidance
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

On a Windows venv (Py 3.11): import fitz/pymupdf/pdfplumber/pypdf/pdfminer all
succeed; a 30-page PDF extracted to 11,791 characters via a native C:/... path.
Passing the MSYS /c/... form reproduces I/O Error: Couldn't open file.

Hermes on Windows fed MSYS /c/... paths to native tools (pdftotext, pymupdf, pdfplumber), which can't open them. Document the conversion to native C:\ paths, the read_file-bytes fallback, and that pypdf/pdfplumber/pdfminer.six/pymupdf are preinstalled.
…bs note

Add 'Hermes on Windows' section: convert MSYS /c/... paths to native C:\ before pymupdf/pdftotext/Python; read_file-bytes fallback; note pymupdf/pypdf/pdfplumber/pdfminer.six are preinstalled in the venv.
@alt-glitch alt-glitch added type/docs Documentation improvements tool/skills Skills system (list, view, manage) platform/windows Native Windows-specific behavior or breakage P3 Low — cosmetic, nice to have sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 23, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing a real Windows/MSYS usability boundary. The local terminal backend deliberately disables MSYS argv conversion (tools/environments/local.py:1178-1198), so native-path guidance is directionally sound.

Problems

  • skills/productivity/ocr-and-documents/SKILL.md:34 proposes getting PDF bytes from read_file, but read_file returns line-numbered text (tools/file_tools.py:1110-1184) and PDF is not an extractable-document type (tools/read_extract.py:18-42). The io.BytesIO(data) fallback cannot run as written.
  • skills/productivity/ocr-and-documents/SKILL.md:42 and skills/productivity/pdf/SKILL.md:25 say pymupdf, pypdf, pdfplumber, pdfminer.six, pdftotext, and qpdf are preinstalled. The Python packages are not declared in pyproject.toml; current source guidance instead calls for installing the PDF packages (skills/productivity/pdf/SKILL.md:23-31, skills/productivity/ocr-and-documents/SKILL.md:60-64).

Suggested changes

  • Remove the invalid read_file bytes fallback and correct the dependency/tool availability claims; preserve the native-path guidance.

Automated hermes-sweeper review.

In Python, use a raw Windows path or forward-slash drive form; never `/c/...`:

```python
path = r"C:\Users\David\Downloads\Valicen brandbook.pdf" # or "C:/Users/David/Downloads/Valicen brandbook.pdf"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

read_file does not expose raw bytes to the agent: it returns line-numbered text, and PDF is not one of its structured extraction types (tools/file_tools.py:1110-1192, tools/read_extract.py:18-42). Please remove this io.BytesIO(data) fallback or replace it with a verified byte-producing path.

```python
import io, pypdf # pre-installed
data = <bytes from read_file>
text = "\n".join(p.extract_text() or "" for p in pypdf.PdfReader(io.BytesIO(data)).pages)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These packages are not declared by the Hermes installation (pyproject.toml has no pymupdf, pypdf, pdfplumber, or pdfminer dependency), and current skill guidance still instructs users to install them. Please avoid promising they are preinstalled unless a Windows installer path is added and verified.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 30, 2026
… bytes fallback, correct lib-availability claim

Per review on NousResearch#70315: read_file returns line-numbered text (not raw bytes) and PDF is not an extractable-document type, so the io.BytesIO fallback could not run — removed it. Replaced the 'preinstalled' claim with an install instruction (packages are not in upstream pyproject). Native-path guidance retained.
…stalled claim, drop read_file bytes fallback

Per review on NousResearch#70315: the PDF libs are not declared in upstream pyproject, so replaced the 'already installed' claim with an install instruction; removed the read_file/io.BytesIO fallback (read_file returns text, not bytes). Native-path guidance retained.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/skills Skills system (list, view, manage) type/docs Documentation improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants