Skip to content

fix(docs): pin the byte-asserted docs/resources files to LF line endings - #8779

Merged
prekshivyas merged 11 commits into
NVIDIA:mainfrom
VishnuR23:fix/starter-prompt-lf-attribute
Aug 12, 2026
Merged

fix(docs): pin the byte-asserted docs/resources files to LF line endings#8779
prekshivyas merged 11 commits into
NVIDIA:mainfrom
VishnuR23:fix/starter-prompt-lf-attribute

Conversation

@VishnuR23

@VishnuR23 VishnuR23 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

A Windows checkout with core.autocrlf=true rewrote docs/resources/starter-prompt.md to CRLF, and the starter-prompt generator rejects a carriage return, so npm run checks:repository failed before the contributor changed anything. .gitattributes now pins every file whose exact working-tree bytes test/starter-prompt-docs.test.ts asserts, so the checkout keeps the line endings those contracts expect.

Related Issue

Fixes #8648.

Changes

  • Pin docs/resources/starter-prompt.md, docs/resources/prompt-assets/*.md, and docs/resources/local-credential-form.html to text eol=lf, with a comment naming the contracts the rule protects.
  • Add attribute tests to test/starter-prompt-docs.test.ts that resolve the checkout line ending through git check-attr for each pinned path, plus one control for an unpinned file.
  • Document the one-time normalization for a checkout that already holds CRLF, in CONTRIBUTING.md.

The generator keeps its existing carriage-return rejection. This PR changes no runtime source file and no workflow.

Why the Rule Covers More Than the Starter Prompt

The invariant is that a file whose exact bytes are asserted must check out with LF. test/starter-prompt-docs.test.ts asserts exact bytes for four more files: the three prompt assets are byte-compared against their pinned Git blobs, and local-credential-form.html is compared against the SHA-256 that users verify before they run the local credential helper.

Pinning only the starter prompt left those four at eol: unspecified. A Windows contributor would still fail the same suite, and the failure text would tell them to repin the digests to CRLF-corrupted content, which would break the published hash for every user. Removing the rules now fails 5 tests rather than 1.

Verification

Reproduced end to end in a scratch clone with core.autocrlf=true:

Step starter-prompt.md
Clone main CRLF
Clone this branch LF

The bug also survives a pull, which is why the normalization note exists. Starting from a CRLF clone of main and checking out this branch leaves the file CRLF while git status reports no change. Only git rm --cached -r . followed by git reset --hard restores LF. I verified that sequence before documenting it.

Attribute coverage after the change:

docs/resources/starter-prompt.md                eol: lf
docs/resources/prompt-assets/dgx-spark.md       eol: lf
docs/resources/prompt-assets/dgx-station.md     eol: lf
docs/resources/prompt-assets/windows-wsl.md     eol: lf
docs/resources/local-credential-form.html       eol: lf
README.md / package.json / agent-skills.mdx     eol: unspecified

Test behavior:

  • With the rules: Tests 24 passed (24).
  • With the rules removed: Tests 5 failed | 19 passed (24).
  • Under a global core.attributesFile containing * text=auto: 24 passed, because the tests spawn git with GIT_ATTR_NOSYSTEM=1 and an absent core.attributesFile. Without that isolation the result would come from the contributor's own Git configuration rather than from this repository.
  • With * text=auto appended to .gitattributes: 24 passed. The control asserts only the absence of the LF pin, so it does not freeze repository-wide normalization policy.

Each attribute lookup asserts that the path exists first, because git check-attr answers unspecified with exit code 0 for a path that does not exist. A failed git spawn now raises an error naming the path, status, signal, and stderr instead of surfacing expected null to be 0.

npm run docs:check-starter-prompt reports Generated Starter Prompt snippet is current, and npm run checks:repository passes.

I verified this on macOS using scratch clones configured with core.autocrlf=true. I did not run a Windows host.

Note on the Issue Scope

The issue also lists the generated output. .gitignore ignores docs/_build/, and docs/_build/StarterPrompt.generated.mdx is untracked, so an eol=lf attribute there never applies at checkout. I left it out rather than add inert configuration, as described in this issue comment.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Maintainer security review passed for the current branch revision: fix(docs): pin the byte-asserted docs/resources files to LF line endings #8779 (comment)
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

local-credential-form.html is a credential-adjacent asset, but this PR changes none of its bytes. It pins the line endings that keep its published SHA-256 verifiable.

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: docs-updated
  • Evidence: Changed documentation path: CONTRIBUTING.md, which gains a Windows Line Endings subsection under Prerequisites. The review checked the added text against WRITING.md and docs/CONTRIBUTING.md for terminology, structure, voice, and code-sample presentation. It confirmed title case for the heading, one instruction per sentence, the condition stated before the action, the destructive-command warning placed before the command block, and a language-specific bash block holding only command text. It verified the documented normalization sequence in a scratch clone rather than citing it from memory. npm run test:titles:check runs inside npm run checks:repository, which passed.
  • Agent: Codex Desktop

Verification Checklist

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result: npx vitest run --project integration test/starter-prompt-docs.test.ts reports Tests 24 passed (24), and 5 failed | 19 passed when the attribute rules are absent.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for 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 docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Notes on the unchecked verification items:

  • Broad gate: the change adds checkout attributes, tests, and one documentation subsection. It is not a broad runtime, test-harness, or coverage-baseline change, so I ran the targeted file. npm run checks:repository also passed through the pre-commit hook, and the pre-push CLI and plugin type checks passed.
  • Doc items: this PR changes CONTRIBUTING.md at the repository root and adds no page under docs/, so the Fern docs build and page-style items do not apply.

Signed-off-by: Vishnu Rajeev 19866703+VishnuR23@users.noreply.github.com

Summary by CodeRabbit

  • Documentation

    • Added guidance for handling Windows line endings, troubleshooting CRLF issues, and normalizing Git checkouts.
    • Clarified LF line-ending requirements for starter prompts, prompt assets, and local credential forms.
    • Added explanatory comments to relevant repository configuration.
  • Tests

    • Added validation for expected LF line-ending behavior and file presence.
    • Confirmed unrelated documentation remains unaffected.

A Windows checkout with core.autocrlf=true converted
docs/resources/starter-prompt.md to CRLF. The starter-prompt generator
rejects a carriage return, so the repository check failed before the
contributor changed the file.

Fixes NVIDIA#8648
@copy-pr-bot

copy-pr-bot Bot commented Aug 11, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: cc9b1782-0725-4f54-8c2f-50f1c2498537

📥 Commits

Reviewing files that changed from the base of the PR and between 87d1c12 and 97bccf9.

📒 Files selected for processing (1)
  • CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • CONTRIBUTING.md

📝 Walkthrough

Walkthrough

The change adds LF checkout rules for selected prompt assets, verifies the Git attribute contract, and documents Windows line-ending diagnosis and checkout normalization.

Changes

Starter-prompt line endings

Layer / File(s) Summary
LF checkout contract and regression coverage
.gitattributes, test/starter-prompt-docs.test.ts
.gitattributes pins selected prompt assets to LF. Tests verify Git attributes, file existence, command failures, and an unrelated documentation case.
Windows checkout guidance
CONTRIBUTING.md
Contributor guidance explains LF enforcement, CRLF diagnosis, and checkout normalization commands. It warns contributors to protect uncommitted work first.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#8657: This change extends its LF checkout policy to explicit prompt assets and adds matching tests and documentation.

Suggested labels: bug-fix

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the scoped LF line-ending fix for byte-asserted documentation and resource files.
Linked Issues check ✅ Passed The PR preserves LF for the starter prompt, keeps CRLF validation unchanged, and adds regression tests for the Git attribute contract [#8648].
Out of Scope Changes check ✅ Passed The attribute updates, regression tests, and contributor documentation directly support the linked issue and stated LF checkout objectives.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: Review the warnings below.
Findings: 0 blockers · 1 warning · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 1 warning · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Model comparison: normalized findings differ; normalized terminology decisions differ; normalized E2E selections match; Nemotron reported the same number of blockers, 1 fewer warning, the same number of suggestions.

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

2 semantic terminology decisions

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

  • justified — byte-exact contract at test/starter-prompt-docs.test.ts:1343: Keep this term in the test title because it distinguishes the files that require an LF checkout attribute.
  • established — line endings at .gitattributes:9: Keep the established term consistently in the attribute comment, contributor guidance, and test title.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite for the commit under review.

Recommended E2E: None

1 warning · 0 suggestions

Warnings

Warnings do not block.

PRA-1 Warning — Use an attribute source that cannot exist

  • Location: test/starter-prompt-docs.test.ts:1287
  • Category: correctness
  • Problem: The test configures `core.attributesFile` to a predictable path in the shared temporary directory but does not ensure that the path is absent. A file at that path can supply attributes, so the test no longer proves that repository `.gitattributes` alone resolves each LF rule.
  • Impact: A host file at the predictable path can make the regression test pass or fail based on external attributes rather than the repository checkout policy.
  • Recommendation: Create a unique temporary directory for the test attribute source and use a non-existent file inside it, or assert before each lookup that the configured attributes file does not exist.
  • Verification: Inspect the test setup and confirm that `core.attributesFile` always names a non-existent path before `git check-attr` runs.
  • Test coverage: Add a focused test setup assertion that the configured `core.attributesFile` does not exist, then verify the EOL assertions still pass with an existing global attributes file excluded.
  • Evidence: test/starter-prompt-docs.test.ts:1287 defines a fixed `os.tmpdir()` path named `nemoclaw-absent-gitattributes`. test/starter-prompt-docs.test.ts:1295-1308 configures that path as `core.attributesFile` for each `git check-attr` invocation. The test comment states that the result must come from repository `.gitattributes` alone.

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

cv and others added 3 commits August 10, 2026 23:49
… tests

The first revision pinned only starter-prompt.md. The prompt assets and
local-credential-form.html carry the same byte-exact assertions, so an
autocrlf checkout still broke them.

The attribute tests now disable the global and system attribute files,
require each path to exist, report a failed git spawn, and assert the
checkout line ending rather than the .gitattributes spelling.

Document the one-time normalization for a checkout that already holds
CRLF, because a new attribute does not rewrite the working tree.
@VishnuR23 VishnuR23 changed the title fix(docs): pin the starter prompt source to LF line endings fix(docs): pin the byte-asserted docs/resources files to LF line endings Aug 11, 2026
@VishnuR23

Copy link
Copy Markdown
Contributor Author

Thanks for merging main in. I kept that merge and pushed the review fixes on top rather than force-pushing over it.

A self-review found that the first revision was incomplete, so the scope grew:

  • The pin now covers every byte-asserted file, not just the starter prompt. test/starter-prompt-docs.test.ts also byte-compares the three prompt assets against their pinned blobs and checks local-credential-form.html against the SHA-256 that users verify before running the local credential helper. Those four were still eol: unspecified, so a Windows contributor would have hit the same suite failure — and the failure text would have told them to repin the digests to CRLF-corrupted content. Removing the rules now fails 5 tests instead of 1.
  • A new attribute does not rewrite an existing working tree. I confirmed in a scratch clone that cloning main with core.autocrlf=true gives CRLF, checking out this branch leaves it CRLF, and git status reports no change. Only git rm --cached -r . plus git reset --hard restores LF, so CONTRIBUTING.md now documents that sequence with a warning that it discards uncommitted work.
  • The tests no longer depend on the contributor's Git configuration. They spawn git with GIT_ATTR_NOSYSTEM=1 and an absent core.attributesFile. Verified by running the suite under a global * text=auto: plain check-attr reports text: auto, and the suite still passes.
  • The control no longer freezes repository-wide policy. It asserts only the absence of the LF pin, so adding * text=auto later keeps it green. I verified that.
  • Each lookup now requires the path to exist, because git check-attr answers unspecified with exit 0 for a path that does not exist, and a failed git spawn now reports status, signal, and stderr.

One judgement call worth your view: the rule names three paths rather than applying * text=auto repository-wide. Naming them keeps a current consumer for each entry, but say the word if you would rather have the broad rule.

@VishnuR23

Copy link
Copy Markdown
Contributor Author

Flagging a red check that is not caused by this diff.

codebase-growth-guardrails failed in its Check out the trusted base revision step, before it read any file from this branch:

fatal: unable to access 'https://github.com/NVIDIA/NemoClaw/':
server certificate verification failed. CAfile: none CRLfile: none
The process '/usr/bin/git' failed with exit code 128

The step retried three times over about 40 seconds and failed each time with the same TLS error, so it never reached the guardrail logic. The same check passed on the previous push to this branch (run 31359918764). I read it as a transient runner problem rather than a growth-budget violation.

I cannot re-run it, because gh run rerun reports Must have admin rights to Repository for a fork contributor. Please re-run the job when convenient. I would rather not push an empty commit only to re-trigger it, but tell me if you would prefer that.

@VishnuR23

Copy link
Copy Markdown
Contributor Author

I need to disclose a duplicate that I should have found before I opened this.

#8657 targets the same issue and was opened on 2026-08-09T22:09:58Z, about 31 hours before this PR. It was cross-referenced on #8648 at the time, so it was visible when I claimed the issue. I searched the open issues and missed the open pull requests, which CONTRIBUTING.md asks contributors to do. That is my error.

The two differ:

#8657 is both earlier and broader, so I do not think this PR should land as a competing change. I checked whether the tests here transfer: with * text=auto eol=lf in place and none of my targeted rules, all five byte-pinned paths resolve to eol: lf and 23 of the 24 tests pass. The one failure is the control that asserts an unpinned file, which has no meaning under a repository-wide rule and should be dropped rather than adapted.

I offered those tests on #8657. Happy to close this PR in favour of that one, or to keep it open if you would rather compare the two approaches. Your call.

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Security review: PASS

Reviewed the complete PR diff through branch revision 87d1c12b7 against current main (0cddc38c8). The change adds LF checkout attributes, contributor recovery guidance, and repository-contract tests. It does not change runtime code, workflows, permissions, network policy, dependency metadata, or the bytes of the credential-adjacent HTML asset.

Review checklist

  1. Authentication and authorization: No authentication, authorization, identity, or privilege boundary changes.
  2. Input validation and injection: The new test invokes git check-attr with a fixed argument vector and passes repository-owned paths after --; it does not invoke a shell. Test diagnostics do not contain secrets.
  3. Secrets and credentials: No secret, token, key, or credential value is added. docs/resources/local-credential-form.html is not modified; the attribute rule preserves the LF bytes used by its published digest contract.
  4. Cryptography and integrity: No cryptographic implementation changes. The LF rule strengthens integrity by preventing checkout conversion from changing byte-validated resources.
  5. Network and SSRF: No network request, endpoint, proxy, DNS, or SSRF surface changes.
  6. Filesystem and path safety: Attribute patterns are limited to the five byte-validated documentation resources. The test verifies each path exists and isolates repository attributes from contributor global/system configuration.
  7. Sandbox and process safety: No production process execution changes. Test-only spawnSync uses a fixed git executable, a bounded 10-second timeout, a fixed subcommand, and no shell expansion.
  8. Supply chain and dependencies: No dependency, action, image, lockfile, build, or release-provenance changes.
  9. Privacy, logging, and denial of service: No user data or telemetry changes. The bounded local test process and small fixed path set add no meaningful resource-exhaustion risk.

Verification

  • Focused integration suite: 24/24 passed.
  • Repository conditional scan: passed; the changed test adds no if statement relative to main.
  • Test-size, source-shape, formatting, CLI type-check, pre-commit, commit-message, and pre-push gates passed.
  • All eight commits in the PR are signed and appear as Verified in GitHub.
  • Diff size: 3 files, +92/-0. This is not a large LOC increase.

No security findings or security-review blocker remain for this branch revision.

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

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 prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comparator review for issue #8648: neither competing PR is mergeable yet, and #8779 is closer to ready.

  • Tier 0: open, contributor-compliant, and CodeRabbit-clear; fails current CI/branch-protection/refresh gates.
  • Correctness/quality score: 16.0/16.0. The file-specific LF pins match the issue; the generated output is correctly excluded because it is untracked; git check-attr tests fail on the pre-fix contract, cover the byte-pinned resource set, and retain an unaffected-path negative control.
  • #8657 scores 9.0/16.0 because it broadens normalization to every tracked text file and has no checked-in attribute regression.

No supersession declaration or transferred work was found. Refresh this branch onto current main, obtain the full required checks, and make the maintainers' choice between the competing PRs explicit before approval.

@wscurran wscurran added area: ci CI workflows, checks, release automation, or GitHub Actions area: docs Documentation, examples, guides, or docs build chore Build, CI, dependency, or tooling maintenance platform: windows Affects native Windows environments labels Aug 11, 2026

@prekshivyas prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The LF attributes match the exact-byte contracts, the tests isolate repository attributes and cover every pinned path plus an unpinned control, and the contributor recovery guidance warns before the destructive normalization command. CI passes on commit 97bccf9, and GitHub reports no merge conflict.

@prekshivyas
prekshivyas enabled auto-merge (squash) August 12, 2026 04:53
@prekshivyas
prekshivyas merged commit 88eecd8 into NVIDIA:main Aug 12, 2026
50 of 53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ci CI workflows, checks, release automation, or GitHub Actions area: docs Documentation, examples, guides, or docs build chore Build, CI, dependency, or tooling maintenance platform: windows Affects native Windows environments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows checkout converts the starter prompt to CRLF and breaks repository checks

6 participants