Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found blocking issues.
[P2] Windows caseness still wrong
pi-transcript.ts:1537-1541 uses case-sensitive compare; C:\Users\Alice vs c:\users\ALICE\work should match on Windows (path.win32.relative === 'work') but returns full path. Tests don't cover this.
Also Standards 4 hard (template, AI disclosure, checks, before/after missing) and 2×P3 (POSIX backslash, HOME root).
Hosted test currently has no checks on this head — needs CI.
简体中文
Windows 路径大小写与模板缺失。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
riba2534
left a comment
riba2534
left a comment
There was a problem hiding this comment.
Pushed 1aed8f08 addressing the Windows caseness note:
- Win32/UNC/drive-letter paths now compare case-insensitively after separator normalization; POSIX stays case-sensitive.
c:\users\ALICE\workunderC:\Users\Alicenow yields~\work.- POSIX backslash in a name is no longer treated as a separator (
/Users/alice\evilstays unchanged). - Empty/root HOME already returned the full path; added regression rows only.
CI on this fork still needs first-time workflow approval.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found no new blocking code issues beyond process.
Fixes for 4f95 Windows caseness and POSIX backslash now closed (verified). Remaining:
- [P3]
HOME=/exact-home regression —shortenCwd('/', '/')now returns/instead of~(trim makes home empty). Fix: handle root case before trim. - Standards 4 hard — PR body still empty (template, AI disclosure, Verification, before/after missing).
- Hosted
teststill has no checks on this head — needs CI green.
简体中文
主要问题已修，剩余为小边界与流程材料。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found no blocking issues.
Windows home-relative shortening now case-insensitive with normalized separators while POSIX keeps backslash as filename char; coverage includes equal-length, UNC, drive mismatch, and traversal cases.
No P0-P3 on code; hosted checks currently have no runs on this head — needs CI green.
简体中文
该头无阻断，待 CI。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
1aed8f0 to
6a3f0e6
Compare
`shortenCwd` tested containment with `cwd.startsWith(home + '/')`. On win32 `os.homedir()` reports `C:\Users\<name>` and `process.cwd()` uses backslashes, so the test was false for every subdirectory of the profile and the statusline printed the full absolute path instead of `~\Videos\clip`. Exact-home still matched, which is why the feature looked partly alive. Decide containment on separator-normalized copies of both paths, and slice the tail out of the original `cwd` so it keeps the separators the platform actually produced. `path.relative` is avoided on purpose: it follows the host platform, so a Windows path would be misread on a POSIX runner. A `..` segment now falls back to the full path rather than emitting a `~/..` form that no longer names the same directory. Comparison stays case-sensitive, like the other path helpers in the tree. The helper is exported so the regression table can inject home and cwd as plain strings and assert the Windows cases without a Windows runner; a second test drives `renderMakaPiStatusLine` through the real `os.homedir()` to cover the wiring. Statusline layout and every other segment are untouched. Closes apache#3825 Generated-by: Cursor Cloud Agent Co-authored-by: riba2534 <riba2534@qq.com>
Review follow-up on apache#3825. Separator normalization alone was not enough: `shortenCwd` still compared case-sensitively, so `c:\users\ALICE\work` did not abbreviate under `C:\Users\Alice` even though Windows treats them as the same directory. Normalizing separators unconditionally was also wrong in the other direction — `\` is a legal POSIX filename character, so a sibling entry named `alice\evil` was read as `alice/evil` and `/Users/alice\evil` was abbreviated to `~\evil`, a path it never was. Split the two rulesets. A win32 path — `process.platform`, a `C:` drive root, or a `\\` UNC root — treats `\` and `/` as interchangeable and compares folded; a POSIX path compares exactly and leaves `\` alone. `path.relative` still isn't used: it follows the host platform, so the Windows cases would need a Windows runner to mean anything. The tail is still sliced out of the original `cwd`, so it keeps the separators and casing the platform produced, and the emitted form agrees with `path.win32.relative` on the win32 rows. Containment is now a fixed-offset prefix comparison rather than `startsWith(home + '/')`. Folding is applied only to the two equal-length slices being compared, never to the string the tail is cut from: `toLowerCase` can change a string's length, which would otherwise shift the cut. Inputs where folding does change the length (`İstanbul` under `Istanbul`) therefore fall back to the full path instead of producing a garbled one. The platform is injectable, following renderMakaPiPendingQueue, so every row in the regression table pins its own platform and asserts the same result on a POSIX and a Windows runner. Added coverage for win32 case folding, POSIX case sensitivity, backslash-in-a-name on POSIX, and empty, `/` and `\` homes, which must not collapse every absolute path to `~`. Refs apache#3825 Generated-by: Cursor Cloud Agent Co-authored-by: riba2534 <riba2534@qq.com>
6a3f0e6 to
eb79662
Compare
|
Closing this one — #4481 landed a fix for #3825 earlier today, so the Windows home-relative path is already abbreviated on Sorry for the wasted effort, and thanks for taking it on. If you pull |
3 participants
Fixes #3825. shortenCwd used home + slash, which never matches Windows subdirectories. Separator-normalized containment; tests in packages/cli/src/tests/pi-transcript.test.ts.