Repository navigation
fix(tools): write the touchpoint manifest as UTF-8 with LF - #6819
nigelfenton wants to merge 3 commits into
Conversation
Ozy311
left a comment
Ozy311
left a comment
There was a problem hiding this comment.
Reviewed adb81365840061470b1dc3f4b506c66cfd3f24c4 against freshly fetched main 464e42eb879f407c676c3615a9b37270da33bb12 (merge base 772abac77e23c93d8e6a40daa3165725106659fa). One reproducible finding:
[P2] Accept Git's CRLF checkout when checking the generated manifest — tools/gen_touchpoint_manifest.py:215–217.
Opening the manifest with newline="" makes --check reject a clean checkout made with core.autocrlf=true: Git materializes the committed LF Markdown as CRLF, while render() produces LF. This repository's .gitattributes does not force LF for aetherd-touchpoints.md, so a normal Git-for-Windows checkout can report the manifest stale immediately, even though regeneration changes no repository content. The old read_text() normalized these line endings and accepted it. I reproduced this with an actual temporary Git index and checkout-index using the head's committed manifest: 286 CRLF lines, git diff --exit-code clean, baseline --check exit 0, PR-head --check exit 1. Keep the explicit UTF-8 decoding but normalize newlines for comparison, or give the generated file an enforced LF checkout rule.
The intended encoding fix is otherwise verified. Fourteen isolated checks passed with production code: forcing cp1252/default Windows newline behavior reproduces baseline UnicodeEncodeError and a zero-byte manifest, while the fixed writer succeeds with UTF-8/LF and its check passes. UTF-8 source includes and JSON notes survive that locale; a mutation dropping the writer's explicit newline argument produces CRLF and is detected. Production-tree render output is byte-identical to the committed manifest for baseline UTF-8, fixed UTF-8, and fixed cp1252 defaults.
The locale/newline behavior above was injected into Python 3.14.5 on macOS; the Git checkout reproduction is a real Git operation. I did not run native Windows or Python 3.12 locally. All four required CI checks (build, check-windows, check-macos, Static checks) passed on this head. No overlap in the changed script from the merge base to current main; head and empty existing review threads/reviews were rechecked before posting.
`--check` read the manifest with `newline=""`, which keeps whatever line endings are on disk. On a `core.autocrlf=true` checkout git materializes the committed LF as CRLF — and `.gitattributes` sets no `text`/`eol` rule for `docs/architecture/aetherd-touchpoints.md` — while `render()` always emits LF. So `--check` called a pristine, `git diff`-clean checkout stale and exited 1 with nothing a contributor could fix by regenerating. Measured on Windows with git's own autocrlf: 286 CR bytes on disk, `newline=""` reads 68923 chars (CR present), `read_text()` reads 68637 (no CR). Read with universal newlines instead, keeping the explicit utf-8 so the locale codec still gets no say. The writer below is unchanged and still pins LF, so what lands in the repository is LF either way. A genuinely stale manifest is still detected. Reported by Ozy311 in review of #6819. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Confirmed and fixed in d46e3ec — thank you, this was a real regression and the diagnosis was exactly right. I reproduced it natively rather than by injection: this box has
I took the normalize-for-comparison option rather than a current = MANIFEST_MD.read_text(encoding="utf-8")Explicit UTF-8 stays (that is the #6818 fix); universal-newline decoding folds CRLF to LF for the comparison only. The writer is untouched and still pins Verified four ways:
The stale case is the one I cared most about — the fix must not just make the check green. A real edit is still caught. 🤖 Addressed by Claude Code |
Ozy311
left a comment
Ozy311
left a comment
There was a problem hiding this comment.
Verified the fix at d46e3ec64a393b019211ed4e80660bcf0c9c22e9; my earlier P2 finding is resolved.
I reran the same real Git fixture with core.autocrlf=true: 286 CRLF lines on disk and a clean git diff --exit-code. Baseline --check exits 0, the previously reviewed head adb81365 exits 1, and this head exits 0. The explicit UTF-8 read with universal-newline normalization fixes the regression without accepting genuinely stale or missing manifests; both negative cases still exit 1.
Sixteen isolated checks pass, including the original cp1252 truncation reproduction, successful fixed UTF-8/LF output under injected Windows defaults, Unicode source/JSON input, and production output byte-identity. Removing the writer's explicit newline argument still produces CRLF under the injected Windows defaults, so the LF-writing guarantee remains independently tested even though the comparison correctly tolerates checkout line endings.
Independent runs were on macOS / Python 3.14.5 with a real Git checkout and injected encoding/newline defaults; I did not independently run native Windows or Python 3.12. Your follow-up supplies the separate native Windows reproduction. All eight current checks, including the four required checks, are successful. Trusted base remains 464e42eb879f407c676c3615a9b37270da33bb12; head and discussion rechecked before posting. No remaining actionable finding in this one-file change.
rfoust
left a comment
rfoust
left a comment
There was a problem hiding this comment.
The final implementation fixes #6818. Explicit UTF-8 decoding/encoding removes the locale dependency; the writer pins LF, and the comparison normalizes CRLF checkouts. I independently reproduced the original cp1252 truncation and verified the fix on the current-main integration. I found no functional defect in the final patch. One contribution-policy blocker remains.
Scope
| File/group | Claimed in title/body? | Verdict |
|---|---|---|
tools/gen_touchpoint_manifest.py: source/JSON/manifest reads, manifest writer, explanatory comments |
Yes | Focused on #6818; no unrelated changes, new public surface, or generated-manifest changes |
Both commits are focused on the reported encoding problem and the follow-up CRLF comparison fix. The checklist's “Commits are signed” claim does not hold.
Blocker
-
[P2] Sign both commits before landing — commit metadata for
adb81365840061470b1dc3f4b506c66cfd3f24c4andd46e3ec64a393b019211ed4e80660bcf0c9c22e9; policy at CONTRIBUTING.md:77–78.GitHub reports
verified: false, reason: unsignedfor both commits. The actual Git commit objects also contain no signature header, so this is not merely an unrecognized signing key. CONTRIBUTING.md explicitly requires “Sign your commits,” and AGENTS.md reiterates “Sign every commit you author.” Please replace the unsigned history with signed commits and refresh CI/review on the resulting head. The checked signing box in this PR is currently inaccurate. I could not read the full administrator-only branch-protection configuration, so I am citing the explicit contribution policy rather than attributing GitHub's generic BLOCKED state to a specific protection rule.
Non-blocking nits
- PR body, Change section: it still says the check uses
MANIFEST_MD.open(...); the final implementation attools/gen_touchpoint_manifest.py:224–225usesread_text(encoding="utf-8"). Update the description to match the final change. tools/gen_touchpoint_manifest.py:243–245: the comment says CRLF output makes--checkreport staleness, but the final comparison intentionally normalizes CRLF. Describe LF as the output-format guarantee, and shorten the long explanatory blocks at lines 212–223 and 236–245 in keeping with AGENTS.md's comment guidance.
Verification and limitations
- PR head:
d46e3ec64a393b019211ed4e80660bcf0c9c22e9. - Current main tested:
da81723f4adb7e39a151f29e0dbb2b4440427af5. - Constructed merge tree:
d31e6a441c6c27ea385c4b1e85ebb4477b9d26d3; conflict-free, differing from that main only in this generator. Main and PR head were refreshed before posting. - 22/22 isolated checks passed on macOS / Python 3.13.16. Production inputs were extracted from Git into scratch storage; the initiating checkout was unchanged. The Windows cp1252 encoding and CRLF-writing defaults were injected through the filesystem-opening seam. I did not independently rerun native Windows or Python 3.12.
- All eight current checks, including
build,check-windows,check-macos, andStatic checks, are successful. Build/static checkout logs identify tested merge4b64dbd832989bd7810a146e76da3e830b48e451, based on464e42eb879f407c676c3615a9b37270da33bb12; those runs do not establish CI coverage of today's main. - The PR adds or modifies no tests and introduces no socket behavior. No C++ build or GUI/bridge session was needed for this tooling-only path; no radio was accessed. No automated review tool was available; the second pass was performed directly.
What I tried to break
- Current main under injected cp1252 reproduces
UnicodeEncodeErrorand a zero-byte manifest. The integration succeeds and writes the committed 71,600-byte UTF-8/LF manifest byte-for-byte. - LF and CRLF manifests pass; genuinely stale and missing manifests fail. Invalid semantic tags remain rejected.
- Unicode source-header paths and JSON notes survive the injected locale. Independently removing each of the four explicit encoding arguments or the writer's newline argument is detected; removing the latter produces CRLF.
Recommendation: request changes for signing only. The functional fix and the earlier CRLF remediation survived this review.
gen_touchpoint_manifest.py crashed on Windows and left the manifest EMPTY:
UnicodeEncodeError: 'charmap' codec can't encode character '→'
docs/architecture/aetherd-touchpoints.md | 286 -----------------------
MANIFEST_MD.write_text(out) passes no encoding, so the text goes out in the
locale codec. On a Windows console that is cp1252, which cannot represent the
"gui->engine" arrow in the heading the script itself generates. The write
truncates first and raises second, so the file is destroyed rather than left
alone. A contributor who does not check git diff afterwards could commit that.
That matters beyond a crash: aetherd-touchpoints.md is generated, and it is one
of the two files that conflict when a long-lived branch merges. The documented
resolution is "take one side, then regenerate" — which on Windows could not be
done at all.
newline="" is the second half. Without it Python translates LF to CRLF on
Windows, so a regenerated manifest would differ from the committed one on every
line and --check would call it stale.
The reads are given the same treatment, for the same reason: the --check
comparison must read what the write produced, and the source/JSON reads should
not depend on the console codepage either.
⚠ --check goes through MANIFEST_MD.open(...) rather than read_text(newline=""):
Path.read_text() only gained newline= in 3.13, and Linux CI runs 3.12. The
first version of this fix used read_text(newline="") and raised TypeError on
Linux — it would have broken the staleness check on the platform that runs it.
Verified on both platforms, before and after:
Windows before: UnicodeEncodeError, manifest truncated to 0 lines
after: "wrote ... 274 touchpoint headers", file byte-identical to
the committed one, --check "manifest up to date" (exit 0)
Linux after: "wrote ... 272 touchpoint headers", byte-identical,
(3.12) --check "manifest up to date" (exit 0), LF preserved
(CRLF=0, LF=284)
Also reverted the fix and re-ran to confirm the failure returns, so the
verification is not vacuous.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`--check` read the manifest with `newline=""`, which keeps whatever line endings are on disk. On a `core.autocrlf=true` checkout git materializes the committed LF as CRLF — and `.gitattributes` sets no `text`/`eol` rule for `docs/architecture/aetherd-touchpoints.md` — while `render()` always emits LF. So `--check` called a pristine, `git diff`-clean checkout stale and exited 1 with nothing a contributor could fix by regenerating. Measured on Windows with git's own autocrlf: 286 CR bytes on disk, `newline=""` reads 68923 chars (CR present), `read_text()` reads 68637 (no CR). Read with universal newlines instead, keeping the explicit utf-8 so the locale codec still gets no say. The writer below is unchanged and still pins LF, so what lands in the repository is LF either way. A genuinely stale manifest is still detected. Reported by Ozy311 in review of #6819. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review nits on #6819. The writer comment still said CRLF output would make --check report the manifest stale; since the previous commit the comparison normalizes line endings deliberately, so that is no longer the reason. State LF as the output-format guarantee instead. Both explanatory blocks are shortened in keeping with AGENTS.md's comment guidance. No behaviour change. Reported by rfoust in review of #6819. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d46e3ec to
fadb7f6
Compare
|
All three points addressed. New head Blocker — signing. You were right, and the checked box was my error, not a key-recognition problem: the commit objects genuinely had no signature header. I had explicitly passed Both commits re-signed, and GitHub now agrees:
Before force-pushing I confirmed The signing checkbox was accurate for neither of my two earlier pushes. It is accurate now. Nit 1 — PR body. Fixed. It described Nit 2 — comments. Fixed in Re-verified all four behaviours after the comment edit, since "comments only" is exactly the assumption worth distrusting:
On your limitation note — you couldn't rerun native Windows or Python 3.12, and that is where both of my earlier mistakes lived. For what it's worth, this box is native Windows with I left the base at 🤖 Addressed by Claude Code |
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
Yes. #6818 has two parts. The first is that write_text(out) used the locale codec, so on cp1252 it truncated the file and then raised on the → in the generated heading. tools/gen_touchpoint_manifest.py:236 now writes with encoding="utf-8", newline="". The second part asks for "the same for any print() path that emits →". That is already met: the only U+2192 in the script is in the rendered heading (line 148), which goes to the file. The console lines (::error::… stale — run, warnings) contain only an em dash, and cp1252 can encode that. The CRLF change to --check (lines 212–219) is explained in the PR body, not only in the thread, so it will reach the squash message.
Scope
| File / hunk | Change | Claimed in title/body | Verdict |
|---|---|---|---|
tools/gen_touchpoint_manifest.py:90 |
source read encoding="utf-8" |
yes ("source and JSON reads") | in scope |
tools/gen_touchpoint_manifest.py:108 |
sidecar JSON read encoding="utf-8" |
yes | in scope |
tools/gen_touchpoint_manifest.py:212-219 |
--check read: utf-8, universal newlines |
yes ("Updated after review") | in scope |
tools/gen_touchpoint_manifest.py:228-236 |
writer encoding="utf-8", newline="" |
yes (the fix itself) | in scope |
Everything in the diff is explained by the issue. git diff --stat against the base shows one file. There is no new public surface, no CHANGELOG entry, and no - line that removes a guard.
Blockers
None.
Non-blocking findings
-
Out of scope: the sibling generator has the same encoding gap but a different failure.
tools/gen_seam_probe_table.py:493has the same bug class:PROBE_TABLE.write_text(out)
Its only non-ASCII output is the em dash in the
// GENERATED FILE — regenerate …header (tests/SeamSignalProbeTable.inc:1). cp1252 can encode that character (0x97), so this does not reproduce #6818's crash-and-truncate. What happens instead:- On a Windows console, a regenerate quietly writes a non-UTF-8 byte.
- The developer's own check reads the file back in cp1252, so it passes locally.
- On Linux,
tools/test_gen_seam_probe_table.py:272callsPROBE_TABLE.read_text()(UTF-8 default) and fails to decode the committed file.
This is reasoned from code; I did not reproduce it. It does not belong in this PR. I'd suggest a short follow-up issue that applies the same
encoding="utf-8", newline=""pattern there.
What I tried to break
- The CRLF / LF comparison in
--check.render()joins with"\n"and the read uses universal newlines, so acore.autocrlf=trueworktree compares equal while a genuinely stale file still differs. An empty file, a BOM-prefixed file, or a stray lone\rall compare unequal, which correctly reports stale. My ownpython3 tools/gen_touchpoint_manifest.py --checkon the macOS LF checkout gavemanifest up to date, rc=0. That proves little about the CRLF case: the author's native Windows autocrlf repro in the thread is the only evidence for it. - The Python floor. The writer uses
Path.write_text(newline=), which needs 3.10+.static-checks.yml:178pins 3.12, and the 3.13-onlyread_text(newline=)trap from the earlier attempt is gone. CI exercises only the--checkpath (static-checks.yml:202). The regenerate/write path never runs in CI, so its evidence is the author's cp1252 regeneration, which they report as byte-identical to HEAD. - The truncate-then-raise mechanism. The code comment and PR body describe it correctly for
'w'-modewrite_text, and explicitencoding=removes the trigger. Noprint()emits a character outside cp1252. - Sibling call sites. I grepped
tools/for encoding-lessread_text/write_text. Most are tests, or scripts that write ASCII or uncommitted files.tools/docs/snapshot_stable.py:150already passesencoding="utf-8".gen_seam_probe_table.pyis the one parallel generator (finding 1 above).check_engine_boundary.py:167reads the same tags JSON withoutencoding=. Under cp1252 its non-ASCII notes would decode as mojibake without raising, and tags are matched as ASCII, so I found no consequence there. - Coverage. There is no
tools/test_gen_touchpoint_manifest.py, and no portable way to force the locale-default codec inside one test process. I'm disclosing that as a coverage limit, not asking for a test. The CI--checkstep guards the comparison side. - Security. security-audit (CodeGuard) found no true positives. The changed file uses only fixed repo-root paths, runs no shell or eval, and its backticks are Markdown inside string literals. The CRITICAL CMDI/PATH hits (
appimage.yml,ci.yml,check_release_assets.py, …) are in files this PR does not touch; the scanned file list reflects main's tip, not the diff. The ones spot-checked were lexical false positives: backticks in comments and messages, fixed URLs,matrix.*interpolation.
All findings are reasoned from code and my one local --check run. I did not reproduce anything on Windows. CI is green on fadb7f63 across all 8 checks; the head sits on 772abac7, not current main.
Recommendation
Approve with non-blocking findings. This is a minimal, correctly targeted fix for #6818. The earlier review rounds (the 3.12 TypeError and the autocrlf false-stale) are resolved, and the PR body now matches the final code. The seam-probe sibling is worth a follow-up issue, not a change here. Thanks @nigelfenton — the before/after evidence, including the reverted-fix check, is thorough.
🤖 aethersdr-agent · cost: $1.1309 · model: claude-opus-5-5
|
Cisco CodeGuard — static analysis of this PR (1357 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $1.1795 · model: claude-opus-5-5 |
|
Thanks — no blockers here, so nothing changed in this PR. Two responses. The seam-probe sibling: reproduced, and filed as #6987. You reasoned it from the code and said so; I measured it on native Windows, and it holds. Worth recording that it is nastier than #6818, for the reason you identified — it never raises:
Measured: a cp1252 regenerate writes One refinement to your On your coverage and evidence limits — both fair, and I'd rather state the gaps than paper over them:
CodeGuard. Agreed on all 1357 — none is on a changed line, and the scanned paths ( Head is 🤖 Addressed by Claude Code |
Intent
tools/gen_touchpoint_manifest.pycan be run on Windows without destroying the file it generates, and--checkcompares like with like on every platform. Fixes #6818.Problem
MANIFEST_MD.write_text(out)passes noencoding=, so the text is written in the locale codec. On a Windows console that is cp1252, which cannot represent thegui→enginearrow in the heading the script itself generates. The write truncates first and raises second, so the manifest is left empty rather than untouched — and a contributor who does not checkgit diffafterwards could commit the truncation.This is not only a crash.
aetherd-touchpoints.mdis generated, and it is one of the two files that conflict when a long-lived branch merges (the header's touchpoint counts differ). Its documented resolution is take one side, then regenerate — which on Windows could not be done at all.Change
Four call sites in one file:
write_text(out, encoding="utf-8", newline="")— the fix proper.newline=""pins LF, so the committed manifest is LF on every platform.--checkread, so the comparison reads what the write produced;⚠ Updated after review. The
--checkread is now plainMANIFEST_MD.read_text(encoding="utf-8")— explicit utf-8, but default (universal) newline handling.Two earlier attempts were wrong and are worth recording:
read_text(encoding="utf-8", newline="")raisedTypeErroron Linux —Path.read_text()only gainednewline=in 3.13, and Linux CI runs 3.12.MANIFEST_MD.open(..., newline="")fixed that but introduced a regression, caught by @Ozy311: reading verbatim means acore.autocrlf=truecheckout, where git materializes the committed LF as CRLF, fails--checkon a pristine tree. Measured here: 286 CR bytes on disk,newline=""reads 68923 chars,read_text()reads 68637.The reader normalizes line endings; the writer still pins LF, so what lands in the repository is LF either way. A genuinely stale manifest is still detected.
Test plan
UnicodeEncodeError, manifest truncated to 0 lines.wrote ... 274 touchpoint headers; file byte-identical to the committed one;--check→manifest up to date, exit 0.wrote ... 272 touchpoint headers; byte-identical;--check→manifest up to date, exit 0; LF preserved (CRLF=0, LF=284).Constitution principle honored
Principle VIII, evidence over assertion: the before/after is reproduced on two platforms and the fix is shown to be load-bearing by removing it.
Checklist
AppSettingscalls (N/A)MeterSmoother(N/A)🤖 Generated with Claude Code