Skip to content

fix(build): preserve LF for shell entrypoints - #1272

Open
xumian520 wants to merge 3 commits into
DeusData:mainfrom
xumian520:fix/wsl-shell-line-endings
Open

fix(build): preserve LF for shell entrypoints#1272
xumian520 wants to merge 3 commits into
DeusData:mainfrom
xumian520:fix/wsl-shell-line-endings

Conversation

@xumian520

Copy link
Copy Markdown

What does this PR do?

Preserve LF checkout semantics for shell entrypoints so Windows clones with core.autocrlf=true can run repository scripts directly from WSL or MSYS. Without this rule, the shebang can be checked out as CRLF and fail with env: $'bash\r': No such file or directory.

This adds a shell line-ending contract test and runs it before the compiler-heavy suite. The test checks all tracked *.sh files and shell hook entrypoints with git check-attr.

Validation

  • bash tests/test_shell_line_endings.sh: 60/60 entrypoints passed
  • bash scripts/build.sh: passed
  • Version smoke test: codebase-memory-mcp dev
  • git diff --check: passed
  • Full suite: 6,614 passed / 6 failed / 4 skipped. The six failures are unrelated WSL/DrvFs/test-infrastructure failures, so this does not claim a fully green local suite.

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test (reproduce-first for bug fixes)

Signed-off-by: xumian520 <126989134+xumian520@users.noreply.github.com>
Signed-off-by: xumian520 <126989134+xumian520@users.noreply.github.com>
Signed-off-by: xumian520 <126989134+xumian520@users.noreply.github.com>
@xumian520
xumian520 requested a review from DeusData as a code owner July 26, 2026 07:11
@DeusData DeusData added the bug Something isn't working label Jul 28, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 28, 2026
@DeusData DeusData added ux/behavior Display bugs, docs, adoption UX windows Windows-specific issues priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jul 28, 2026
@DeusData

DeusData commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Thank you for the contribution and for adding a regression contract around LF shell entrypoints on Windows checkouts. This is now triaged as a high-priority Windows build bug for 0.9.1-rc. Our community PR queue is currently quite full, so it may take a little time before we can complete the review and, if approved, merge it. We are doing our best to support community contributions and will return with code-grounded feedback as capacity opens.

@DeusData

Copy link
Copy Markdown
Owner

Thank you — this is a good fit and I would like to merge it. Three small things first, and one of them is our fault rather than yours.

What I verified: the eol=lf attribute is the right mechanism (it forces LF on checkout, which is exactly what a shell entrypoint needs), both scripts/git-hooks/commit-msg and scripts/hooks/pre-commit exist on main so the explicit attribute lines bind to real files, and no tracked shell blob currently contains CRLF — so renormalisation is a no-op and this will not produce phantom-diff churn. The test genuinely fails without the .gitattributes hunk, so it binds. It also protects a failure mode we have actually been bitten by, where a stray CR makes a script unrunnable.

1. Needs a rebase (ours). main has since taken "Step 0h: smoke release-fixture contract" in scripts/test.sh, so your Step 0h collides. Renumber to 0i on rebase.

2. Assert the loop saw something. If git ls-files returns nothing — a non-git context, a stripped export — the loop passes vacuously and reports "0 files". A checked > 0 assertion turns that from a silent pass into a real failure. We have been burned by vacuous greens before, so this one matters more than its size suggests.

3. Optional, your call: the test asserts the attribute, not the bytes. A CRLF blob committed before renormalisation would still pass. Grepping the blobs for a literal CR would close that, but the attribute check is the important half and I would not block on it.

On cost, since this touches every test leg: it adds one cheap deterministic check to step 0 — negligible runtime, no flake surface, no change to trigger scope. That is the right kind of contract to add.

@DeusData

Copy link
Copy Markdown
Owner

Reviewed alongside #1314, which fixes the same class of bug — and yours is the one we want to take. It needs a rebase first.

Why yours. Both PRs carry the identical core line (*.sh text eol=lf), but you went further in two ways that matter:

  1. You found the complete entrypoint set, not just the obvious glob. I verified this by shebang-scanning every tracked non-.sh file on main: scripts/git-hooks/commit-msg and scripts/hooks/pre-commit are the only two extensionless shell entrypoints in the tree, and your rules cover both. That is not a hypothetical gap — a Windows contributor whose commit-msg DCO hook checks out with CRLF hits exactly this bug, and the *.sh glob alone would never have caught it.
  2. You shipped a regression guard. tests/test_shell_line_endings.sh follows our step-0 contract convention exactly, and it is dynamic over git ls-files, so every future shell script is covered automatically and it fails closed if the attribute is ever dropped. That is precisely how we like regressions pinned here — the one-line fix without the guard is something anyone could silently delete later.

The implementation is clean: pure git plumbing, no dependencies, and the NUL-delimited check-attr -z parse is correct. I also confirmed the premise is real — main's .gitattributes currently has only linguist/diff attributes for vendored grammars, with zero eol/text rules — and that this lands with zero renormalization churn, since no tracked .sh file contains CRLF in the index today.

What is needed: a rebase.

main has grown Steps 0h through 0j since you branched (smoke-fixture, parallel-harness and venue-parity contracts), so your "Step 0h" slot and its surrounding context now collide. Renumbering to Step 0k should resolve it. The .gitattributes hunk still applies cleanly. Your CI is currently green but on a stale base, so it needs re-earning on the rebased head.

Three smaller things while you are in there, none blocking:

  • Commit 1d611bd wires scripts/test.sh to run the contract before 2c4803a adds the test file, so scripts/test.sh is red at that intermediate commit. Harmless under our merge-commit policy, but it leaves a non-bisectable point in history.
  • The guard asserts the attribute rather than the file bytes. A blob committed with CRLF before the attribute existed would still pass. That is sound today (I verified the index is CRLF-free), but a one-line git grep -I $'\r' -- '*.sh' byte check would make it airtight.
  • The .gitattributes comment says the entrypoints "must stay executable" — that is about the exec bit, not line endings. Worth rewording now that we have a separate exec-bit contract.

Rebase and ping me; I would like to land this.

For the record, I have told @mlandolfi90 on #1314 that yours is the superset we intend to take, and credited them properly — their bug report is what made this concrete, with the exact failure signature from a real fresh Windows clone.

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

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. ux/behavior Display bugs, docs, adoption UX windows Windows-specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants