Skip to content

fix(file_tools): strip doubled base prefix for cwd-shaped relative paths (#67185) - #67318

Open
tusharui wants to merge 1 commit into
NousResearch:mainfrom
tusharui:fix/write-file-doubled-path
Open

tusharui wants to merge 1 commit into
NousResearch:mainfrom
tusharui:fix/write-file-doubled-path

Conversation

@tusharui

Copy link
Copy Markdown
Contributor

Problem

When a model emits a relative path that textually mirrors the working directory (e.g. home/user/dev/notes/file.md — an absolute path missing its leading /), _resolve_path_for_task silently creates a doubled path like /home/user/dev/home/user/dev/notes/file.md instead of failing or writing to the intended location.

Fix

Add _strip_doubled_base_prefix() to detect and strip the duplicated prefix using two strategies (common prefix + suffix-matching) that work on both POSIX and Windows. Integrated into _resolve_path_for_task for both path branches. Also adds a heuristic warning in _path_resolution_warning for bare-absolute paths whose first segment matches known root directories (home, Users, tmp, var, etc.).

Files changed

  • tools/file_tools.py — new helper + integration into resolve + heuristic warning (~60 LOC)
  • tests/tools/test_file_tools_doubled_path.py — new test file (17 tests)

Tests

  • _strip_doubled_base_prefix: full strip, partial strip, no-strip for normal/absolute/single-segment paths, POSIX path
  • _resolve_path_for_task: cwd-shaped path, deep nesting, normal relative, absolute, tilde
  • _path_resolution_warning: bare-absolute home/... and tmp/... warn, normal relative/absolute don't

Fixes #67185

…ths (NousResearch#67185)

When a model emits a relative path that textually mirrors the working
directory (e.g. home/user/dev/notes/file.md — an absolute path missing
its leading /), _resolve_path_for_task silently creates a doubled path
like /home/user/dev/home/user/dev/notes/file.md instead of failing.

Add _strip_doubled_base_prefix() to detect and strip the duplicated
prefix using two strategies (common prefix + suffix-matching) that work
on both POSIX and Windows. Integrate into _resolve_path_for_task for
both path branches.

Also add heuristic warning in _path_resolution_warning for bare-absolute
paths whose first segment matches known root directories (home, Users,
tmp, var, etc.).

Adds 17 tests covering full/partial strip, no-strip for normal paths,
integration with _resolve_path_for_task, and bare-absolute warnings.
@alt-glitch alt-glitch added type/bug Something isn't working tool/file File tools (read, write, patch, search) P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation 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 labels Jul 19, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related to #67185 and the open #67216/#67220/#67232 cluster. This branch auto-corrects paths, whereas the others use warning-only or different rewrite contracts; a maintainer choice is needed.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the focused reproduction and regression coverage. The underlying bug is present on current main: tools/file_tools.py:397-398 joins a non-absolute path to the task base, while :428-429 suppresses the existing warning for an in-workspace doubled target.

Problems

  • tools/file_tools.py:419-431 treats a two-segment suffix match as a duplicated base. With base /home/user/dev, legitimate relative user/dev/file.md is silently redirected to /home/user/dev/file.md; the PR's tests/tools/test_file_tools_doubled_path.py:43-47 codifies analogous partial stripping.
  • The container branch returns at tools/file_tools.py:456-457 before the new host-branch calls at :471 and :479, so it does not fix the same input for container backends.
  • tools/file_tools.py:512-520 reports that correction occurred for any home/tmp-style first segment even when _strip_doubled_base_prefix() returned None.

Suggested changes

  • Prefer a warning-only, full-root-replay detector rather than rewriting ambiguous relative paths; preserve valid workspace subtrees.
  • Cover container and explicit Windows resolution flows, plus suffix-only and root-named relative false positives.

Automated hermes-sweeper review.

Comment thread tools/file_tools.py
):
overlap = max(overlap, suffix_len)
break
if overlap < 2:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A two-segment overlap is not sufficient evidence of a duplicated cwd. With base /home/user/dev, legitimate relative user/dev/file.md matches the base suffix and is rewritten to /home/user/dev/file.md, silently dropping user/dev. Require a full normalized replay or retain the path and emit a warning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged - 2-segment suffix match is too ambiguous. I'm replacing this with a warning-only full-root-replay detector that only flags when the entire normalized base path is repeated in the resolved result, not partial suffix matches

Comment thread tools/file_tools.py
return resolved.resolve()
base = _resolve_base_dir(task_id, container_paths=False)
resolved = (base / p).resolve()
stripped = _strip_doubled_base_prefix(base.resolve(), resolved)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This integration only covers the POSIX host branch. The container branch above returns at line 457 before reaching this call, so the same cwd-shaped relative input remains doubled for Docker/container backends.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the fix only covers the POSIX host branch. I'll restructure so the detection runs after all three resolution paths (container, Windows, POSIX) instead of only the last one.

Comment thread tools/file_tools.py
return None
# Heuristic: detect bare-absolute paths (missing leading /).
first_seg = Path(filepath).parts[0] if Path(filepath).parts else ""
if first_seg in _BARE_ABSOLUTE_ROOT_SEGS:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This warning claims the doubled prefix was auto-stripped without checking whether stripping happened. A normal relative tmp/cache.txt under a base with no matching tmp prefix remains unchanged but receives the correction warning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the warning fires without checking if stripping actually happened. I'm removing this first-segment heuristic. The new approach only warns when the full-root replay detector matches, so no false positives for legitimate relative paths like tmp/cache.txt.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary

Eight PRs address #67185: #67216 and #67232 preserve ambiguous relative-path semantics and return a warning for an exact workspace-prefix replay, while the other six use path-rewrite variants with narrower coverage, broader matching, or warning-propagation gaps. The visible diffs show that #67216 and #67232 share the same production mechanism, with #67216 adding a warning-precedence regression test.

Related pull requests

Duplicates

#67216 and #67232 are substantive duplicates: their tools/file_tools.py production hunks are byte-identical according to the current-head comparison, while #67216 carries the additional test_doubled_warning_precedes_out_of_workspace_check regression. The other PRs overlap on #67185 but implement materially different rewrite contracts.

Suggested consolidation

Keep #67216 open with a salvage path: retain its focused warning-only production change and broad regression coverage, consistent with the recorded best-fix selection and the maintainer-bot keep_open verdict. Close #67232 as duplicate of #67216 despite its keep_open verdict because its production hunk is byte-identical and #67216 has the extra precedence test; keep #67218 closed as superseded, and request author action on #67220, #67318, #67421, #67426, and #67572 to rebase onto the warning-only contract or split out any independently salvageable tests, explicitly addressing their respective false-positive, backend-coverage, and warning-propagation blockers.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I67185(["issue #67185 (open)"])
    P67318["PR #67318 (open)"]
    P67318 -.->|partial| I67185
    class I67185 open
    class P67318 open
    class P67318 target
    click I67185 "https://github.com/NousResearch/hermes-agent/issues/67185"
    click P67318 "https://github.com/NousResearch/hermes-agent/pull/67318"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 8 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 74 kB of PR diffs, 20 kB of issue/PR text, 25 kB of discussion (36 comments), 9 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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

Labels

needs-decision Awaiting maintainer decision before any implementation 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 tool/file File tools (read, write, patch, search) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

write_file: cwd-shaped relative path (absolute path missing leading /) silently resolves to doubled path

4 participants