Skip to content

feat(scripts): add check-encoding-safety.py (sibling of check-windows-footguns) - #66669

Closed
pnascimento9596 wants to merge 2 commits into
NousResearch:mainfrom
pnascimento9596:feat/check-encoding-safety
Closed

feat(scripts): add check-encoding-safety.py (sibling of check-windows-footguns)#66669
pnascimento9596 wants to merge 2 commits into
NousResearch:mainfrom
pnascimento9596:feat/check-encoding-safety

Conversation

@pnascimento9596

@pnascimento9596 pnascimento9596 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

feat(scripts): add check-encoding-safety.py (sibling of check-windows-footguns)

Addresses #66668

What / why

The env-class cluster is not hypothetical: #66607 closed, #66609
merged to main via salvage #66825 (2026-07-18), authorship preserved
(51e1fb8)
shows a UTF-8 BOM in jobs.json crashing load_jobs;
three open fix PRs (#65124, #66475, #66483) cover the same
user-writable encoding/write class; accepted-class #57247 / #57249 is
the same .env family Hermes users edit by hand. The salvage itself
widened the fix to three additional readers (backup.py,
curator_backup.py, prompt_builder.py context scan). This checker still
flags residual prompt_builder sites on the hit list
(prompt_builder.py:1401 and prompt_builder.py:1858), demonstrating the
class is live and the checker's coverage is real. CONTRIBUTING already
documents Notepad/Windows BOM provenance and mandates utf-8-sig for
Windows-touched config.

This PR lands step 1 of a smaller-ask ladder: an opt-in, stdlib-only
AST checker (scripts/check-encoding-safety.py) sibling of
scripts/check-windows-footguns.py. It flags reintroduction at review
time (R1 plain utf-8 on user-writable reads, R2 latin-1 fallback without
BOM strip, R3 errors="replace" rewrite) without changing runtime
behavior or wiring CI/docs yet.

Known recall boundary: opaque path locals (e.g. the two readers #66825 widened to) are not bound to the allowlist - precision-first tradeoff, assignment tracing is a follow-on; details in #66668.

How to test

venv/bin/python -m pytest tests/scripts/test_check_encoding_safety.py -q
# → 22 passed (Gate 1: 6/6 historical fixtures flagged; fixed forms clean)

venv/bin/python scripts/check-encoding-safety.py --all
# → 85 hits / 786 files @ d59b79fad at push; 81 hits / 791 files @ c78aa0bad as of 2026-07-18 (R1=75, R2=2, R3=4)
Update: 85/808 @ 614dc194e (root-module scan gap fixed per sweeper review; +4 hits are newly-visible top-level modules). -> 97/850 @ bd6437d60 (rebase only, no rule changes)
# Classification (hardened): A=24 / B=49 / C=12

venv/bin/python scripts/check-windows-footguns.py scripts/check-encoding-safety.py
# → no footguns, 1 file scanned, exit 0

Footprint

Implementation footprint (vs upstream/main @ d59b79f): 16 files,
+1700/-0
- script (+1120), tests (+168), 14 fixtures. No CI,
pyproject, or docs in this PR.

Platforms tested

  • macOS 26.5 (Darwin 25.5.0 arm64), Python 3.11.15 - real Gate 1
    (22/22) + full-tree --all (85/786) + C-proof runtime evidence in the
    linked design issue.
  • Windows - reasoned only: scripts/check-windows-footguns.py scripts/check-encoding-safety.py clean (exit 0); the lint script itself
    has no OS-specific code paths (AST/stdlib-only; pathlib + allowlist
    path fragments). Not re-run on a native Windows host in this PR.

Non-goals

Test plan (CI)

  • pytest tests/scripts/test_check_encoding_safety.py -q (22 passed)
  • Existing suites remain green on Linux CI
  • No required encoding-safety CI gate added by this PR

CI note (at push, run 29627150709): all required
checks pass ('All required checks pass' = SUCCESS;
ruff, Windows footguns, all 8 test slices green).
The single red mark is the non-required 'CI timing
report' job, which failed historically on fork PRs; passed since the 2026-07-19 run
(empty AUTOFIX_BOT_PAT in the workflow env - same
residual visible on #66609 and other open fork
PRs). Not specific to this change.

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

Labels

needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants