Skip to content

fix(e2e): accept non-root sandbox ownership for WhatsApp QR preload (#4522) - #4633

Closed
hunglp6d wants to merge 1 commit into
NVIDIA:mainfrom
hunglp6d:fix/nightly-e2e-whatsapp-qr-preload-owner-451f26f
Closed

fix(e2e): accept non-root sandbox ownership for WhatsApp QR preload (#4522)#4633
hunglp6d wants to merge 1 commit into
NVIDIA:mainfrom
hunglp6d:fix/nightly-e2e-whatsapp-qr-preload-owner-451f26f

Conversation

@hunglp6d

@hunglp6d hunglp6d commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

The messaging-providers-e2e nightly job failed because test-messaging-providers.sh assertion M-WA6b expected the WhatsApp compact-QR preload file to be owned root:444, but the sandbox runs in non-root mode where emit_sandbox_sourced_file() (in scripts/lib/sandbox-init.sh) produces sandbox:444 instead. The file permissions (mode 444 = read-only) are the security-relevant property; ownership differs only by privilege-separation posture. This PR widens the assertion to accept both ownership variants.

Related Issue

Fixes #4636

Changes

  • test/e2e/test-messaging-providers.sh: Add an elif branch to the M-WA6b assertion that accepts sandbox:444 alongside root:444, with a comment explaining why non-root mode produces a different owner.

Validation

A focused custom-e2e.yaml workflow was run on a sibling branch to confirm this fix repairs the regression. The workflow re-runs only the jobs from the original nightly that this PR targets, on ubuntu-latest, off the same fix commit as this PR.

The validation branch is intentionally not the head of this PR — it carries an extra .github/workflows/custom-e2e.yaml commit that is scaffolding, not part of the fix. Re-run the validation by pushing any commit to the validation branch.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Verification

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes

AI Disclosure

  • AI-assisted — tool: Claude Code

Signed-off-by: Hung Le hple@nvidia.com

Summary by CodeRabbit

  • Tests

    • Improved end-to-end messaging provider tests to enhance WhatsApp compact-QR preload verification. Now correctly recognizes sandbox file ownership configurations as valid installation cases in addition to existing root ownership scenarios and failure detection.
  • Bug Fixes

    • Fixed test case handling to properly validate alternative file ownership modes during messaging provider setup verification.

…VIDIA#4522)

The M-WA6b test expected root:444 for the WhatsApp compact-QR preload,
but emit_sandbox_sourced_file() produces sandbox:444 in non-root mode
(documented in sandbox-init.sh).  Accept both root:444 and sandbox:444
since mode 444 (read-only) is the security-relevant property.

Signed-off-by: Hung Le <hple@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jun 2, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2fca5154-6f4a-445f-a254-bfa10563c4db

📥 Commits

Reviewing files that changed from the base of the PR and between ef1d4af and 616bb48.

📒 Files selected for processing (1)
  • test/e2e/test-messaging-providers.sh

📝 Walkthrough

Walkthrough

This PR updates a WhatsApp compact-QR preload file ownership check in the messaging providers E2E test. The assertion now accepts sandbox:444 ownership/mode as a valid success case for non-root sandbox installations, alongside the existing root:444 case, while preserving failure handling for truly unexpected states.

Changes

WhatsApp Preload File Ownership Assertion

Layer / File(s) Summary
WhatsApp preload ownership assertion widening
test/e2e/test-messaging-providers.sh
The M-WA6b assertion adds an elif branch that treats sandbox:444 ownership as a successful installation outcome. The branch includes explanatory comments clarifying that non-root sandbox mode naturally produces this ownership variant while preserving the critical read-only mode (444). The existing root:444 success case and failure paths remain intact.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

A sandbox plays in file permissions fine,
The test now smiles at sandbox:444 in line,
Non-root is welcome, the QR preload blooms,
Less test-flakiness haunting those E2E rooms! 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: widening the WhatsApp QR preload file ownership check to accept non-root sandbox ownership.
Linked Issues check ✅ Passed The PR directly addresses issue #4636 by accepting sandbox:444 as valid, fixing the test failure described in the linked issue.
Out of Scope Changes check ✅ Passed All changes are scoped to the messaging-providers test file and directly fix the failing assertion; no unrelated modifications present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@sandl99
sandl99 marked this pull request as ready for review June 2, 2026 13:24
@sandl99 sandl99 self-assigned this Jun 2, 2026
@sandl99
sandl99 requested a review from cv June 2, 2026 13:27
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Selective E2E Results — ❌ Some jobs failed

Run: 26822746929
Target ref: main
Requested jobs: messaging-providers-e2e
Summary: 0 passed, 1 failed, 0 skipped

Job Result
messaging-providers-e2e ❌ failure

Failed jobs: messaging-providers-e2e. Check run artifacts for logs.

@sandl99

sandl99 commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

Superseded by #4671, which carries the same fix on an internal NVIDIA/NemoClaw branch so E2E can be triggered without fork-branch restrictions.

@sandl99 sandl99 closed this Jun 2, 2026
@wscurran wscurran added area: e2e End-to-end tests, nightly failures, or validation infrastructure area: messaging Messaging channels, bridges, manifests, or channel lifecycle feature PR adds or expands user-visible functionality and removed E2E labels Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: e2e End-to-end tests, nightly failures, or validation infrastructure area: messaging Messaging channels, bridges, manifests, or channel lifecycle feature PR adds or expands user-visible functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nightly-e2e: messaging-providers-e2e M-WA6b fails on non-root sandbox ownership (run 26790528855)

3 participants