Skip to content

fix(e2e): accept non-root WhatsApp QR preload owner - #4671

Merged
cv merged 2 commits into
mainfrom
u/sdang/fix-messaging-providers-wa-preload-owner
Jun 2, 2026
Merged

fix(e2e): accept non-root WhatsApp QR preload owner#4671
cv merged 2 commits into
mainfrom
u/sdang/fix-messaging-providers-wa-preload-owner

Conversation

@sandl99

@sandl99 sandl99 commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

The messaging providers E2E check now accepts the non-root sandbox owner for the WhatsApp compact QR preload while still requiring read-only mode. This matches emit_sandbox_sourced_file() behavior when the sandbox setup runs without UID 0 and avoids failing the nightly messaging-providers job on a harmless owner difference.

Related Issue

Fixes #4636

Changes

  • Accept /tmp/nemoclaw-whatsapp-qr-compact.js as sandbox:444 in non-root sandbox mode.
  • Preserve the existing root:444, missing-file, and unexpected-mode branches.
  • Document why ownership can differ while the security-relevant read-only mode remains 444.

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

  • bash -n test/e2e/test-messaging-providers.sh passes.
  • git diff --check passes.
  • npx vitest run --project cli test/e2e-script-workflow.test.ts passes.
  • npx prek run --all-files ran but failed on an unrelated existing timeout: test/cli.test.ts > CLI dispatch > channels mutation dry-run paths dispatch through oclif.
  • 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
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: San Dang sdang@nvidia.com

Summary by CodeRabbit

  • Tests
    • Expanded end-to-end tests for WhatsApp compact-QR pairing to accept a sandbox-owned preload when accompanied by a sandbox-owned entrypoint log, in addition to the existing root-owned case.
    • Added a contract-style E2E test that validates the pairing verification script contains the expected sandbox-vs-root checks and logging assertions.

@sandl99 sandl99 self-assigned this Jun 2, 2026
@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: 48a0452e-4d2e-4aec-a6cc-2f4aac789e52

📥 Commits

Reviewing files that changed from the base of the PR and between f56f48b and 8f05400.

📒 Files selected for processing (2)
  • test/e2e-script-workflow.test.ts
  • test/e2e/test-messaging-providers.sh
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/e2e/test-messaging-providers.sh

📝 Walkthrough

Walkthrough

The PR accepts sandbox:444 (with sandbox:600 start-log) as a valid alternative to root:444 for the WhatsApp compact-QR preload file, and adds a Vitest that reads the E2E script to assert the new gating logic is present.

Changes

WhatsApp E2E assertion and test

Layer / File(s) Summary
Preload owner/mode check update
test/e2e/test-messaging-providers.sh
Stats both /tmp/nemoclaw-whatsapp-qr-compact.js and /tmp/nemoclaw-start.log, keeps root:444 PASS, and adds elif to PASS when preload is sandbox:444 and start-log is sandbox:600; preserves failure handling.
E2E script contract test
test/e2e-script-workflow.test.ts
Imports readFileSync and adds a test that loads e2e/test-messaging-providers.sh, asserting presence of the new stat comparisons and entrypoint_start_log_stat reference.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#4607: Introduced the WhatsApp compact-QR preload injection and related start-log behavior that this PR's test now validates.

Suggested labels

fix, NemoClaw CLI

Suggested reviewers

  • cv

Poem

🐰 A little rabbit checks the gate,
Root or sandbox — both are straight.
QR nestled, read-only bright,
Tests now spot the subtle sight.
Hops of joy for passing night.

🚥 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 'fix(e2e): accept non-root WhatsApp QR preload owner' directly and clearly describes the primary change: widening the E2E assertion to accept sandbox:444 ownership alongside root:444.
Linked Issues check ✅ Passed The PR fully addresses issue #4636's requirements: accepts sandbox:444 for WhatsApp preload when non-root startup is signaled, preserves 444 mode enforcement, maintains root:444 requirement in root mode, and adds regression tests.
Out of Scope Changes check ✅ Passed All changes are within scope: the shell script modification directly fixes the M-WA6b assertion per issue #4636, and the test file addition validates the ownership-gating logic against startup signals.
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 docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch u/sdang/fix-messaging-providers-wa-preload-owner

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

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: None
Optional E2E: messaging-providers-e2e

Dispatch hint: messaging-providers-e2e

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • None. No required E2E is needed because this PR changes only E2E/contract test files and does not modify installer, onboarding, sandbox lifecycle, credentials implementation, network policy assets, inference routing, deployment code, or user-flow runtime behavior. Run the optional messaging-providers-e2e job if you want to validate the changed test script itself.

Optional E2E

  • messaging-providers-e2e (high; sandbox lifecycle plus messaging provider coverage, timeout 75 minutes): Optional validation for the modified shell E2E path. This job runs test/e2e/test-messaging-providers.sh and directly covers the changed WhatsApp QR-only channel, policy/no-provider path, credential placeholder isolation, and preload ownership assertion.

New E2E recommendations

  • None.

Dispatch hint

  • Workflow: .github/workflows/nightly-e2e.yaml
  • jobs input: messaging-providers-e2e

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

E2E Scenario Advisor Recommendation

Required scenario E2E: None
Optional scenario E2E: None

Workflow run

Full scenario advisor summary

E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required scenario E2E

  • None. Changed files are outside the scenario E2E suite and scenario workflows/catalogs. They affect legacy test/e2e workflow/script coverage, which is owned by the general E2E advisor, not the scenario E2E advisor.

Optional scenario E2E

  • None.

Relevant changed files

  • None.

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor

Findings: 0 needs attention, 0 worth checking, 0 nice ideas
Since last review: 2 prior items resolved, 0 still apply, 0 new items found

Workflow run details

This is an automated advisory review. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 26825869185
Target ref: u/sdang/fix-messaging-providers-wa-preload-owner
Workflow ref: main
Requested jobs: messaging-providers-e2e
Summary: 1 passed, 0 failed, 0 skipped

Job Result
messaging-providers-e2e ✅ success

@sandl99 sandl99 added E2E integration: whatsapp WhatsApp integration or channel behavior labels Jun 2, 2026
@sandl99
sandl99 requested a review from cv June 2, 2026 14:35
@sandl99 sandl99 added the nightly-e2e Nightly E2E test failures label Jun 2, 2026
@sandl99

sandl99 commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the advisor follow-up in 8f05400.

  • Invalid state: a root-mode sandbox reporting the trusted WhatsApp NODE_OPTIONS preload as sandbox:444 should still fail, because that would weaken the /tmp trust boundary.
  • Source boundary: /tmp/nemoclaw-start.log is written by the entrypoint before sandbox-init.sh is sourced; it is root:600 in root mode and sandbox:600 in non-root mode.
  • Source-fix constraint: the localized E2E fix only accepts /tmp/nemoclaw-whatsapp-qr-compact.js as sandbox:444 when that independent start-log stat proves non-root startup. Root-mode still requires root:444.
  • Regression test: test/e2e-script-workflow.test.ts now asserts the messaging-providers script gates sandbox:444 acceptance on the start-log signal and includes that signal in failures.
  • Removal condition: remove this fallback when OpenShell/NemoClaw no longer supports the non-root startup mode where privilege separation is disabled, or when the runtime gains a root-owned trusted source-file mechanism in that mode.

Focused verification passed: bash -n test/e2e/test-messaging-providers.sh; npx prek run shfmt --files test/e2e/test-messaging-providers.sh; git diff --check; npx vitest run --project cli test/e2e-script-workflow.test.ts.

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 26827213224
Target ref: u/sdang/fix-messaging-providers-wa-preload-owner
Workflow ref: main
Requested jobs: messaging-providers-e2e
Summary: 1 passed, 0 failed, 0 skipped

Job Result
messaging-providers-e2e ✅ success

@cv cv self-assigned this Jun 2, 2026
@cv
cv merged commit 560b206 into main Jun 2, 2026
30 checks passed
@cv
cv deleted the u/sdang/fix-messaging-providers-wa-preload-owner branch June 2, 2026 17:20
@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 nightly-e2e Nightly E2E test failures 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 integration: whatsapp WhatsApp integration or channel behavior

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