fix(docker): classify OpenClaw patch drift#3869
Conversation
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
📝 WalkthroughWalkthroughRefactors the Dockerfile OpenClaw fetch-guard patch into a build-time classifier that applies, skips, or fails with diagnostics based on dist shape, and adds sandboxed test helpers plus comprehensive regression tests covering all patch outcomes. ChangesFetch-guard patch classification and test coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
|
✨Related open issues: |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Dockerfile (1)
713-721: Verify the new healthcheck does not rely on an unstated base-image dependency.
curlis not installed anywhere in this Dockerfile, so this probe now depends on whateversandbox-basehappens to contain. Please validate it against the oldest supported base tag, or switch the probe topython3to keep it self-contained. Also cover this with the recommended Docker E2E jobs before merge. As per coding guidelines:Dockerfile: This file affects the sandbox container image. Layer ordering, permissions, and baked config changes are only testable with a real container build.🤖 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` around lines 713 - 721, The HEALTHCHECK now uses curl but the Dockerfile never installs curl, creating an unstated base-image dependency; update the HEALTHCHECK to use the existing python3 binary (replace the curl call with a small python3 HTTP probe) or explicitly install curl earlier in the Dockerfile so the probe is self-contained; locate the HEALTHCHECK block (symbol: HEALTHCHECK and the inline CMD that references curl) and either convert its final line to a python3-based check that opens http://127.0.0.1:{port}/health or add a package install step for curl in the image build and ensure you validate the change against the oldest supported base tag and E2E Docker jobs before merging.
🤖 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 `@Dockerfile`:
- Around line 713-721: The HEALTHCHECK now uses curl but the Dockerfile never
installs curl, creating an unstated base-image dependency; update the
HEALTHCHECK to use the existing python3 binary (replace the curl call with a
small python3 HTTP probe) or explicitly install curl earlier in the Dockerfile
so the probe is self-contained; locate the HEALTHCHECK block (symbol:
HEALTHCHECK and the inline CMD that references curl) and either convert its
final line to a python3-based check that opens http://127.0.0.1:{port}/health or
add a package install step for curl in the image build and ensure you validate
the change against the oldest supported base tag and E2E Docker jobs before
merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1bf94adb-547b-4bb0-a1cc-f78ff7f06fee
📒 Files selected for processing (2)
Dockerfiletest/fetch-guard-patch-regression.test.ts
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 `@test/fetch-guard-patch-regression.test.ts`:
- Around line 174-187: The test currently checks that the set
REVIEWED_OPENCLAW_PATCH_CLASSIFIER_VERSIONS contains the blueprint-pinned
version but doesn't assert that the single
CURRENT_REVIEWED_OPENCLAW_PATCH_CLASSIFIER_VERSION equals that pinned version;
update the test (near readBlueprintMinOpenClawVersion /
readDockerfileBaseOpenClawVersion usage) to add an assertion that
CURRENT_REVIEWED_OPENCLAW_PATCH_CLASSIFIER_VERSION === blueprintMinVersion so
the declared "current reviewed classifier version" cannot drift from the pinned
OpenClaw version.
🪄 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: 6d00b0f7-56a0-4a7d-9eda-a18b55c2014d
📒 Files selected for processing (1)
test/fetch-guard-patch-regression.test.ts
| it("requires classifier review when the pinned OpenClaw build version changes", () => { | ||
| const reviewMessage = | ||
| "Update fetch-guard classifier expectations before changing the OpenClaw build version."; | ||
|
|
||
| const blueprintMinVersion = readBlueprintMinOpenClawVersion(); | ||
| const baseImageVersion = readDockerfileBaseOpenClawVersion(); | ||
|
|
||
| expect(baseImageVersion, "Dockerfile.base and blueprint must pin the same OpenClaw version.").toBe( | ||
| blueprintMinVersion, | ||
| ); | ||
| expect([...REVIEWED_OPENCLAW_PATCH_CLASSIFIER_VERSIONS], reviewMessage).toContain( | ||
| blueprintMinVersion, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Assert the “current reviewed classifier version” matches the pinned OpenClaw version.
Line 184 verifies the reviewed set contains the blueprint version, but it does not verify CURRENT_REVIEWED_OPENCLAW_PATCH_CLASSIFIER_VERSION is the same pinned version. That can let the execution version drift while this guard still passes.
Suggested patch
expect([...REVIEWED_OPENCLAW_PATCH_CLASSIFIER_VERSIONS], reviewMessage).toContain(
blueprintMinVersion,
);
+ expect(
+ CURRENT_REVIEWED_OPENCLAW_PATCH_CLASSIFIER_VERSION,
+ "The current classifier test version must track the pinned OpenClaw version.",
+ ).toBe(blueprintMinVersion);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("requires classifier review when the pinned OpenClaw build version changes", () => { | |
| const reviewMessage = | |
| "Update fetch-guard classifier expectations before changing the OpenClaw build version."; | |
| const blueprintMinVersion = readBlueprintMinOpenClawVersion(); | |
| const baseImageVersion = readDockerfileBaseOpenClawVersion(); | |
| expect(baseImageVersion, "Dockerfile.base and blueprint must pin the same OpenClaw version.").toBe( | |
| blueprintMinVersion, | |
| ); | |
| expect([...REVIEWED_OPENCLAW_PATCH_CLASSIFIER_VERSIONS], reviewMessage).toContain( | |
| blueprintMinVersion, | |
| ); | |
| }); | |
| it("requires classifier review when the pinned OpenClaw build version changes", () => { | |
| const reviewMessage = | |
| "Update fetch-guard classifier expectations before changing the OpenClaw build version."; | |
| const blueprintMinVersion = readBlueprintMinOpenClawVersion(); | |
| const baseImageVersion = readDockerfileBaseOpenClawVersion(); | |
| expect(baseImageVersion, "Dockerfile.base and blueprint must pin the same OpenClaw version.").toBe( | |
| blueprintMinVersion, | |
| ); | |
| expect([...REVIEWED_OPENCLAW_PATCH_CLASSIFIER_VERSIONS], reviewMessage).toContain( | |
| blueprintMinVersion, | |
| ); | |
| expect( | |
| CURRENT_REVIEWED_OPENCLAW_PATCH_CLASSIFIER_VERSION, | |
| "The current classifier test version must track the pinned OpenClaw version.", | |
| ).toBe(blueprintMinVersion); | |
| }); |
🤖 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/fetch-guard-patch-regression.test.ts` around lines 174 - 187, The test
currently checks that the set REVIEWED_OPENCLAW_PATCH_CLASSIFIER_VERSIONS
contains the blueprint-pinned version but doesn't assert that the single
CURRENT_REVIEWED_OPENCLAW_PATCH_CLASSIFIER_VERSION equals that pinned version;
update the test (near readBlueprintMinOpenClawVersion /
readDockerfileBaseOpenClawVersion usage) to add an assertion that
CURRENT_REVIEWED_OPENCLAW_PATCH_CLASSIFIER_VERSION === blueprintMinVersion so
the declared "current reviewed classifier version" cannot drift from the pinned
OpenClaw version.
Summary
Classifies the compiled OpenClaw fetch-guard dist during the sandbox image build so known compatible layouts apply or skip patches explicitly, while unknown drift fails with OpenClaw version and dist details. This keeps mutable base-image/OpenClaw drift from producing opaque Dockerfile failures.
Related Issue
Fixes #3713
Changes
Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Additional focused verification run:
/home/yimoj/.npm/_npx/52027bd8fc0022aa/node_modules/node/bin/node ./node_modules/vitest/vitest.mjs run test/fetch-guard-patch-regression.test.tsgit diff --checkhadolint Dockerfileopenclaw@2026.4.24,openclaw@2026.5.18,openclaw@2026.5.19-beta.1,openclaw@2026.5.19-beta.2, and a no-strict-layout mutation ofopenclaw@2026.5.18codex review -c sandbox_mode="danger-full-access" --uncommittedran twice with bounded timeouts; the first actionable probe found a Patch 2 spoofed-shape skip issue, which is fixed and covered by regression tests. The final rerun timed out while invoking broadnpm testand emitted no final findings block or additional actionable finding in the saved log.Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit
Chores
Tests