Skip to content

fix(toolshed): accept // seals so C/C++ generated files can be sealed - #2539

Open
LeSingh1 wants to merge 1 commit into
NVIDIA:mainfrom
LeSingh1:toolshed-seal-cpp-comment-prefix
Open

fix(toolshed): accept // seals so C/C++ generated files can be sealed#2539
LeSingh1 wants to merge 1 commit into
NVIDIA:mainfrom
LeSingh1:toolshed-seal-cpp-comment-prefix

Conversation

@LeSingh1

@LeSingh1 LeSingh1 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Problem

toolshed/check_generated_file_seals.py declares three comment styles for the seal line, one per generated-file family:

_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"//",
}

and validate_generated_file_seal deliberately compares the seal's captured prefix against expected_comment_prefix(filepath), so a .rst file cannot be sealed with a #:

if match.group("prefix") != expected_prefix:
    print(f"INVALID generated-file seal comment prefix in {filepath!r}")

But the marker regex only accepts two of the three declared prefixes:

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

// can never be captured, so fullmatch returns None for a sealed .c/.cpp/.h file and it is rejected as MALFORMED generated-file seal before the prefix comparison ever runs. The b"//" entries in _COMMENT_CHARS — and the branch that would validate them — are dead.

Reproduced by writing one identical seal three ways and calling validate_generated_file_seal directly:

gen.c      prefix=b'//'  regex_match=False  ->  MALFORMED generated-file seal in '.../gen.c'   False
gen.pyx    prefix=b'#'   regex_match=True   ->  True
gen.rst    prefix=b'..'  regex_match=True   ->  True

Only the prefix differs; the token, format, digest, and body are byte-identical.

The hook runs over ^cuda_bindings/ with types: [text], which already contains C headers (cuda_bindings/cuda/bindings/_lib/param_packer.h), so the first generated .h or .cpp to be sealed would be rejected with a message that points at the file rather than at the checker.

Fix

Add // to the alternation, with a comment tying it to _COMMENT_CHARS so the two do not drift apart again. One line of behavior change; no other logic touched.

Tests

This script had no tests. Added toolshed/tests/test_check_generated_file_seals.py covering the seal round-trip for every entry in _COMMENT_CHARS (parametrized off the dict itself, so a future entry the regex cannot match fails immediately rather than silently becoming dead code), plus the wrong-prefix-for-extension rejection, the tampered-content rejection, the unsupported-extension rejection, and the never-sealed passthrough. They call validate_generated_file_seal(path, set()) directly, so no git subprocess and no CUDA are involved.

The new directory is wired into the existing nightly tooling job next to ci/tools/tests:

python -m pytest -v --noconftest ci/tools/tests toolshed/tests

Verification

All of this was actually executed (pure Python, no GPU):

# with the fix
18 passed

# with toolshed/check_generated_file_seals.py restored from upstream/main
FAILED test_every_declared_comment_prefix_validates[.c-//]
FAILED test_every_declared_comment_prefix_validates[.cpp-//]
FAILED test_every_declared_comment_prefix_validates[.h-//]
FAILED test_marker_regex_accepts_every_declared_prefix[//]
FAILED test_edited_content_is_rejected
5 failed, 13 passed

Combined pytest --noconftest ci/tools/tests toolshed/tests: 74 passed. ruff check / ruff format --check clean on both changed Python files (no new findings vs. an upstream/main baseline), toolshed/check_spdx.py clean on the new file, python -m py_compile clean.

Verified index-safely (cp aside, git show upstream/main:<path> >, run, restore) — no staged reverts.

`check_generated_file_seals.py` declares three comment styles for the seal
line, one per generated-file family:

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

and `validate_generated_file_seal` compares the seal's captured prefix
against `expected_comment_prefix(filepath)` so a `.rst` file cannot be
sealed with a `#`, and so on. But the marker regex only ever accepts two of
the three:

    rb"^(?P<prefix>#|\.\.) "

`//` can never be captured, so `fullmatch` returns None for any sealed
`.c` / `.cpp` / `.h` file and it is rejected as `MALFORMED generated-file
seal` before the prefix comparison runs at all. The `b"//"` entries in
`_COMMENT_CHARS` and the branch that would validate them are dead.

Add `//` to the alternation, with a note tying it to `_COMMENT_CHARS` so
the two do not drift again.

This also adds the first tests for the script, under `toolshed/tests/`, and
runs them alongside the existing `ci/tools/tests` in the nightly tooling
job. The parametrized case is driven from `_COMMENT_CHARS` itself, so a
future entry whose prefix the regex cannot match fails immediately instead
of silently becoming dead code.
@copy-pr-bot

copy-pr-bot Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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.

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

Labels

CI/CD CI/CD infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant