Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the cwd-prefix failure; the reported doubling is still present on current main at tools/file_tools.py:397.
Problems
- The new prefix check in
tools/file_tools.py:410-414silently rewrites an ambiguous relative path. A real subtree whose relative name begins with the workspace components is indistinguishable from a missing leading slash. The #67185 discussion instead proposes warning the model while preserving the path semantics. - Native Windows remains on the unmodified
ntpath.joinroute attools/file_tools.py:386-392; #67185 reports the same doubling there. The added test attests/tools/test_resolve_path.py:90covers only the POSIX path route. - This branch also includes
51cd2733f590, the same QQBot-only commit that is the full change in open #66976. The member comment here requested splitting it.
Suggested changes
- Split out
51cd2733f590. - Preserve ambiguous relative inputs and surface a doubled-path warning with the likely absolute target.
- Cover the intended behavior on POSIX, native Windows, and container resolution paths.
Automated hermes-sweeper review.
| base_rel = base.relative_to("/") | ||
| base_rel_str = str(base_rel) | ||
| input_str = p.as_posix() | ||
| if base_rel_str and ( |
There was a problem hiding this comment.
This condition cannot distinguish a missing leading slash from a legitimate relative subtree whose name repeats the workspace components. Rewriting it silently changes valid relative-path semantics; please emit a doubled-path warning instead, as proposed in the #67185 discussion.
|
|
||
| # Input mirrors tmp_path's components with the leading '/' missing. | ||
| cwd_shaped = str(tmp_path.relative_to("/")) | ||
| result = _resolve_path(f"{cwd_shaped}/notes/x.md") |
There was a problem hiding this comment.
This covers only the POSIX Path route. Native Windows still takes the unmodified ntpath.join branch in tools/file_tools.py:391; add coverage there before treating the bug class as fixed.
|
Reviewed against issue #67185 and current Core change ( base_rel_str = str(base.relative_to("/"))
if base_rel_str and (input_str == base_rel_str or input_str.startswith(base_rel_str + "/")):
stripped = input_str[len(base_rel_str):].lstrip("/")
p = Path(stripped) if stripped else Path(".")This lands the file at the intended location for the reported case, and is scoped to an exact base-echo (so it doesn't have the broad root-dir false-positive that #67220's second branch does). Reasonable as far as it goes. Two caveats: (a) it silently rewrites rather than warning — the issue explicitly prefers surfacing a warning (like Blocking concerns:
Recommendation: drop the Verdict: Core idea is workable but silent-rewrite + unrelated prompt_builder scope + non-functional tests block it as-is. |
06606c9 to
03d3fc7
Compare
03d3fc7 to
e53f87f
Compare
Summary
Fixes #67185 — a model that emits a relative path mirroring the working directory (an absolute path missing its leading '/') was silently written to a doubled location.
Root cause
tools/file_tools.py::_resolve_path_for_task joined any non-absolute input onto the base dir. When the input already begins with the base's own components (e.g. base
/home/user/dev, inputhome/user/dev/notes/x.md), the result was/home/user/dev/home/user/dev/notes/x.md. The write succeeded (parents created), so the failure was silent — the digest just never appeared where expected.Fix
In the Linux host branch, detect when the relative input starts with the base directory's own components and strip that duplicated prefix so the file lands at the intended location. The guard is wrapped in try/except (ValueError when the base is not under '/', e.g. Windows drive roots), so it is a no-op on platforms where it cannot apply.
Changes
Validation
Closes #67185.