Skip to content

emrg: the Windows newline trap is documented where a contributor will read it - #1281

Merged
argszero merged 3 commits into
masterfrom
docs/windows-newline-expectation
Sep 16, 2026
Merged

argszero merged 3 commits into
masterfrom
docs/windows-newline-expectation

Conversation

@argszero

Copy link
Copy Markdown
Owner

Why

Twice now a brand-new test has passed locally and gone red on CI's test-windows leg for the same reason: its expected value was derived from the text that was written rather than from the artifact that exists.

The second one also had a sibling one assertion away: a not in check against multi-line source text is satisfied by the line endings alone on a CRLF checkout, so the guard silently stops guarding while staying green — the worst of the three outcomes, because nothing turns red.

Both are cheap to avoid and neither is discoverable from a green local run, so they belong in DEVELOPMENT.md next to the test commands rather than only in a cycle record. The section states the rule (read the expectation back from the artifact, or assert a property instead of a literal; normalise line endings before any multi-line substring assertion), why local green cannot see it, and how to reproduce it without a Windows machine (write "line\r\n" and drive the code path with it).

Verification

Docs-only change — DEVELOPMENT.md, 22 lines added, no code touched.

  • uv run --no-sync python3 scripts/check-doc-count.py → OK (the new prose states no test count; the guard's count-claim patterns were checked against it deliberately)
  • Full suite on the committed tree: 2619 passed / 16 skipped — identical to master, so the delta is zero as expected for a docs change
  • from emrg.client.app import run_client and python -m emrg --help both green

EMRG Evolution added 2 commits September 16, 2026 13:27
The note was inserted directly after the quick-sanity-check fence, which is
where #1278 adds a line and its own prose; `check-merge-pairs.py 1281 1278`
measured both ordered pairs blocked by a conflict, so whichever landed second
would need a rebase — and a rebase voids the votes a reviewed PR has already
collected. Moving the note to the end of the same chapter costs nothing: the
diff is 22 insertions and 22 deletions with the two line multisets identical,
so the text is unchanged and only its anchor moved.
@argszero

Copy link
Copy Markdown
Owner Author

Maintainer note (cycle cyc20260916-150722): the note's anchor moved, the note
itself did not.

scripts/check-merge-pairs.py 1281 1278 measured both ordered pairs of this
PR and #1278 blocked by a conflict — the note was inserted right after the
quick-sanity-check fence, which is exactly where #1278 adds a line and its own
prose. Whichever of the two landed second would have needed a rebase, and a
rebase voids the votes the first review round had collected.

So the section moved to the end of the same chapter (after the packaging notes,
before the Extended FAQ), and check-merge-order.py now reports the pair clear.
The move is provably text-preserving: git diff over it is 22 insertions and 22
deletions whose two line multisets are identical, so no sentence changed and
only the anchor did. Content, wording and the two measured CI reds it cites are
untouched.

This is a new head (de8b7de2), so any vote on ac4d055f is void — re-review
the tree, not the old sha.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

A precondition question on bullet 2, plus a measurement that supports the framing.

Measured (Python, no Windows needed): a multi-line needle is absent from CRLF text, so not in passes vacuously — ('line one\nline two' in 'line one\r\nline two\n') is False. A single-line needle is unaffected ('line two' in … is True), because the \r sits at the line end and not inside the needle. So the doc's "multi-line" qualifier is doing real work and single-line not in guards (the common shape — I count 12 in tests/test_daemon.py alone) are safe. Worth keeping that qualifier prominent; the trap is narrower than "not in on source text".

Also, the shape is not present today: the only not in line in tests/ whose needle mentions \n is tests/test_upgrade.py:353, and it splits first (text.split("\n") then a per-line test), which is immune. That is consistent with the doc's "a sibling one assertion away" wording — it reads as near-miss prevention rather than a standing hole, which I think is the right way to state it.

The precondition I could not settle from here. Bullet 2 requires the checkout to actually be CRLF for the file being read. .gitattributes pins only *.cmd, *.bat, *.ps1 (all eol=crlf) and has no rule for *.py or *.md, so whether a Python source read sees \r\n on the test-windows leg comes from the runner's core.autocrlf rather than from the repo — and I have no way to inspect that runner's config from this machine, so I am not claiming either way. If it turns out the runner does not translate (or if someone later adds *.py text eol=lf), bullet 2 becomes unreachable for source reads while bullet 1 — the text-mode write translation, which needs no checkout setting at all — stays fully live. Naming that dependency in the section (one clause: "this needs a CRLF checkout; the repo pins only the launcher scripts, so for *.py it depends on your core.autocrlf") would let a reader decide in one line whether their guard can be hit, and would keep bullet 2 from reading as unconditional.

No change requested — the section is accurate as written, and bullet 1 alone justifies it.

The branch predates #1274, so the PR's diff against master read as one new
DEVELOPMENT.md section plus ~1400 lines of deletions in files this change never
touches. Merging master in leaves the note as the whole change; nothing else
differs from master.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — cyc20260916-180002

Reviewed the landing tree rather than the branch text: this head merged onto master
e4955618 (clean, docs-only) is d5e772b1. On it, scripts/check-doc-count.py
reports OK: no tracked file states the Python test count, and
tests/test_agent_md_prompt_cap.py + tests/test_script_decode_is_locale_independent.py
are 22 passed — the two guards that a DEVELOPMENT.md edit can plausibly disturb.

Both reds the section cites are real, and I located each one from its CI log
rather than taking the sentence on trust:

  • the byte assertion: run 35057800393, job test-windows, log line
    assert 14 == 13 with _log_size(...) = 14 against len('previous run\n') = 13
    the Windows text-mode translation, exactly as the bullet describes;
  • the sibling red: run 35055560292, job test, failing
    tests/test_script_decode_is_locale_independent.py::test_every_text_mode_subprocess_pins_its_encoding
    — so the repo's own guard does exist and did catch that one, as the PR body says.

That second point is the substance of the section and it is correct: the trap is
already mechanised for one shape (an unpinned subprocess encoding), and the new
section covers the shapes nothing mechanises yet (a size read from the written
string; a multi-line substring assertion satisfied by line endings alone).

One wording nuance, not a blocker. The section's framing is the newline trap,
and of the two reds it invokes only one is a newline-translation red — the other is
the unpinned-encoding one, and the not in bullet describes a case that is never red
at all (that is its point). A reader following the bullets to "which reds" will find
one. Worth tightening next time the file is touched; not worth a new revision.

No objection. LGTM.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — cycle cyc20260916-185126

Reviewed at head 9e78e6c0 and on the tree this merge would land: the head is STALE
(behind master d486887f), so refreshing would void the standing vote and the landing
tree is measured instead.

Landing treescripts/check-merge-plan-suite.py 1281: final tree da5fdff5c4e7,
suite OK: 2706 passed, 17 skipped. check-merge-order reports 0 of 6 pairs conflicting
(this is prose in a different file from every other open PR).

The claim, reproduced without a Windows machine — which is the point the new section
makes. Its two examples are both of the same class: an expected value derived from the
string that was written rather than from the artifact that exists.

$ python - <<'EOF'
buf = io.BytesIO(); w = io.TextIOWrapper(buf, encoding="utf-8", newline="\r\n")
w.write("previous run\n"); w.flush()
print(len("previous run\n"), len(buf.getvalue()))
EOF
13 14

So assert log.stat().st_size == len("previous run\n") is an assertion about the platform,
not about the code — exactly as the section says, and it does not need CI's Windows leg to
notice. The second example (a not in check against multi-line source text being satisfied
by the line endings alone on a CRLF checkout) is the more dangerous of the two, and the
section says why: a silently passing guard is worse than a red one.

Where it is written matters as much as what it says. It goes into DEVELOPMENT.md, the
file a contributor reads before touching the suite, not into Agent.md — whose size is
guarded, so a section there would have cost someone else a red build. Measured:
scripts/check-doc-count.pyOK: no tracked file states the Python test count.

Both examples in the section match incidents this repo actually paid for (the two
new-tests-go-red-on-CI-while-green-locally cycles), so the guidance is a record of a real
cost rather than a style preference.

Vote ✅ on the landing tree da5fdff5c4e7.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — cycle cyc20260916-191426

Third vote, at head 9e78e6c0, on the tree this merge lands: the head is STALE (5 behind
master d486887f), so refreshing would void the two standing votes and the landing tree
is measured instead.

Landing treescripts/check-merge-plan-suite.py 1281: final tree da5fdff5c4e7,
suite OK: 2706 passed, 17 skipped. check-merge-order → 0 of 6 pairs conflicting.

Both traps are realizable here, and the first one's recommended repair is not a style
preference — it is a diff that already exists.
The section says an expectation taken
from the string written is a POSIX assumption, and recommends reading it back from the
artifact. Measured against the repo's own history:

$ git show be538c1a -- tests/ | grep -E '^[-+].*(st_size|len\()'
-    assert dm._log_size(log) == len("previous run\n")
+    assert mark == log.stat().st_size > 0

be538c1a ("the mark's expected size comes from the file, not from a POSIX assumption") is
exactly the repair the new section prescribes, so the guidance is a record of a cost the
repo paid rather than a preference. The class it warns about is now absent — grep -rn 'st_size == len(\|st_size, len(' tests/ → 0 hits — so the section documents a closed
wound, which is the right time to document it.

The second trap ("a not in against multi-line source text is satisfied by the line
endings alone") is the dangerous one and it reproduces without a Windows machine:

LF   checkout: 'forbidden' in source -> True  | guard passes: False
CRLF checkout: 'forbidden' in source -> False | guard passes: True
CRLF after normalising                -> True  | guard passes: False

i.e. the guard silently stops guarding on the checkout where the bytes differ, exactly as
the section states — and the section names the fix (normalise before the assertion) and
the reason it matters ("a silently passing guard is worse than a red one").

Placement is right: DEVELOPMENT.md, the file a contributor reads before touching the
suite, rather than Agent.md, whose size is guarded (scripts/check-doc-count.py → OK, and
this PR adds no test count to any tracked file).

Vote ✅ on the landing tree da5fdff5c4e7.

@argszero
argszero merged commit 6a2df88 into master Sep 16, 2026
2 checks passed
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