Skip to content

fix(windows): support Unicode repository paths - #1816

Open
z2250389 wants to merge 1 commit into
DeusData:mainfrom
z2250389:codex-z2250389/fix-windows-unicode-git-root
Open

z2250389 wants to merge 1 commit into
DeusData:mainfrom
z2250389:codex-z2250389/fix-windows-unicode-git-root

Conversation

@z2250389

@z2250389 z2250389 commented Aug 24, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes #1240.

Windows still passed UTF-8 repository paths to narrow CRT filesystem APIs at three boundaries:

  • Git root existence checks used stat(), so valid Unicode repositories reported root_exists=false.
  • The Graph UI browser used opendir() / readdir(), so Unicode folders were omitted or could not be opened.
  • Daemon session-context and background-index admission used stat(), so UI indexing failed even after navigation succeeded.

This replaces those calls with the existing UTF-8-aware filesystem helpers (cbm_is_dir, cbm_opendir, cbm_readdir, and cbm_closedir) and adds reproduce-first regression tests for Git discovery, UI browsing, MCP session context, and UI indexing.

Validation

  • ASan/UBSan targeted suites: daemon_application httpd git_context — 113 passed, 6 Windows platform skips
  • Security audit: passed
  • Graph UI production build: 738 modules built; 0 npm vulnerabilities
  • Windows UI smoke test: browsed C:/Users/〈Path to folder〉/道具箱/〈repo〉, indexed successfully, and detected branch main
  • Live graph result: 4,560 nodes / 23,863 edges

The full Windows cppcheck sweep was timeboxed after 35 minutes; it reported four pre-existing findings on untouched lines and no finding on the changed lines.

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test) — targeted affected suites pass; the full Windows suite has existing platform failures
  • Lint passes (make -f Makefile.cbm lint-ci) — diff-scoped checks pass; full Windows cppcheck reports existing findings and exceeded the 35-minute local timebox
  • New behavior is covered by a test (reproduce-first for bug fixes)

@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 DeusData added bug Something isn't working windows Windows-specific issues ux/behavior Display bugs, docs, adoption UX priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Aug 24, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thank you for the careful Windows work and the focused regression coverage. I checked this against current main: src/git/git_context.c still computes root_exists through narrow stat(), src/ui/http_server.c still uses opendir() and readdir(), and daemon root admission still uses stat() on canonical paths. That grounds the Unicode-path failure at the three boundaries described here.

I have labeled this as a high-priority Windows UX bug and routed it for focused review. Since the PR remains a draft, we will treat “ready for review” as the contributor handoff. The current CI matrix is green. Our review queue is full, so detailed review may take a little time, but this is now classified and visible. Thanks again for isolating the platform boundaries and covering them separately.

@z2250389
z2250389 marked this pull request as ready for review August 24, 2026 15:00
@z2250389
z2250389 requested a review from DeusData as a code owner August 24, 2026 15:00
@DeusData

DeusData commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Quick mechanical note ahead of review: this now conflicts with main (mergeable: CONFLICTING), so it needs a rebase before it can land whatever the review concludes. Worth doing at your convenience rather than waiting on me.

Nothing to change in the approach as far as I can see. Replacing stat() / opendir() / readdir() with cbm_is_dir / cbm_opendir / cbm_readdir at those three boundaries is exactly the convention this repo already holds for cbm_fopen — narrow CRT filesystem calls are the recurring way Unicode repository paths break on Windows, and the three sites you name (git root existence, the UI browser, and daemon session-context/index admission) are a coherent set rather than a scattering.

The end-to-end Windows evidence is the part that will make this quick to review: browsing an actual 道具箱 path, indexing it, detecting main, and reporting 4,560 nodes / 23,863 edges is far better than a unit test alone, because it exercises the whole chain the bug actually broke.

I will come back with a full review. One heads-up so it is not a surprise: our Actions pool is heavily backlogged today (one job executing repo-wide against 23 queued), so re-runs are slow for reasons unrelated to this PR.

Signed-off-by: Daichi Matsumoto <222130627+z2250389@users.noreply.github.com>
@z2250389
z2250389 force-pushed the codex-z2250389/fix-windows-unicode-git-root branch from 445f387 to 2ea7305 Compare September 4, 2026 11:37
@z2250389

z2250389 commented Sep 4, 2026

Copy link
Copy Markdown
Author

ご指摘のリベースと競合解消を完了しました。

  • ベース: main8f64c36d85754293895f929801bdffc58cead730
  • 更新後のヘッド: 2ea73059f894df564e21be409908c2b5d3d53b37
  • 現在の差分: 5ファイル、+138 / -16。Gitルート判定とUIディレクトリ列挙のUnicode対応、および回帰テストを維持しています。
  • main に入ったデーモンのUTF-8対応とsession-contextテストをそのまま採用し、重複する変更のみ除去しました。上流のsensitive-rootテストも維持しています。

今回の確認: git diff --checkscripts/check-no-test-skips.shscripts/check-dco.sh upstream/main..HEADscripts/security-audit.sh はすべて終了コード0でした。セキュリティ監査の既存の REVIEW 表示箇所は、今回の差分に含まれない main と同一のコードです。

このPCには対応するCビルド用ツールチェーンがないため、新ヘッドのCテストをローカル実行したとは報告していません。元のPR本文のE2E結果はリベース前のものです。新ヘッドの PR CIDCOCodeQL は起動され、現時点ではキュー待ちです。

GitHub APIでも mergeable: true を確認しています。

@DeusData

Copy link
Copy Markdown
Owner

Approved on merit. And an apology first: you completed the rebase on 2026-09-04, reported it precisely — base 8f64c36d, head 2ea73059, 5 files, +138/-16, and exactly which of main's daemon UTF-8 changes you adopted versus removed as duplicates — and then heard nothing from us for sixteen days. That is a thorough handover met with silence, and it is on us.

The change is the right shape for this codebase, and it is the same rule that governs cbm_fopen: any path that can come from a user or a repository has to go through the UTF-8 wrappers, or non-ASCII breaks on Windows only, where fewer people will notice it.

Both call sites you fixed are real instances of that:

  • src/git/git_context.c: stat(path, &st)cbm_is_dir(path). Narrow stat on Windows cannot see a path outside the active code page, so a repository under a non-ASCII directory was reported as not existing — a silent wrong answer rather than an error.
  • src/ui/http_server.c: opendir/readdir/closedir → the cbm_* equivalents, with ent->d_nameent->name. Directory enumeration had the same blind spot, so entries simply vanished from the listing.

The regression coverage is the part that makes it durable — particularly daemon_application_ui_index_accepts_non_ascii_directory, because the Graph UI enters through the daemon's background coordinator rather than the MCP tool path, and a fix proven only on the tool path would have left that route broken. Noting that in the comment is what tells the next reader why the second test is not redundant.

One mechanical thing: main has moved again since September and this is CONFLICTING once more. I am asking for one more rebase — and this time with a commitment attached rather than an open-ended wait: push it and I will review and merge promptly rather than letting it age again.

If you would prefer not to chase main a third time, say so and I will carry the rebase myself with you credited as the author. Either is fine; I would rather ask than assume.

丁寧なご報告をありがとうございました。

This branch has not been deployed

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

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. ux/behavior Display bugs, docs, adoption UX windows Windows-specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows: Git discovery fails for valid repository at Unicode path

3 participants