fix(checks): give every tracked text file LF on checkout - #8657
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe repository now normalizes tracked text files to LF endings and documents Windows checkout recovery steps. ChangesLF line-ending enforcement
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 367: Update the Windows line-ending remediation instructions in
CONTRIBUTING.md to require contributors to commit or stash local changes, or
verify that the working tree is clean, before running git reset --hard; preserve
the existing cache removal and reset steps.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 17df4ffd-4a4c-4acf-95a5-e3ee8722b311
📒 Files selected for processing (3)
.gitattributesCONTRIBUTING.mdtest/checkout-line-endings.test.ts
cv
left a comment
There was a problem hiding this comment.
Please remove the test (too much IO for such a simple config change) and fix the CodeRabbit requests.
d71be2e to
e8226b0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
e8226b0 to
1280551
Compare
A Windows checkout with core.autocrlf=true rewrites tracked text files to CRLF. Repository checks then fail before a contributor changes a file. The starter-prompt generator rejects a carriage return, and these compare a tracked file byte-for-byte against a pinned digest: - the credential-helper pin - the pinned prompt assets - the Hermes image layout - the reviewed MCP runtime bundle - the reviewed npm lockfiles - the pinned workflow actions and scripts - the Deep Agents Code and E2E image fixtures Set text=auto eol=lf for every path instead of naming files, so a file added later is covered too. text=auto keeps Git's binary detection, so Git does not convert the 110 binary files. No tracked file holds CRLF or mixed line endings today, so this normalizes no content. Git converts a file when it copies that file out of the index, not when .gitattributes changes, so CONTRIBUTING.md gives an existing Windows checkout the fix. Fixes NVIDIA#8648 Signed-off-by: harjoth <harjoth.khara@gmail.com>
1280551 to
8b5b544
Compare
|
Done both. Removed Took CodeRabbit's suggestion on the CONTRIBUTING.md note, plus four words: The diff is 9 lines across |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Security reviewVerdict: PASS — no findings. I reviewed branch revision
The change is small and does not require a large-LOC flag. Both commits are signed off, and GitHub marks them Verified. |
|
Apologies — I opened #8779 against the same issue without noticing this PR first. Yours came 31 hours earlier and is broader, so I have said on mine that it should not land as a competing change. Offering something concrete instead. #8779 carries regression tests for the checkout contract, and I checked that they work against your rule rather than mine. With What transfers:
What does not transfer: my sixth test asserts that a file without a byte-exact contract stays unpinned. That has no meaning under a repository-wide rule, so it should be dropped rather than adapted. Take any of it if useful — copy it directly, or tell me and I will open a PR against your branch. No attribution needed. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
The latest PR commit removes the requested test and retains the requested data-loss warning and worktree checks.
|
Merge-train comparison for #8648 Both PRs remain blocked on a maintainer policy choice:
Neither PR has approval or all required checks. I will not close either PR until a maintainer selects the policy. The unselected PR can then close with this comparison and the selected replacement as evidence. |
prekshivyas
left a comment
There was a problem hiding this comment.
Comparator review for issue #8648: neither competing PR is mergeable yet; #8779 is closer to the accepted issue scope.
#8657's repository-wide * text=auto eol=lf policy does fix the reported source file, but it expands checkout behavior for every tracked text file and carries no checked-in git check-attr regression. Score: 9.0/16.0. #8779 uses file-specific pins, explains the untracked generated output, and provides positive/negative resolved-attribute tests; score: 16.0/16.0.
Both PRs are behind current main, missing the current required checks, and still need branch-protection approval. No supersession declaration or transferred work was found. Please obtain an explicit maintainer scope choice and converge on one PR rather than advancing both.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> # Conflicts: # .gitattributes
prekshivyas
left a comment
There was a problem hiding this comment.
Blocking: the new global .gitattributes rule, * text=auto eol=lf, contradicts test/starter-prompt-docs.test.ts, which still asserts that docs/resources/agent-skills.mdx does not resolve to eol: lf. The applicable test path was skipped in required CI, so green status does not validate this behavior. Please update the obsolete assertion and run the full affected test.
|
Supersession evidence is now conclusive:
I am not resolving the conflict or changing this branch. PR #8779 is the implemented replacement. This PR is being left open for a human maintainer to close. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Dismissed as stale at b1bee67. The obsolete negative eol assertion is now a positive checkout contract test for LF, and the normal pre-commit checks passed.
cv
left a comment
There was a problem hiding this comment.
Approved at b1bee67. All 39 exact-commit checks pass. The LF checkout contract test replaces the obsolete negative assertion, the advisor reports no blocking findings, and no unresolved review thread remains. Current main is newer but non-overlapping, so the requested branch policy does not permit an unnecessary main merge.
Summary
A Windows checkout with
core.autocrlf=truerewrites tracked text files to CRLF, so repository checks fail before a contributor changes anything..gitattributessets no line-ending rule, so Git converts every text file in the repository.This sets
* text=auto eol=lf, which gives every tracked text file LF on every checkout.Related Issue
Fixes #8648
Changes
.gitattributes:* text=auto eol=lf.CONTRIBUTING.md: how to re-check-out an existing Windows clone, next to the existingcore.hooksPathnote. Git converts a file when it copies that file out of the index, not when.gitattributeschanges, so a clone made before this rule keeps its CRLF bytes.The current-main merge updates
test/starter-prompt-docs.test.tsto verify the repository-wide LF attribute contract.Why a repository-wide rule and not per-file rules
The issue proposes a rule for the starter-prompt source. That file is one of many. These checks all compare a tracked file byte-for-byte against a pinned digest, or reject a carriage return:
scripts/generate-starter-prompt.mts:19rejects any carriage return.scripts/checks/local-credential-helper-pin.mts:66compares the SHA-256 of the working-tree bytes.test/starter-prompt-docs.test.ts:731byte-compares each prompt asset with its pinned Git blob.test/hermes-final-image-layout.test.ts:502compares 16 SHA-256 digests with the valuesagents/hermes/Dockerfiledeclares.test/mcp-tool-discovery-image-contract.test.ts:306pins four reviewed runtime bundle digests.There are more.
scripts/audit-reviewed-npm-graph.mts:265digests reviewed npm lockfiles,tools/e2e/workflow-boundary-policy.mts:4pins workflow action and script content, andtest/e2e-fixture-dependency-review.test.tsandtest/langchain-deepagents-code-image.test.tspin image fixtures. The Verification section shows those failing on a CRLF checkout too.An enumerated list was written first and reviewed; the review found 18 files the list had missed. A list that must name every pinned file is wrong the moment someone pins a new one, and a check built on that list cannot detect the omission. One rule covers all of them and every future one.
Why this is safe
text=autokeeps Git's own binary detection.git ls-files --eolreports the same 110 binary files before and after the rule, so no binary file is touched.git ls-files --eolreports 0i/crlfand 0i/mixed. The rule therefore normalizes no content and produces no renormalization commit.linguist-generated,diff=markdown, and-diffrules still resolve.git check-attrconfirms this for the bundle and skills paths.One behavior change worth naming:
scripts/bootstrap-windows.ps1andtools/wsl/ci-helper.ps1will check out with LF on Windows instead of CRLF. Both are already LF in the index and on every Linux and macOS checkout today. PowerShell runs LF scripts.Type of Change
Quality Gates
CONTRIBUTING.mdgains the existing-Windows-clone remediation. Nodocs/page changes: no public API, CLI, configuration, default, error, or product behavior changes, and no tracked content changes.Documentation Writer Review
docs-updatedCONTRIBUTING.mddocuments the existing-Windows-clone remediation and requires a clean worktree beforegit reset --hard..gitattributespreserves Git binary detection while assigning LF to tracked text.test/starter-prompt-docs.test.tsverifies the repository-wide attribute contract. No public docs page changes because this affects contributor checkout behavior.Verification
Two real clones with
core.autocrlf=true, one at base8096cdd7band one at this commit.Tracked files that check out with CRLF:
Repository checks:
Pinned-digest suites in the same two trees,
--no-file-parallelism:Every one of the 27 failures is fixed, and none fails in both trees. They include
keeps security entrypoint hashes synchronized with the copied files,pins the reviewed image runtime artifacts exactly,keeps local prompt assets byte-aligned with their pinned revision blobs (#6990), andrejects a cache seed that does not match the lockfile integrity.On the rebased tree, every tracked text file resolves the rule:
Index forms are unchanged by the rule: 5140
i/lf, 110i/-text, 3i/none, 3 empty.npm run checks:repository,npm run typecheck:cli,npx @biomejs/biome check, andnpm run docsall pass.Limits, stated plainly:
The Windows behavior is proven by a
core.autocrlf=truecheckout on macOS, not on a Windows host.core.autocrlfis the setting that performs the conversion and it behaves the same on every platform.An existing Windows working tree that already holds CRLF is not rewritten by pulling this commit.
CONTRIBUTING.mdgives the fix, but the commit cannot repair a working tree by itself..github/workflows/wsl-e2e.yaml:33and.github/workflows/platform-vitest-main.yaml:174already setcore.autocrlf false. Those lines are now redundant. They are left in place because they are harmless and removing them is a separate decision.PR description includes a
Signed-off-by:line and every commit appears asVerifiedin GitHubNormal
pre-commit,commit-msg, andpre-pushhooks passedTargeted behavior tests pass for the current change set — the normal pre-commit hook passed repository checks after the attribute-contract test update.
Applicable broad gate passed —
npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:Quality Gates section completed with required justifications or waivers
No secrets, API keys, or credentials committed
npm run docsbuilds without warnings — the build completed with 0 errors and 2 existing Fern warningsDoc pages follow the style guide (doc changes only)
New doc pages include SPDX header and frontmatter (new pages only)
Signed-off-by: Harjoth Khara harjoth.khara@gmail.com
Summary by CodeRabbit
Documentation
Chores