fix(security): extend Perl remediation to sibling images - #7844
fix(security): extend Perl remediation to sibling images#7844senthilr-nv wants to merge 10 commits into
Conversation
senthilr-nv
added
dependencies
|
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:
📝 WalkthroughWalkthroughThe PR adds deterministic Perl 5.44 Debian package creation and runtime validation to managed base images, and changes Hermes and Deep Agents Code publication to native per-platform digest builds followed by validated multi-platform manifests. ChangesManaged base-image Perl remediation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NativeRunners
participant DigestArtifacts
participant ManifestJob
NativeRunners->>DigestArtifacts: upload amd64 and arm64 image digests
DigestArtifacts->>ManifestJob: download platform digests
ManifestJob->>ManifestJob: generate publication tags
ManifestJob->>ManifestJob: create validated multi-platform manifest
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
Dockerfile.base (1)
228-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInventory records the Perl packages one layer before they are installed.
security-packages.txtis written in the layer ending at Line 235, while the Perl.debfiles are installed and verified in the nextRUN(Lines 237-255). The build still fails closed if that later layer fails, so this is only an ordering nit; moving these two entries into the Perl install layer would keep the recorded inventory true at write time.🤖 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 `@Dockerfile.base` around lines 228 - 229, Move the perl-base and perl entries from the current security-packages inventory block into the later RUN layer that installs and verifies the Perl .deb files, so security-packages.txt records them only after installation in that same layer.test/perl-critical-cve-remediation.test.ts (1)
227-232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocumentation-prose assertions add little behavioral confidence.
toContain("This change does not cross an upstream Perl release range")andtoContain("vulnerability rescan")lock the review document's wording rather than any contract; a copy edit breaks CI without a defect. ThePERL-01/PERL-07ledger-ID checks are the durable part — consider keeping those and dropping the sentence-level matches.As per path instructions: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/perl-critical-cve-remediation.test.ts` around lines 227 - 232, Update the test case “records the migration boundary and external proof gates (`#7338`)” to remove sentence-level dependencyReview assertions for “This change does not cross an upstream Perl release range” and “vulnerability rescan”. Retain the durable PERL-01 and PERL-07 ledger-ID assertions, along with the existing observable platform coverage assertion unless it is similarly wording-dependent.Source: Path instructions
scripts/security/build-perl-security-packages.sh (1)
30-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment block describes four unrelated decisions but sits above the download.
Lines 30-43 stack the
d_syscallprotorationale, theTEST_JOBSrationale, and theExtUtils::Constantsplit rationale directly above thecurlinvocation they do not describe. Moving each paragraph next to the code it justifies (./Configure, thetest_harnessinvocations) keeps the removal conditions discoverable when those lines change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/security/build-perl-security-packages.sh` around lines 30 - 43, Reposition the explanatory comment paragraphs in the build script so each decision is adjacent to the code it governs: place the d_syscallproto rationale by ./Configure, the TEST_JOBS rationale by the test_harness invocations, and the ExtUtils::Constant split rationale by its isolated and parallel test commands. Keep each paragraph’s removal conditions and leave unrelated download steps without these comments.test/dcode-base-image-workflow.test.ts (1)
201-202: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the expected digest-output literal.
The same
type=image,...,push-by-digest=true,...string is repeated at Lines 506-508. A module-level constant keeps both assertions in sync when the output contract changes.🤖 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 `@test/dcode-base-image-workflow.test.ts` around lines 201 - 202, Define a module-level constant for the shared expected digest-output literal, then replace the duplicated expected-output strings in the assertions near the existing declaration and the later digest-output assertion with that constant so both remain synchronized..github/workflows/base-image.yaml (1)
353-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPublication script is duplicated verbatim in
build-and-push-openclaw.Lines 353-399 and Lines 439-485 are byte-identical apart from the image expression. The atomicity guarantee now depends on two copies staying in sync. Folding OpenClaw into this matrixed job (or extracting the step into a composite action) removes the divergence risk.
As per path instructions: "Derive job inventories and aggregate dependencies from one source of truth or validate them deterministically."
🤖 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/base-image.yaml around lines 353 - 399, The multi-platform manifest publication script is duplicated between the matrix job and build-and-push-openclaw, creating divergence risk. Consolidate both paths by folding OpenClaw into the matrix-driven job or extracting the shared Create and verify multi-platform manifest logic into a reusable composite action, while preserving the existing digest validation, tag creation, and platform verification 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.
Inline comments:
In @.github/workflows/base-image.yaml:
- Around line 307-310: Change the build-and-push job in
.github/workflows/base-image.yaml (lines 307-310) so each matrix image manifest
depends only on its own amd64 and arm64 builds, rather than the shared
build-sibling-platforms job; use per-image platform jobs or keyed digest
artifacts. Update docs/security/managed-base-perl-5.44-dependency-review.md
(lines 52-54) to retain the per-image atomicity wording and PERL-07 disposition,
and update test/dcode-base-image-workflow.test.ts (line 515) so
manifestJob?.needs expects the narrowed per-image dependency.
In `@scripts/security/build-perl-security-packages.sh`:
- Around line 83-87: Update the grep pattern in the test guard to match the
emitted ../cpan/ExtUtils-Constant/t/Constant.t path, while preserving the
exact-count check and existing build gate behavior.
In `@test/dcode-base-image-workflow.test.ts`:
- Line 515: Update the assertion for manifestJob.needs in the relevant test to
match the per-image dependency behavior instead of requiring the full
build-sibling-platforms matrix. Keep the expectation aligned with the narrowed
needs value defined by the workflow and preserve coverage of manifest job
dependency gating.
In `@test/perl-critical-cve-remediation.test.ts`:
- Around line 92-107: Add assertions in the managed-images loop around
PERL_PACKAGE_REVISION and the perl-builder output to verify the installed
perl-base package version via dpkg-query matches fixedPackageVersion. Ensure the
test catches Dockerfiles whose PERL_PACKAGE_REVISION default differs from the
recorded inventory, while preserving the existing PERL_VERSION, PERL_SHA256, and
builder assertions.
---
Nitpick comments:
In @.github/workflows/base-image.yaml:
- Around line 353-399: The multi-platform manifest publication script is
duplicated between the matrix job and build-and-push-openclaw, creating
divergence risk. Consolidate both paths by folding OpenClaw into the
matrix-driven job or extracting the shared Create and verify multi-platform
manifest logic into a reusable composite action, while preserving the existing
digest validation, tag creation, and platform verification behavior.
In `@Dockerfile.base`:
- Around line 228-229: Move the perl-base and perl entries from the current
security-packages inventory block into the later RUN layer that installs and
verifies the Perl .deb files, so security-packages.txt records them only after
installation in that same layer.
In `@scripts/security/build-perl-security-packages.sh`:
- Around line 30-43: Reposition the explanatory comment paragraphs in the build
script so each decision is adjacent to the code it governs: place the
d_syscallproto rationale by ./Configure, the TEST_JOBS rationale by the
test_harness invocations, and the ExtUtils::Constant split rationale by its
isolated and parallel test commands. Keep each paragraph’s removal conditions
and leave unrelated download steps without these comments.
In `@test/dcode-base-image-workflow.test.ts`:
- Around line 201-202: Define a module-level constant for the shared expected
digest-output literal, then replace the duplicated expected-output strings in
the assertions near the existing declaration and the later digest-output
assertion with that constant so both remain synchronized.
In `@test/perl-critical-cve-remediation.test.ts`:
- Around line 227-232: Update the test case “records the migration boundary and
external proof gates (`#7338`)” to remove sentence-level dependencyReview
assertions for “This change does not cross an upstream Perl release range” and
“vulnerability rescan”. Retain the durable PERL-01 and PERL-07 ledger-ID
assertions, along with the existing observable platform coverage assertion
unless it is similarly wording-dependent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7594beef-9c6b-4992-997d-a1113a22ea78
📒 Files selected for processing (9)
.github/workflows/base-image.yamlDockerfile.baseagents/hermes/Dockerfile.baseagents/langchain-deepagents-code/Dockerfile.baseci/source-shape-test-budget.jsondocs/security/managed-base-perl-5.44-dependency-review.mdscripts/security/build-perl-security-packages.shtest/dcode-base-image-workflow.test.tstest/perl-critical-cve-remediation.test.ts
💤 Files with no reviewable changes (1)
- ci/source-shape-test-budget.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/base-image.yaml:
- Around line 437-483: The Hermes manifest step at
.github/workflows/base-image.yaml lines 437-483 must validate each digest source
resolves to exactly linux/amd64 and linux/arm64 before docker buildx imagetools
create publishes any tags; preserve artifact architecture provenance, reject
duplicates or mismatches, and only then create and inspect the manifest. Apply
the same pre-publication source-platform validation to the Deep Agents Code step
at .github/workflows/base-image.yaml lines 521-567, leaving both publication
flows gated on the exact two required architectures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 57de819d-55a8-4ab0-9d42-369f23d02ade
📒 Files selected for processing (8)
.github/workflows/base-image.yamlDockerfile.baseagents/hermes/Dockerfile.baseagents/langchain-deepagents-code/Dockerfile.baseci/source-shape-test-budget.jsonscripts/security/build-perl-security-packages.shtest/dcode-base-image-workflow.test.tstest/perl-critical-cve-remediation.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- agents/langchain-deepagents-code/Dockerfile.base
- scripts/security/build-perl-security-packages.sh
- test/perl-critical-cve-remediation.test.ts
- agents/hermes/Dockerfile.base
- test/dcode-base-image-workflow.test.ts
…ibling-images Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Product-scope and CI handoff for exact head The linked #7338 defines early-warning/rescan behavior and explicitly requires product/security ownership for maintained-image scope; it does not accept the separate decision to build, package, install, and publish Perl 5.44 across the Hermes and Deep Agents Code base images. Before this can be approved as canonical NemoClaw behavior, please link an accepted issue/design decision that names those image consumers and defines ownership, lifecycle/removal criteria, architecture compatibility, security validation, and publication evidence. This is the repository Product Scope Gate, so I am not modifying the branch while that decision is missing. The current head is also materially red: both image builds and CLI test shards fail, the plain-progress/Dockerfile contract rejects the added |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
ACK — the product-scope and CI findings were valid. The PR description now records the intended product boundary: this is a security remediation for the existing managed OpenClaw, Hermes, and Deep Agents Code images, not a new image or integration. It also defines NemoClaw ownership, the Debian removal condition, native architecture support, and required security/publication evidence. No code changes were made for product scope. Head
Local evidence on the exact head: 5 focused integration files / 55 tests passed, |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Follow-up head |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/helpers/base-apt-security-functions.ts`:
- Around line 103-104: Model perl-base and perl installation independently: in
test/helpers/base-apt-security-functions.ts lines 103-104, return each package
version only when its own installation marker is set; in lines 188-202, gate the
runtime double on the perl-base marker. In test/hermes-share-mount-deps.test.ts
lines 81-82 and test/sandbox-base-security-packages.test.ts lines 134-135,
record both package paths independently. In
test/sandbox-base-security-packages.test.ts line 177, initialize both markers
for the completed-image fixture.
In `@test/sandbox-base-security-packages.test.ts`:
- Around line 143-147: Update the test expectations around includesPerl and the
prepared inventory so OpenClaw follows the same Perl inventory and immutable
0444 mode contract as Hermes and Deep Agents Code. Remove the
image-name-specific exception and require the shared Perl inventory for every
managed image.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 32dc9d10-b183-4e1a-98e9-c4cf91864462
📒 Files selected for processing (10)
Dockerfileagents/hermes/Dockerfileagents/langchain-deepagents-code/Dockerfilescripts/check-dcode-profile-import-gate.shscripts/security/build-perl-security-packages.shtest/helpers/base-apt-security-functions.tstest/hermes-share-mount-deps.test.tstest/langchain-deepagents-code-profile-build-gate.test.tstest/perl-critical-cve-remediation.test.tstest/sandbox-base-security-packages.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/sandbox-base-security-packages.test.ts (1)
175-175: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not pre-seed
perl_packages_installedin the completed-image contract mock.Setting this marker before executing the extracted verification command makes
dpkg-queryreport pinned Perl versions even if the Dockerfile omitted or failed to install the Perl packages. Set the marker only when the mocked install path requests the Perl artifact, or explicitly assert that the completed-image install command contains and executes both pinned Perl packages.As per path instructions, tests must not use broad mocks that bypass the behavior under test.
🤖 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 `@test/sandbox-base-security-packages.test.ts` at line 175, Remove the pre-seeded perl_packages_installed marker from the completed-image contract mock. Update the mocked install path to set it only when the Perl artifact is requested, and ensure the verification asserts both pinned Perl packages are actually installed without broad mocks bypassing the behavior under test.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.
Outside diff comments:
In `@test/sandbox-base-security-packages.test.ts`:
- Line 175: Remove the pre-seeded perl_packages_installed marker from the
completed-image contract mock. Update the mocked install path to set it only
when the Perl artifact is requested, and ensure the verification asserts both
pinned Perl packages are actually installed without broad mocks bypassing the
behavior under test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5242027c-b1cf-48ed-9a0d-f5a43dd36292
📒 Files selected for processing (1)
test/sandbox-base-security-packages.test.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Superseded by internal PR #7864. The branch now lives directly in NVIDIA/NemoClaw so trusted CI can run without the fork credential-control path; the rebased commits remain signed and GitHub Verified. |
|
The exact-head protected run blocks approval at Two concrete image failures are visible in the selected evidence:
The ordinary suite, both primary review-advisor lanes, DCO, and static security checks are green, and I found no live unresolved review thread. However, the protected image/lifecycle evidence is directly in scope for this three-image remediation and cannot be waived as unrelated infrastructure. The branch also needs refresh onto current I will re-review the next quiet revision. This is a plain review comment, not Changes Requested. |
2 participants
Summary
Hermes and Deep Agents Code base images currently retain Debian Perl 5.40.1 even though the OpenClaw base already installs the reviewed Perl 5.44.0 security packages.
This change gives all three managed images the same checksum-pinned, fully tested Perl build and publishes the sibling multi-platform images from native runners.
Product Scope
This remediation stays within NemoClaw's existing managed OpenClaw, Hermes, and Deep Agents Code base-image surface. It does not add a new image, integration, or supported workflow.
NemoClaw maintainers own the temporary Perl packaging lifecycle. Remove it when Debian trixie provides an acceptable fixed package. Compatibility and publication evidence cover native
linux/amd64andlinux/arm64builds, checksum-bound source, the complete selected upstream test set, package and runtime probes, immutable inventory checks, and atomic multi-platform publication.Changes
Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/security/managed-base-perl-5.44-dependency-review.mdremains accurate and sufficient; no additional user-facing Fern page or changelog entry is needed./root/documentation_writer_review)DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable97c5bbb09.npm run check:diffpassed against refreshedmain.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Senthil Ravichandran senthilr@nvidia.com