perf(sandbox): reduce image build time by skipping broad permission repair#4018
Merged
Conversation
Signed-off-by: zyang-dev <267119621+zyang-dev@users.noreply.github.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request refactors the Dockerfile's legacy OpenClaw layout migration to track and conditionally apply permission repairs, adds build-step timing instrumentation to the sandbox stream processor, includes sandboxed regression tests for layout/permission handling, and centralizes entrypoint port validation test assertions. ChangesLegacy OpenClaw Layout & Conditional Permissions
Build Step Timing Instrumentation
Entrypoint Port Rejection Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Comment |
zyang-dev
changed the title
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: Review used trusted metadata and the provided diff only; no PR scripts, package-manager commands, Docker builds, or tests were executed by this advisor.; The diff was provided as truncated-if-large context; conclusions focus on visible changed hunks and trusted metadata.; No full workflow logs were reviewed, so CI internals were not independently inspected.; No passed required E2E runs for head 38f860e were available in trusted context.; Selective E2E results for prior SHAs c21bb80 and 42e229c were not counted as passed evidence for this head SHA.; PR body validation claims, build timing measurements, and freshly built sandbox validation claims were treated as untrusted evidence only.; Review thread content from PR comments was treated as untrusted evidence; only trusted metadata that two threads remain unresolved was used as a hard gate. Full advisor summaryPR Review AdvisorBase: Blocked by GitHub mergeStateStatus=BLOCKED, 2 unresolved review threads, missing required sandbox/OpenClaw E2E pass evidence for head 38f860e, and unoffset create-stream.ts monolith growth. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
Signed-off-by: zyang-dev <267119621+zyang-dev@users.noreply.github.com>
Signed-off-by: zyang-dev <267119621+zyang-dev@users.noreply.github.com>
wscurran
added
Docker
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26269896680
|
ericksoa
approved these changes
May 22, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Reviewed current head c21bb80.
I checked the Dockerfile .openclaw permission fast path, the legacy marker fallback, the create-stream timing changes, and the added sandbox-provisioning coverage. I did not find a PR-specific blocking issue.
Local validation passed:
- npx vitest run src/lib/sandbox/create-stream.test.ts test/sandbox-provisioning.test.ts
- npm run typecheck -- --pretty false
- npm run checks
- npm run build:cli
- git diff --check origin/main...HEAD
Runtime/CI evidence I used:
- self-hosted sandbox image build, arm64 build, sandbox E2E, gateway isolation E2E, and non-root smoke passed for this head.
- manually dispatched shields-config-e2e and sandbox-operations-e2e passed for this head.
Two live failures remain, but I did not attribute either to this PR: test-e2e-port-overrides is currently failing with the same empty invalid-port output pattern on unrelated PR runs, and openclaw-plugin-runtime-exdev-e2e fails because current main has reverted the OpenClaw runtime-deps EXDEV fix (c84b6f1), with the log showing the raw cross-device rename error rather than a permissions regression from this diff.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Dockerfile.base (1)
181-181: Run the recommended image-impact E2E suite before merge.Because this changes
Dockerfile.base, please run the selective nightly E2E jobs to validate runtime behavior in a real container build environment:
cloud-e2e,sandbox-survival-e2e,hermes-e2e,rebuild-openclaw-e2e.As per coding guidelines: "
Dockerfile.base: This file affects the sandbox container image... only testable with a real container build" and the listedgh workflow run nightly-e2e.yaml ...command.🤖 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` at line 181, This change updates Dockerfile.base (ARG OPENCLAW_VERSION) and must be validated by running the selective nightly E2E jobs before merging: trigger the nightly-e2e workflow and run the cloud-e2e, sandbox-survival-e2e, hermes-e2e, and rebuild-openclaw-e2e jobs against the branch to validate the real container build/runtime; if any job fails, fix the Dockerfile.base changes (related to ARG OPENCLAW_VERSION) and re-run the same suite until all pass.
🤖 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 `@scripts/patch-openclaw-tool-catalog.js`:
- Around line 275-283: When targetFiles.length === 0 you currently pick the
first match from selectionFiles via hasBuiltInToolCatalog and return {status:
"skipped-built-in"}, which hides cases where multiple selection-*.js files
contain built-in catalogs; update the logic around
selectionFiles/hasBuiltInToolCatalog to collect all matching files, and only
return {status: "skipped-built-in", file, version} if exactly one match is
found; if more than one match is found return/throw an explicit ambiguous result
(e.g. {status: "ambiguous-built-in", files: [...], version} or throw a clear
error) so the caller can notice dist shape drift instead of silently choosing
the first file.
In `@test/e2e-port-overrides.sh`:
- Around line 65-68: The current check logs info when the expected validation
text isn't found but still calls pass("$label rejected by entrypoint (exit
$rc)"), allowing false positives; change the logic so that after running grep on
"$out" for "must be an integer between 1024 and 65535" you only call
pass("$label rejected by entrypoint (exit $rc)") if the grep succeeded, and call
fail with a clear message (e.g., include $label, $rc and $out) when the grep did
not match; update the block around the grep, info, pass functions to make
failure conditional and reference the existing variables out, rc, and label and
helper functions info, pass, and fail.
---
Nitpick comments:
In `@Dockerfile.base`:
- Line 181: This change updates Dockerfile.base (ARG OPENCLAW_VERSION) and must
be validated by running the selective nightly E2E jobs before merging: trigger
the nightly-e2e workflow and run the cloud-e2e, sandbox-survival-e2e,
hermes-e2e, and rebuild-openclaw-e2e jobs against the branch to validate the
real container build/runtime; if any job fails, fix the Dockerfile.base changes
(related to ARG OPENCLAW_VERSION) and re-run the same suite until all pass.
🪄 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: dea40e96-4cfb-4539-bb9f-f4610ebe1350
📒 Files selected for processing (7)
DockerfileDockerfile.baseagents/openclaw/manifest.yamlnemoclaw-blueprint/blueprint.yamlscripts/patch-openclaw-tool-catalog.jstest/e2e-port-overrides.shtest/openclaw-tool-catalog-patch.test.ts
✅ Files skipped from review due to trivial changes (1)
- nemoclaw-blueprint/blueprint.yaml
Comment on lines
+275
to
+283
| if (targetFiles.length === 0) { | ||
| const builtInCatalogFile = selectionFiles.find((file) => { | ||
| const text = fs.readFileSync(file, "utf-8"); | ||
| return hasBuiltInToolCatalog(text); | ||
| }); | ||
| if (builtInCatalogFile) { | ||
| return { status: "skipped-built-in", file: builtInCatalogFile, version }; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce a single built-in candidate before returning skipped-built-in.
When targetFiles.length === 0, this path returns the first built-in match. If multiple selection-*.js files contain built-in catalog signatures, the script silently picks one and reports success, which can hide dist shape drift.
Suggested fix
if (targetFiles.length !== 1) {
if (targetFiles.length === 0) {
- const builtInCatalogFile = selectionFiles.find((file) => {
+ const builtInCatalogFiles = selectionFiles.filter((file) => {
const text = fs.readFileSync(file, "utf-8");
return hasBuiltInToolCatalog(text);
});
- if (builtInCatalogFile) {
- return { status: "skipped-built-in", file: builtInCatalogFile, version };
+ if (builtInCatalogFiles.length === 1) {
+ return { status: "skipped-built-in", file: builtInCatalogFiles[0], version };
+ }
+ if (builtInCatalogFiles.length > 1) {
+ throw new Error(
+ `Expected exactly one built-in selection target, found ${builtInCatalogFiles.length}`,
+ );
}
}
throw new Error(`Expected exactly one selection-*.js target, found ${targetFiles.length}`);
}🤖 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/patch-openclaw-tool-catalog.js` around lines 275 - 283, When
targetFiles.length === 0 you currently pick the first match from selectionFiles
via hasBuiltInToolCatalog and return {status: "skipped-built-in"}, which hides
cases where multiple selection-*.js files contain built-in catalogs; update the
logic around selectionFiles/hasBuiltInToolCatalog to collect all matching files,
and only return {status: "skipped-built-in", file, version} if exactly one match
is found; if more than one match is found return/throw an explicit ambiguous
result (e.g. {status: "ambiguous-built-in", files: [...], version} or throw a
clear error) so the caller can notice dist shape drift instead of silently
choosing the first file.
Comment on lines
+65
to
+68
| if ! echo "$out" | grep -q "must be an integer between 1024 and 65535"; then | ||
| info "$label rejected with exit $rc but validation text was not captured; entrypoint script text is checked below" | ||
| fi | ||
| pass "$label rejected by entrypoint (exit $rc)" |
Contributor
There was a problem hiding this comment.
Do not pass when validation text is missing.
This helper can report PASS even if the rejection reason changes or disappears, which makes the test flaky as a guardrail.
Suggested fix
- if ! echo "$out" | grep -q "must be an integer between 1024 and 65535"; then
- info "$label rejected with exit $rc but validation text was not captured; entrypoint script text is checked below"
- fi
- pass "$label rejected by entrypoint (exit $rc)"
+ if ! echo "$out" | grep -q "must be an integer between 1024 and 65535"; then
+ fail "$label rejected with exit $rc but missing expected validation text: $out"
+ return
+ fi
+ pass "$label rejected by entrypoint (exit $rc)"📝 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.
Suggested change
| if ! echo "$out" | grep -q "must be an integer between 1024 and 65535"; then | |
| info "$label rejected with exit $rc but validation text was not captured; entrypoint script text is checked below" | |
| fi | |
| pass "$label rejected by entrypoint (exit $rc)" | |
| if ! echo "$out" | grep -q "must be an integer between 1024 and 65535"; then | |
| fail "$label rejected with exit $rc but missing expected validation text: $out" | |
| return | |
| fi | |
| pass "$label rejected by entrypoint (exit $rc)" |
🤖 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/e2e-port-overrides.sh` around lines 65 - 68, The current check logs info
when the expected validation text isn't found but still calls pass("$label
rejected by entrypoint (exit $rc)"), allowing false positives; change the logic
so that after running grep on "$out" for "must be an integer between 1024 and
65535" you only call pass("$label rejected by entrypoint (exit $rc)") if the
grep succeeded, and call fail with a clear message (e.g., include $label, $rc
and $out) when the grep did not match; update the block around the grep, info,
pass functions to make failure conditional and reference the existing variables
out, rc, and label and helper functions info, pass, and fail.
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26272360471
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26272729419
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR reduces sandbox image build time by avoiding broad recursive
.openclawpermission repair on current unified-layout sandbox bases. It keeps the conservative repair path for legacy.openclaw-datamigrations and adds Docker build step timing to make future build bottlenecks visible.DGX Spark build-time comparison:
178.1s68.0sChanges
.openclaw-datasymlink verification so it only runs when legacy migration occurs..openclawpermission repair with a legacy-only broad repair and a targeted fast path for current unified layouts. The fast path relies on the current unified.openclawlayout being provisioned earlier in the image:Dockerfile.basecreates the known state directories assandbox:sandboxwith group-write/setgid permissions, andopenclaw plugins installruns asUSER sandbox, soplugin-runtime-depscontents are already owned bysandbox:sandboxbefore the final repair step..openclawrepair path: current layouts use targeted permission repair, while legacy.openclaw-datamigrations still use broad recursive repair..openclawlayout contract check so future directory/file layout changes fail tests until the targeted permission repair assumptions are reviewed.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Additional validation:
/sandbox/.openclaw/plugin-runtime-depsand sampled contents are owned bysandbox:sandbox; no non-sandbox:sandboxentries were found, and a sandbox-user write probe inplugin-runtime-depssucceeded.Signed-off-by: zyang-dev 267119621+zyang-dev@users.noreply.github.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests