Skip to content

Add content seals and validation for generated CUDA Bindings files#2310

Merged
mdboom merged 28 commits into
NVIDIA:mainfrom
rwgk:test_generated_file_seal
Jul 8, 2026
Merged

Add content seals and validation for generated CUDA Bindings files#2310
mdboom merged 28 commits into
NVIDIA:mainfrom
rwgk:test_generated_file_seal

Conversation

@rwgk

@rwgk rwgk commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Regenerate CUDA Bindings with cybind MR 460, adding format 1 content seals to 68 generated Python, Cython, template, and RST files.
  • Add toolshed/check_generated_file_seals.py to validate seal syntax, format, comment style, and the recorded SHA-256 digest.
  • Run the validator from pre-commit for text files under cuda_bindings/.

Each seal contains a machine-readable generated-file marker and a SHA-256 digest of the complete file content excluding the seal line itself. This makes ordinary manual edits detectable while also providing a consistent marker for generated-file discovery. The digest is an integrity check for accidental changes, not a cryptographic attestation of provenance.

Dependency

The generated files in this PR were produced by cybind MR 460, which implements the producer side of the seal format. This PR implements the corresponding CUDA Python consumer-side validation and should remain a draft until that format is finalized.

Relation to PR 2297

PR 2297 proposes removing the license-era CI: Restricted Paths Guard; its discussion also asks what should replace that workflow's incidental protection against manual edits to generated files.

For that generated-file protection, a separate GitHub Actions workflow is unnecessary. This PR puts the check in the repository's existing pre-commit suite. A manual edit changes the content without updating its seal, the seal hook fails, and the required pre-commit CI check blocks the PR from merging. The same rule applies to every contributor, without employee-detection logic or path-specific review labels.

The seal is intentionally an integrity check, not an authorization boundary. If the project retains a separate policy that only NVIDIA employees may modify particular paths, that policy would require separate enforcement; it is not needed to detect ordinary manual changes to cybind-generated files.

Example failure

After manually modifying a sealed generated file, pre-commit reports:

Check generated-file seals...............................................Failed
- hook id: check-generated-file-seals
- exit code: 1

Manual changes detected in 'cuda_bindings/cuda/bindings/cynvrtc.pyx'. It is a generated file and should not be edited directly.
Recorded content SHA-256: 9725757222bfc514253ab6a6113b4e6b1c33fab841fe2fff8ebb1012ecf7715b
Computed content SHA-256: f752f676ca6b4a5a99608c86c4581ca78958a0b0d233b2a931db201bfdc6aefd

Unrelated piggy-backed tiny fix (to avoid making a separate PR)

  • commit d873361: Make the RDC test-binary helper consistently use its resolved NVCC command for both compilation and library creation.

@rwgk rwgk added this to the cuda.bindings next milestone Jul 7, 2026
@rwgk rwgk self-assigned this Jul 7, 2026
@rwgk rwgk added P1 Medium priority - Should do cuda.bindings Everything related to the cuda.bindings module labels Jul 7, 2026
@copy-pr-bot

copy-pr-bot Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@rwgk

rwgk commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test

@github-actions

This comment has been minimized.

@rwgk rwgk marked this pull request as ready for review July 7, 2026 16:30
@rwgk rwgk requested review from leofang and mdboom July 7, 2026 16:31
@mdboom

mdboom commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

I'll come back to review this in a little bit, but let's merge #2316 to catch us up to the main branch of the generator before merging this.

@mdboom mdboom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's improve the message -- the rest is just suggestions.

Also, let's merge #2316 first.

Comment on lines +102 to +103
f"MISMATCHED cybind-generated file seal in {filepath!r}: "
f"recorded {recorded_digest.decode()}, computed {computed_digest.decode()}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This message might be a bit confusing for someone who doesn't understand the motivation here. Maybe:

"Manual changes detected in {filepath!r}. It is a generated file and should not be edited directly."

And then show the hashes if you want (though I don't know how useful they are to the user).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. The failure now explains that a manual change was detected in a generated file and that the file should not be edited directly. I kept the recorded and computed hashes as secondary diagnostic information (e.g. if someone accidentally cuts off characters in the seal line, or parsing the seal line goes subtly wrong, that'd be pretty obvious).

_MARKER_REGEX = re.compile(
rb"^(?P<prefix>#|\.\.) "
+ re.escape(_TOKEN_BYTES)
+ rb" format=(?P<format>[0-9]+); content-sha256=(?P<digest>[0-9a-f]{64})\n$"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the \n$ here means it might fail on Windows where the files could have \r\n line endings. Would this work just as well?

Suggested change
+ rb" format=(?P<format>[0-9]+); content-sha256=(?P<digest>[0-9a-f]{64})\n$"
+ rb" format=(?P<format>[0-9]+); content-sha256=(?P<digest>[0-9a-f]{64})$"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tested this on Windows, and it's actually fine. (However a bunch of other things in our pre-commit hooks are broken on Windows -- I'll file a separate issue for that).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we should keep the explicit \n. Cybind normalizes generated content to LF before sealing and writes the sealed output as bytes, and cuda-python enforces * text eol=lf in .gitattributes.

Also, because the checker uses fullmatch, removing \n would reject the existing LF-terminated marker lines rather than make the expression line-ending agnostic. Supporting CRLF would require a coordinated change to the marker parsing and content-digest normalization, rather than this regex-only change.

_MARKER_REGEX = re.compile(
rb"^(?P<prefix>#|\.\.) "
+ re.escape(_TOKEN_BYTES)
+ rb" format=(?P<format>[0-9]+); content-sha256=(?P<digest>[0-9a-f]{64})\n$"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not strictly necessary because we control both ends, but users would probably expect arbitrary amounts of space between the keys.

Suggested change
+ rb" format=(?P<format>[0-9]+); content-sha256=(?P<digest>[0-9a-f]{64})\n$"
+ rb" *format=(?P<format>[0-9]+) *; *content-sha256=(?P<digest>[0-9a-f]{64})\n$"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to keep one canonical spelling here. We control both the producer and consumer, and this is a new machine-generated format rather than something people are expected to author manually. Strict spacing catches unexpected format drift early and also keeps ad-hoc tooling, including simple grep-based inspection, straightforward and predictable.

Comment thread toolshed/check_generated_file_seals.py Outdated
+ re.escape(_TOKEN_BYTES)
+ rb" format=(?P<format>[0-9]+); content-sha256=(?P<digest>[0-9a-f]{64})\n$"
)
_SOURCE_SUFFIXES = frozenset({".py", ".pxd", ".pxi", ".pyx"})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is basically used to get a comment character for specific file types. To make this more extensible for more filetypes, I'd just be super explicit:

_COMMENT_CHARS = {
   ".py": b"#",
   ".pxd": b"#",
   ".pxi": b"#",
   ".pyx": b"#",
   ".pyx.in": b"#",
   ".pxd.in": b"#",
   ".pxi.in": b"#",
   ".rst": b"..",
   ".c": b"//",
   ".cpp": b"//",
   ".h": b"//",
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

marker_indexes = [index for index, line in enumerate(lines) if _TOKEN_BYTES in line]

if not marker_indexes:
if normalized_path in previously_sealed_paths:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's probably not worth fixing, but worth noting that if a user renames and edits a file in the same commit, it will not get flagged.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As long as the renamed file is staged, a rename followed by an edit is still detected: the seal moves with the file, and the changed content no longer matches its recorded digest.

This pre-commit check is is meant to be an integrity check for ordinary accidental changes. I don't think we should expand the scope beyond that. A rename of a generated file is probably not an accident. Guarding against removing a seal or forging one is also out of scope.


def load_previously_sealed_paths():
process = subprocess.run( # noqa: S603
["git", "grep", "-l", "-I", "-F", GENERATED_FILE_SEAL_TOKEN, "HEAD", "--"], # noqa: S607

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No point in grepping outside of cuda_bindings (since the pre-commit config limits it there as well).

Suggested change
["git", "grep", "-l", "-I", "-F", GENERATED_FILE_SEAL_TOKEN, "HEAD", "--"], # noqa: S607
["git", "grep", "-l", "-I", "-F", GENERATED_FILE_SEAL_TOKEN, "HEAD", "--", "cuda_bindings"], # noqa: S607

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@github-actions github-actions Bot added the cuda.core Everything related to the cuda.core module label Jul 8, 2026
Comment thread cuda_bindings/cuda/bindings/cudla.pyx Outdated

# <<<< PREAMBLE CONTENT >>>>

# CYTHON-BINDINGS-GENERATED-DO-NOT-MODIFY-THIS-FILE: format=1; content-sha256=6e7ac86c22e602c08df8250f3b50c135945378aa8ae4ddb4e10174fd979c4aa5

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Slightly weird placement here (but not worth blocking on).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree this isn't pretty, but yes, it'll be great to keep to ball rolling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

FYI — I'm taking another look, because I want to retest anyway (after merging the cybind mainline into MR 460).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I found a nice solution that's already pushed to MR 460. 6 files are changed here:

While working on that, I discovered that grep -l -F CYTHON-BINDINGS-GENERATED-DO-NOT-MODIFY-THIS-FILE matched toolshed/check_generated_file_seals.py, so I fixed that, too:

I'm holding off triggering the CI again until #2323 is merged.


if [[ "${OS:-}" == "Windows_NT" ]]; then
nvcc -lib -o "${SCRIPTPATH}/saxpy.lib" "${SCRIPTPATH}/saxpy.o"
"${NVCC}" -lib -o "${SCRIPTPATH}/saxpy.lib" "${SCRIPTPATH}/saxpy.o"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems unrelated? Maybe accidentally brought over from another branch?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See my comment under 2281:

#2281 (comment)

It's a really tiny fix, I hope to avoid creating a separate PR just for that by piggy-backing here.

@rwgk rwgk marked this pull request as draft July 8, 2026 16:30
@rwgk rwgk marked this pull request as ready for review July 8, 2026 16:52

@mdboom mdboom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Now that the generator side of this has been merged, we should probably regenerate one more time over here (probably a no-op) and then assuming that's clean, let's merge this.

@mdboom mdboom merged commit dcff8e6 into NVIDIA:main Jul 8, 2026
204 of 206 checks passed
@rwgk rwgk deleted the test_generated_file_seal branch July 8, 2026 21:46
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Doc Preview CI
Preview removed because the pull request was closed or merged.

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

Labels

cuda.bindings Everything related to the cuda.bindings module cuda.core Everything related to the cuda.core module P1 Medium priority - Should do

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants