ci(rest): build command artifacts once per target - #4618
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:
Summary by CodeRabbit
WalkthroughThe PR adds a manifest-driven REST command bundle builder, verifier, and test suite. It defines Linux and Darwin targets, propagates build identity metadata through CI, creates deterministic archives, and uploads bundle artifacts with checksums and manifests. ChangesREST command bundle pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Prepare
participant BuildWorkflow
participant BundleCLI
participant GoToolchain
participant ArtifactStore
Prepare->>BuildWorkflow: pass version, timestamp, and commit SHAs
BuildWorkflow->>BundleCLI: build selected target bundle
BundleCLI->>GoToolchain: compile manifest outputs
GoToolchain-->>BundleCLI: return binaries with metadata
BundleCLI->>BundleCLI: verify binaries and archive
BundleCLI-->>BuildWorkflow: return bundle files
BuildWorkflow->>ArtifactStore: upload archive, checksum, and manifest
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
ᕱ⑅ᕱ ✅ Action performedFull review finished. |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-08-05 20:27:29 UTC | Commit: 71a2210 |
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
nv-dmendoza
left a comment
nv-dmendoza
left a comment
There was a problem hiding this comment.
LGTM, some refactoring by using the already required inputs might be helfpul
| run: | | ||
| set -euo pipefail | ||
|
|
||
| full_sha=$(git rev-parse HEAD) |
There was a problem hiding this comment.
arent some of these already passed in as required inputs?
There was a problem hiding this comment.
@chet This job already produces a number of values https://github.com/NVIDIA/infra-controller/blob/main/.github/workflows/ci.yaml#L103, should we use identifiers from there instead?
There was a problem hiding this comment.
Hey @nv-dmendoza @thossain-nv yup good call! Updated this to reuse the identity from rest-prepare-build-info.yml: it now exports binary_version, build_timestamp, short_sha, and full_sha -- and then rest-ci.yml passes those into the binary build. The resolver left in rest-build-binaries.yml is only for direct manual dispatch (where there is no prepare caller). Lemme know if that's better!
![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 (1)
rest-api/scripts/rest_command_bundle.py (1)
425-463: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStream bundle members instead of holding every binary in memory.
create_bundletakesdict[str, bytes], andbuild_bundlefills it with the full image of all 13 outputs.read_bundlethen materializes the same set again during verification, andinspect_binaryreads each binary a third time at line 382. Peak memory therefore scales with the total size of the target inventory, which is the least desirable property for a 4-CPU runner.Pass paths instead of bytes, and let
tarfilecopy from a file object. Determinism is unaffected becausewrite_tar_memberalready sets every header field explicitly.♻️ Proposed streaming signature
-def write_tar_member( - archive: tarfile.TarFile, - name: str, - value: bytes, - mode: int, - mtime: int, -) -> None: +def write_tar_header(name: str, size: int, mode: int, mtime: int) -> tarfile.TarInfo: header = tarfile.TarInfo(name=name) - header.size = len(value) + header.size = size header.mode = mode header.mtime = mtime header.uid = 0 header.gid = 0 header.uname = "" header.gname = "" - archive.addfile(header, fileobj=io.BytesIO(value)) + return header def create_bundle( path: Path, manifest: bytes, checksums: bytes, - binaries: dict[str, bytes], + binaries: dict[str, Path], mtime: int, ) -> None: with path.open("wb") as destination: with gzip.GzipFile( filename="", mode="wb", fileobj=destination, mtime=mtime ) as compressed: with tarfile.open(fileobj=compressed, mode="w") as archive: - write_tar_member(archive, "manifest.json", manifest, 0o644, mtime) - write_tar_member(archive, "SHA256SUMS", checksums, 0o644, mtime) + for name, value in ( + ("manifest.json", manifest), + ("SHA256SUMS", checksums), + ): + header = write_tar_header(name, len(value), 0o644, mtime) + archive.addfile(header, fileobj=io.BytesIO(value)) for name in sorted(binaries): - write_tar_member(archive, name, binaries[name], 0o755, mtime) + source_path = binaries[name] + header = write_tar_header( + name, source_path.stat().st_size, 0o755, mtime + ) + with source_path.open("rb") as source: + archive.addfile(header, fileobj=source)Apply the same treatment to
read_bundleby extracting each member to a temporary file and returning(Path, mode).verify_bundlethen digests withsha256_filerather thansha256_bytes.🤖 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 `@rest-api/scripts/rest_command_bundle.py` around lines 425 - 463, Update create_bundle, build_bundle, and write_tar_member to pass binary Paths and stream file contents into the tar archive instead of storing all bytes in memory. Change read_bundle to extract each member into a temporary file and return (Path, mode), preserving existing safety validation and cleanup. Update verify_bundle to use sha256_file and adjust inspect_binary and related callers to consume paths without materializing complete binaries.
🤖 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/rest-build-binaries.yml:
- Around line 172-180: Add an actions/setup-python@v5 step before the “Build and
verify ${{ matrix.target }} bundle” step, configuring python-version to '3.12'
so scripts/rest_command_bundle.py runs with the required pinned interpreter;
leave the existing build command unchanged.
In @.github/workflows/rest-prepare-build-info.yml:
- Around line 136-143: Update the version-generation step around BINARY_VERSION
to validate the selected value before producing the authoritative binary_version
output. Mirror the existing resolve-manual-identity guards for empty values and
versions whose first character is non-alphanumeric, failing the prepare job
immediately while preserving the current main-branch, release-tag, and
branch-SHA selection logic.
In `@rest-api/scripts/rest_command_bundle.py`:
- Around line 361-378: Update the bundle preflight around the existing
file-command usage and main error handling to verify that the external file
utility is available before processing bundles, converting absence or invocation
failure into an actionable BundleError. Declare the utility dependency in the
workflow with an explicit version check or installation step, and
centralize/version the architecture and linkage markers used by the
file-description validation so supported outputs include static-pie linked and
the runner’s expected Mach-O wording.
In `@rest-api/scripts/rest-command-bundles.md`:
- Around line 3-35: Expand rest-command-bundles.md to document the complete
bundle contract: manifest schemas, command inputs/defaults, accepted targets,
archive members and modes, checksum formats, dirty-tree behavior, provenance
limitations, and failure cases. Keep rest-command-manifest.json as the canonical
artifact inventory, and replace the issue-only CICD-08 reference with a release,
support, or tested-compatibility boundary linked to the canonical REST contract.
Run rumdl and verify the rendered page, links, anchors, and navigation.
In `@rest-api/scripts/rest-command-manifest.json`:
- Around line 319-358: Update the darwin-arm64 entries for api, site-agent, and
flow in the manifest to replace empty ldflags with the same metadata.Version,
metadata.BuildTime, and metadata.GitCommit -X linker flags used by their Linux
counterparts. Do not add -extldflags=-static to these Darwin entries.
---
Nitpick comments:
In `@rest-api/scripts/rest_command_bundle.py`:
- Around line 425-463: Update create_bundle, build_bundle, and write_tar_member
to pass binary Paths and stream file contents into the tar archive instead of
storing all bytes in memory. Change read_bundle to extract each member into a
temporary file and return (Path, mode), preserving existing safety validation
and cleanup. Update verify_bundle to use sha256_file and adjust inspect_binary
and related callers to consume paths without materializing complete binaries.
🪄 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: Enterprise
Run ID: 7845049c-b0f0-4263-9298-ef04a08acf04
📒 Files selected for processing (9)
.github/workflows/rest-build-binaries.yml.github/workflows/rest-ci.yml.github/workflows/rest-lint-and-test.yml.github/workflows/rest-prepare-build-info.ymlrest-api/Makefilerest-api/scripts/rest-command-bundles.mdrest-api/scripts/rest-command-manifest.jsonrest-api/scripts/rest_command_bundle.pyrest-api/scripts/test_rest_command_bundle.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.github/workflows/rest-prepare-build-info.yml (1)
136-143: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate
BINARY_VERSIONin the producing step.This step produces the authoritative
binary_version, but it applies no format check. Two consumers do:.github/workflows/rest-build-binaries.ymlline 163 andvalidate_identityinrest-api/scripts/rest_command_bundle.py. Both require an alphanumeric first character.Line 120 strips a leading hyphen, so a ref name that sanitizes to an empty string yields
BRANCH_SHA_TAG="-${SHORT_SHA}". The failure then appears in all three matrix jobs instead of here. The sibling jobresolve-manual-identityin.github/workflows/rest-build-binaries.ymllines 92-103 already guards both cases; mirror that logic.🛡️ Proposed producer-side validation
if [ "${IS_MAIN_BRANCH}" = "true" ]; then BINARY_VERSION="${SEMANTIC_VERSION}" elif [ -n "${RELEASE_TAG}" ]; then BINARY_VERSION="${RELEASE_TAG}" + elif [ -n "${BRANCH_NAME}" ]; then + BINARY_VERSION="${BRANCH_SHA_TAG}" else - BINARY_VERSION="${BRANCH_SHA_TAG}" + echo "::error::cannot derive a binary version from ${GITHUB_REF_NAME}" + exit 1 fi + if [[ ! "${BINARY_VERSION}" =~ ^[a-zA-Z0-9][a-zA-Z0-9._-]*$ ]]; then + echo "::error::derived binary version is not safe: ${BINARY_VERSION}" + exit 1 + fi BUILD_TIMESTAMP=$(date -u +"%Y-%m-%dT%H:%M:%SZ")🤖 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/rest-prepare-build-info.yml around lines 136 - 143, Add producer-side validation immediately after the BINARY_VERSION selection in the build-info step, mirroring resolve-manual-identity’s checks: reject an empty value and any value whose first character is not alphanumeric, emitting an error and exiting nonzero. Keep valid version selection and BUILD_TIMESTAMP generation unchanged..github/workflows/rest-build-binaries.yml (1)
172-180: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPin the Python interpreter used by the bundle builder.
This step invokes
python3without declaring a version, so the build depends on whichever interpreter the runner image ships.rest-api/scripts/rest_command_bundle.pyuses modern typing syntax such asbool | Nonein theverify_bundlesignature, which requires Python 3.10 or later. Add anactions/setup-python@v5step before this one and pinpython-version.♻️ Proposed interpreter pinning
+ - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.12' + - name: Build and verify ${{ matrix.target }} bundle run: | python3 scripts/rest_command_bundle.py build \Based on learnings, relative paths in
runsteps resolve against the effectivedefaults.run.working-directory, which the upload paths at lines 201-203 confirm isrest-api.🤖 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/rest-build-binaries.yml around lines 172 - 180, Add an actions/setup-python@v5 step immediately before the “Build and verify” step, pinning python-version to Python 3.10 or newer. Keep the existing rest_command_bundle.py invocation and relative paths unchanged.Source: Learnings
🧹 Nitpick comments (5)
rest-api/scripts/rest_command_bundle.py (1)
591-608: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pairing resolved outputs to contracts by name instead of by position.
zip(outputs, contracts, strict=True)makes output ordering an undocumented part of the resolved-manifest contract.build_bundlepreserves source order, so this holds today. A reordered but otherwise correct manifest produces a misleadingoutputs[i].name is ... expected ...error. Keying both sides by(target, name)and comparing the key sets first would produce a precise error and remove the ordering dependency.🤖 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 `@rest-api/scripts/rest_command_bundle.py` around lines 591 - 608, Update the resolved-output validation loop in build_bundle to match outputs and contracts by their (target, name) identity rather than zip position. Build keyed mappings for both collections, compare their key sets first, and raise a precise BundleError for missing or unexpected keys before validating each matched output’s exact_contract fields.rest-api/scripts/test_rest_command_bundle.py (1)
173-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
verify_bundlecross-checks.The suite covers
create_bundle,read_bundle, the checksum parsers, andvalidate_resolved_manifestin isolation. It never exercisesverify_bundle, so the composed rules stay untested: the exact member set, the0o644and0o755mode requirements, equality between the embedded and externalmanifest.json, and theSHA256SUMSinventory match. Those rules are the security-relevant part of the contract.
verify_bundlecallsinspect_binary, which requiresgoandfile. Patchbundle.inspect_binarywithunittest.mock.patchto return fixed details, then assert that a well-formed bundle passes and that a mutated member set, a wrong mode, and a tamperedSHA256SUMSeach raiseBundleError.🤖 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 `@rest-api/scripts/test_rest_command_bundle.py` around lines 173 - 228, Add tests targeting BundleFormatTest.verify_bundle using unittest.mock.patch on bundle.inspect_binary with fixed binary details. Cover a valid bundle that passes, plus separate BundleError cases for an altered member set, an incorrect required mode, and tampered SHA256SUMS content, ensuring the composed manifest, mode, inventory, and binary checks are exercised..github/workflows/rest-lint-and-test.yml (1)
55-57: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin Python for the command-bundle check.
make check-command-bundleinvokespython3, andrest_command_bundle.pyuseszip(..., strict=True), which requires Python 3.10 or newer. The existing Python setup is in another job and does not affectstyle. Addactions/setup-pythonwith an explicit version before this step, and document the minimum version in the script.🤖 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/rest-lint-and-test.yml around lines 55 - 57, Update the workflow job containing “Check REST command bundle contract” to run actions/setup-python with an explicit Python 3.10-or-newer version before make check-command-bundle, and add a comment in rest_command_bundle.py documenting that Python 3.10 is the minimum required version for zip(..., strict=True).Source: Learnings
.github/workflows/rest-build-binaries.yml (2)
150-170: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider moving this validation directly after checkout.
The checks are correct and complete: candidate identity, short-SHA prefix, version format, and timestamp format. The timestamp regex matches the format that
.github/workflows/rest-prepare-build-info.ymlline 143 emits and line 28 documents.The step currently runs after
Set up Goandgo mod download. An invalid identity therefore wastes a toolchain setup and a module download in each of the three matrix jobs. Place this step immediately afterCheckout codeto fail earlier.🤖 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/rest-build-binaries.yml around lines 150 - 170, Move the “Validate bundle identity” step to immediately after the “Checkout code” step, before “Set up Go” and “go mod download,” while preserving all existing candidate identity, SHA prefix, version, and timestamp checks unchanged.
111-119: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd concurrency control for direct manual builds.
.github/workflows/rest-ci.ymlcovers its reusable call, but.github/workflows/rest-build-binaries.ymlalso supportsworkflow_dispatchwithout aconcurrencyblock. Repeated manual dispatches can run all three matrix jobs concurrently.🤖 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/rest-build-binaries.yml around lines 111 - 119, Add a workflow-level concurrency block to rest-build-binaries.yml for workflow_dispatch executions, using a stable group key and cancel-in-progress behavior consistent with rest-ci.yml. Place it alongside the existing workflow configuration so repeated manual dispatches serialize or cancel prior runs while preserving the build-binaries matrix behavior.Source: Path instructions
🤖 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.
Duplicate comments:
In @.github/workflows/rest-build-binaries.yml:
- Around line 172-180: Add an actions/setup-python@v5 step immediately before
the “Build and verify” step, pinning python-version to Python 3.10 or newer.
Keep the existing rest_command_bundle.py invocation and relative paths
unchanged.
In @.github/workflows/rest-prepare-build-info.yml:
- Around line 136-143: Add producer-side validation immediately after the
BINARY_VERSION selection in the build-info step, mirroring
resolve-manual-identity’s checks: reject an empty value and any value whose
first character is not alphanumeric, emitting an error and exiting nonzero. Keep
valid version selection and BUILD_TIMESTAMP generation unchanged.
---
Nitpick comments:
In @.github/workflows/rest-build-binaries.yml:
- Around line 150-170: Move the “Validate bundle identity” step to immediately
after the “Checkout code” step, before “Set up Go” and “go mod download,” while
preserving all existing candidate identity, SHA prefix, version, and timestamp
checks unchanged.
- Around line 111-119: Add a workflow-level concurrency block to
rest-build-binaries.yml for workflow_dispatch executions, using a stable group
key and cancel-in-progress behavior consistent with rest-ci.yml. Place it
alongside the existing workflow configuration so repeated manual dispatches
serialize or cancel prior runs while preserving the build-binaries matrix
behavior.
In @.github/workflows/rest-lint-and-test.yml:
- Around line 55-57: Update the workflow job containing “Check REST command
bundle contract” to run actions/setup-python with an explicit Python
3.10-or-newer version before make check-command-bundle, and add a comment in
rest_command_bundle.py documenting that Python 3.10 is the minimum required
version for zip(..., strict=True).
In `@rest-api/scripts/rest_command_bundle.py`:
- Around line 591-608: Update the resolved-output validation loop in
build_bundle to match outputs and contracts by their (target, name) identity
rather than zip position. Build keyed mappings for both collections, compare
their key sets first, and raise a precise BundleError for missing or unexpected
keys before validating each matched output’s exact_contract fields.
In `@rest-api/scripts/test_rest_command_bundle.py`:
- Around line 173-228: Add tests targeting BundleFormatTest.verify_bundle using
unittest.mock.patch on bundle.inspect_binary with fixed binary details. Cover a
valid bundle that passes, plus separate BundleError cases for an altered member
set, an incorrect required mode, and tampered SHA256SUMS content, ensuring the
composed manifest, mode, inventory, and binary checks are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ad3848c8-04d9-4910-a6fe-1caecce385d8
📒 Files selected for processing (9)
.github/workflows/rest-build-binaries.yml.github/workflows/rest-ci.yml.github/workflows/rest-lint-and-test.yml.github/workflows/rest-prepare-build-info.ymlrest-api/Makefilerest-api/scripts/rest-command-bundles.mdrest-api/scripts/rest-command-manifest.jsonrest-api/scripts/rest_command_bundle.pyrest-api/scripts/test_rest_command_bundle.py
|
Thanks for this @chet, checking this out locally to tweak a few things! |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
rest-api/scripts/test_rest_command_bundle.py (3)
287-290: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact gzip FLG byte.
Line 288 checks only the
FNAMEbit, whilevalidate_gzip_headerrequiresheader[3] == 0, meaning no optional header fields at all. Assert the same condition so the test pins the contract the verifier enforces.💚 Proposed test change
- self.assertEqual(archive_bytes[3] & 0x08, 0) + self.assertEqual(archive_bytes[3], 0)🤖 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 `@rest-api/scripts/test_rest_command_bundle.py` around lines 287 - 290, Update the gzip header assertions in the test around archive_bytes and validate_gzip_header to assert that the complete FLG byte at archive_bytes[3] equals 0, rather than checking only the FNAME bit, matching the verifier’s required header contract.
144-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the negative manifest cases to the remaining rejection rules.
load_source_manifestalso rejects an unsupportedschema_version, an unknown top-level field, a declared target with no outputs, and a bare$VERSIONlinker-flag form. None of those paths has a case here. Each is a one-line addition to thecasesmapping.💚 Proposed test addition
"string cgo": lambda value: value["outputs"][0].__setitem__( "cgo_enabled", "0" ), + "wrong schema version": lambda value: value.__setitem__( + "schema_version", 2 + ), + "unknown manifest field": lambda value: value.__setitem__( + "extra", True + ), + "unused target": lambda value: value["targets"].append( + {"name": "linux-riscv64", "goos": "linux", "goarch": "riscv64"} + ), + "bare dollar token": lambda value: value["outputs"][0]["ldflags"].append( + "-X=example.Value=$VERSION" + ), }🤖 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 `@rest-api/scripts/test_rest_command_bundle.py` around lines 144 - 187, Extend the cases mapping in test_invalid_manifest_cases with one mutation for each remaining load_source_manifest rejection rule: unsupported schema_version, unknown top-level field, a declared target with no outputs, and a bare $VERSION linker-flag form. Keep each mutation as a one-line case and ensure every mutated manifest is still asserted to raise bundle.BundleError.
205-214: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the inclusive timestamp boundaries.
The test rejects
1969-12-31T23:59:59Zand2106-02-07T06:28:16Zbut never asserts that the two accepted boundaries pass.parse_build_timestampuses0 <= gzip_seconds <= 0xFFFFFFFF, so an off-by-one change to either comparison would not fail this test.💚 Proposed test addition
invalid = ( "2026-8-5T1:2:3Z", "1969-12-31T23:59:59Z", "2106-02-07T06:28:16Z", ) + for timestamp in ("1970-01-01T00:00:00Z", "2106-02-07T06:28:15Z"): + with self.subTest(timestamp=timestamp): + bundle.parse_build_timestamp(timestamp) for timestamp in invalid:🤖 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 `@rest-api/scripts/test_rest_command_bundle.py` around lines 205 - 214, Extend the timestamp tests around parse_build_timestamp to explicitly assert that the inclusive boundary values corresponding to gzip seconds 0 and 0xFFFFFFFF are accepted. Keep the existing rejection cases for timestamps just outside those boundaries, so both comparison edges are covered.rest-api/scripts/rest_command_bundle.py (2)
1037-1075: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueWhole-binary buffering on both the build and verify paths. The module passes complete binary contents through in-memory
bytesobjects instead of streaming from the staged files, so peak memory scales with the entire uncompressed bundle on both paths, and roughly doubles during the in-place verification thatbuild_bundleperforms.
rest-api/scripts/rest_command_bundle.py#L1037-L1075: keep the staged binary paths instead ofbinary_contents, and computesha256andsizewithsha256_fileandstat()socreate_bundlecan stream each member from an open file.rest-api/scripts/rest_command_bundle.py#L581-L628: return member metadata plus a stream or extract each member directly to the verifier-owned temporary path, soverify_bundlenever holds all members at once.🤖 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 `@rest-api/scripts/rest_command_bundle.py` around lines 1037 - 1075, Replace whole-binary buffering in rest-api/scripts/rest_command_bundle.py:1037-1075 by retaining staged binary paths, computing metadata with sha256_file and stat(), and updating create_bundle to stream members from open files. In rest-api/scripts/rest_command_bundle.py:581-628, update the verify_bundle path to return metadata with streams or extract each member directly to verifier-owned temporary files so members are processed one at a time without retaining the complete bundle in memory.
1006-1021: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNeutralize or record the ambient Go environment.
The build inherits the full process environment and overrides only
CGO_ENABLED,GOARCH, andGOOS. Variables such asGOFLAGS,GOEXPERIMENT,CGO_CFLAGS, andGOAMD64still reach the compiler, and the resolved manifest does not record them. Two runs from the same candidate can therefore produce different bytes while passing verification, which weakens the deterministic-bundle claim.Either clear the build-affecting variables explicitly or record their values in the resolved manifest.
♻️ Proposed refactor
environment = os.environ.copy() + # Build-affecting variables must not leak in from the runner. + for name in ("GOFLAGS", "GOEXPERIMENT", "GOAMD64", "GOARM64"): + environment.pop(name, None) environment.update(🤖 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 `@rest-api/scripts/rest_command_bundle.py` around lines 1006 - 1021, Update the build environment setup in the command build flow around subprocess.run so ambient Go build-affecting variables such as GOFLAGS, GOEXPERIMENT, CGO_CFLAGS, and GOAMD64 cannot vary outputs unnoticed: either remove them from environment before invoking the compiler or capture their resolved values in the manifest used for verification. Preserve the existing CGO_ENABLED, GOARCH, and GOOS target configuration.rest-api/scripts/rest-command-bundles.md (2)
15-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the supported-target restriction as a manifest rule.
parse_targetrejects any target whosenameis notlinux-amd64,linux-arm64, ordarwin-arm64. This rule applies tocheckas well, so a new manifest target fails validation before any build. The document currently presents the accepted target set only as abuild/verifyinput (Line 84).Add the constraint to the
targetsbullet so the manifest contract is complete.📝 Proposed documentation change
- `targets` is a non-empty array. Each entry has the non-empty strings `name`, `goos`, and `goarch`; `name` must be the literal `<goos>-<goarch>` value, and - target names must be unique. + target names must be unique. `name` must also be one of the supported values + `linux-amd64`, `linux-arm64`, or `darwin-arm64`; every other target fails + validation, including in `check`.As per coding guidelines: "Document interface contracts completely, including spelling, requiredness, defaults, accepted values, units, bounds, interactions, ordering, fallback behavior, outputs, side effects, errors, and unsupported paths; verify claims from code, schemas, or exercised output."
🤖 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 `@rest-api/scripts/rest-command-bundles.md` around lines 15 - 33, Update the targets requirement in the source manifest documentation to state that each target name must be one of linux-amd64, linux-arm64, or darwin-arm64, including for check validation. Preserve the existing name-format and uniqueness requirements.Source: Coding guidelines
199-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReconcile the dirty-checkout claim with
--allow-dirty.Line 201 states that the builder rejects a dirty checkout without qualification. Lines 104-107 state that
--allow-dirtypermits a dirty checkout and records it. Add the qualifier here so the two statements agree.📝 Proposed documentation change
-manifest records that source check. The standalone verifier checks the recorded +manifest records that source check, and `--allow-dirty` is the only way to +accept a dirty checkout. The standalone verifier checks the recorded-Git `HEAD` is the requested candidate and rejects a dirty checkout. The resolved +Git `HEAD` is the requested candidate and, unless `--allow-dirty` is passed, +rejects a dirty checkout. The resolvedAs per coding guidelines: "When changing a documented fact or behavior, search all relevant documentation surfaces, reconcile conflicting occurrences or establish and link to one canonical explanation".
🤖 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 `@rest-api/scripts/rest-command-bundles.md` around lines 199 - 204, Update the builder description near the Git HEAD and dirty-checkout validation to state that dirty checkouts are rejected by default, except when --allow-dirty is supplied, which permits and records them. Search related documentation surfaces for the same unconditional claim and reconcile them with this qualified behavior.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 `@rest-api/scripts/rest_command_bundle.py`:
- Around line 927-946: Update the comparison loop after inspect_binary in the
bundle validation flow to include file_description alongside go_version,
vcs_modified, and vcs_revision. Ensure raw_output["file_description"] is
compared with inspection.file_description so the manifest value matches the
inspected binary.
---
Nitpick comments:
In `@rest-api/scripts/rest_command_bundle.py`:
- Around line 1037-1075: Replace whole-binary buffering in
rest-api/scripts/rest_command_bundle.py:1037-1075 by retaining staged binary
paths, computing metadata with sha256_file and stat(), and updating
create_bundle to stream members from open files. In
rest-api/scripts/rest_command_bundle.py:581-628, update the verify_bundle path
to return metadata with streams or extract each member directly to
verifier-owned temporary files so members are processed one at a time without
retaining the complete bundle in memory.
- Around line 1006-1021: Update the build environment setup in the command build
flow around subprocess.run so ambient Go build-affecting variables such as
GOFLAGS, GOEXPERIMENT, CGO_CFLAGS, and GOAMD64 cannot vary outputs unnoticed:
either remove them from environment before invoking the compiler or capture
their resolved values in the manifest used for verification. Preserve the
existing CGO_ENABLED, GOARCH, and GOOS target configuration.
In `@rest-api/scripts/rest-command-bundles.md`:
- Around line 15-33: Update the targets requirement in the source manifest
documentation to state that each target name must be one of linux-amd64,
linux-arm64, or darwin-arm64, including for check validation. Preserve the
existing name-format and uniqueness requirements.
- Around line 199-204: Update the builder description near the Git HEAD and
dirty-checkout validation to state that dirty checkouts are rejected by default,
except when --allow-dirty is supplied, which permits and records them. Search
related documentation surfaces for the same unconditional claim and reconcile
them with this qualified behavior.
In `@rest-api/scripts/test_rest_command_bundle.py`:
- Around line 287-290: Update the gzip header assertions in the test around
archive_bytes and validate_gzip_header to assert that the complete FLG byte at
archive_bytes[3] equals 0, rather than checking only the FNAME bit, matching the
verifier’s required header contract.
- Around line 144-187: Extend the cases mapping in test_invalid_manifest_cases
with one mutation for each remaining load_source_manifest rejection rule:
unsupported schema_version, unknown top-level field, a declared target with no
outputs, and a bare $VERSION linker-flag form. Keep each mutation as a one-line
case and ensure every mutated manifest is still asserted to raise
bundle.BundleError.
- Around line 205-214: Extend the timestamp tests around parse_build_timestamp
to explicitly assert that the inclusive boundary values corresponding to gzip
seconds 0 and 0xFFFFFFFF are accepted. Keep the existing rejection cases for
timestamps just outside those boundaries, so both comparison edges are covered.
🪄 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: Enterprise
Run ID: 7c6ea812-4c48-4bdb-a718-1cf94df88650
📒 Files selected for processing (9)
.github/workflows/rest-build-binaries.yml.github/workflows/rest-ci.yml.github/workflows/rest-lint-and-test.yml.github/workflows/rest-prepare-build-info.ymlrest-api/Makefilerest-api/scripts/rest-command-bundles.mdrest-api/scripts/rest-command-manifest.jsonrest-api/scripts/rest_command_bundle.pyrest-api/scripts/test_rest_command_bundle.py
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/rest-ci.yml
- rest-api/scripts/rest-command-manifest.json
- .github/workflows/rest-lint-and-test.yml
- .github/workflows/rest-prepare-build-info.yml
|
@coderabbitai full_review, thanks! |
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rest-api/scripts/test_rest_command_bundle.py (1)
153-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo documented rejection rules have no negative case here: the invalid dollar form at
rest_command_bundle.pyline 192, and the "targets have no outputs" rule at line 264. Both are contract guarantees stated inrest-command-bundles.md. Consider adding them tocases.♻️ Proposed test additions
"string cgo": lambda value: value["outputs"][0].__setitem__( "cgo_enabled", "0" ), + "literal dollar": lambda value: value["outputs"][0]["ldflags"].append( + "-X=example.Value=$$VERSION" + ), + "unused target": lambda value: value["targets"].append( + {"name": "darwin-amd64", "goos": "darwin", "goarch": "amd64"} + ), }🤖 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 `@rest-api/scripts/test_rest_command_bundle.py` around lines 153 - 179, Extend the negative-test `cases` mapping in the test setup to cover the documented invalid dollar-form rejection and the rule that a target with no outputs is rejected. Add mutations that produce each invalid configuration, using the existing fixture structure and assertion flow so both contract guarantees are exercised alongside the current cases.
🤖 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/rest-build-binaries.yml:
- Around line 26-56: Make binary_version, build_timestamp, short_sha, and
full_sha optional workflow_call inputs so callers using derive_identity can omit
them. In the resolve-manual-identity flow, add explicit validation before the
build when derive_identity is false, requiring all four values and failing with
a clear error if any are missing; preserve derived-identity behavior when
derive_identity is true.
---
Nitpick comments:
In `@rest-api/scripts/test_rest_command_bundle.py`:
- Around line 153-179: Extend the negative-test `cases` mapping in the test
setup to cover the documented invalid dollar-form rejection and the rule that a
target with no outputs is rejected. Add mutations that produce each invalid
configuration, using the existing fixture structure and assertion flow so both
contract guarantees are exercised alongside the current cases.
🪄 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: Enterprise
Run ID: f5cef3df-9abb-4862-bc15-bc114833fbc6
📒 Files selected for processing (9)
.github/workflows/rest-build-binaries.yml.github/workflows/rest-ci.yml.github/workflows/rest-lint-and-test.yml.github/workflows/rest-prepare-build-info.ymlrest-api/Makefilerest-api/scripts/rest-command-bundles.mdrest-api/scripts/rest-command-manifest.jsonrest-api/scripts/rest_command_bundle.pyrest-api/scripts/test_rest_command_bundle.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
rest-api/scripts/rest_command_bundle.py (2)
976-989: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify that the dirty check covers the whole repository, not only
rest-api.
command_outputrunsgit status --porcelain=v1 --untracked-files=allwithcwd=repo_root, butgitreports on the entire work tree regardless of the working directory.repo_rootis the nestedrest-apimodule, so an unrelated edit anywhere in the monorepo setssource_dirtyand blocks a local build without--allow-dirty.CI is unaffected because the checkout is clean. For local ergonomics, either scope the pathspec to the module or state the repository-wide scope in the error text.
♻️ Proposed scoping
source_status = command_output( - ["git", "status", "--porcelain=v1", "--untracked-files=all"], repo_root + [ + "git", + "status", + "--porcelain=v1", + "--untracked-files=all", + "--", + ".", + ], + repo_root, )If the repository-wide scope is deliberate, keep the command and extend the message instead.
🤖 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 `@rest-api/scripts/rest_command_bundle.py` around lines 976 - 989, Update the dirty-check handling around source_status and source_dirty to make its scope explicit: either restrict git status to the rest-api module with an appropriate pathspec, or retain the repository-wide check and revise the BundleError message to state that changes anywhere in the repository block labeling. Preserve the existing allow_dirty behavior.
462-471: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider reusing the binary bytes instead of reading each file twice.
inspect_binaryreads the whole binary at Line 464, andbuild_bundlereads the same file again at Line 1047.verify_bundlealso holds the member bytes in memory before it callsinspect_binary. For 37 large Go binaries this doubles the read volume without adding any verification value. Pass the already-available bytes intoinspect_binaryand let the metadata scan operate on that buffer.This is a throughput refinement only; correctness is unaffected.
🤖 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 `@rest-api/scripts/rest_command_bundle.py` around lines 462 - 471, Update inspect_binary to accept already-loaded binary bytes and perform its linker metadata scan on that buffer instead of calling path.read_bytes(). At its callers, including build_bundle and verify_bundle, pass the existing member or file bytes through so each binary is read only once while preserving the current missing-metadata validation.rest-api/scripts/test_rest_command_bundle.py (2)
144-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNegative-case tables assert only the exception type, so a case can pass through an unintended guard. Both mutation tables mutate manifest members by list position and assert
bundle.BundleErroralone.rest_command_bundle.pyraisesBundleErrorfrom many guards, so a mutation that reaches an earlier guard still satisfies the assertion while the branch named in the case label loses coverage. Assert the expected message for each case and select manifest members by name rather than by index.
rest-api/scripts/test_rest_command_bundle.py#L144-L160: pair each case with its expected message and useassertRaisesRegex; inset_unsupported_target, locate thelinux-amd64target by name instead oftargets[0], so the case exercises theis not supportedguard rather than thename != f"{goos}-{goarch}"guard.rest-api/scripts/test_rest_command_bundle.py#L439-L456: pair each case with its expected message; note that"unsafe name"currently fails at theexact_contractname comparison invalidate_resolved_manifest, not at theNAME_REcheck it is named for, so assert the message that the intended guard emits.🤖 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 `@rest-api/scripts/test_rest_command_bundle.py` around lines 144 - 160, Update the invalid-manifest cases at rest-api/scripts/test_rest_command_bundle.py:144-160 and rest-api/scripts/test_rest_command_bundle.py:439-456 to pair each mutation with its expected error message and assert it with assertRaisesRegex rather than checking only BundleError. In set_unsupported_target, locate the linux-amd64 target by its name instead of targets[0] so the intended unsupported-target guard is exercised; for the unsafe name case, assert the message from the exact_contract name-comparison guard in validate_resolved_manifest, which currently handles the mutation.
216-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
darwin-arm64and architecture mismatches.
PHYSICAL_ARCHITECTURE_MARKERSincludes("Mach-O 64-bit", "arm64"), but the tests cover only Linux markers. Add positivedarwin-arm64coverage and assert that an incorrect architecture marker raisesBundleError.🤖 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 `@rest-api/scripts/test_rest_command_bundle.py` around lines 216 - 267, Add tests alongside test_file_description_accepts_known_static_wording for darwin-arm64 using the Mach-O 64-bit and arm64 markers, covering valid file descriptions. Add a matching architecture-mismatch case that passes an incorrect architecture marker to bundle.validate_file_description and asserts BundleError, while preserving the existing Linux coverage.
🤖 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.
Nitpick comments:
In `@rest-api/scripts/rest_command_bundle.py`:
- Around line 976-989: Update the dirty-check handling around source_status and
source_dirty to make its scope explicit: either restrict git status to the
rest-api module with an appropriate pathspec, or retain the repository-wide
check and revise the BundleError message to state that changes anywhere in the
repository block labeling. Preserve the existing allow_dirty behavior.
- Around line 462-471: Update inspect_binary to accept already-loaded binary
bytes and perform its linker metadata scan on that buffer instead of calling
path.read_bytes(). At its callers, including build_bundle and verify_bundle,
pass the existing member or file bytes through so each binary is read only once
while preserving the current missing-metadata validation.
In `@rest-api/scripts/test_rest_command_bundle.py`:
- Around line 144-160: Update the invalid-manifest cases at
rest-api/scripts/test_rest_command_bundle.py:144-160 and
rest-api/scripts/test_rest_command_bundle.py:439-456 to pair each mutation with
its expected error message and assert it with assertRaisesRegex rather than
checking only BundleError. In set_unsupported_target, locate the linux-amd64
target by its name instead of targets[0] so the intended unsupported-target
guard is exercised; for the unsafe name case, assert the message from the
exact_contract name-comparison guard in validate_resolved_manifest, which
currently handles the mutation.
- Around line 216-267: Add tests alongside
test_file_description_accepts_known_static_wording for darwin-arm64 using the
Mach-O 64-bit and arm64 markers, covering valid file descriptions. Add a
matching architecture-mismatch case that passes an incorrect architecture marker
to bundle.validate_file_description and asserts BundleError, while preserving
the existing Linux coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 00a58d66-eac8-4f47-bba4-31d8dfc7e928
📒 Files selected for processing (9)
.github/workflows/rest-build-binaries.yml.github/workflows/rest-ci.yml.github/workflows/rest-lint-and-test.yml.github/workflows/rest-prepare-build-info.ymlrest-api/Makefilerest-api/scripts/rest-command-bundles.mdrest-api/scripts/rest-command-manifest.jsonrest-api/scripts/rest_command_bundle.pyrest-api/scripts/test_rest_command_bundle.py
🚧 Files skipped from review as they are similar to previous changes (7)
- rest-api/Makefile
- rest-api/scripts/rest-command-bundles.md
- .github/workflows/rest-ci.yml
- rest-api/scripts/rest-command-manifest.json
- .github/workflows/rest-lint-and-test.yml
- .github/workflows/rest-prepare-build-info.yml
- .github/workflows/rest-build-binaries.yml
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai review, thanks! |
|
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rest-api/scripts/rest_command_bundle.py (2)
367-377: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the space-separated
-Xform.
metadata_valuesrecognizes only the-X=name=valueform. If a future manifest entry uses-X name=value, this function returns no values, and the embedded-metadata check at Lines 462-471 silently becomes a no-op. The manifest currently uses only the=form, so no defect exists today.To make the contract explicit, reject the unsupported form instead of ignoring it.
♻️ Proposed hardening
for flag in ldflags: if not flag.startswith("-X="): + if flag == "-X" or flag.startswith("-X "): + raise BundleError( + f"-X linker flags must use the -X=name=value form: {flag}" + ) continue🤖 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 `@rest-api/scripts/rest_command_bundle.py` around lines 367 - 377, Update metadata_values to explicitly detect space-separated -X flags such as "-X name=value" and raise BundleError for that unsupported form instead of silently skipping it. Preserve the existing extraction and validation behavior for "-X=name=value" flags and ignore unrelated linker flags.
581-628: 🚀 Performance & Scalability | 🔵 TrivialConsider bounding the in-memory member size.
read_bundlereads every member fully intoentries, andverify_bundlethen writes each binary to a temporary directory. For a 37-command inventory the verifier holds the complete binary set in memory. On the CI runners this is acceptable today.If the inventory grows, add a per-member and total size limit derived from the resolved manifest
sizefields beforesource.read(). That bounds verifier memory and also rejects an oversized member early.🤖 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 `@rest-api/scripts/rest_command_bundle.py` around lines 581 - 628, Add per-member and aggregate size limits to read_bundle, using the resolved manifest size fields as the source of truth before calling source.read(). Reject any member exceeding its declared limit or the total allowed size, and preserve existing validation and entry-loading behavior for valid bundles.
🤖 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.
Nitpick comments:
In `@rest-api/scripts/rest_command_bundle.py`:
- Around line 367-377: Update metadata_values to explicitly detect
space-separated -X flags such as "-X name=value" and raise BundleError for that
unsupported form instead of silently skipping it. Preserve the existing
extraction and validation behavior for "-X=name=value" flags and ignore
unrelated linker flags.
- Around line 581-628: Add per-member and aggregate size limits to read_bundle,
using the resolved manifest size fields as the source of truth before calling
source.read(). Reject any member exceeding its declared limit or the total
allowed size, and preserve existing validation and entry-loading behavior for
valid bundles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5c523360-fb7c-46b2-94dd-6bbafa25ff91
📒 Files selected for processing (9)
.github/workflows/rest-build-binaries.yml.github/workflows/rest-ci.yml.github/workflows/rest-lint-and-test.yml.github/workflows/rest-prepare-build-info.ymlrest-api/Makefilerest-api/scripts/rest-command-bundles.mdrest-api/scripts/rest-command-manifest.jsonrest-api/scripts/rest_command_bundle.pyrest-api/scripts/test_rest_command_bundle.py
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/rest-prepare-build-info.yml
- rest-api/scripts/rest-command-bundles.md
- rest-api/Makefile
- .github/workflows/rest-lint-and-test.yml
- .github/workflows/rest-ci.yml
- rest-api/scripts/rest-command-manifest.json
- .github/workflows/rest-build-binaries.yml
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
Overall looks good! Wondering if we could simplify the implementation so we don't have to maintain a manifest.
| run: | | ||
| set -euo pipefail | ||
|
|
||
| full_sha=$(git rev-parse HEAD) |
There was a problem hiding this comment.
@chet This job already produces a number of values https://github.com/NVIDIA/infra-controller/blob/main/.github/workflows/ci.yaml#L103, should we use identifiers from there instead?
| files and checks them without trusting the archive to describe itself. | ||
| """ | ||
|
|
||
| from __future__ import annotations |
There was a problem hiding this comment.
Should we try to contain the logic for target based builds within the workflows?
There was a problem hiding this comment.
Yes so! ..I rebased onto #4644 (once it went in), which pulls the target scheduling into the workflow now. rest-build-binaries.yml starts three separate build jobs (x86, ARM, and macOS ARM), and then the Python helper owns the command/output "contract" inside each job. I kept that split, because the packaging follow-up needs that same inventory. Does that make sense/work?
REST was building each command in its own job, which repeated checkout, Go setup, and dependency download 11 times before compiling the same three targets. So, each target now gets one verified command bundle while lint and test run beside compilation. Primary callouts are: - `.github/ci/rest-command-manifest.json` defines the 37-output contract: 13 binaries for each Linux architecture and 11 for Darwin, with runtime linker metadata for the version-reporting commands and the existing standalone target/CGO boundaries. - `.github/ci/rest_command_bundle.py` builds from the prepared candidate identity, rejects dirty CI sources, and verifies package paths, architecture, linkage, metadata, checksums, permissions, and inventory before uploading one bundle per target. - `rest-ci.yml` starts the three target jobs beside `rest-lint-and-test.yml`; `rest-ci-pass` still requires both branches to succeed, so this removes repeated setup without weakening the required check. - **Expected green-run effect:** roughly zero to one minute from this PR by itself. - **What it really buys us:** about 8--18 fewer runner-minutes and the checked binary inputs NVIDIA#4582 needs to stop compiling service commands again inside Docker. Tests added! This supports NVIDIA#4581 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4618.docs.buildwithfern.com/infra-controller |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/ci/test_rest_command_bundle.py (1)
458-469: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative cases for the member-order rule and the remaining identity rules.
The suite covers the archive format and the manifest contract well. Two enforced rules have no negative test:
read_bundlerejects out-of-order members atrest_command_bundle.pylines 667-673.test_bundle_is_deterministic_and_preserves_modesonly reads a correctly ordered archive, so a regression that drops the order check would still pass.write_archivealready lets you emit members in the wrong order.- The mutation table at lines 526-565 omits
source_dirty,version,build_timestamp,vcs_revision, and non-positivesize.validate_resolved_manifestenforces all five, and they are the rules that stop an uploaded bundle from choosing its own identity.Both additions reuse the existing fixtures.
💚 Suggested additions
+ def test_bundle_rejects_out_of_order_members(self) -> None: + members = [ + self.regular_member("SHA256SUMS", b"", 0o644), + self.regular_member("manifest.json", b"{}\n", 0o644), + ] + with tempfile.TemporaryDirectory() as temp: + path = Path(temp) / "bundle.tar.gz" + self.write_archive(path, members) + with self.assertRaisesRegex( + bundle.BundleError, "not in deterministic order" + ): + bundle.read_bundle(path)Extend the mutation table in
test_resolved_manifest_matches_authoritative_contract:"floating-point schema version": ( lambda changed: changed.__setitem__("schema_version", 1.0), "resolved manifest has an unsupported schema", ), + "dirty source": ( + lambda changed: changed.__setitem__("source_dirty", True), + "source_dirty does not match the expected source state", + ), + "wrong version": ( + lambda changed: changed.__setitem__("version", "other"), + "resolved manifest version is", + ), + "wrong build timestamp": ( + lambda changed: changed.__setitem__( + "build_timestamp", "2026-08-05T12:34:57Z" + ), + "resolved manifest build_timestamp is", + ), + "foreign vcs revision": ( + lambda changed: changed["outputs"][0].__setitem__( + "vcs_revision", "4" * 40 + ), + "vcs_revision does not match the candidate", + ), + "zero size": ( + lambda changed: changed["outputs"][0].__setitem__("size", 0), + "size must be a positive integer", + ),Also applies to: 526-565
🤖 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/ci/test_rest_command_bundle.py around lines 458 - 469, Extend the bundle test coverage by adding a negative case that passes members in the wrong order to write_archive and asserts read_bundle raises bundle.BundleError. Update test_resolved_manifest_matches_authoritative_contract’s mutation table to cover mutations of source_dirty, version, build_timestamp, vcs_revision, and non-positive size, reusing the existing fixtures and asserting each mutation is rejected..github/ci/rest_command_bundle.py (1)
1062-1103: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider streaming binary bytes instead of retaining every command in memory.
build_bundleaccumulates the complete contents of every command inbinary_contentsand holds them untilcreate_bundlefinishes. Each binary is also read fully twice: once at line 506 insideinspect_binaryfor the-Xmetadata scan, and again at line 1102. For the Linux targets that means 13 Go binaries resident at the same time, so peak memory tracks the total bundle size rather than the largest single binary.The binaries already exist on disk in
bin_root.create_bundlecan accept paths and stream each member throughtarfile.addfile(header, fileobj=...), andsha256_filecan replace the in-memory digest. That keeps peak memory bounded by the largest binary.This is a scaling characteristic rather than a present defect, so treat it as optional hardening for the runner.
🤖 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/ci/rest_command_bundle.py around lines 1062 - 1103, Optionally harden build_bundle to avoid retaining complete binary contents in binary_contents: preserve each built binary’s path, update create_bundle to accept paths and stream members with tarfile.addfile, and use sha256_file for digest calculation. Keep inspect_binary behavior unchanged while ensuring bundle creation and manifest generation no longer require loading every binary into memory..github/workflows/rest-build-binaries.yml (1)
66-71: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound
resolve-manual-identitywith a job timeout.
build-binariessetstimeout-minutes: 30at line 130. This job has no bound, so it inherits the 360-minute default. It performs a full-depth checkout, which is the slowest step in the reusable workflow. A stalled fetch would hold a runner for six hours.♻️ Proposed bound
if: inputs.identity_source == 'selected-ref' runs-on: ${{ inputs.runner || 'linux-amd64-cpu4' }} + timeout-minutes: 10 outputs:🤖 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/rest-build-binaries.yml around lines 66 - 71, Add a job-level timeout to resolve-manual-identity, matching the 30-minute bound used by build-binaries. Place timeout-minutes within the resolve-manual-identity job definition so full-depth checkout and any stalled fetch cannot use the default six-hour limit.Source: Path instructions
.github/workflows/rest-prepare-build-info.yml (1)
138-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffExtract the shared binary-version derivation into one script. The identity precedence rule — main-branch semantic version, then release tag, then sanitized branch plus short SHA, then failure — is now implemented twice in shell. Both copies also repeat the same branch sanitization pipeline and the same
^[a-zA-Z0-9][a-zA-Z0-9._-]*$safety regex. The inline comments at both sites instruct maintainers to keep the two copies aligned manually, which confirms the drift hazard rather than removing it. A single script under.github/ci/, invoked by both jobs, would make the rule impossible to change in one place only. The two copies agree today, so treat this as a maintainability improvement rather than a defect.
.github/workflows/rest-prepare-build-info.yml#L138-L153: replace the inline precedence chain and validation regex with a call to the shared script, then read the derived version from its output..github/workflows/rest-build-binaries.yml#L95-L114: replace the duplicated sanitization, precedence chain, and validation regex inresolve-manual-identitywith the same shared script call.🤖 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/rest-prepare-build-info.yml around lines 138 - 153, Extract the shared binary-version derivation, including branch sanitization, precedence, failure handling, and safety validation, into one script under .github/ci/. In .github/workflows/rest-prepare-build-info.yml#L138-L153 and .github/workflows/rest-build-binaries.yml#L95-L114, replace the duplicated inline logic with calls to that script and consume its derived version output; remove the manual alignment comments and preserve the existing precedence and error behavior at both sites.
🤖 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/rest-lint-and-test.yml:
- Around line 35-39: Update the workflow job running check-command-bundle to use
a Python-version matrix containing 3.10 and 3.13, and reference the matrix value
in the setup-python step. Preserve the existing check execution while adding
coverage for the supported minimum Python version.
---
Nitpick comments:
In @.github/ci/rest_command_bundle.py:
- Around line 1062-1103: Optionally harden build_bundle to avoid retaining
complete binary contents in binary_contents: preserve each built binary’s path,
update create_bundle to accept paths and stream members with tarfile.addfile,
and use sha256_file for digest calculation. Keep inspect_binary behavior
unchanged while ensuring bundle creation and manifest generation no longer
require loading every binary into memory.
In @.github/ci/test_rest_command_bundle.py:
- Around line 458-469: Extend the bundle test coverage by adding a negative case
that passes members in the wrong order to write_archive and asserts read_bundle
raises bundle.BundleError. Update
test_resolved_manifest_matches_authoritative_contract’s mutation table to cover
mutations of source_dirty, version, build_timestamp, vcs_revision, and
non-positive size, reusing the existing fixtures and asserting each mutation is
rejected.
In @.github/workflows/rest-build-binaries.yml:
- Around line 66-71: Add a job-level timeout to resolve-manual-identity,
matching the 30-minute bound used by build-binaries. Place timeout-minutes
within the resolve-manual-identity job definition so full-depth checkout and any
stalled fetch cannot use the default six-hour limit.
In @.github/workflows/rest-prepare-build-info.yml:
- Around line 138-153: Extract the shared binary-version derivation, including
branch sanitization, precedence, failure handling, and safety validation, into
one script under .github/ci/. In
.github/workflows/rest-prepare-build-info.yml#L138-L153 and
.github/workflows/rest-build-binaries.yml#L95-L114, replace the duplicated
inline logic with calls to that script and consume its derived version output;
remove the manual alignment comments and preserve the existing precedence and
error behavior at both sites.
🪄 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: Enterprise
Run ID: 4213283e-f09e-4be7-80cd-64956dce9787
📒 Files selected for processing (9)
.github/ci/rest-command-bundles.md.github/ci/rest-command-manifest.json.github/ci/rest_command_bundle.py.github/ci/test_rest_command_bundle.py.github/workflows/rest-build-binaries.yml.github/workflows/rest-ci.yml.github/workflows/rest-lint-and-test.yml.github/workflows/rest-prepare-build-info.ymlrest-api/Makefile
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.13' | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 3 'setup-python|python-version|check-command-bundle' .github/workflowsRepository: NVIDIA/infra-controller
Length of output: 2524
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Makefile contract ---'
makefile="$(git ls-files | awk '$0 == "rest-api/Makefile" {print; exit}')"
if [ -z "$makefile" ]; then
printf '%s\n' 'rest-api/Makefile not found'
else
nl -ba "$makefile" | sed -n '100,125p'
fi
printf '%s\n' '--- All workflow Python setup and check coverage ---'
while IFS= read -r workflow; do
printf '\n[%s]\n' "$workflow"
rg -n -C 4 'setup-python|python-version|check-command-bundle' "$workflow" || true
done < <(git ls-files '.github/workflows/**')
printf '%s\n' '--- Repository Python-version declarations ---'
rg -n -i -C 2 'python( |-|_)version|requires-python|python_requires|3\.10|3\.11|3\.12|3\.13' \
--glob '!*.lock' --glob '!vendor/**' --glob '!node_modules/**' . || trueRepository: NVIDIA/infra-controller
Length of output: 191
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- Makefile contract ---'
if [ -f rest-api/Makefile ]; then
nl -ba rest-api/Makefile | sed -n '100,125p'
else
printf '%s\n' 'rest-api/Makefile not found'
fi
printf '%s\n' '--- Workflow coverage ---'
rg -n -C 4 'setup-python|python-version|check-command-bundle' .github/workflows || true
printf '%s\n' '--- Python-version declarations ---'
rg -n -i -C 2 'python( |-|_)version|requires-python|python_requires|3\.10|3\.11|3\.12|3\.13' \
--glob '!*.lock' --glob '!vendor/**' --glob '!node_modules/**' . || trueRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Makefile contract ---'
nl -ba rest-api/Makefile | sed -n '108,118p'
printf '%s\n' '--- Workflow files ---'
git ls-files '.github/workflows/*'
printf '%s\n' '--- Python setup and command-bundle checks in workflows ---'
for workflow in $(git ls-files '.github/workflows/*'); do
matches=$(rg -n 'setup-python|python-version|check-command-bundle' "$workflow" || true)
if [ -n "$matches" ]; then
printf '\n[%s]\n%s\n' "$workflow" "$matches"
fi
done
printf '%s\n' '--- Focused Python declarations ---'
for file in pyproject.toml setup.py setup.cfg tox.ini Pipfile .python-version; do
if [ -f "$file" ]; then
printf '\n[%s]\n' "$file"
rg -n -i 'python|3\.10|3\.11|3\.12|3\.13' "$file" || true
fi
doneRepository: NVIDIA/infra-controller
Length of output: 227
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Makefile contract ---'
sed -n '108,118p' rest-api/Makefile
printf '%s\n' '--- Workflow files ---'
git ls-files '.github/workflows/*'
printf '%s\n' '--- Python setup and command-bundle checks in workflows ---'
for workflow in $(git ls-files '.github/workflows/*'); do
matches=$(rg -n 'setup-python|python-version|check-command-bundle' "$workflow" || true)
if [ -n "$matches" ]; then
printf '\n[%s]\n%s\n' "$workflow" "$matches"
fi
done
printf '%s\n' '--- Focused Python declarations ---'
for file in pyproject.toml setup.py setup.cfg tox.ini Pipfile .python-version; do
if [ -f "$file" ]; then
printf '\n[%s]\n' "$file"
rg -n -i 'python|3\.10|3\.11|3\.12|3\.13' "$file" || true
fi
doneRepository: NVIDIA/infra-controller
Length of output: 1861
Add Python 3.10 coverage for check-command-bundle.
rest-api/Makefile requires Python 3.10 or newer, but this check runs only on Python 3.13. Add a Python-version matrix that includes the supported lower bound.
🤖 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/rest-lint-and-test.yml around lines 35 - 39, Update the
workflow job running check-command-bundle to use a Python-version matrix
containing 3.10 and 3.13, and reference the matrix value in the setup-python
step. Preserve the existing check execution while adding coverage for the
supported minimum Python version.
Source: Path instructions
@thossain-nv @nv-dmendoza Yeah -- I re-looked at this a bit re: manifest, but I think per our side-discussion(s), the manifest part is part of the architecture in this case to allow pipeline cooperation. The workflow owns target scheduling, and the manifest gives the build and packaging steps one shared config of binaries, filenames, compiler/linker settings, and expected outputs. If we don't like it, totally understand, but to me I see it as us stepping out of basic workflows and going into more of an "advanced" mode to better approach/handle what is becoming a pretty complex CICD infrastructure. |
3 participants
REST was building each of its 11 standalone commands in a separate job. Every job repeated checkout, Go setup, and dependency download before building the same Linux amd64, Linux arm64, and Darwin arm64 targets -- and then the production image jobs compiled the service commands again later.
So, this turns the standalone side around: one job owns each target, builds its complete command inventory once, verifies what it produced, and uploads one bundle. Compilation also starts beside lint and test now;
rest-ci-passstill requires all of them to succeed.12m11s, but other jobs varied, so we are not claiming its entire1m47sbaseline difference as this PR's savings.16m58sfewer standalone runner-minutes, eight fewer jobs, and checked bundles ready7m32searlier. Those bundles are the inputs Package REST Service Images Without Recompiling Go Commands #4582 needs to stop compiling the ten service commands again inside Docker.Primary callouts are:
.github/ci/rest-command-manifest.jsonis the source-controlled 37-output contract: 13 binaries for each Linux architecture and 11 for Darwin. It records the package, target availability, CGO mode, output name, and runtime linker metadata that the builder and future packaging consumer must agree on..github/ci/rest_command_bundle.pychecks the exact candidate identity and source state, builds one target, then verifies package paths, Go build settings, physical architecture, linkage, embedded metadata, checksums, permissions, and exact inventory before the bundle can be uploaded.Measured result
This compares the last full hosted run before the review-hardening follow-up (REST run) with the representative pre-change REST baseline:
113-8(-72.7%)27m33s10m35s-16m58s(-61.6%)4m34s4m00s-34s(-12.4%)12m55s5m23s-7m32s(-58.3%)739,263,406582,672,278-156,591,128(-21.2%)That REST run passed 52 jobs with 12 expected skips on attempt 1. Its summary and
rest-ci-passgate both succeeded. Lint, tests, and image builds remain longer than the standalone lane, so the 34-second lane change is the direct green-run credit; the earlier bundle availability and runner-time reduction are what enable the larger follow-up.Related issues
This supports #4581
Part of #4572
Prepares #4582
Type of Change
Breaking Changes
Testing
Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)
An earlier branch head built and verified all 37 outputs locally: Linux amd64 in 51 seconds, Linux arm64 in 44 seconds, and Darwin arm64 in 51 seconds.
After the latest review changes, rebuilt and verified Darwin arm64's complete 11-command bundle in one local run; the compressed bundle was 247,931,456 bytes.
Passed all 19 bundle unit tests plus the 37-output source-contract check.
Passed
make check-command-bundle, Python bytecode compilation,actionlintwith ShellCheck,git diff --check, andrumdl.Passed
cargo make format-nightly,cargo make clippy, andcargo make carbide-lintsusing the isolated PR target.The last fully hosted pre-feedback SHA passed Core CI, REST CI, and the PR-description workflow on attempt 1. Hosted checks for the amended review head are pending.
Additional Notes
The REST API is a nested Go module, so Go does not expose
vcs.revisionin these binaries. The builder verifies the exact Git HEAD and a clean source tree before compilation; the resolved manifest reportsunavailableinstead of claiming metadata the binary does not contain.This PR intentionally leaves the production Dockerfiles compiling as they do today. #4582 is where those image builds switch to the verified Linux bundles, so we can measure this consolidation on its own first.