Skip to content

fix(file-safety): return POSIX inner_path from classify_sandbox_mirro… - #63235

Closed
aturzone wants to merge 1 commit into
NousResearch:mainfrom
aturzone:fix/sandbox-mirror-inner-path-posix
Closed

fix(file-safety): return POSIX inner_path from classify_sandbox_mirro…#63235
aturzone wants to merge 1 commit into
NousResearch:mainfrom
aturzone:fix/sandbox-mirror-inner-path-posix

Conversation

@aturzone

@aturzone aturzone commented Jul 12, 2026

Copy link
Copy Markdown

…r_target on Windows

classify_sandbox_mirror_target built inner_path with str(Path(*parts)), which emits OS-native separators — "profiles\group1\SOUL.md" on Windows. inner_path is a logical HERMES_HOME-relative identifier surfaced to the model ("the authoritative file is likely <inner_path>"), not an on-disk path, and its classify_container_mirror_target sibling already builds it with .as_posix(). Emit it POSIX-style so the two mirror classifiers agree and the hint stays canonical on every platform. mirror_root stays native (str) — it names a real host dir for display, matching the sibling.

tests: the sandbox-mirror suite already encodes the POSIX inner_path contract but only ran green on POSIX CI (the project's CI is Linux-only); on native Windows five cases failed. Normalize the two mirror_root assertions the same way the container-mirror tests already do, and add a regression guard that inner_path never contains a native separator.

Assisted-by: Claude Code (Opus 4.8)

What does this PR do?

Related Issue

Fixes #

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

How to Test

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform:

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

For New Skills

  • This skill is broadly useful to most users (if bundled) — see Contributing Guide
  • SKILL.md follows the standard format (frontmatter, trigger conditions, steps, pitfalls)
  • No external dependencies that aren't already available (prefer stdlib, curl, existing Hermes tools)
  • I've tested the skill end-to-end: hermes --toolsets skills -q "Use the X skill to do Y"

Screenshots / Logs

…r_target on Windows

classify_sandbox_mirror_target built inner_path with str(Path(*parts)),
which emits OS-native separators — "profiles\group1\SOUL.md" on Windows.
inner_path is a logical HERMES_HOME-relative identifier surfaced to the
model ("the authoritative file is likely <inner_path>"), not an on-disk
path, and its classify_container_mirror_target sibling already builds it
with .as_posix(). Emit it POSIX-style so the two mirror classifiers agree
and the hint stays canonical on every platform. mirror_root stays native
(str) — it names a real host dir for display, matching the sibling.

tests: the sandbox-mirror suite already encodes the POSIX inner_path
contract but only ran green on POSIX CI (the project's CI is Linux-only);
on native Windows five cases failed. Normalize the two mirror_root
assertions the same way the container-mirror tests already do, and add a
regression guard that inner_path never contains a native separator.

Assisted-by: Claude Code (Opus 4.8)
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint 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 12, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused cross-platform fix. The premise holds on current main: classify_sandbox_mirror_target() builds the model-facing logical inner_path with str(Path(...)) at agent/file_safety.py:564, while its container-mirror sibling already uses inner.as_posix() at agent/file_safety.py:648. The warning exposes this value at agent/file_safety.py:595 through the file-tool guard (tools/file_tools.py:776).

The PR aligns those two classifiers and adds a direct separator regression assertion. GitHub reports the PR as mergeable against current main.

Automated hermes-sweeper review.

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

Ubuntu 26.04 on linux-5800x.

Live call of classify_sandbox_mirror_target on main already returns POSIX inner_path here (profiles/group1/memories/MEMORY.md, no backslashes), which is expected on Linux. The real footgun is Windows str(Path(...)).

PR side:

  • same live path still POSIX
  • tests/agent/test_file_safety_sandbox_mirror.py: 14 passed (0.08s), including test_inner_path_uses_posix_separators
  • aligns sandbox mirror hints with the container-mirror .as_posix() sibling

Looks good.

@aturzone aturzone closed this by deleting the head repository Aug 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants