ci: deterministic capnp codegen via pinned Docker image + drift check#132
Merged
Conversation
Add a check_capnp_generated job that regenerates the committed capnp code and fails if it drifted from the schema (the build no longer regenerates it). The capnpc-rust plugin is pinned to the exact version (=0.23.2, matching the capnp runtime) because the generated bytes depend on the plugin version and a floating range would yield spurious diffs. Regenerate hypersync_net_types_capnp.rs with capnpc 0.23.2 so the committed code matches the pinned plugin; the previous file predated the 0.23 codegen (panic! helper calls). Document the required toolchain in the Makefile. Co-authored-by: claude <noreply@anthropic.com>
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a Dockerized Cap'n Proto Rust codegen (Dockerfile + Makefile), a ChangesCap'n Proto Generated Code Validation
sequenceDiagram
participant GitHubActions
participant Makefile
participant DockerBuild
participant DockerContainer
GitHubActions->>Makefile: make -C hypersync-net-types generate_capnp_types
Makefile->>DockerBuild: docker build (capnp-codegen.Dockerfile)
DockerBuild->>DockerContainer: run build stage (install capnproto, rustfmt, capnpc)
DockerContainer->>DockerContainer: capnp compile hypersync_net_types.capnp -> src/__generated__
DockerContainer->>DockerContainer: rustfmt generated file
DockerContainer->>Makefile: export generated files to repo via buildkit --output
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Co-authored-by: claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)
78-99: ⚡ Quick winScope job token permissions explicitly.
This job only reads repo contents; relying on default token permissions is broader than needed. Add a job-level
permissions: { contents: read }.Suggested patch
check_capnp_generated: runs-on: ubuntu-latest + permissions: + contents: read steps:🤖 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/ci.yaml around lines 78 - 99, The CI job "check_capnp_generated" currently uses default token permissions; scope it down by adding a job-level permissions entry to only allow repository content read access. Update the job definition for check_capnp_generated to include permissions: { contents: read } (so the job still can read the repo for git diff but cannot perform broader actions), ensuring the permissions block is at the same indentation level as runs-on and steps in that job.Source: Linters/SAST tools
🤖 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 @.github/workflows/ci.yaml:
- Around line 87-90: The workflow step named "Install capnp schema compiler"
installs packages without refreshing apt metadata; modify that step to run `sudo
apt-get update` (or `sudo apt-get update -y` if preferred) immediately before
`sudo apt-get install -y capnproto libcapnp-dev` so the apt cache is refreshed
on runners; locate the step by its name string "Install capnp schema compiler"
in the CI job and insert the update command at the top of the step's run block.
- Around line 81-86: Replace the archived actions-rs/toolchain@v1 by using a
maintained Rust setup action (e.g., dtolnay/rust-toolchain or
actions-rust-lang/setup-rust-toolchain) and pin that replacement to a specific
commit SHA; also replace mutable tags for actions/checkout@v3 and
Swatinem/rust-cache@v2 with their respective full commit SHAs so all three uses:
entries are immutable and point to exact commits.
---
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 78-99: The CI job "check_capnp_generated" currently uses default
token permissions; scope it down by adding a job-level permissions entry to only
allow repository content read access. Update the job definition for
check_capnp_generated to include permissions: { contents: read } (so the job
still can read the repo for git diff but cannot perform broader actions),
ensuring the permissions block is at the same indentation level as runs-on and
steps in that job.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36d0c9e5-91da-4b4a-af22-001f94e73439
⛔ Files ignored due to path filters (1)
hypersync-net-types/src/__generated__/hypersync_net_types_capnp.rsis excluded by!**/__generated__/**
📒 Files selected for processing (2)
.github/workflows/ci.yamlhypersync-net-types/Makefile
JonoPrest
requested changes
Jun 9, 2026
JonoPrest
left a comment
JonoPrest
left a comment
Collaborator
There was a problem hiding this comment.
Same problem as hyper fuel. Capnp codegen is not fully deterministic depending on which version of capnp you're using. But it's difficult to align these versions on all environments since they don't publish binaries for all platforms. We can always set the make cmd to actually use docker rather than local capnp to get deterministic codegen though
Collaborator
Sounds like a decent idea 👍 |
Run codegen inside a pinned toolchain image (rust + capnproto + capnpc-rust) via the Makefile and the check_capnp_generated job, instead of relying on the host's locally installed capnp/rustfmt. This makes the committed generated code byte-for-byte reproducible across developer machines and CI, avoiding spurious diffs from platform/version mismatches. Co-authored-by: claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yaml (1)
73-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet least-privilege permissions for
check_capnp_generated.This job currently inherits default token permissions. It only needs read access for checkout/diff, so add an explicit
permissionsblock.Suggested patch
check_capnp_generated: runs-on: ubuntu-latest + permissions: + contents: read steps: - uses: actions/checkout@v3🤖 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/ci.yaml around lines 73 - 82, The check_capnp_generated job currently uses default token permissions; update the job named check_capnp_generated to grant least privilege by adding an explicit permissions block (e.g., permissions: contents: read) so the workflow only has read access needed for actions/checkout and running git diff; place the permissions block at the top level of the job definition near the runs-on line to apply only to this job and keep the existing steps and commands unchanged.Source: Linters/SAST tools
🤖 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 `@hypersync-net-types/capnp-codegen.Dockerfile`:
- Around line 4-11: The Dockerfile uses mutable inputs — the base image tag
`FROM rust:1.94-slim-bookworm` and a floating Debian package install `apt-get
install ... capnproto` — causing nondeterministic builds; fix it by pinning the
base image to a specific digest (replace the `FROM rust:1.94-slim-bookworm` tag
with its immutable sha256 digest) and pinning the capnproto package (install a
specific Debian package version or download and install the exact .deb for
capnproto matching the chosen Debian release), ensure the existing `ARG
CAPNPC_VERSION` is used to align tools with the pinned system package, and
update the RUN step to install the exact package version instead of the
unversioned package name so CI builds are reproducible.
---
Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 73-82: The check_capnp_generated job currently uses default token
permissions; update the job named check_capnp_generated to grant least privilege
by adding an explicit permissions block (e.g., permissions: contents: read) so
the workflow only has read access needed for actions/checkout and running git
diff; place the permissions block at the top level of the job definition near
the runs-on line to apply only to this job and keep the existing steps and
commands unchanged.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f197e174-b1e9-4983-baf5-2c6fd27c60f1
📒 Files selected for processing (3)
.github/workflows/ci.yamlhypersync-net-types/Makefilehypersync-net-types/capnp-codegen.Dockerfile
Pin the Docker base image to an immutable digest so the codegen toolchain (notably rustfmt) can't drift when the rust:1.94-slim-bookworm tag moves, keeping the generated output fully reproducible. Co-authored-by: claude <noreply@anthropic.com>
DZakh
changed the title
JonoPrest
reviewed
Jun 9, 2026
Collapse the Makefile target into a single `docker build --output` that runs codegen inside the image and exports the generated file directly, dropping the separate `docker run`, the volume mount and UID/GID mapping. A .dockerignore keeps the build context to just the schema. Move the codegen base to Debian trixie so the capnp compiler is 1.1.0 (>=1.0) instead of bookworm's 0.9.2, and pin both the base image digest and the capnproto apt version since the compiler version affects the generated schema blobs. Regenerate the committed code accordingly. Co-authored-by: claude <noreply@anthropic.com>
Add a top-level `permissions: contents: read` block since every CI job is read-only, and document the .dockerignore schema-only build context. Co-authored-by: claude <noreply@anthropic.com>
JonoPrest
approved these changes
Jun 9, 2026
JonoPrest
left a comment
JonoPrest
left a comment
Collaborator
There was a problem hiding this comment.
I like it 👍🏼
Also realised capnpc was not previously aligned with the version we were using in cargo.toml (0.23)
This might be worth adding a CI test to make sure that the versions in dockerfile and cargo.tomls match
Add a step to check_capnp_generated that fails if the capnpc version pinned in capnp-codegen.Dockerfile drifts from any `capnp` dependency in the workspace Cargo.toml files. The codegen plugin and the runtime are released in lockstep, so they must stay aligned. Co-authored-by: claude <noreply@anthropic.com>
JonoPrest
approved these changes
Jun 9, 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
3 participants
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.
Summary
Refactored generated Cap'n Proto code to use dedicated helper functions from the
capnp::introspectmodule instead of inlinepanic!macros. This improves code maintainability and consistency across the generated schema bindings.Key Changes
panic!("invalid field index {}", index)calls with::capnp::introspect::panic_invalid_field_index(index)panic!("invalid annotation indices ({:?}, {}) ", child_index, index)calls with::capnp::introspect::panic_invalid_annotation_indices(child_index, index)Additional Changes
check_capnp_generatedto verify that committed Cap'n Proto generated code stays in sync with the schemahypersync-net-types/Makefileexplaining the exact tools and versions required to regenerate the Cap'n Proto code (capnp compiler and capnpc-rust plugin v0.23.2).capnpschema filesImplementation Details
These changes were generated by the Cap'n Proto code generator (capnpc-rust v0.23.2) and represent a refactoring of the generated code to use more maintainable helper functions rather than inline panic macros. The helper functions provide better error messages and centralized panic handling.
https://claude.ai/code/session_01BFU2E3mDhWx5sg1AQKuQvB
Summary by CodeRabbit