Repository navigation
build(bazel): model PyO3/napi cdylibs and packaging handoff (#7) - #422
Conversation
Wire rust_shared_library targets for Python and Node bindings, map the CLI lib as a link dependency, and assemble CI smoke packages from Bazel natives without maturin/napi recompile. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
core
WalkthroughThe PR adds Bazel targets for PyO3 and napi-rs cdylibs, updates project-skills embedding for Bazel build scripts, introduces Bazel-only Python and Node package assembly, and adds smoke and classification tests. ChangesBazel binding build graph
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant Bazel
participant BindingCdylibs
participant PackageAssembler
participant SmokeArtifacts
Bazel->>BindingCdylibs: Build Python and Node cdylibs
BindingCdylibs->>PackageAssembler: Pass native libraries and package sources
PackageAssembler->>SmokeArtifacts: Create wheel and Node archive
PackageAssembler->>SmokeArtifacts: Write native evidence
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
scripts/ci/assemble_bazel_binding_packages.py (2)
34-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
_dieasNoReturn.
_diealways raisesSystemExit, but the annotation says-> None. Type checkers then treat line 77 in_napi_platform_tagand line 85 in_read_versionas paths that fall through and returnNone, which conflicts with their declared-> str. Change the annotation so callers narrow correctly.♻️ Proposed annotation fix
-from pathlib import Path +from pathlib import Path +from typing import NoReturn-def _die(message: str, code: int = 2) -> None: +def _die(message: str, code: int = 2) -> NoReturn: print(f"assemble_bazel_binding_packages: {message}", file=sys.stderr) raise SystemExit(code)🤖 Prompt for 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. In `@scripts/ci/assemble_bazel_binding_packages.py` around lines 34 - 36, Update the return annotation of _die to NoReturn and import NoReturn from the appropriate typing module, so type checkers recognize every call as non-returning and preserve the declared str return paths in _napi_platform_tag and _read_version.
61-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider deriving the platform tag from the Bazel target platform.
_napi_platform_tagreads the host platform throughplatform.system()andplatform.machine(). The wheel tag selection at lines 126-137 uses the same host values. Inside a Bazel action, the host is the executor, not necessarily the target platform of therust_shared_library. Under remote execution or cross-compilation, the addon name and the wheel tag can disagree with the cdylib bytes that were actually packaged. Nothing in the script cross-checks the tag againstnative.The PR objectives defer the cross-platform matrix to
#6, and the comment at lines 124-125 records the scope. So this is acceptable for the smoke path. When#6lands, pass the target triple in as an explicit flag instead of detecting it.🤖 Prompt for 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. In `@scripts/ci/assemble_bazel_binding_packages.py` around lines 61 - 77, Keep the current host-platform detection in _napi_platform_tag and the wheel tag selection unchanged for this smoke-path scope. Do not add Bazel native-platform derivation or cross-compilation handling; when cross-platform support is implemented in `#6`, replace host detection with an explicit target-triple flag.scripts/ci/test-assemble-bazel-binding-packages.py (2)
51-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage through the Bazel genrule surface.
These tests call
assemble_pythonandassemble_nodedirectly. They prove the Python functions behave correctly. They do not exercise the genrule shell in tools/bazel/bindings/BUILD.bazel, which selects the native artifact at lines 26 and 50, derivesPKG_ROOTat lines 28 and 52, and passes--write-evidence. Every defect in that shell layer stays invisible to this suite.The
build_testat tools/bazel/bindings/BUILD.bazel lines 7-13 covers only the cdylib targets, not the packaging genrules. Add a Bazel test that builds//:python_wheel_smokeand//:node_package_smokeand asserts the resulting artifacts, including the evidence JSON once it is a declared output.As per coding guidelines: "Logical plans and wrapper tests are not sufficient end-to-end proof; validate behavior through the real execution surface when required."
🤖 Prompt for 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. In `@scripts/ci/test-assemble-bazel-binding-packages.py` around lines 51 - 93, Add a Bazel-level test targeting the packaging genrules `//:python_wheel_smoke` and `//:node_package_smoke`, rather than only invoking `assemble_python` and `assemble_node` directly. Declare the generated evidence JSON as an output where necessary, then inspect both produced archives and verify their native payloads, required package files, and `recompiled` evidence through the actual genrule execution path.Source: Coding guidelines
23-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the no-recompile guard tests.
These two tests do not pin down the central safety property of this PR.
test_main_refuses_recompile_looking_argvasserts only that the exit code is 2.argparsealso exits with code 2 for unrecognized arguments. The trailingmaturin buildtokens at lines 45-46 are unrecognized positionals. The test therefore passes even if_assert_no_recompile_argsis removed frommainentirely. Assert the stderr message instead, so the test distinguishes the guard fromargparse.
test_forbidden_recompile_pattern_catches_tool_invocationsasserts only positive matches. A regex that degraded to match everything would still pass. Add a negative case.💚 Proposed assertions
def test_forbidden_recompile_pattern_catches_tool_invocations(self) -> None: for token in ( "maturin build --release", "maturin develop -m x", "napi build --platform", "cargo build -p graphforge-bindings-py", "cargo rustc -p graphforge-bindings-node", ): self.assertIsNotNone(FORBIDDEN_RECOMPILE.search(token), token) + for token in ( + "--language python --native x.so --package-root . --out out.whl", + "maturin upload dist/*", + "cargo metadata --format-version 1", + ): + self.assertIsNone(FORBIDDEN_RECOMPILE.search(token), token) def test_main_refuses_recompile_looking_argv(self) -> None: - with self.assertRaises(SystemExit) as raised: + with self.assertRaises(SystemExit) as raised, contextlib.redirect_stderr(io.StringIO()) as err: main( [ "--language", "python", "--native", "x.so", "--package-root", ".", "--out", "out.whl", "maturin", "build", ] ) self.assertEqual(raised.exception.code, 2) + self.assertIn("silent native recompile", err.getvalue())Add the supporting imports:
+import contextlib +import io import jsonAs per coding guidelines: "Keep real evidence: do not lie, skip tests, weaken assertions, or claim green checks without running the relevant checks."
🤖 Prompt for 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. In `@scripts/ci/test-assemble-bazel-binding-packages.py` around lines 23 - 49, Strengthen test_forbidden_recompile_pattern_catches_tool_invocations with representative safe-token negative cases asserting FORBIDDEN_RECOMPILE does not match, while retaining the existing positive cases. In test_main_refuses_recompile_looking_argv, assert the captured stderr contains the guard’s specific recompile-rejection message in addition to exit code 2, distinguishing _assert_no_recompile_args from argparse failures; add only the supporting imports needed to capture stderr.Source: Coding guidelines
tools/bazel/bindings/BUILD.bazel (1)
26-29: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHarden the native-artifact selection and the interpreter choice.
Two points on this command:
- Line 26 filters
$(locations ...)by extension and takeshead -n1. If therust_shared_librarytarget reports more than one matching file, the genrule silently packages the first one. Assert that exactly one file matches, so an unexpected artifact set fails the build instead of producing a package built from an arbitrary choice.- Line 29 runs the system
python3fromPATH. The action result then depends on the executor's interpreter. The comment in scripts/ci/BUILD.bazel line 14 records that this avoidsrules_python, so the tradeoff is deliberate. Note that the same interpreter decides whethertomllibis available for the_read_versionfix proposed on scripts/ci/assemble_bazel_binding_packages.py.The node genrule at line 50 uses the same
head -n1pattern.♻️ Proposed strict selection
-NATIVE=$$(echo $(locations //crates/graphforge-bindings-py:graphforge_bindings_py) | tr ' ' '\\n' | grep -E '\\.(so|dylib|dll)$$' | head -n1) +NATIVE_MATCHES=$$(echo $(locations //crates/graphforge-bindings-py:graphforge_bindings_py) | tr ' ' '\\n' | grep -E '\\.(so|dylib|dll)$$') +if [ "$$(printf '%s\\n' "$$NATIVE_MATCHES" | wc -l)" -ne 1 ]; then + echo "expected exactly one python cdylib, got: $$NATIVE_MATCHES" >&2 + exit 1 +fi +NATIVE="$$NATIVE_MATCHES"As per coding guidelines: "Fix root causes; never hide failures with skips, retries, sleeps, blanket ignores, fallback behavior, or weakened assertions."
🤖 Prompt for 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. In `@tools/bazel/bindings/BUILD.bazel` around lines 26 - 29, Harden native artifact selection in both the Python and node genrules by validating that the extension-filtered locations contain exactly one file before assigning it, and fail on zero or multiple matches instead of using head -n1. Update the interpreter invocation for assemble_bazel_binding_packages_py to use a declared, deterministic Python executable rather than PATH-based python3, while preserving the required tomllib support and the intentional rules_python tradeoff.Source: Coding guidelines
🤖 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 `@scripts/ci/assemble_bazel_binding_packages.py`:
- Around line 123-137: Update the wheel-tag selection block in the assembly flow
to avoid retaining the initial "py3-none-any" value when no supported host
branch matches. After the platform checks, fail through the existing _die
mechanism, matching _napi_platform_tag’s unsupported-host behavior, while
preserving the platform-specific tags for recognized Linux, Darwin, and Windows
hosts.
- Around line 165-178: Update the RECORD generation in the wheel assembly loop
to encode each SHA-256 raw digest with unpadded URL-safe Base64, replacing
hashlib.sha256(data).hexdigest() with the required digest conversion before
building record_lines. Also replace the record_body list concatenation with list
unpacking using [*record_lines, ...] to satisfy RUF005.
In `@tools/bazel/bindings/BUILD.bazel`:
- Around line 23-34: Declare both evidence JSON files as outputs of their
respective genrules: add python-bazel-native-evidence.json to python_wheel_smoke
at tools/bazel/bindings/BUILD.bazel lines 23-34, and
node-bazel-native-evidence.json to node_package_smoke at lines 47-58. Keep the
existing --write-evidence paths unchanged so Bazel tracks and preserves each
generated artifact.
---
Nitpick comments:
In `@scripts/ci/assemble_bazel_binding_packages.py`:
- Around line 34-36: Update the return annotation of _die to NoReturn and import
NoReturn from the appropriate typing module, so type checkers recognize every
call as non-returning and preserve the declared str return paths in
_napi_platform_tag and _read_version.
- Around line 61-77: Keep the current host-platform detection in
_napi_platform_tag and the wheel tag selection unchanged for this smoke-path
scope. Do not add Bazel native-platform derivation or cross-compilation
handling; when cross-platform support is implemented in `#6`, replace host
detection with an explicit target-triple flag.
In `@scripts/ci/test-assemble-bazel-binding-packages.py`:
- Around line 51-93: Add a Bazel-level test targeting the packaging genrules
`//:python_wheel_smoke` and `//:node_package_smoke`, rather than only invoking
`assemble_python` and `assemble_node` directly. Declare the generated evidence
JSON as an output where necessary, then inspect both produced archives and
verify their native payloads, required package files, and `recompiled` evidence
through the actual genrule execution path.
- Around line 23-49: Strengthen
test_forbidden_recompile_pattern_catches_tool_invocations with representative
safe-token negative cases asserting FORBIDDEN_RECOMPILE does not match, while
retaining the existing positive cases. In
test_main_refuses_recompile_looking_argv, assert the captured stderr contains
the guard’s specific recompile-rejection message in addition to exit code 2,
distinguishing _assert_no_recompile_args from argparse failures; add only the
supporting imports needed to capture stderr.
In `@tools/bazel/bindings/BUILD.bazel`:
- Around line 26-29: Harden native artifact selection in both the Python and
node genrules by validating that the extension-filtered locations contain
exactly one file before assigning it, and fail on zero or multiple matches
instead of using head -n1. Update the interpreter invocation for
assemble_bazel_binding_packages_py to use a declared, deterministic Python
executable rather than PATH-based python3, while preserving the required tomllib
support and the intentional rules_python tradeoff.
🪄 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: Pro
Run ID: 97cbd255-caa0-4010-920c-be868fd22319
⛔ Files ignored due to path filters (3)
.github/workflows/test.ymlis excluded by!**/.github/**docs/development/bazel-bootstrap.mdis excluded by!**/*.md,!**/docs/**docs/development/bazel-migration-ledger.mdis excluded by!**/*.md,!**/docs/**
📒 Files selected for processing (14)
BUILD.bazelMODULE.bazelcrates/graphforge-bindings-node/BUILD.bazelcrates/graphforge-bindings-py/BUILD.bazelcrates/graphforge-cli/BUILD.bazelcrates/graphforge-cli/build.rsproject-skills/BUILD.bazelscripts/ci/BUILD.bazelscripts/ci/assemble_bazel_binding_packages.pyscripts/ci/classify-changes.shscripts/ci/test-assemble-bazel-binding-packages.pyscripts/ci/test-classify-changes.shtools/bazel/bindings/BUILD.bazeltools/bazel/gf_rust.bzl
Move the skill filegroup to the workspace root and satisfy ruff on the packaging handoff scripts so Python binding parity stays byte-identical. Co-authored-by: Cursor <cursoragent@cursor.com>
Fail closed on unsupported wheel hosts, encode RECORD digests per PEP 427, and declare packaging evidence JSON as Bazel genrule outputs. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
rules_rust(rust_shared_library) plus the CLI lib/build.rslink dependency required to compile them.//:python_wheel_smoke,//:node_package_smoke) that assembles CI smoke packages from Bazel-built natives without invokingmaturin build/napi build/cargorecompile.Bazel Bootstrapto build binding cdylibs and run packaging unit tests.Test plan
bazelisk build //crates/graphforge-cli:graphforge_clibazelisk build //:binding_cdylibs //:python_wheel_smoke //:node_package_smokepython3 scripts/ci/test-assemble-bazel-binding-packages.pyscripts/ci/test-classify-changes.shpython3 scripts/ci/cargo-bazel-drift-check.pycargo check -p graphforge-cli --libBazel Bootstrap+CI Gategreen on exact head SHAFixes #7
Made with Cursor
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Note
Add Bazel targets for PyO3/napi cdylibs and packaging handoff scripts
gf_rust_shared_libraryandgf_cargo_build_scriptmacros in gf_rust.bzl for consistent cdylib and build-script definitions across crates.rust_shared_librarytargets for the Python (PyO3) and Node (napi) binding crates, with platform-specific linker flags for macOS dynamic lookup on the Python side.python_wheel_smokeandnode_package_smokegenrules in tools/bazel/bindings/BUILD.bazel to produce smoke packages and evidence JSON from Bazel outputs.GRAPHFORGE_PROJECT_SKILLS_MANIFESTenv var for Bazel runfiles and copies skill files intoOUT_DIRfor embedding.Macroscope summarized f83559a.