Repository navigation
fix(sandbox): tolerate brave provider profile re-import in docker build - #5467
ShauryaaSharma wants to merge 1 commit into
Conversation
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Dockerfile’s Brave plugin setup now captures ChangesBrave Doctor Error Guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Dockerfile (1)
694-712: Run the Dockerfile E2E matrix for this build-path change.This guard is in a container-build-sensitive path and should be validated through the recommended image-level E2E jobs.
As per coding guidelines, "Dockerfile ... changes are only testable with a real container build" and the recommended suite is
cloud-e2e,sandbox-survival-e2e,hermes-e2e,rebuild-openclaw-e2e, andopenclaw-tui-chat-correlation-e2e.gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=cloud-e2e,sandbox-survival-e2e,hermes-e2e,rebuild-openclaw-e2e,openclaw-tui-chat-correlation-e2e🤖 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 694 - 712, This comment requests validation of the Dockerfile changes through E2E testing rather than requesting a code fix. Execute the specified E2E test workflow to validate the container build changes to the openclaw plugin installation and brave-doctor logic. Use the provided gh workflow command to run the nightly-e2e.yaml workflow with the jobs parameter set to cloud-e2e, sandbox-survival-e2e, hermes-e2e, rebuild-openclaw-e2e, and openclaw-tui-chat-correlation-e2e on your feature branch to ensure the Dockerfile modifications work correctly in a real container build environment.
🤖 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 694-712: This comment requests validation of the Dockerfile
changes through E2E testing rather than requesting a code fix. Execute the
specified E2E test workflow to validate the container build changes to the
openclaw plugin installation and brave-doctor logic. Use the provided gh
workflow command to run the nightly-e2e.yaml workflow with the jobs parameter
set to cloud-e2e, sandbox-survival-e2e, hermes-e2e, rebuild-openclaw-e2e, and
openclaw-tui-chat-correlation-e2e on your feature branch to ensure the
Dockerfile modifications work correctly in a real container build environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ed99ec79-c072-4f64-ba23-2992c42d48db
📒 Files selected for processing (1)
Dockerfile
AryanGupta1112
left a comment
There was a problem hiding this comment.
LGTM. The fix gracefully handles the expected profile re-import scenario while preserving failure behavior for all other non-zero exits. Nice targeted fix.
Approved in CodeRabbit Change Stack
|
✨ Thanks for the proposed fix addressing the Brave provider profile re-import failure during Docker builds. This proposes a way to tolerate the 'already exists' error from Related open issues: |
|
Was going through @coderabbitai Nitpicks. This needs a maintainer to run the suite (cloud-e2e, sandbox-survival-e2e, hermes-e2e, rebuild-openclaw-e2e, openclaw-tui-chat-correlation-e2e), same gate as the copy-pr-bot approval. Happy to adjust the fix if any of those surface an issue. |
|
Tip For best results, initiate chat on the files or code changes.
The handoff needed from a maintainer is:
|
openclaw plugins install brave-plugin now imports the brave provider profile as a side effect. The subsequent openclaw doctor --fix call tries to import it again and exits non-zero with "custom provider profile 'brave' already exists", killing the build under set -eu. Mirror the same tolerance already present in the host-side TypeScript (brave-provider-profile.ts:80, /already exists/i check): capture doctor's combined output, and if it exits non-zero only because the profile is already registered, treat it as success. Any other non-zero exit still propagates as a real build failure. Fixes NVIDIA#5367 Signed-off-by: ShauryaaSharma <shauryasofficial27@gmail.com>
0d584ff to
5b93458
Compare
|
Rebased onto current NVIDIA:main. Branch is up to date and merges cleanly. This has been open for ~2 weeks now with one approval already in. @wscurran @ericksoa, could one of you trigger nightly-e2e.yaml against this branch and provide a write-access review when you get a chance? Happy to iterate on any feedback. |
|
Maintainer verification on current
The reported Closing without replacement. The sole PR commit is also unsigned/unverified and cannot satisfy the release gate. |
Summary
openclaw plugins install brave-pluginnow imports the brave provider profile asa side effect. The subsequent
openclaw doctor --fix --non-interactivetries toimport it again, exits non-zero with "custom provider profile 'brave' already
exists", and kills the Docker build under
set -eu.The fix mirrors the tolerance already present in the host-side TypeScript
(
brave-provider-profile.ts:80): capture doctor's output, and if the onlyfailure is "already exists", treat it as success. Any other non-zero exit still
propagates as a real build failure.
Related Issue
Fixes #5367
Changes
Dockerfile: wrapopenclaw doctor --fixin the Brave path with anif ! var=$(cmd 2>&1)guard that passes through "already exists" and failshard on all other non-zero exits
Type of Change
Verification
Signed-off-by: ShauryaaSharma shauryasofficial27@gmail.com
Summary by CodeRabbit