Skip to content

Fix Windows shell and path handling#1966

Closed
ferologics wants to merge 2 commits intobadlogic:mainfrom
ferologics:fix/windows-tests
Closed

Fix Windows shell and path handling#1966
ferologics wants to merge 2 commits intobadlogic:mainfrom
ferologics:fix/windows-tests

Conversation

@ferologics
Copy link
Copy Markdown
Contributor

@ferologics ferologics commented Mar 8, 2026

Summary

  • route !command config values through pi's configured shell abstraction instead of the platform default shell
  • normalize Windows paths to slash-style for matching, completion, display, and tool output while keeping native paths for filesystem access
  • harden package-manager git commands against interactive credential prompts on Windows
  • update platform-assumptive tests and add Windows regressions for config-command execution and autocomplete path handling

Fixes #1775.
Also addresses #1769.
Related prior attempt: #1902.

Validation

  • ./test.sh
  • npm run check

Out of scope / follow-ups

  • I did not add a windows-latest CI job in this PR. The goal here was to get the Windows test suite green locally first; CI can be added in a follow-up once this behavior lands cleanly.
  • There is still room to consolidate some slash-path normalization helpers across packages, but I left that out to keep this PR focused on fixing Windows shell/path behavior and regressions.

@yhslai
Copy link
Copy Markdown

yhslai commented Mar 9, 2026

Hello, there are still some tests failed for me.

I'm at commit 948fde2 and running ./test.sh under Git bash.

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Tests 6 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 FAIL  test/tree-selector.test.ts > TreeSelectorComponent > branch navigation and folding with ctrl+arrow keys > ctrl+right unfolds a folded node, then does segment jump when unfolded
AssertionError: expected 'asst-4a' to be 'user-3a' // Object.is equality

Expected: "user-3a"
Received: "asst-4a"

 ❯ test/tree-selector.test.ts:367:45
    365|
    366|    selector.handleInput(CTRL_LEFT); // asst-4a → user-3a
    367|    expect(list.getSelectedNode()?.entry.id).toBe("user-3a");
       |                                             ^
    368|
    369|    selector.handleInput(CTRL_LEFT); // fold user-3a

Full log is attached:

test.log

@ferologics
Copy link
Copy Markdown
Contributor Author

Thanks — I tracked this down.

The remaining failures were not from the Windows shell/path fix itself, but from selector tests (tree-selector / related keybinding-driven tests) depending on the global editor keybindings singleton. If someone has custom local keybindings configured, those raw Ctrl/Alt arrow escape sequences can stop mapping to the expected actions and the tests fail locally.

I fixed that by resetting those tests to DEFAULT_EDITOR_KEYBINDINGS, so they no longer depend on the runner's local hotkey setup.

I also cleaned up the branch history and force-pushed it so this PR no longer includes the unrelated --session path changes.

Revalidated with:

  • ./test.sh
  • npm run check

@yhslai
Copy link
Copy Markdown

yhslai commented Mar 10, 2026

Sorry, my bad.

After running npm install and npm run build the tests all pass.

ℹ tests 442
ℹ suites 92
ℹ pass 442
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 986.3826

I also manually tested the path slash bug in #1769 and this PR fixes it for me too.

@badlogic
Copy link
Copy Markdown
Owner

sorray, i did this myself :/

@badlogic badlogic closed this Mar 14, 2026
@jamwil
Copy link
Copy Markdown

jamwil commented Mar 14, 2026

When did it become so trendy to be so rude.

@ferologics ferologics deleted the fix/windows-tests branch March 14, 2026 07:33
@yhslai
Copy link
Copy Markdown

yhslai commented Mar 14, 2026

@badlogic @ferologics Hello, I've noticed that this PR is closed because the main issue is fixed on main already. However there is a minor thing missing: the custom keybinding causing tests to fail. This PR seemed to fix that but the latest main doesn't. Could you consider incorporating that fix into main too?

@yhslai
Copy link
Copy Markdown

yhslai commented Mar 14, 2026

A simple keybindings.json to make one test fail:

{
    "submit": ["shift+enter"],
    "newLine": ["enter"]
}

Relevant log:


 FAIL  test/session-selector-rename.test.ts > session selector rename > enters rename mode on Ctrl+R and submits with Enter
AssertionError: expected "spy" to be called 1 times, but got 0 times
 ❯ test/session-selector-rename.test.ts:98:25
     96|   await flushPromises();
     97|
     98|   expect(renameSession).toHaveBeenCalledTimes(1);
       |                         ^
     99|   expect(renameSession).toHaveBeenCalledWith(sessions[0]!.path, "XOld");
    100|  });

@badlogic
Copy link
Copy Markdown
Owner

When did it become so trendy to be so rude.

@jamwil i truly wonder what about "sorry, i implemented this myself" is rude? have you also considered that there may be out-of-channel coms between me and fero (a friend of mine)?

@jamwil
Copy link
Copy Markdown

jamwil commented Mar 15, 2026

I hadn’t considered that, no. From the context in this thread alone it seemed like someone put a fair amount of effort into a PR only to have it closed unceremoniously and with a comment that would be hard to interpret as anything but rude, were it not for the fact that you know each other and communicated via other means, I suppose.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Some tests fail on Windows

4 participants