Tabs and line breaks silently dropped from exec_command workdir and apply_patch paths (PathUri::join) #52688

Activity

1. sishuaigong

   added

   bugSomething isn't workingSomething isn't working

   on Oct 9, 2026
2. github-actions

   added

   CLIIssues related to the Codex CLIIssues related to the Codex CLI

   tool-callsIssues related to tool callingIssues related to tool calling

   on Oct 9, 2026
3. argszero commented on Oct 9, 2026

   

   argszero

   on Oct 9, 2026

   More actions

   Confirmed independently on current main (10da569252), and reproduced by running a test against
   codex-utils-path-uri rather than by reading:

   PathUri::from_str("file:///tmp/ws")?.join("config.txt\tx")
     => file:///tmp/ws/config.txtx      // tab silently gone; join_descendant inherits it
   PathUri::from_str("file:///tmp/ws")?.join("a\nb")   => file:///tmp/ws/ab    // LF gone
   PathUri::from_str("file:///tmp/ws")?.join("a\rb")   => file:///tmp/ws/ab    // CR gone
   PathUri::from_str("file:///tmp/ws")?.join("a\0b")   => Err(InvalidFileUriPath)
   

   Two things worth adding to your root cause:

   1. join is inconsistent with from_abs_path, which already gets this right.

   AbsolutePathBuf::from_absolute_path("/tmp/ws/config.txt\tx")
     -> PathUri::from_abs_path  =>  file:///tmp/ws/config.txt%09x
        and to_abs_path round-trips to the real "/tmp/ws/config.txt\tx"
   

   So the crate can already represent a control-character path — only the build path (join) cannot
   produce one. I'd treat that asymmetry, rather than "control characters are unsupported", as the bug
   to fix: the fix should make join agree with from_abs_path. NUL being rejected up front
   (InvalidFileUriPath) is the existing precedent if rejection is preferred over preservation.

   2. Pre-encoding the segment does not work. PathSegmentsMut::push percent-encodes % itself, so

   base.join("config.txt%09x")  =>  file:///tmp/ws/config.txt%2509x   // double-encoded
   

   There is no input string that makes push emit %09. A fix therefore has to bypass the WHATWG path
   setter for the affected components — percent-encode with the serializer's own encode set and
   Url::parse the result, or route through the same conversion from_abs_path uses.
   absolute_path_normalization.rs::path_uri_from_segments has the same segments.extend(...) pattern,
   so it is exposed the same way.

   I have a fix in progress along the "make join agree with from_abs_path" line and will open a PR
   against the fork with a regression test.

   👍React with 👍1Reacted by Sishuai Gong
4. argszero commented on Oct 9, 2026

   

   argszero

   on Oct 9, 2026

   More actions

   Implemented a fix along the "make join agree with from_abs_path" line and validated it against the
   current tree. Details below, including two things I could only settle by running code.

   Both construction routes are affected, not just join's relative branch.

   PathUri::parse("file:///workspace")?.join("config.txt\tx")        => file:///workspace/config.txtx
   PathUri::parse("file:///workspace")?.join("/tmp/config.txt\tx")   => file:///tmp/config.txtx
   

   The second line takes the absolute-path branch through
   absolute_path_normalization.rs::path_uri_from_segments, which has the same segments.extend(...)
   shape. Since exec_command resolves workdir via native_environment_cwd.join(workdir)
   (exec_command.rs:192) and apply_patch via cwd.join(...) (parser.rs:90), both the relative and
   absolute spellings hit the bug, so a fix has to cover both call sites.

   Only three characters are involved. url::PathSegmentsMut::push encodes everything else exactly
   as you would want; tab, LF and CR are the only ones the WHATWG path state drops. I verified that
   character-for-character against Url::from_file_path, which agrees with the setter for every
   character tested (space, %, ?, #, \, {, `, ", non-ASCII, and the other C0 controls
   0x01/0x07/0x1F, plus 0x7F) and differs only for the three drops, where it emits %09/%0A/
   %0D. That is what makes the fix narrow: encode those three, and leave every other character to the
   setter so the resulting spelling does not move.

   A %-based pre-encode cannot work, and neither can a post-hoc repair. push percent-encodes %
   itself, so an already-escaped segment is double-encoded, and repairing the escapes afterwards cannot
   distinguish a dropped tab from a literal %09 the user typed. The fix therefore writes the encoded
   segment to the path rather than pushing raw text.

   Fix (fork branch, codex-rs/utils/path-uri only):

   • new path_encoding.rs: encode_path_segment splits a segment into runs at tab/LF/CR, encodes each
     run through the setter itself (so the spelling is unchanged for every other character), and joins
     them with explicit %09/%0A/%0D;
   • lib.rs::join appends encode_path_segment(component) to the encoded path instead of pushing the
     raw component, keeping the existing ../pop_if_empty handling untouched;
   • absolute_path_normalization.rs::path_uri_from_segments builds the path from the same encoder.

   Literal % is unaffected: a%09b still spells a%2509b, so the existing
   encoded_filename_characters_round_trip_without_becoming_uri_metadata behavior is preserved. Note
   that join_native_bytes already encoded its segments before writing the path — only the UTF-8 route
   was losing them, which is why this reads as a gap rather than a design choice.

   Validation

   • cargo test -p codex-utils-path-uri — 86 passed (3 new regression tests: relative and absolute
     joins across POSIX/Windows conventions, the round trip back to the native path with the real tab,
     and the %09-stays-literal guard);
   • cargo test -p codex-apply-patch — 97 passed;
   • cargo clippy -p codex-utils-path-uri --all-targets and cargo fmt --check clean.

   The change is on a fork branch, since this repository does not take external pull requests:
   argszero#7