Validate wheel ABI, linkage, and runtime compatibility - #473
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe release workflow now builds three native wheel targets. Wheel validation parses ELF, Mach-O, and PE binaries, checks ABI exports and linkage, and runs installation smoke tests with NumPy and ChangesRelease wheel validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow as release.yml
participant Verifier as verify_wheel.py
participant Wheel as Release wheel
participant Runtime as Installed runtime
Workflow->>Verifier: Validate wheel platform, ABI, and linkage
Verifier->>Wheel: Inspect native binary
Workflow->>Runtime: Install wheel and run factorize_fixed
Runtime-->>Workflow: Return version and kernel results
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR narrows releases to runner-native targets and adds native wheel architecture, ABI-export, dynamic-linkage, installation, and kernel smoke validation.
Confidence Score: 2/5This PR should not merge until the cross-platform symbol checks, Linux shared-library linkage check, and macOS deployment-version comparison are corrected. The release gate unconditionally sends Mach-O and PE artifacts through an ELF-only symbol validator, rejects normal Linux shared libraries for lacking an executable interpreter segment, and applies the macOS compatibility comparison in the wrong direction. Files Needing Attention: scripts/verify_wheel.py and .github/workflows/release.yml
|
| Filename | Overview |
|---|---|
| scripts/verify_wheel.py | Adds native-format inspection and linkage policies, but contains three release-impacting validation defects across ELF, Mach-O, and PE paths. |
| .github/workflows/release.yml | Narrows the matrix and strengthens smoke checks, but its unconditional flags expose unsupported symbol validation and an invalid Linux PT_INTERP requirement. |
| scripts/verify_ci_workflow.py | Updates workflow invariants to require the narrowed matrix and new verifier options; no independent defect identified. |
| tests/test_verify_wheel.py | Adds header and ELF fixtures, but does not exercise real shared-library PT_INTERP behavior or Mach-O/PE symbol validation. |
| tests/test_verify_ci_workflow.py | Updates the workflow-verifier fixture to match the strengthened release command. |
Reviews (1): Last reviewed commit: "Require runtime verification for publish..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
scripts/verify_wheel.py (2)
970-995: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider catching
struct.errorinmain.The new parsers call
struct.unpack_fromon attacker- or build-controlled offsets. Any residual out-of-range read raisesstruct.error, which theexceptclause at line 996 does not cover. The verifier then exits with a traceback and a nonzero code from an unhandled exception, not the intendedwheel verification failedmessage.♻️ Proposed change
- except (AssertionError, KeyError, zipfile.BadZipFile) as e: + except (AssertionError, KeyError, struct.error, zipfile.BadZipFile) as e:🤖 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/verify_wheel.py` around lines 970 - 995, Update the exception handling in main around verify_wheel to also catch struct.error raised by parser offset reads, producing the existing “wheel verification failed” handling instead of an uncaught traceback. Preserve the current handling for the exceptions already covered.
227-243: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the last symbol entry against a partial read.
symbol_lengthis not required to be a multiple ofentry_size. If the table is ragged, the final iteration callsstruct.unpack_frompast the declared table end and can raisestruct.error.maincatches onlyAssertionError,KeyError, andzipfile.BadZipFile, so the verifier exits with a traceback instead of the intended failure message.♻️ Proposed bound
- for offset in range(symbol_offset, symbol_offset + symbol_length, entry_size): + for offset in range(symbol_offset, symbol_offset + symbol_length - symbol_size + 1, entry_size):🤖 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/verify_wheel.py` around lines 227 - 243, Update the symbol-entry loop around the symbol parsing logic to process only complete entries, ensuring each iteration’s offset plus entry_size does not exceed symbol_offset + symbol_length. Skip or stop before parsing a ragged final entry so struct.unpack_from cannot read beyond the declared symbol table.tests/test_verify_wheel.py (2)
108-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd positive export coverage for the three formats.
Every fixture here is a bare header.
_elf_exported_symbolsreturns early at line 171 because the data is 20 bytes,_macho_exported_symbolsreturns early becausencommandsis 0, and_pe_exported_symbolsreturns early because the data directory is out of range. The tests therefore never execute the section-table, symbol-table, or export-directory paths. Thesh_link/sh_entsizedefect flagged inscripts/verify_wheel.pylines 204-222 is invisible to this suite.Add a fixture that carries a real
SHT_DYNSYMsection plus string table and assert that_require_exported_symbolsacceptsxy_abi_version. Add equivalent Mach-OLC_SYMTABand PE export-directory fixtures.🤖 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 `@tests/test_verify_wheel.py` around lines 108 - 146, Add positive export-symbol coverage for ELF, Mach-O, and PE in the tests around test_native_binary_header_matches_wheel_platform. Build fixtures containing a valid ELF SHT_DYNSYM with its linked string table, a Mach-O LC_SYMTAB with the exported symbol, and a PE export directory exposing xy_abi_version, then call _require_exported_symbols and assert acceptance. Ensure the ELF fixture exercises section-table symbol parsing, including valid sh_link and sh_entsize values, rather than relying on the existing header-only binaries.
174-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name promises dependency-family coverage that the body does not assert.
test_elf_linkage_validates_glibc_floor_and_dependency_familyasserts the glibc floor twice but never asserts a rejection for a dependency outside_ELF_ALLOWED_GLIBC, and never exercises themusllinux_branch. Add a case with an unexpectedDT_NEEDEDname and a case with a musl interpreter, or rename the test to match its scope.🤖 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 `@tests/test_verify_wheel.py` around lines 174 - 184, Expand test_elf_linkage_validates_glibc_floor_and_dependency_family to assert rejection of an unexpected DT_NEEDED dependency and to exercise the musllinux_ interpreter branch, using the existing _elf_linkage_fixture and _require_elf_linkage helpers. If dependency-family and musl coverage are not added, rename the test to describe only glibc-floor validation..github/workflows/release.yml (1)
104-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect the interpreter path explicitly instead of chaining with
||.The
||fallback runs the second interpreter whenever the first command exits nonzero, including when the assertions fail rather than when the interpreter is missing. On Linux and macOS a genuine assertion failure is then reported as a missing./smoke/Scripts/python.exe, which hides the real cause. The 300-character probe is also duplicated verbatim, so the two copies can drift.♻️ Proposed restructure
- name: Verify the wheel installs and loads the native core # Every published wheel is built for the runner architecture and must # pass an import plus representative native-kernel call. shell: bash run: | uv venv smoke uv pip install -p smoke dist/*.whl numpy anywidget "reflex>=0.9.6" - ./smoke/bin/python -c "import importlib.metadata as m, numpy as np, reflex_xy, xy.kernels as k; assert k.BACKEND=='native', k.BACKEND; assert reflex_xy.__version__ == m.version('xy'); codes, unique = k.factorize_fixed(np.asarray(['a', 'b', 'a'], dtype='S1')); assert codes.tolist() == [0, 1, 0] and unique.tolist() == [0, 1]; print('native', k.__file__)" \ - || ./smoke/Scripts/python.exe -c "import importlib.metadata as m, numpy as np, reflex_xy, xy.kernels as k; assert k.BACKEND=='native', k.BACKEND; assert reflex_xy.__version__ == m.version('xy'); codes, unique = k.factorize_fixed(np.asarray(['a', 'b', 'a'], dtype='S1')); assert codes.tolist() == [0, 1, 0] and unique.tolist() == [0, 1]; print('native', k.__file__)" + py=./smoke/bin/python + [ -x "$py" ] || py=./smoke/Scripts/python.exe + cat > smoke_probe.py <<'PY' + import importlib.metadata as m + import numpy as np + import reflex_xy + import xy.kernels as k + assert k.BACKEND == "native", k.BACKEND + assert reflex_xy.__version__ == m.version("xy") + codes, unique = k.factorize_fixed(np.asarray(["a", "b", "a"], dtype="S1")) + assert codes.tolist() == [0, 1, 0] + assert unique.tolist() == [0, 1] + print("native", k.__file__) + PY + "$py" smoke_probe.pyIf you move the probe into a checked-in script such as
scripts/wheel_smoke.py, update the required substrings inscripts/verify_ci_workflow.pylines 1274-1278 and the fixture string intests/test_verify_ci_workflow.pylines 1307-1310.🤖 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 @.github/workflows/release.yml around lines 104 - 108, Update the smoke-test step to select the platform-appropriate interpreter explicitly rather than chaining duplicate probes with ||, so assertion failures remain visible and only the valid executable is invoked. Extract the shared validation into a checked-in script such as scripts/wheel_smoke.py, and update the corresponding required-substring checks in verify_ci_workflow.py and its test fixture to match the new workflow content.scripts/verify_ci_workflow.py (1)
1282-1286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
native: falseguard now watches a matrix key that no step reads.
.github/workflows/release.ymlremoved theif: matrix.nativecondition from the smoke step, somatrix.nativeis unused metadata. This check passes as long as nobody writes the exact textnative: false, and it does not verify that the smoke step actually runs for each entry. A future entry added without thenativekey at all bypasses the guard.Prefer one of two options. Remove the
nativekey from the matrix and this guard, since the smoke step is now unconditional. Or keep the key and assert that the smoke step carries noif:condition.🤖 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/verify_ci_workflow.py` around lines 1282 - 1286, Update the wheels workflow validation around wheels_job and the release matrix so it matches the unconditional smoke step: remove the obsolete native matrix key and the native: false guard, or retain that metadata while validating that the smoke step has no if: condition. Ensure entries missing native cannot bypass the intended smoke-step coverage check.
🤖 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/verify_wheel.py`:
- Around line 970-995: Update scripts/verify_wheel.py at lines 970-995 to catch
struct.error alongside the existing exceptions in main, preserving the standard
verification-failure message and exit code 1. At lines 227-243, bound the ELF
symbol loop to process only complete Elf*_Sym entries when symbol_length is not
a multiple of entry_size. At lines 584-589, validate that len(data) is at least
0x40 before reading e_lfanew at offset 0x3C.
- Around line 137-149: Update the Mach-O endianness selection in the detection
branch around `_macho_exported_symbols` so both big-endian magics,
`\xfe\xed\xfa\xce` and `\xfe\xed\xfa\xcf`, select `">"`, matching
`_require_macho_linkage`; preserve little-endian handling for the corresponding
reversed magics.
- Around line 929-946: Update the wheel verification flow around the
native-library checks to reject require_linkage when expect_platform is None
with an explicit assertion or validation error. Open the ZipFile once, read
native_libs[0] once, and reuse the extracted bytes for _require_native_target,
_require_exported_symbols, and the platform-specific linkage helper while
preserving the existing platform dispatch.
- Around line 428-435: Remove the ELF-only format guard from
_require_exported_symbols and validate required against info.exported_symbols
for ELF, Mach-O, and PE, using the existing _macho_exported_symbols and
_pe_exported_symbols paths. Extend the related tests to cover successful
required-symbol matches for all three formats.
- Around line 204-222: Store section_link and section_entry_size as separate
tuple fields in the sections collection, update its annotation and all
unpacking/indexing in the dynamic-symbol handling flow accordingly, and use
sh_link only for string_index and sh_entsize for entry_size. Add a fixture
containing a valid ELF section header and dynamic symbol table so
_require_exported_symbols validates a genuine binary without the false
invalid-table assertion.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 104-108: Update the smoke-test step to select the
platform-appropriate interpreter explicitly rather than chaining duplicate
probes with ||, so assertion failures remain visible and only the valid
executable is invoked. Extract the shared validation into a checked-in script
such as scripts/wheel_smoke.py, and update the corresponding required-substring
checks in verify_ci_workflow.py and its test fixture to match the new workflow
content.
In `@scripts/verify_ci_workflow.py`:
- Around line 1282-1286: Update the wheels workflow validation around wheels_job
and the release matrix so it matches the unconditional smoke step: remove the
obsolete native matrix key and the native: false guard, or retain that metadata
while validating that the smoke step has no if: condition. Ensure entries
missing native cannot bypass the intended smoke-step coverage check.
In `@scripts/verify_wheel.py`:
- Around line 970-995: Update the exception handling in main around verify_wheel
to also catch struct.error raised by parser offset reads, producing the existing
“wheel verification failed” handling instead of an uncaught traceback. Preserve
the current handling for the exceptions already covered.
- Around line 227-243: Update the symbol-entry loop around the symbol parsing
logic to process only complete entries, ensuring each iteration’s offset plus
entry_size does not exceed symbol_offset + symbol_length. Skip or stop before
parsing a ragged final entry so struct.unpack_from cannot read beyond the
declared symbol table.
In `@tests/test_verify_wheel.py`:
- Around line 108-146: Add positive export-symbol coverage for ELF, Mach-O, and
PE in the tests around test_native_binary_header_matches_wheel_platform. Build
fixtures containing a valid ELF SHT_DYNSYM with its linked string table, a
Mach-O LC_SYMTAB with the exported symbol, and a PE export directory exposing
xy_abi_version, then call _require_exported_symbols and assert acceptance.
Ensure the ELF fixture exercises section-table symbol parsing, including valid
sh_link and sh_entsize values, rather than relying on the existing header-only
binaries.
- Around line 174-184: Expand
test_elf_linkage_validates_glibc_floor_and_dependency_family to assert rejection
of an unexpected DT_NEEDED dependency and to exercise the musllinux_ interpreter
branch, using the existing _elf_linkage_fixture and _require_elf_linkage
helpers. If dependency-family and musl coverage are not added, rename the test
to describe only glibc-floor validation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5010e04b-b00c-4ee4-af95-07c3ac3cdbb7
📒 Files selected for processing (5)
.github/workflows/release.ymlscripts/verify_ci_workflow.pyscripts/verify_wheel.pytests/test_verify_ci_workflow.pytests/test_verify_wheel.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
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 `@scripts/verify_ci_workflow.py`:
- Around line 1290-1297: Update the release wheel validation around
_matrix_include_entries and reject an empty matrix_entries list before
evaluating native flags. Preserve the existing failure for any entry whose
native value is not true, ensuring workflows using unsupported matrix forms
cannot bypass runtime verification.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18a4319a-e011-47ef-b8ac-caee81cd0dc2
📒 Files selected for processing (3)
scripts/verify_ci_workflow.pyscripts/verify_wheel.pytests/test_verify_wheel.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_verify_wheel.py
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/wheel_smoke.py`:
- Around line 12-16: Replace the bare assertions in the smoke-test checks with
explicit conditionals that raise an appropriate exception when the backend,
version, codes, or unique values do not match the expected results. Update the
checks around kernels.BACKEND, reflex_xy.__version__, and factorize_fixed so
validation remains active under optimized Python execution.
In `@spec/process/production-readiness.md`:
- Line 390: Update the release checklist and design-dossier platform matrix to
match release.yml: retain only Linux x86-64, macOS arm64, Windows x64, and the
separate PyEmscripten wheel, and remove aarch64, armv7, musllinux, macOS x86-64,
Windows x86, and Windows arm64 entries.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55d7d25c-508c-4b91-aeca-04acf32fb20f
📒 Files selected for processing (6)
.github/workflows/release.ymlscripts/verify_ci_workflow.pyscripts/verify_wheel.pyscripts/wheel_smoke.pyspec/process/production-readiness.mdtests/test_verify_wheel.py
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/release.yml
- scripts/verify_ci_workflow.py
- scripts/verify_wheel.py
- tests/test_verify_wheel.py
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/verify_ci_workflow.py">
<violation number="1" location="scripts/verify_ci_workflow.py:580">
P1: The release validator can accept a workflow that does not execute `scripts/wheel_smoke.py`: the new check only looks for its text inside active run lines, so an `echo` or other non-invoking command passes. Requiring the smoke command as an exact executable line, preferably through a hard-gated exact-command step or shell control-flow validation, would preserve the native runtime publication gate.</violation>
<violation number="2" location="scripts/verify_ci_workflow.py:1299">
P3: An invalid wheel smoke gate is reported twice because the same `_require_step_run_contains` check is duplicated; removing the second invocation keeps one actionable diagnostic instead of duplicate errors.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1 participant
Fixes #448
Summary
Validate native wheel headers against platform tags.
Verify required xy_abi_version exports.
Inspect Linux, macOS, and Windows dynamic linkage.
Reject unsupported dependencies and ABI mismatches.
Add negative and fixture-based coverage.
Require runtime installation and a representative native kernel call.
Narrow the release matrix to targets that CI can install and execute
reliably.
Update workflow validation to prevent unverified wheel targets.
Testing
Summary by CodeRabbit
Release Improvements
Reliability
Documentation