fix(ci): don't parse guest_launch_measurements for HostOS proposals - #11458
Open
r-birkner wants to merge 2 commits into
Open
fix(ci): don't parse guest_launch_measurements for HostOS proposals#11458r-birkner wants to merge 2 commits into
r-birkner wants to merge 2 commits into
Conversation
repro-check read guest_launch_measurements from the proposal payload before branching on the proposal type. HostOS election proposals don't carry that field, so `repro-check -p <hostos-proposal-id>` died with an uncaught KeyError, which looked like a reproducibility mismatch. Move the payload parsing into parse_proposal_payload(), which reads the measurements only in the GuestOS branch, and detect the proposal type from the payload keys instead of substring-matching the serialized JSON (which also matches *_to_unelect entries). Add a test covering both payload shapes.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
HostOS processing still unconditionally performs the GuestOS measurements comparison with None.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Refactors election proposal parsing to distinguish GuestOS and HostOS payloads safely.
Changes:
- Adds payload-type-specific parsing.
- Corrects measurement return typing and removes unused state.
- Adds parser tests and a Bazel test target.
File summaries
| File | Description |
|---|---|
ci/scripts/repro-check |
Refactors proposal parsing. |
ci/scripts/repro_check_test.py |
Tests payload parsing behavior. |
ci/scripts/BUILD.bazel |
Defines the Python test target. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| measurement["measurement"] = list(bytes.fromhex(measurement["measurement"])) | ||
| return (payload["replica_version_to_elect"], sha256_hex, measurements, None) | ||
| if payload.get("hostos_version_to_elect"): | ||
| return (payload["hostos_version_to_elect"], None, None, sha256_hex) |
run() called compare_proposal_measurements_vs_cdn() unconditionally, and that method only skipped when there was no proposal id. For a HostOS proposal the measurements are None, so it compared the GuestOS CDN measurements against None and then crashed building the mismatch message in format_measurements(None) -- or, with --hostos, read a launch-measurements.json that was never downloaded. Guard on the measurements the same way compare_proposal_vs_cdn() guards on the hashes. Also make parse_proposal_payload() copy the measurements instead of converting them in place, so it doesn't mutate the caller's payload. The previous test only covered the extracted parser, which is why it missed this. Add tests that drive run() end to end for both proposal types with the network and the local build stubbed out; the HostOS one fails with the originally reported KeyError on the pre-fix script.
basvandijk
approved these changes
Sep 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
repro-check -p <proposal-id>crashed with an uncaughtKeyError: 'guest_launch_measurements'on every HostOS version-election proposal.process_proposal()read and transformedguest_launch_measurementsfrom the proposal payload before branching on the proposal type. HostOS election payloads only carryhostos_version_to_elect,hostos_versions_to_unelect,release_package_sha256_hexandrelease_package_urls— no launch measurements. Sincemain()only catchesVerificationErrorandRuntimeError, theKeyErrorsurfaced as a traceback with exit code 1, indistinguishable from a real reproducibility mismatch. As HostOS and GuestOS are elected in separate proposals each release, this affected roughly half of all election proposals.The measurements are only used in the GuestOS return branch anyway, so parsing them up front was both wrong and unnecessary.
Changes:
parse_proposal_payload(), which readsguest_launch_measurementsonly in the GuestOS branch.json.dumps(proposal_data)— the old check also matched versions appearing in*_to_unelectlists.process_proposal()return annotation (the measurements element is adict, not astr) and drop the unusedself.proposal_launch_measurementsattribute.ci/scripts/repro_check_test.py(+ apy_testin a newci/scripts/BUILD.bazel) covering a HostOS payload without measurements, the GuestOS hex→bytes conversion, and the missing-version error.Verified with
bazel test //ci/scripts:repro_check_test,bazel build //ci/scripts:all --nobuild, andbazel run //:buildifier.