Skip to content

fix: tolerate non-utf8 filenames in file discovery - #12932

Open
dso2ng wants to merge 1 commit into
NousResearch:mainfrom
dso2ng:fix/non-utf8-file-discovery
Open

fix: tolerate non-utf8 filenames in file discovery#12932
dso2ng wants to merge 1 commit into
NousResearch:mainfrom
dso2ng:fix/non-utf8-file-discovery

Conversation

@dso2ng

@dso2ng dso2ng commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • make CLI project file discovery tolerate non-UTF-8 filenames during bare @ completion
  • apply the same filesystem-safe decoding to context reference folder discovery
  • add regression tests covering non-UTF-8 filenames in both code paths

Why

Some repositories contain filenames that are valid on the filesystem but not valid UTF-8. Hermes should not crash when scanning those trees for @ file completion or folder context references.

Root cause

subprocess.run(..., text=True) decoded rg --files output as strict UTF-8. If the working tree contained non-UTF-8 filenames, Hermes raised UnicodeDecodeError while building completions or folder listings.

Behavior before

  • bare @ completion could crash on mixed-encoding trees
  • folder context discovery could fail when rg --files returned non-UTF-8 names

Behavior after

  • file discovery subprocess calls use text=False
  • each output line is decoded with os.fsdecode(...), preserving filesystem semantics via surrogateescape
  • Hermes keeps normal behavior for UTF-8 paths while tolerating mixed-encoding trees without crashing

How to test

  • python -m pytest tests/hermes_cli/test_path_completion.py tests/agent/test_context_references.py -o 'addopts=' -q
  • In a repository containing at least one non-UTF-8 filename, verify that:
    • bare @ completion no longer crashes
    • folder/context reference expansion still works

Platforms tested

  • Linux (local Hermes development environment on omarchy)

Related issues / PRs

Notes

  • This PR is intentionally scoped to filesystem-safe decoding in file discovery and the corresponding regression coverage.

@dso2ng

dso2ng commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

Maintainer context: I did a repo search for related utf-8 / UnicodeDecodeError work before narrowing this PR.

Related but distinct items:

This PR is intentionally scoped to a different failure surface:

  • filesystem filename bytes returned from subprocess-based file listing (rg --files),
  • bare @ completion in the CLI,
  • and folder/context reference expansion.

So it is in the same bug family, but it is not trying to reopen the migration or Windows-stdio fixes.

@dso2ng

dso2ng commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

Maintainer note: this fork-based PR has focused local verification already completed:

  • python -m pytest tests/hermes_cli/test_path_completion.py tests/agent/test_context_references.py -o 'addopts=' -q

Current blocker appears to be GitHub Actions approval for fork workflows: the PR checks are showing action_required with zero-job runs, so CI has not actually started yet.

If a maintainer approves / runs the PR workflows in the GitHub UI, I can follow up on any real CI failures after that.

@dso2ng

dso2ng commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

@kshitijk4poor @teknium1 friendly ping on this bugfix as well: it is a small, focused fix for non-UTF-8 filename handling in CLI file discovery/context expansion, with regression coverage already in place.

Current blocker seems to be fork-workflow approval (action_required / 0s runs). If you approve/run the PR workflows in the GitHub UI, I can follow up on any actual CI failures once the checks really start.

@dso2ng
dso2ng force-pushed the fix/non-utf8-file-discovery branch from 0dec17f to a7df9f0 Compare April 21, 2026 08:34
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard labels Apr 22, 2026
@dso2ng

dso2ng commented May 14, 2026

Copy link
Copy Markdown
Contributor Author

Refresh update: I merged current origin/main into this branch and resolved the hermes_cli/commands.py conflict while preserving the byte-safe filesystem decoding path.

Updated head: ed5cdfd62

Focused local verification on the refreshed branch:

  • /home/dso2ng/.hermes/hermes-agent/venv/bin/python -m pytest tests/hermes_cli/test_path_completion.py tests/agent/test_context_references.py -o 'addopts=' -q -> 42 passed
  • /home/dso2ng/.hermes/hermes-agent/venv/bin/python -m py_compile hermes_cli/commands.py agent/context_references.py tests/hermes_cli/test_path_completion.py tests/agent/test_context_references.py
  • git diff --check

The PR is mergeable again locally / via GitHub metadata. The current GitHub Actions runs on the refreshed head still show action_required with zero jobs, so they appear to be waiting for maintainer approval to run fork workflows.

@dso2ng

dso2ng commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Refresh update: I merged current origin/main into this branch and resolved the tests/agent/test_context_references.py conflict while preserving both the non-UTF-8 filename regression coverage and the newer upstream binary-file context-reference test.

Updated head: 120d83689

Focused local verification on the refreshed head:

  • git diff --check -> passed
  • python -m py_compile agent/context_references.py hermes_cli/commands.py tests/agent/test_context_references.py tests/hermes_cli/test_path_completion.py -> passed
  • python -m pytest tests/hermes_cli/test_path_completion.py tests/agent/test_context_references.py -o 'addopts=' -q -> 43 passed, 1 warning
  • public diff non-ASCII guard -> non_ascii_added_lines=0

GitHub now reports the PR as mergeable again. The new workflow runs are still at the fork workflow approval gate (action_required with zero jobs), not a code/test failure.

@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 the focused filesystem-decoding fix. The context-reference premise is present on current main: agent/context_references.py:533-547 runs rg --files with text=True, and the proposed byte/os.fsdecode conversion is the right direction.

Problems

  • tests/hermes_cli/test_path_completion.py:187 does not prove the classic CLI fix. Current main uses errors="replace" at hermes_cli/commands.py:1662-1667, so an unusable @file:bad-�.txt completion still passes the startswith("@file:bad-") assertion. Assert that the returned filename suffix round-trips through os.fsencode to b"bad-\xff.txt".
  • The matching TUI path remains affected: ui-tui/src/hooks/useCompletion.ts:34-37 routes @ completion to complete.path, while tui_gateway/server.py:12361,12380 UTF-8 replacement-decodes git path output. Please include byte-safe decoding and regression coverage there as well.

Automated hermes-sweeper review.

completions = list(completer.get_completions(doc, event))
texts = [completion.text for completion in completions]
assert "@file:good.txt" in texts
assert any(text.startswith("@file:bad-") for text in texts)

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.

This only checks the prefix. Current main decodes rg output with errors="replace" (hermes_cli/commands.py:1662-1667), so it produces @file:bad-�.txt and this assertion still passes. Assert an os.fsencode() round-trip of the path suffix to prove surrogateescape preservation.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026
@dso2ng
dso2ng force-pushed the fix/non-utf8-file-discovery branch from 120d836 to dc44cef Compare July 13, 2026 02:32
@dso2ng

dso2ng commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — addressed on the current head dc44cef04.

  • The CLI completion regression now asserts the returned filename suffix round-trips with os.fsencode(...) == b"bad-\\xff.txt", so UTF-8 replacement decoding cannot satisfy the test.
  • The matching TUI path now uses byte-safe filesystem decoding for Git -z output, with regression coverage in tests/test_tui_gateway_server.py.

Focused verification on the updated head:

python -m pytest tests/hermes_cli/test_path_completion.py tests/agent/test_context_references.py tests/test_tui_gateway_server.py -o 'addopts=' -q
368 passed

Could you please re-review the updated head?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants