Repository navigation
fix(vectors): pin conformance vectors to LF with .gitattributes (#1160) - #1177
miles-on-nightshift merged 2 commits into
Conversation
…-ai#1160) * Add repository-level .gitattributes to auto-detect text files with eol=lf and preserve exact binary bytes for spec/vectors/** (-text). * In spec/vectors/build.py, pass encoding="utf-8" and newline="" to (d / "INTEGRITY").write_text() and INDEX.write_text() to prevent CRLF emission and locale encoding dependency on Windows. * In spec/vectors/build_capsules.py, pass encoding="utf-8" and newline="" to INDEX.write_text(). * Resolves false-positive integrity mismatches when repository is checked out on platforms with core.autocrlf enabled. Signed-off-by: Ama Senevirathne <amasenevirathne7@gmail.com>
miles-on-nightshift
left a comment
There was a problem hiding this comment.
Correct fix for the Windows CRLF issue. .gitattributes with * text=auto eol=lf normalises all text files on checkout, and spec/vectors/** -text keeps the binary/byte-exact fixtures untouched. The build.py change pins encoding and newline explicitly so regeneration on any OS produces identical output. CI passing. @plur9 ready for merge.
|
Thanks for the approval! Both this and #1176 are green on all checks — happy to make any final adjustments if needed, otherwise ready to merge whenever suits. |
crtahlin
left a comment
There was a problem hiding this comment.
Requesting changes — the generator fix uses a Python 3.10 API, so build.py --check now crashes on Python 3.9, which is what macOS ships as /usr/bin/python3.
Thank you for this. The diagnosis is right, the two-rule .gitattributes is right, and you correctly saw that attributes alone would not have been enough — git governs checkout and commit, not what Python writes on a Windows machine. The blocking issue is one line each and the file already contains the idiom that fixes it.
Blocking
-
spec/vectors/build.py:220,225,530andspec/vectors/build_capsules.py:288—Path.write_text()gained itsnewlinekeyword only in Python 3.10. On 3.9 all four calls raiseTypeError: write_text() got an unexpected keyword argument 'newline'. Verified on two separate interpreters: this machine's/usr/bin/python3(3.9.6) and a clean 3.9.25 —python3 spec/vectors/build.py --checkcrashes at line 220, and a full regeneration crashes at the same place. Both scripts run clean on 3.9 before this change.That silently raises the toolchain floor from 3.9 to 3.10 for a directory the README describes as "Python, stdlib only" and whose CI comment says the gates must "run anywhere". CI cannot catch it:
ci.yml:118-123uses the runner's barepython3with nosetup-pythonstep.The file already has the answer —
build.py:211,212,231andbuild_capsules.py:242usewrite_bytes. So:(d / "INTEGRITY").write_bytes((correct + "\n").encode("utf-8"))
Byte-exact, no version floor, and more obviously correct than relying on a text-mode newline flag.
Should fix
.gitattributes:5—spec/vectors/**is broader than the fixture corpus.git check-attrconfirmsbuild.py,build_capsules.py,verify.pyandREADME.mdalso get-text, so the very scripts that enforce byte stability are themselves exempt from LF normalization and a Windows contributor could commit CRLF into them. Either narrow tospec/vectors/packs/**,spec/vectors/capsules/**plus the two JSON indexes, or addspec/vectors/*.py text eol=lfafter line 5.
Notes
- The fix protects fresh clones only. Upgrading an existing
core.autocrlf=trueclone from main to this commit leaves 47 vector files still CRLF,verify.pystill reporting 34 failures, andgit statusreporting the tree clean — so nothing points at the cause. Recovery isrm -rf spec/vectors && git checkout -- spec/vectors. Filed as #1181 rather than asked of you, since it is a README and CONTRIBUTING change rather than part of this fix. build.py:517,build_capsules.py:276— the writes are newline-pinned but the matching--checkreads still usePath.read_text(), which applies universal-newline translation and would silently accept a CRLFindex.jsonas matching. Not a regression, arguably the tolerant choice, but the drift gate cannot detect a CRLF index on its own.- The undeclared interpreter floor that let this through is filed as #1182 — a CI gap rather than something to fix here.
Verified
The original defect and the fix were both reproduced end to end. A clone of main with core.autocrlf=true rewrites 49 of 62 vector files to CRLF and verify.py reports 34 failures; the same clone at this commit keeps all 62 files LF, and all three Python gates plus the 102-test spec-vectors suite pass.
The repository-wide rule is safe here, and I checked rather than assumed: git ls-files --eol shows 933 of 955 files already stored LF and nothing stored CRLF, git add --renormalize . on a fresh checkout stages nothing, there are no .bat/.cmd/.ps1 files and no CRLF-asserting fixtures. The two rules also interact correctly — git check-attr -a reports text: unset for SKILL.md, engrams.yaml, INTEGRITY, .jsonld and the .plur capsules, so -text wins over * text=auto for every vector path. Choosing -text rather than binary was right: binary would also have unset the diff attribute, and vector diffs stay readable.
Tests: PASS on Python 3.13 (all three gates green, 102/102 vitest). FAIL on Python 3.9, which is the blocking finding above.
…e corpus - Replace Path.write_text(newline="") (3.10+) with write_bytes at the four write sites; byte-identical output, no interpreter floor. - Narrow spec/vectors/** -text to packs/**, capsules/** and the two JSON indexes so the gate scripts and README stay under LF normalization.
|
Thanks @crtahlin — excellent review, both points addressed in 87fa481: Blocking (Py 3.9): all four Should-fix (.gitattributes scope): narrowed Verified locally (Python 3.12): |
|
Confirming CI on 87fa481 is fully green: |
crtahlin
left a comment
There was a problem hiding this comment.
Approved. Both points from my earlier review are resolved, exactly as suggested — thank you for the quick and careful turnaround.
All four write_text(..., newline="") calls now use the file's own write_bytes(....encode("utf-8")) idiom, which is byte-exact and carries no interpreter floor. And -text is narrowed to the generated corpus — packs/**, capsules/**, index.json and capsules.json — so the gate scripts and the README fall back under LF normalization with the rest of the repository.
You mentioned you could not test on Python 3.9 from a Windows host, so I did:
- Python 3.9. The three gates pass on the macOS system interpreter, 3.9.6, and on a separate 3.9.25 build. Restoring the old
write_text(newline=)atbuild.py:220makes 3.9.6 fail with the originalTypeError, so the green run genuinely exercises the fixed line. - Byte-identical output. A full regeneration on 3.9.6 of all 13 packs and 13 capsules leaves 0 files differing from the committed corpus.
- Other interpreters. The gates also pass on 3.11, 3.13 and 3.14.
spec-vectorspasses 102 of 102. - The original defect. Under
core.autocrlf=true,mainconverts 49 of 62 vector files on checkout andverify.pyreports 34 failures. This head converts 0 of 62 and passes, withbuild.py --checkandbuild_capsules.py --checkboth matching. - Coverage of the narrowed rule. Every tracked file under
spec/vectors/was checked withgit check-attr. The only ones outside the four-textpatterns areREADME.mdand the three scripts, which is the intent.
issue-guard does not appear on the latest commit because it triggers only on opened, edited and reopened, not on a push. That is expected, not a failure.
The two related items from the earlier review — documenting how to repair a clone already checked out as CRLF (#1181), and enforcing the interpreter floor the gates depend on (#1182) — are tracked separately and are not needed here.
|
Friendly 48h follow-up on this PR — the requested changes are implemented and CI is green (evidence posted above). Happy to adjust further if anything else is needed. Thanks for the review! |
…the record (#1164 review) Second round of review fixes, following the maintainer's re-review. The guard now enforces only when the dismissed review had requested changes. The `review_dismissed` timeline event carries the review's state before dismissal in `dismissed_review.state`. An approved or commented review passes, and an unknown state is enforced rather than waved through. This matters because the `dismiss_stale_reviews` rule removes an approval on every push, recording the pusher as actor with no message. At the previous head that would have failed on #1177 with "@amasen02 dismissed a review without a message" — a public red check against an external contributor for something nobody chose to do. Removing an approval never bypasses review, so nothing is lost by not enforcing there. A missing timeline event still fails closed. Every observed case (#1122, #1138, and one of the two on #1017) was a manual dismissal of a changes-requested review, which is exactly the case this guard exists for, so passing on a missing event would reopen the hole. The earlier suggestion to treat "no event, and the review's commit is not the head" as an automatic dismissal was wrong for the same reason: it would have passed all three. The header no longer says the empty dismissals were people skipping a reason. A census by previous review state, 2026-08-18 to 2026-09-11, finds fourteen dismissals: six empty and eight with a message. All six empty ones were of approvals, and no changes-requested review was dismissed empty. The guard is preventive, and the header now says so. The unverified fork explanation is gone. The header no longer suggests adding this as a required status check. The job runs only on a dismissal, so a pull request without one never reports it, and its result attaches to the head at dismissal time, so the next push leaves it behind. A gate would need a `pull_request` trigger that re-evaluates every dismissal. The remediation text now names what actually clears a failure — ask the reviewer to re-review, or push a commit — because a dismissed review cannot be dismissed again. A dismissal made only of invisible characters no longer passes: `trim()` leaves zero-width spaces, byte-order marks and directional controls in place. The class that strips them is written with escapes, because a literal U+2028 or U+2029 breaks both the script and the YAML. Verified: 24 cases against stubs, all passing, four of them replayed from the live timeline. #1177 and #929 (approvals dismissed with no message) pass, #946 (changes requested, with a message) passes, and #1138 (changes requested, no event) fails closed. Removing the prior-state gate turns five cases red, including both live approval replays; replacing the strip with plain `trim()` turns three red.
miles-on-nightshift
left a comment
There was a problem hiding this comment.
Verified:
- CI: all 4 Node matrix variants pass (test 20/22/24/26, dedup-calibration, smoke-packaged)
.gitattributes:* text=auto eol=lfnormalises text files;spec/vectors/packs/**,spec/vectors/capsules/**, and the two index JSON files are scoped-textso their raw bytes are not touched on checkout — exactly the path the §5.5 and §6.7 integrity hashes coverwrite_bytesinbuild.pyandbuild_capsules.py: correct fix for Pythonwrite_textinserting CRLF on Windows even with an explicit encoding arg;INDEX.write_bytesand bothINTEGRITYwrites now encode UTF-8 explicitly before write, matching the spec's byte-stability requirement- Sibling check: all three
INTEGRITYwrite sites and both index writes in both build scripts are covered
Summary
Resolves #1160.
The conformance vector corpus (
spec/vectors) provides byte-exact fixtures (§5.5 integrity hashes and §6.7 capsules) for multi-language implementations.As observed in #1160:
.gitattributes. When cloned withcore.autocrlf=true(common default on Windows), text files underspec/vectorsreceived CRLF line endings, mutating all raw-byte SHA-256 integrity hashes (§5.5) and reporting 11 false-positiveMISMATCHnotices across the vector corpus during verification.spec/vectors/build.py:220,225,530andbuild_capsules.py:288,INTEGRITY,index.json, andcapsules.jsonwere written withPath.write_textwithoutencoding="utf-8"ornewline="". On Windows, Python translated\nto\r\n, causingbuild.py --checkto report drift on files it generated itself.Proposed Changes
.gitattributes:* text=auto eol=lfto ensure LF normalization on commit and checkout across all platforms.spec/vectors/** -textto preserve exact byte stability across platforms for conformance vector fixtures.spec/vectors/build.py:encoding="utf-8", newline=""to(d / "INTEGRITY").write_text(...)andINDEX.write_text(...)so generated fixtures use universal LF newlines and explicit UTF-8 on Windows.spec/vectors/build_capsules.py:encoding="utf-8", newline=""toINDEX.write_text(...).Verification
python spec/vectors/build.py --check: Vectors and index.json match what this script builds. (Exit code 0, 0 drift).python spec/vectors/build_capsules.py --check: Capsules and capsules.json match what this script builds. (Exit code 0, 0 drift).python spec/vectors/verify.py --index spec/vectors/index.json --capsules spec/vectors/capsules.json: 26 fixture(s) checked, 0 failure(s) (all false-positive mismatches eliminated).npx vitest run spec-vectors: 102/102 passed.packages/corebuilds cleanly viatsup.