fix(tui): emit forward-slash mention labels on windows - #500
Conversation
Three tests fail on windows-latest on main:
ui::modern::mentions::tests::expand_inlines_a_single_file
ui::modern::mentions::tests::expand_inlines_multiple_files_once_each
ui::modern::app::tests::submit_inlines_mentions_but_transcript_keeps_the_typed_text
assertion failed: out.prompt.contains("<file path=\"src/main.rs\">")
This is a product bug, not a test bug. `display_path` labelled an inlined
file with whatever `to_string_lossy` produced, so a Windows session hands
the model `<file path="src\main.rs">` for a file the user mentioned as
`@src/main.rs`. The model quotes that label back in later tool calls, so
the shape it was shown disagrees with the shape it was asked about.
Join the relative path's components with `/` instead. Joining components
rather than substituting characters is what keeps this correct on Unix,
where a backslash is a legal filename character: it stays inside one
component and is never mistaken for a separator.
These tests had not run on Windows since the mentions work landed — an
unused-variable error in the same module failed the build before the test
binary existed, so the whole agent bin target was skipped.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 503c9bd65a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fs::write(dir.path().join("we\\ird.txt"), "hi\n").unwrap(); | ||
| let out = expand_mentions("see @we\\ird.txt", dir.path()).expect("expanded"); | ||
| assert!( | ||
| out.prompt.contains("we\\ird.txt"), |
There was a problem hiding this comment.
This assertion also matches the unchanged user-text prefix (see @we\ird.txt), so it passes even if display_path mangles the generated label to we/ird.txt. Consequently, the test does not protect the Unix filename behavior it claims to verify; assert on the complete <file path="we\ird.txt"> tag instead.
Useful? React with 👍 / 👎.
|
Superseded by #499, which merged the identical components-join fix for display_path (this branch now rebases to an empty diff). One thing worth salvaging from this version: the doc comment here explains why joining components beats character substitution (backslash is a legal filename character on unix) — happy to fold that wording into the merged comment in a docs touch-up if wanted. |
1 participant
Summary
Test (windows-latest)is currently red onmain, and a release PR (#496, v0.28.0) is open against it. Three tests fail:This is a product bug, not a test bug.
display_pathlabels an inlined file with whateverto_string_lossyreturns, so on Windows the model receivesfor a file the user mentioned as
@src/main.rs. The model quotes that label back in subsequent tool calls, so the path shape it was shown disagrees with the one it was asked about.Fix
Join the relative path's components with
/:Joining components rather than substituting characters is the point. A backslash is a legal filename character on Unix, so a blanket
replace('\\', "/")would mangle a real file namedwe\ird.txt. Component-joining never sees it as a separator, so one implementation is correct on both platforms with nocfggate.Why these tests only started failing now
They had not run on Windows at all since the mentions work landed. An
unused variable: direrror in the same module failed the build under-D warnings, which killed the wholeagentbin test target — so every test in that binary was skipped on Windows. #497 fixed the build error and surfaced these three (534 passed, 3 failed).Verification
file_labels_use_forward_slashes_on_every_platform— pins the label shape, runs on all platforms.a_backslash_inside_a_unix_filename_survives—#[cfg(unix)], creates a file literally namedwe\ird.txtand asserts the label is not rewritten. This is the case the naive fix gets wrong.mentionstests pass;clippy --all-targets -- -D warningsandfmt --checkclean.