Repository navigation
fix(*): pin line endings to lf so windows checkouts pass the byte gates - #457
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the full github/main...HEAD diff at cd5af3c6b490. I covered the repository rules in AGENTS.md/CLAUDE.md and CONTEXT-MAP.md, the diff and relevant history, the byte-sensitive callers, tracked file types and binary handling, backward compatibility, architecture boundaries, and whether tests were weakened (they were not changed).
I reproduced a core.autocrlf=true checkout locally: the base produced CRLF in generated.ts and install.ps1, while this revision produced LF only. git add --renormalize . remained clean, and no tracked index file reports CRLF or mixed endings.
Verification passed after installing the locked UI dependencies:
npm run gen:check --prefix ui-web(178 methods)npm test --prefix ui-web -- --reporter=dot(108 files, 1836 tests)npm run build --prefix ui-webuv run python ui-web/build.pynode ui-web/scripts/check-page.mjsnode ui-web/scripts/count-shared-globals.mjsnode ui-web/scripts/check-css.mjsnode ui-web/scripts/check-class-namespace.mjs- source-language, large-file, commit-message, and diff-whitespace checks
The assembled dist/index.html contained 13,534 LF bytes and zero CRLF bytes. The initial generation command failed only because ui-web/node_modules was absent; it passed after npm ci --prefix ui-web.
A Windows clone under core.autocrlf=true lands every tracked file as CRLF. Nothing in the tree asks for that, and the checks that read bytes rather than lines then report a defect that is not there: - ui-web/scripts/check-page.mjs extracts each script payload with an anchor on "<script>\n", matches none, and fails with "extracted 0 script payloads"; - ui-web's boot-order and open-conversation tests slice their source on "\n}\n", which never matches, so the sliced function comes out empty and 9 tests fail with ReferenceError; - gen-rpc-client --check compares its own LF output against the file on disk and reports permanent drift in src/rpc/generated.ts. The repository already stores LF, so this adds no renormalization diff: git add --renormalize . over the whole tree stages nothing beyond the two files here. Only the checkout filter changes. ui-web/build.py is the other half of the same defect. It assembled the page under the default newline translation, so dist/index.html came out CRLF on Windows even from an LF source tree. That artifact ships inside the wheel, so it must not depend on which platform assembled it. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
cd5af3c to
77cec32
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This revision is a clean rebase onto the current main: git range-diff reports the PR commit as identical to the reviewed revision, and the old and new commits have the same stable patch ID. I re-read the full github/main...HEAD diff and checked the repository rules, affected callers and history, backward compatibility, architecture boundaries, and test integrity; the refreshed base creates no new interaction with the unchanged two-file patch.
Fresh verification on 77cec32ec4e1 passed: RPC generation drift check, all 108 UI test files / 1,836 tests, the production UI build, all four post-build page/CSS/namespace gates, source-language check, large-file check, commit-message check, and git diff --check.
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the two tracked-prompt writers must emit LF before this can merge.
The new review thread identifies a Windows regression that invalidates my earlier clean stance. .gitattributes pins the carried prompt assets to LF, but agents/raven-code/run.py and plugins-dist/ppt-engine/raven_ppt/plugin/hook.py still write their copies with platform-default newline translation. On Windows, the resulting CRLF files fail the existing byte-equality contracts against the LF sources. Direct reproduction confirmed the mismatch for the raven-code guide and all three ppt-engine identity prompts; explicit no-translation writing preserves equality.
Relevant test result: uv run pytest tests/test_agents_code_launcher.py::test_the_seeded_guide_is_the_carried_asset -q -o addopts="" passed on Linux. The matching ppt-engine test was skipped at module import and the node could not be collected because this environment lacks pptx; I verified that path by reproducing its exact read/write byte transformation instead.
The existing open thread carries the concrete locations, so I added no duplicate inline finding and left that reviewer-owned thread unresolved.
…latform's The .gitattributes half of this change pins the tracked prompt sources to LF on checkout. Two launchers copy those sources into a product home and two tests assert the copy equals the source byte for byte, so pinning the read side alone is what breaks them: both writers take the default newline translation, which turns every \n into os.linesep, and on Windows that is a CRLF copy of an LF source. Before this branch the two sides agreed by accident. With autocrlf=true and no attributes file the source checked out CRLF, the writer wrote CRLF, and the assertion held. Pinning only the source is the regression. agents/raven-design/run.py already writes its staged shim with newline="" and says why; raven-code and the ppt-engine hook were missed. The same keyword at both call sites is the whole change. Verified on Windows 11 with core.autocrlf=true, which is where the two assertions actually part company: tests/test_ppt_engine_plugin.py::test_the_first_turn_seeds_the_deck_identity_into_the_granted_home fails on this branch without it (b'\r' != b'\n' at index 6) and passes on main, whose CRLF checkout matched the CRLF write. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
The previous commit wrote its comment through a heredoc that turned an escaped newline into a real one, splitting the line and leaving agents/raven-code/run.py unparseable. Same comment, no escape. Verified: ruff check passes over agents, plugins-dist and raven, and the module parses. tests/test_ppt_engine_plugin.py plus tests/test_agents_code_launcher.py report the same 45 failures on this branch as on main, all of them the pre-existing Windows gaps this branch does not touch, so the seeding regression is closed and nothing new opened. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new revision fixes the Windows regression at both identified writers with explicit no-translation writes. I reviewed the delta and full github/main...HEAD diff, the repository rules, affected callers and history, equivalent text-copy paths, backward compatibility, architecture boundaries, and test integrity; no tests were weakened or removed, and I found no remaining concrete regression from the LF checkout policy.
Verification on e0959298a149 passed:
uv run pytest tests/test_agents_code_launcher.py tests/test_ppt_engine_plugin.py -q -o addopts=""(122 passed)- Ruff check and format checks for all changed Python files
- source-language, large-file, commit-message, and diff-whitespace checks
I closed my blocking reply in the existing thread. The thread was opened by another reviewer, so I left resolution to them.
|
**Not a blocker -- the blocking finding is fixed at both call sites and I re-verified it. One leftover is a sentence, not code. The fix lands
Re-run on the new head merged onto
The middle row is the interesting one, and it is not a gap you can close: on Linux
One sentence in
|
…es (#457) ## Summary A Windows checkout of this repo with the default `core.autocrlf=true` rewrites every text file to CRLF, and two byte-exact gates in `ui-web` then fail on a tree that is byte-identical to the one CI passes on Linux: - the boot-order and open-conversation node tests slice their source on `"\n}\n"`, which never matches a CRLF file, so the sliced function comes out empty and 9 tests fail with `ReferenceError`; - `gen-rpc-client --check` compares its own LF output against the file on disk and reports permanent drift in `src/rpc/generated.ts`. `.gitattributes` pins the checkout filter so the working tree matches what the repository already stores. The repository stores LF today, so this adds no renormalization diff: `git add --renormalize .` over the whole tree stages nothing beyond the two files in this PR. Only the checkout filter changes. `ui-web/build.py` is the other half of the same defect. It assembled the page under Python's default newline translation, so `dist/index.html` came out CRLF on Windows even from an LF source tree, and `check-page.mjs` could no longer extract the script payloads (its `<script>\n` anchor needs the LF to be the next byte). That artifact ships inside the wheel, so it must not depend on which platform assembled it. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification Run on Windows 11 with `core.autocrlf=true`, in a clean worktree cut from `main`: - `git add --renormalize .` after the change stages nothing (confirms no renormalization diff); - `file ui-web/src/rpc/generated.ts` reports CRLF on `main` and LF after a forced re-checkout on this branch; - `npm run gen:check` (ui-web): `generated.ts matches the contract (178 methods)`; - `npx vitest run` (ui-web): 108 files, 1836 tests passed; - `npm run --prefix ui-web build && python ui-web/build.py`: `dist/index.html` is LF; - `node ui-web/scripts/check-page.mjs`: `OK (1,482,975 bytes, 3 scripts parse)`; - `node ui-web/scripts/count-shared-globals.mjs`, `check-css.mjs`, `check-class-namespace.mjs`: all OK. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk No runtime behaviour changes for users on Linux or macOS, where the checkout already produced LF. On Windows the working tree stops being rewritten to CRLF, so a contributor with existing local checkouts may see one large renormalization touch on the next checkout; the committed bytes do not move. The built `dist/index.html` is now LF on every platform, which is what the Linux-built wheel already shipped. Rollback is deleting `.gitattributes` and dropping the `newline=""` argument in `ui-web/build.py`. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A --------- Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Summary
A Windows checkout of this repo with the default
core.autocrlf=truerewrites everytext file to CRLF, and two byte-exact gates in
ui-webthen fail on a tree that isbyte-identical to the one CI passes on Linux:
"\n}\n",which never matches a CRLF file, so the sliced function comes out empty and 9 tests
fail with
ReferenceError;gen-rpc-client --checkcompares its own LF output against the file on disk andreports permanent drift in
src/rpc/generated.ts..gitattributespins the checkout filter so the working tree matches what therepository already stores. The repository stores LF today, so this adds no
renormalization diff:
git add --renormalize .over the whole tree stages nothingbeyond the two files in this PR. Only the checkout filter changes.
ui-web/build.pyis the other half of the same defect. It assembled the page underPython's default newline translation, so
dist/index.htmlcame out CRLF on Windowseven from an LF source tree, and
check-page.mjscould no longer extract the scriptpayloads (its
<script>\nanchor needs the LF to be the next byte). That artifactships inside the wheel, so it must not depend on which platform assembled it.
Type
Verification
Run on Windows 11 with
core.autocrlf=true, in a clean worktree cut frommain:git add --renormalize .after the change stages nothing (confirms norenormalization diff);
file ui-web/src/rpc/generated.tsreports CRLF onmainand LF after a forcedre-checkout on this branch;
npm run gen:check(ui-web):generated.ts matches the contract (178 methods);npx vitest run(ui-web): 108 files, 1836 tests passed;npm run --prefix ui-web build && python ui-web/build.py:dist/index.htmlis LF;node ui-web/scripts/check-page.mjs:OK (1,482,975 bytes, 3 scripts parse);node ui-web/scripts/count-shared-globals.mjs,check-css.mjs,check-class-namespace.mjs: all OK.Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
No runtime behaviour changes for users on Linux or macOS, where the checkout already
produced LF. On Windows the working tree stops being rewritten to CRLF, so a
contributor with existing local checkouts may see one large renormalization touch on
the next checkout; the committed bytes do not move. The built
dist/index.htmlis nowLF on every platform, which is what the Linux-built wheel already shipped.
Rollback is deleting
.gitattributesand dropping thenewline=""argument inui-web/build.py.Related Issues
N/A