Skip to content

fix(doctor): make Git diagnostics robust to non-UTF-8 host locales - #4338

Merged
huangruiteng merged 3 commits into
loopx-project:mainfrom
LIHUA919:codex/doctor-git-utf8
Sep 14, 2026
Merged

huangruiteng merged 3 commits into
loopx-project:mainfrom
LIHUA919:codex/doctor-git-utf8

Conversation

@LIHUA919

@LIHUA919 LIHUA919 commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

loopx doctor can fail while decoding Git output on a non-UTF-8 Windows host. Pin UTF-8 with replacement decoding for its Git reads, preserving Unicode refs and the existing unavailable/unknown outcomes for failed commands.

The Git metadata readers, revision relation enum, and trusted-release-ref lookup now live together in doctor_git.py. Their production callers and tests use that owner. This keeps doctor.py at 1,311 lines and the new module at 222, within the existing 1,500-line maintenance budget. The five moved definitions have identical ASTs to the pre-extraction implementation; no new executor abstraction or compatibility wrapper is introduced.

Related to #4155; this covers the doctor slice of the broader subprocess audit.

Validation:

  • python3 -m pytest -q tests/test_doctor_git_encoding.py tests/test_doctor_install_freshness.py tests/test_doctor_installation_scope.py tests/canary/test_maintainability_ratchet.py: 43 passed with the new module tracked.
  • The 13 decoding regressions failed on the original implementation. Coverage includes real Git Unicode branches/tags/remotes, clean/dirty state, malformed pipes, directional ancestry, repository trust rejection and failed Git commands.
  • The maintenance test failed at the previous PR head (1,510 lines) and passed on the original baseline (1,500 lines); it now passes without changing thresholds or exceptions.
  • Actual doctor CLI over a disposable real Git repository under simulated CP936 preserves Unicode refs and ancestry before/after extraction. JSON assertions match; unrelated synthetic installation-health failures remain unchanged.
  • New module and encoding tests pass Ruff; compile, public/private scan and git diff --check pass.
  • Exact head 7245a1ac609c0ff40521e4c3f48e724d1a7fc6ae: native Windows, all four Python shards, pytest aggregate, DCO, and merge-gate passed in CI run 34763308096. Native Windows includes the new decoding regressions.

Delivery affects the existing CLI diagnostic path. No settings or frontend contract changes are needed. The future-facing pass was applied at the doctor Git ownership boundary, with existing error and trust semantics preserved. No self-merge is requested.

Signed-off-by: Lihua <1017343802@qq.com>
Signed-off-by: Lihua <1017343802@qq.com>
Signed-off-by: Lihua <1017343802@qq.com>

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

动机

评审目标是确认本 PR 在精确 head 7245a1ac609c0ff40521e4c3f48e724d1a7fc6ae 上,是否让 loopx doctor 在 GBK 等非 UTF-8 host locale 下稳定读取 Git,并且没有改变安装 freshness 的既有语义。作者还将 Git 诊断从混合的 doctor 模块中抽成单一 bounded owner。

改动思路

doctor_git.py 集中承载 GitRevisionRelation、metadata、ancestor relation 和 trusted release ref 三类只读操作。每个 subprocess.run 显式指定 UTF-8 与 errors="replace";OSError/nonzero 仍返回原来的 None/UNKNOWN。doctor.py 只负责 collect_doctor/build_install_freshness 的报告组合,调用点和字段保持不变。

具体改动

  • Git metadata 读取 commit、branch/tag、status 时不再依赖 host 默认 decoder,Unicode ref 可保留,坏字节变成可观测 replacement character。
  • ancestor relation 仍按两次 merge-base --is-ancestor 的 exit code 映射 same/ahead/behind/diverged/unknown。
  • trusted release ref 仍要求 GitHub repository URL 匹配且 rev-parse --verify 成功;坏 remote 输出不会伪造可信 ref。
  • 旧的 inline enum/helper 定义从 doctor.py 删除,生产调用切换到 doctor_git.py;新增真实临时 Git repo、Unicode remote/ref、坏 stdout/stderr、非零返回和失败 fallback 测试,并加入 Python CI。

对主干的风险

没有阻塞性问题。显式 UTF-8 只影响 Git 诊断读取,不增加写操作、安装切换、权限或持久状态。errors="replace" 可能让异常 ref 文本可显示,但 trusted ref 仍经过 repository match 与 commit 验证,不会把 replacement 当成可信 commit。唯一的 P2 兼容性注意是:原来这些名字虽未以下划线开头,但仓库没有调用点或文档承诺;若外部代码把 loopx.doctor.git_metadata_for_root 等当成公共 API,模块移动会产生 ImportError,可补充薄 re-export/deprecation alias。

我的整体评价

APPROVE。实现把 locale-sensitive 的故障修正在实际 Git subprocess 边界,并保留全部旧 fallback/分类语义;35 个聚焦测试、ruff 和远端 required checks 均通过。P2 兼容性建议不阻塞本 PR,因为未发现仓库内或公开文档中的 direct import 契约。

English verdict: APPROVE for exact head 7245a1ac609c0ff40521e4c3f48e724d1a7fc6ae. Git diagnostics now decode UTF-8 with replacement on every subprocess, preserve Unicode refs and safe None/UNKNOWN fallbacks, and keep doctor composition semantics unchanged. 35 focused tests, ruff, and all required checks pass. P2: add re-export aliases only if loopx.doctor helper imports are a promised public API.

@huangruiteng
huangruiteng merged commit 1e3c10e into loopx-project:main Sep 14, 2026
25 checks passed
kokokoXUY added a commit to kokokoXUY/loopx that referenced this pull request Sep 27, 2026
The usage-ping state file is written by Node as UTF-8 and read back through
`Path.read_text()` without a codec, and the control call reads the Node
process with `text=True` and no codec. Both use
`locale.getpreferredencoding(False)`, which is `cp936` on a zh-CN Windows
host, so one non-ASCII character in the state either raises
`UnicodeDecodeError` or is decoded as mojibake - the failure mode loopx-project#4338,
loopx-project#4942 and loopx-project#4997 fixed elsewhere.

`tests/test_runtime_subprocess_utf8.py` is red on current main for exactly
this reason (`loopx/usage_ping.py:53`); it is green again with this change.

The new guard covers the other half of the same class: text-mode file reads
and writes under `loopx/` that do not pin UTF-8. It reports the four
`read_text()` sites fixed here and nothing else, and its own fixture pins the
narrowing that keeps a keyed `read_text("INSTALLER")` lookup from being
reported as file I/O.

Signed-off-by: kokokoXUY <13682395396@163.com>

Rebased onto current main; the subprocess site was already pinned upstream.
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.

2 participants