fix(hermes): align dashboard WhatsApp session path - #8229
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
|
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:
📝 WalkthroughWalkthroughThe PR adds compiled Hermes messaging preloads, manifest-driven runtime setup, WhatsApp session-path normalization, startup integration, permission checks, secret scans, and supporting tests. ChangesHermes messaging runtime
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant HermesStartup as Hermes startup
participant SandboxInit as sandbox-init.sh
participant MessagingManifest as messaging manifest
participant RuntimeEnv as trusted runtime environment
HermesStartup->>SandboxInit: Prepare messaging runtime
SandboxInit->>MessagingManifest: Load and validate runtime plan
SandboxInit->>RuntimeEnv: Apply aliases and install preloads
SandboxInit->>RuntimeEnv: Publish connect preload exports
SandboxInit-->>HermesStartup: Complete runtime preparation
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 9dcef91 in the TypeScript / code-coverage/cliThe overall coverage in commit 9dcef91 in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: San Dang <sdang@nvidia.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
1 warning · 0 suggestionsWarningsWarnings do not block.
|
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/hermes-whatsapp-session-path-patch.test.ts`:
- Line 85: Update the test title in the parametrize decorator to correct the
terminology from pairing source to gateway consumer, and add the issue-suffixed
reference (8184) at the end of the title following the coding guidelines for
local issue references in test titles. The test at line 85 should reflect that
the adapter is a gateway consumer while including the tracked defect identifier
in the final position of the test description.
🪄 Autofix
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: 60047bb8-bcb9-4c4e-b411-7e7d494a3879
📒 Files selected for processing (4)
agents/hermes/Dockerfileagents/hermes/patch-whatsapp-session-path.mtstest/hermes-final-image-layout.test.tstest/hermes-whatsapp-session-path-patch.test.ts
cjagwani
left a comment
There was a problem hiding this comment.
Exact-head CI failure is deterministic and branch-related on d7d2c17e4f9318deb33d388a1a9deb263f6dd8d7.
The Dockerfile adds seven build-only WhatsApp source/output integrity inputs, but MANAGED_STARTUP_PROFILE_EXCLUDED_DOCKER_INPUTS.hermes does not classify them. The repository contract in managed-startup-profile.test.ts therefore reports all seven as unowned Docker inputs. This is not a flaky shard failure and should not be rerun unchanged.
Please add these inputs to the Hermes build-only exclusion authority in src/lib/onboard/managed-startup/profile.ts with the appropriate integrity-pin reason, then rerun the focused startup-profile contract alongside the existing WhatsApp patch/image tests. Do not weaken the exhaustive Docker-input check.
The primary advisor lane also failed before producing a review, so fresh exact-head advisor evidence is still required after the fix.
Signed-off-by: San Dang <sdang@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
test/hermes-messaging-runtime-setup.test.ts (2)
54-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe stub bypasses the behavior this test claims to prove.
The title states that manifest connect preloads reach the trusted runtime environment. Line 64 replaces
emit_messaging_connect_runtime_preload_exportswith a literal string, so the test proves only thatwrite_runtime_shell_envcalls the function. It does not prove that a connect preload recorded byinstall_messaging_runtime_preloadsbecomes a--requireentry inNODE_OPTIONS.Source the real
emit_messaging_connect_runtime_preload_exportsfromscripts/lib/sandbox-init.sh, point_MESSAGING_CONNECT_PRELOADS_FILEat a temporary list that contains a real file, then evaluate the emitted environment and assert the resultingNODE_OPTIONSvalue.Based on path instructions for
**/*.test.{ts,js,mts,mjs,cts,cjs}: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."🤖 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/hermes-messaging-runtime-setup.test.ts` around lines 54 - 78, Update the test around runtimeShellEnvFunction(HERMES_START) to source and invoke the real emit_messaging_connect_runtime_preload_exports implementation from scripts/lib/sandbox-init.sh instead of stubbing it. Configure _MESSAGING_CONNECT_PRELOADS_FILE with a temporary list containing a real preload file, evaluate the emitted environment, and assert NODE_OPTIONS contains the corresponding --require entry so the test verifies install-to-runtime propagation.Source: Path instructions
12-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing shell-function extraction helper.
test/hermes-start.test.tsalready providesextractShellFunctionFromSourcefor the same purpose. This localshellFunctionduplicates that logic and stops at the first\n}, so a future nested block or a column-0}inside a target function silently truncates the extracted body and the test still passes on a partial function.Move the existing helper into a shared test support module and import it here.
🤖 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/hermes-messaging-runtime-setup.test.ts` around lines 12 - 19, Replace the local shellFunction helper with the shared extractShellFunctionFromSource utility currently defined in hermes-start.test.ts. Move that helper into a shared test support module, update both tests to import it, and remove the duplicated implementation so extraction handles nested blocks consistently.agents/hermes/Dockerfile (1)
327-331: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the preload flatten step against basename collisions and a silently empty copy.
The
cpflattens every*/runtime/*.jsinto one directory by basename. If two channels ship a runtime file with the same basename, the secondcpsilently overwrites the first, and a manifestsourcepath then resolves to another channel's code. Theif [ -d ... ]guard also lets the build succeed when the copy at Line 145 produces nothing; the failure then surfaces only at startup asMissing runtime preload source.Add a collision check and assert that at least one file was installed.
♻️ Proposed hardening for the flatten step
- && if [ -d /usr/local/lib/nemoclaw/preloads-compiled-channels ]; then \ - find /usr/local/lib/nemoclaw/preloads-compiled-channels -path '*/runtime/*.js' -type f \ - -exec sh -c 'for file do cp "$file" "/usr/local/lib/nemoclaw/preloads/$(basename "$file")"; done' sh {} +; \ - fi \ + && test -d /usr/local/lib/nemoclaw/preloads-compiled-channels \ + && find /usr/local/lib/nemoclaw/preloads-compiled-channels -path '*/runtime/*.js' -type f \ + -exec sh -c 'for file do \ + target="/usr/local/lib/nemoclaw/preloads/$(basename "$file")"; \ + if [ -e "$target" ]; then \ + echo "ERROR: duplicate messaging runtime preload basename: $target" >&2; \ + exit 1; \ + fi; \ + cp "$file" "$target"; \ + done' sh {} + \🤖 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 `@agents/hermes/Dockerfile` around lines 327 - 331, Harden the preload flatten step in the Dockerfile before copying files: detect duplicate basenames among the runtime JavaScript files and fail the build instead of overwriting an existing preload. Also track the number of copied files and fail when no runtime preload was installed, while preserving cleanup of preloads-compiled-channels.scripts/lib/sandbox-init.sh (1)
1185-1206: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSensitive Data Exposure (CWE-1053)
Reachability: Internal
Reachability path
● Entry scripts/nemoclaw-start.sh:5010 write_runtime_shell_env │ ▼ ● Hop agents/hermes/start.sh:362 prepare_hermes_messaging_runtime │ ▼ ● Sink scripts/lib/sandbox-init.shAdd negative-path tests for
verify_messaging_runtime_secret_scans.Current tests stub this function. Cover a matching file, the configured exit code, a message that excludes the scanned content, and a missing file.
🤖 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/lib/sandbox-init.sh` around lines 1185 - 1206, Add negative-path tests for verify_messaging_runtime_secret_scans that exercise a file matching the configured secretScans pattern, assert the function exits with the scan’s configured exitCode, verify stderr contains the path/message but not the scanned secret content, and confirm missing files are skipped without failure. Replace the existing stubbed-function coverage with these focused cases.Source: Path instructions
test/messaging-runtime-preload-packaging.test.ts (1)
38-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTest the packaged payload instead of Dockerfile text.
These assertions only match Dockerfile fragments. They do not prove that the final Hermes image contains usable compiled preloads.
Assert the final payload contains the expected runtime JavaScript file. Use the final-image test surface for this check.
As per path instructions, “Prefer observable outcomes through the public boundary over source-text ... assertions.”
🤖 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/messaging-runtime-preload-packaging.test.ts` around lines 38 - 52, Update the Hermes packaging test around “packages the same manifest-owned runtime preloads for Hermes” to inspect the final image payload rather than matching Dockerfile source text. Use the existing final-image test surface to assert that the expected compiled runtime JavaScript preload is present and usable, removing or replacing the implementation-detail assertions.Source: Path instructions
🤖 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/lib/sandbox-init.sh`:
- Around line 1059-1064: Ensure apply_messaging_runtime_env_aliases returns
success when processing an alias with no message by preventing the conditional
printf command from determining the while-loop status. Keep exporting each alias
and printing non-empty messages unchanged, and make the function’s final status
successful for valid rows so the bare call in nemoclaw-start.sh does not abort
under set -e.
In `@src/lib/messaging/channels/whatsapp/runtime/whatsapp-hermes-session.ts`:
- Line 29: Preserve explicit session_path values in the WhatsApp Hermes session
argument handling by assigning HERMES_WHATSAPP_SESSION_PATH only when no path
was provided; update
src/lib/messaging/channels/whatsapp/runtime/whatsapp-hermes-session.ts lines
29-29 accordingly. Add a regression test in
src/lib/messaging/channels/whatsapp/runtime/whatsapp-hermes-session.test.ts
lines 13-61 that supplies a noncanonical explicit path and verifies it remains
unchanged.
---
Nitpick comments:
In `@agents/hermes/Dockerfile`:
- Around line 327-331: Harden the preload flatten step in the Dockerfile before
copying files: detect duplicate basenames among the runtime JavaScript files and
fail the build instead of overwriting an existing preload. Also track the number
of copied files and fail when no runtime preload was installed, while preserving
cleanup of preloads-compiled-channels.
In `@scripts/lib/sandbox-init.sh`:
- Around line 1185-1206: Add negative-path tests for
verify_messaging_runtime_secret_scans that exercise a file matching the
configured secretScans pattern, assert the function exits with the scan’s
configured exitCode, verify stderr contains the path/message but not the scanned
secret content, and confirm missing files are skipped without failure. Replace
the existing stubbed-function coverage with these focused cases.
In `@test/hermes-messaging-runtime-setup.test.ts`:
- Around line 54-78: Update the test around
runtimeShellEnvFunction(HERMES_START) to source and invoke the real
emit_messaging_connect_runtime_preload_exports implementation from
scripts/lib/sandbox-init.sh instead of stubbing it. Configure
_MESSAGING_CONNECT_PRELOADS_FILE with a temporary list containing a real preload
file, evaluate the emitted environment, and assert NODE_OPTIONS contains the
corresponding --require entry so the test verifies install-to-runtime
propagation.
- Around line 12-19: Replace the local shellFunction helper with the shared
extractShellFunctionFromSource utility currently defined in
hermes-start.test.ts. Move that helper into a shared test support module, update
both tests to import it, and remove the duplicated implementation so extraction
handles nested blocks consistently.
In `@test/messaging-runtime-preload-packaging.test.ts`:
- Around line 38-52: Update the Hermes packaging test around “packages the same
manifest-owned runtime preloads for Hermes” to inspect the final image payload
rather than matching Dockerfile source text. Use the existing final-image test
surface to assert that the expected compiled runtime JavaScript preload is
present and usable, removing or replacing the implementation-detail assertions.
🪄 Autofix
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: 5bf7c77e-fb8b-49a1-8383-74aa2c993ae5
📒 Files selected for processing (21)
agents/hermes/Dockerfileagents/hermes/start.shscripts/lib/sandbox-init.shscripts/nemoclaw-start.shsrc/lib/messaging/channels/whatsapp/manifest.tssrc/lib/messaging/channels/whatsapp/runtime/whatsapp-hermes-session.test.tssrc/lib/messaging/channels/whatsapp/runtime/whatsapp-hermes-session.tssrc/lib/messaging/manifest/types.tstest/generate-hermes-config.test.tstest/hermes-final-image-layout.test.tstest/hermes-messaging-runtime-setup.test.tstest/hermes-start.test.tstest/http-proxy-fix-sync.test.tstest/messaging-runtime-preload-packaging.test.tstest/nemoclaw-start-guard-recovery.test.tstest/nemoclaw-start-runtime-env-alias.test.tstest/nemoclaw-start-slack-runtime.test.tstest/nemoclaw-start-telegram-runtime.test.tstest/nemoclaw-start.test.tstest/nemotron-inference-fix.test.tstest/support/entrypoint-script-fixture.ts
Signed-off-by: San Dang <sdang@nvidia.com>
cjagwani
left a comment
There was a problem hiding this comment.
Re-reviewed exact head c3e80d3c3142a7ef2ec43d949650fbdffb684f6c using the manifest-first messaging contract. Moving the WhatsApp session shim into the channel manifest/runtime preload and keeping the shared initializer channel-neutral resolves the prior unowned integrity-input shape; that architecture is appropriate.
The replacement currently fails deterministically in the Hermes image build, however. tsconfig.runtime-preloads.json requires Node types, but the Hermes runtime-preload-builder compiles from /opt/nemoclaw-root while its node_modules lives at the sibling /opt/mcp-tool-discovery-runtime/node_modules. TypeScript therefore cannot resolve @types/node and reports missing process, require, Buffer, and NodeJS. The OpenClaw preload builder already establishes the required local node_modules symlink before invoking tsc; the Hermes stage does not.
Please give the Hermes compile root an explicit dependency-resolution path (the analogous builder-only symlink is the smallest current fix), and extend messaging-runtime-preload-packaging.test.ts to protect that Hermes resolution step. Then run the focused WhatsApp manifest/runtime/preload tests and obtain a green exact-head Hermes direct managed-startup build. Rerunning the unchanged head will reproduce the same compile failure.
cjagwani
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 2cc4096fcf26787beaa44d0426e79edc688f6dcd against the manifest-first messaging contract. The Hermes dependency-resolution fix exactly closes the deterministic build failure and is protected by the preload-packaging contract; the exact-head Hermes direct managed-startup and both image builds now pass. The broader follow-up keeps WhatsApp-specific behavior in the channel manifest/runtime asset while the shared sandbox initializer remains driven by serialized manifest data, so it does not reintroduce channel-specific core branching. The earlier integrity-input failure is removed with the obsolete standalone patcher rather than bypassed.
I found no remaining code blocker in this head. Remaining exact-head CI/E2E and the terminal advisor receipts remain visible and are not waived by this approval.
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/messaging-runtime-secret-scan.test.ts (1)
50-52: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up the temporary directory in a
finallyblock.
fs.rmSync(tmpDir, ...)runs only after setup andspawnSynccomplete normally. If any operation throws, the temporary directory remains. Wrap setup and execution intry/finallyto prevent test artifacts from accumulating.Suggested fix
- prepareConfig(configPath); - // existing plan and script writes - const result = spawnSync("bash", [scriptPath], { encoding: "utf-8", timeout: 5000 }); - fs.rmSync(tmpDir, { recursive: true, force: true }); - return { configPath, result }; + try { + prepareConfig(configPath); + // existing plan and script writes + const result = spawnSync("bash", [scriptPath], { encoding: "utf-8", timeout: 5000 }); + return { configPath, result }; + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }🤖 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/messaging-runtime-secret-scan.test.ts` around lines 50 - 52, Update the temporary-directory helper around spawnSync so all setup and script execution occur inside a try block, with fs.rmSync(tmpDir, { recursive: true, force: true }) in a finally block. Preserve returning configPath and result on successful execution while ensuring cleanup also occurs when setup or execution throws.
🤖 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.
Outside diff comments:
In `@test/messaging-runtime-secret-scan.test.ts`:
- Around line 50-52: Update the temporary-directory helper around spawnSync so
all setup and script execution occur inside a try block, with fs.rmSync(tmpDir,
{ recursive: true, force: true }) in a finally block. Preserve returning
configPath and result on successful execution while ensuring cleanup also occurs
when setup or execution throws.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a2c69368-84ea-4346-acc0-c813793fc45e
📒 Files selected for processing (1)
test/messaging-runtime-secret-scan.test.ts
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
Security review addendum —
|
|
Closing after re-testing from The observed For the normal
That explains why the unpatched flow works for other users. The preload in this PR instead rewrites the Node bridge The separate dashboard Closing this PR because the proposed fix is not required for the reproduced CLI flow and is not safe to merge as implemented. |
Correction: dashboard scope is validThe earlier closure analysis was correct for the The dashboard has a different boundary:
The final patch at The Baileys Security review —
|
Security review —
|
|
Automated-review classification: both advisor lanes failed because required analysis sections were omitted. The published receipt reports zero blockers and no actionable follow-up, so this is an advisor protocol failure rather than a repository or test failure. I am using one bounded failed-job rerun; if it repeats, I will stop retrying and document the external blocker. |
|
Final gate disposition: the protected E2E run completed successfully across Hermes, full end-to-end coverage, both managed-image architectures, inference switching, and both security-posture targets. The single bounded advisor retry is also complete. GPT-5.6 Terra passed; Nemotron repeated the previously classified protocol failure, while the publisher completed and reported no blocker or actionable finding. I will not rerun that advisor lane again. This is an external advisor-protocol blocker, not a repository or test failure. All other technical, documentation, and security gates are green. The PR remains unmergeable until the repository accepts the advisor state and an independent reviewer approves it. |
|
Status correction: the current rollup is now fully green—45 checks pass with no pending or failed gates, including protected E2E and the current advisor receipt. The earlier advisor failure is no longer a current blocker. The PR remains mergeable and is waiting only for repository approval; I will re-audit and merge normally once approval is present. |
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
Security review — final head
|
Advisor warning disposition
Adding a Docker-dependent unit test or another image-build implementation would duplicate the existing trusted image workflow and broaden this dashboard-only fix. Live CLI pairing and gateway behavior remain unchanged, while WhatsApp pairing/send/receive continues under the existing runtime E2E gate. |
Summary
Hermes
v2026.7.20resolves dashboard WhatsApp pairing state relative to the dashboard's isolatedHERMES_HOME, while the gateway reads the main Hermes home. This change patches only the dashboard session helper so both use/sandbox/.hermes/platforms/whatsapp/session; Hermes CLI pairing and gateway code remain unchanged.Related Issue
Addresses the dashboard WhatsApp session-path mismatch in #8184. Upstream Baileys code 515, missing sync keys, connection reliability, and message-delivery defects remain outside this PR.
Changes
The direct source patch is required because the pinned upstream Hermes release computes the dashboard path from its isolated home. The production patch application and
test/hermes-whatsapp-dashboard-session-patch.test.tsprotect this compatibility contract.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/security/hermes-0.19.0-dependency-review.mdrecords the replacement Hermes multi-platform base-image OCI index, its trusted workflow run, and source commit. Existing WhatsApp documentation already describes the corrected Hermes session path and rebuild-persistence contract. The docs build completed with 0 errors and 2 existing warnings.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run test/hermes-whatsapp-dashboard-session-patch.test.ts test/hermes-final-image-layout.test.ts src/lib/agent/base-image-hermes-resolution.test.ts src/lib/agent/base-image-hermes.test.tspassed 24/24.npm run docsbuilds without warnings (doc changes only) — it completed with 0 errors and 2 existing warnings.Additional image evidence:
sha256:3d54b928baef9df403227e846f73079d13ca8424a27cd5268ca97bac3f030b27.mainboth contain the corrected dashboard helper.scripts/check-production-build-args.shpassed for the new immutable base reference.Signed-off-by: San Dang sdang@nvidia.com