Skip to content

fix(cli): support non-ASCII Windows agent paths - #2244

Open
lorenzozanee wants to merge 1 commit into
DeusData:mainfrom
lorenzozanee:fix/windows-unicode-agent-detection
Open

lorenzozanee wants to merge 1 commit into
DeusData:mainfrom
lorenzozanee:fix/windows-unicode-agent-detection

Conversation

@lorenzozanee

Copy link
Copy Markdown
Contributor

What does PR do?

On Windows, agent detection could report no agents when the profile path contained non-ASCII characters because the lookup used narrow stat(). This switches detection to the existing UTF-8 path metadata wrapper, which uses the wide Windows filesystem API.

Adds a Windows regression test covering a non-ASCII profile path containing .claude. Production syntax and diff checks pass; related suites passed on the existing baseline binary. The Windows runtime regression, full rebuilt test suite, and lint require follow-up in CI or a suitable Windows/tooling environment.

Checklist

  • Every commit signed off (git commit -s) — required, CI rejects unsigned commits (DCO, CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior covered test (reproduce-first bug fixes)

Fixes #2145

Signed-off-by: lorenzozanee <wyz0707@proton.me>
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@DeusData

Copy link
Copy Markdown
Owner

Thank you — this is the right fix, and I checked the part of it most likely to bite before saying so.

The production change is correct on every platform. dir_exists moving from stat/lstat to cbm_path_info_utf8 is the same rule that governs cbm_fopen: a path that can come from a user has to go through the UTF-8 wrappers or it breaks on Windows outside the active code page. My one worry was POSIX: the old code deliberately used lstat there, so a symlink to a directory was not a directory, and main recently made symlink-following an explicit per-call-site opt-in. I read the wrapper — its POSIX branch uses lstat as well — so that behaviour is preserved exactly. No semantic change on macOS or Linux; on Windows both old and new report a directory.

One real failure, and it is a one-character fix. test / test-windows (…, 2/2) is red on your own new test:

cli_detect_agents_handles_non_ascii_windows_profile
    FAIL tests/test_cli.c:13085: could not create non-ASCII Windows profile path

It is not a Unicode problem. test_mkdirp (tests/test_cli.c:718) is errno-style — it returns 0 on success and -1 on failure:

return cbm_mkdir(tmp) == 0 || errno == EEXIST ? 0 : -1;

and the test reads it as a boolean:

if (!test_mkdirp(claude_dir))
    FAIL("could not create non-ASCII Windows profile path");

!0 is true, so the test fails precisely when the directory was created successfully — which means your production fix never even got exercised on the runner. if (test_mkdirp(claude_dir) != 0) fixes it. An easy one to write; the helper's name reads like a predicate and it is not one.

The other red is ours, not yours: test / test-windows-guards is a documented setup lottery on our side that reddens unrelated PRs. Please ignore it.

Push the one-line fix and I will re-review promptly — once that test actually runs, it becomes the proof that the production change works on a real non-ASCII Windows profile, which is the part I cannot verify locally today.

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

Labels

None yet

Projects

None yet

2 participants