Skip to content

fix(hermes): align dashboard WhatsApp session path - #8229

Open
sandl99 wants to merge 35 commits into
mainfrom
fix/hermes-whatsapp-session-path
Open

fix(hermes): align dashboard WhatsApp session path#8229
sandl99 wants to merge 35 commits into
mainfrom
fix/hermes-whatsapp-session-path

Conversation

@sandl99

@sandl99 sandl99 commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hermes v2026.7.20 resolves dashboard WhatsApp pairing state relative to the dashboard's isolated HERMES_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

  • Revert the earlier broad runtime and configuration implementation in one explicit commit.
  • Extend the existing pinned-Hermes source patch so dashboard onboarding stores and detects WhatsApp credentials in the gateway's session directory.
  • Add a focused regression test that applies the production patch and verifies that Hermes CLI and gateway sources are not patched.
  • Publish the patched amd64 and arm64 Hermes base images, pin their immutable multi-platform OCI index in the final Dockerfile, and record the artifact evidence in the Hermes dependency review.

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.ts protect this compatibility contract.

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)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: The final four-file, nine-category security review passed with no findings: fix(hermes): align dashboard WhatsApp session path #8229 (comment)
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: docs-updated
  • Evidence: docs/security/hermes-0.19.0-dependency-review.md records 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.
  • Agent: Codex CLI

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit: Not applicable; scripts/prepare-dgx-station-host.sh is unchanged.
  • Station profile/scenario: Not applicable.
  • Result: Not applicable.
  • Supporting evidence: Not applicable.

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — npx 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.ts passed 24/24.
  • Applicable broad gate passed — Not applicable to this narrow pinned-source compatibility patch; the trusted multi-platform base build, merged-main final-image build, and fresh GitHub CI provide image-level validation.
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only) — it completed with 0 errors and 2 existing warnings.
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Additional image evidence:

  • Trusted base workflow 31031662054, attempt 1, passed for amd64 and arm64 and published OCI index sha256:3d54b928baef9df403227e846f73079d13ca8424a27cd5268ca97bac3f030b27.
  • The published base and the final image built after merging current main both contain the corrected dashboard helper.
  • scripts/check-production-build-args.sh passed for the new immutable base reference.

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

Signed-off-by: San Dang <sdang@nvidia.com>
@sandl99 sandl99 added integration: whatsapp WhatsApp integration or channel behavior integration: hermes Hermes integration behavior area: messaging Messaging channels, bridges, manifests, or channel lifecycle labels Aug 4, 2026
@sandl99 sandl99 self-assigned this Aug 4, 2026
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR adds compiled Hermes messaging preloads, manifest-driven runtime setup, WhatsApp session-path normalization, startup integration, permission checks, secret scans, and supporting tests.

Changes

Hermes messaging runtime

Layer / File(s) Summary
Runtime preload packaging
agents/hermes/Dockerfile, test/messaging-runtime-preload-packaging.test.ts, test/hermes-final-image-layout.test.ts
Hermes compiles messaging channel TypeScript sources and packages JavaScript preload files with duplicate and metadata checks.
WhatsApp Hermes session preload
src/lib/messaging/channels/whatsapp/*, src/lib/messaging/manifest/types.ts, test/generate-hermes-config.test.ts
WhatsApp uses a persistent Hermes session path and installs a required boot and connect preload that validates and normalizes bridge session arguments.
Manifest-driven runtime setup
scripts/lib/sandbox-init.sh, scripts/nemoclaw-start.sh, test/messaging-runtime-secret-scan.test.ts, test/nemoclaw-start-runtime-env-alias.test.ts
Runtime metadata is validated and normalized. Environment aliases, preloads, secret scans, and dynamic permission targets are applied during setup.
Hermes startup integration
agents/hermes/start.sh, test/hermes-messaging-runtime-setup.test.ts, test/hermes-start.test.ts
Root and non-root Hermes startup runs messaging preparation, publishes connect preload exports, and uses shared temporary-file permission validation.
Entrypoint source and regression tests
test/support/entrypoint-script-fixture.ts, test/*runtime*.test.ts, test/nemoclaw-start*.test.ts, test/http-proxy-fix-sync.test.ts, test/nemotron-inference-fix.test.ts, ci/test-file-size-budget.json
Shell tests load shared sandbox initialization and startup scripts, use the explicit messaging setup terminator, and update related fixtures and budgets.

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
Loading

Suggested labels: bug-fix, area: security

Suggested reviewers: cv

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the Hermes WhatsApp session-path fix, which is the primary objective of the pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/hermes-whatsapp-session-path

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

@github-code-quality

github-code-quality Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 9dcef91 in the fix/hermes-whatsapp-... branch remains at 96%, unchanged from commit c176af5 in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 9dcef91 in the fix/hermes-whatsapp-... branch remains at 82%, unchanged from commit c176af5 in the main branch.

Show a code coverage summary of the most impacted files.
File main c176af5 fix/hermes-whatsapp-... 9dcef91 +/-
src/lib/onboard...-transaction.ts 94% 94% 0%
src/lib/sandbox...rce-identity.ts 87% 87% 0%
src/lib/domain/.../connect-env.ts 89% 97% +8%

Updated August 05, 2026 18:13 UTC

Signed-off-by: San Dang <sdang@nvidia.com>
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: Review the warnings below.
Findings: 0 blockers · 1 warning · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 1 warning · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 2 warnings · 1 suggestion
  • Model comparison: normalized findings differ; normalized terminology decisions differ; normalized E2E selections differ; Nemotron reported the same number of blockers, 1 more warning, 1 more suggestion.
2 terminology differences from the second opinion

Advisory only. These are normalized differences from the primary terminology receipt.

  • session directory at test/hermes-whatsapp-dashboard-session-patch.test.ts:13: selected only by the second-opinion lane as established.
  • dashboard WhatsApp session at docs/security/hermes-0.19.0-dependency-review.md:236: selected only by the second-opinion lane as justified.
2 additional E2E selections from the second opinion

Advisory only. The primary lane did not select these E2E jobs or targets.

  • hermes-discord: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
  • hermes-slack: The completed second-opinion lane identified E2E coverage that the primary lane omitted.

Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate.

2 semantic terminology decisions

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

  • justified — dashboard pairing state at agents/hermes/whatsapp-proxy.patch:5: Retain the term because the dashboard-specific modifier identifies the changed state owner.
  • justified — gateway session directory at test/hermes-whatsapp-dashboard-session-patch.test.ts:13: Retain the term because the gateway modifier identifies the required shared state location.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: cloud-onboard, full-e2e, hermes-e2e, hermes-inference-switch, managed-image-multiarch-startup, security-posture

1 optional E2E recommendation
  • rebuild-hermes
1 warning · 0 suggestions

Warnings

Warnings do not block.

PRA-1 Warning — Cover the patched helper in the pinned Hermes image

  • Location: test/hermes-whatsapp-dashboard-session-patch.test.ts:13
  • Category: tests
  • Problem: The new test applies the hunk to a synthetic web_server.py fixture, but it does not verify that the exact base image selected by agents/hermes/Dockerfile contains the expected source or that its dashboard helper resolves the shared session path.
  • Impact: A base-image digest whose upstream source context differs from the fixture can leave dashboard pairing state in its isolated HERMES_HOME despite the hunk-level test passing.
  • Recommendation: Add an image-level regression that uses the pinned Hermes base and verifies the dashboard session helper resolves /sandbox/.hermes/platforms/whatsapp/session while the Hermes CLI path remains unchanged.
  • Verification: Inspect the image-build test or probe and confirm it exercises the BASE_IMAGE digest from agents/hermes/Dockerfile rather than a synthetic source fixture.
  • Test coverage: An image-level test for the Dockerfile-pinned Hermes base that imports or invokes _whatsapp_session_path(), asserts the shared path, and confirms the CLI pairing path is unchanged.
  • Evidence: test/hermes-whatsapp-dashboard-session-patch.test.ts:13-23 writes a local web_server.py fixture before applying agents/hermes/whatsapp-proxy.patch. agents/hermes/Dockerfile:10 changes the pinned BASE_IMAGE digest. The test has no Dockerfile or pinned-image reference.

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8295d20 and d7d2c17.

📒 Files selected for processing (4)
  • agents/hermes/Dockerfile
  • agents/hermes/patch-whatsapp-session-path.mts
  • test/hermes-final-image-layout.test.ts
  • test/hermes-whatsapp-session-path-patch.test.ts

Comment thread test/hermes-whatsapp-session-path-patch.test.ts Outdated
@apurvvkumaria apurvvkumaria self-assigned this Aug 4, 2026

@cjagwani cjagwani left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (5)
test/hermes-messaging-runtime-setup.test.ts (2)

54-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The 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_exports with a literal string, so the test proves only that write_runtime_shell_env calls the function. It does not prove that a connect preload recorded by install_messaging_runtime_preloads becomes a --require entry in NODE_OPTIONS.

Source the real emit_messaging_connect_runtime_preload_exports from scripts/lib/sandbox-init.sh, point _MESSAGING_CONNECT_PRELOADS_FILE at a temporary list that contains a real file, then evaluate the emitted environment and assert the resulting NODE_OPTIONS value.

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 win

Reuse the existing shell-function extraction helper.

test/hermes-start.test.ts already provides extractShellFunctionFromSource for the same purpose. This local shellFunction duplicates 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 win

Guard the preload flatten step against basename collisions and a silently empty copy.

The cp flattens every */runtime/*.js into one directory by basename. If two channels ship a runtime file with the same basename, the second cp silently overwrites the first, and a manifest source path then resolves to another channel's code. The if [ -d ... ] guard also lets the build succeed when the copy at Line 145 produces nothing; the failure then surfaces only at startup as Missing 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 win

Sensitive 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.sh

Add 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 lift

Test 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

📥 Commits

Reviewing files that changed from the base of the PR and between d7d2c17 and c3e80d3.

📒 Files selected for processing (21)
  • agents/hermes/Dockerfile
  • agents/hermes/start.sh
  • scripts/lib/sandbox-init.sh
  • scripts/nemoclaw-start.sh
  • src/lib/messaging/channels/whatsapp/manifest.ts
  • src/lib/messaging/channels/whatsapp/runtime/whatsapp-hermes-session.test.ts
  • src/lib/messaging/channels/whatsapp/runtime/whatsapp-hermes-session.ts
  • src/lib/messaging/manifest/types.ts
  • test/generate-hermes-config.test.ts
  • test/hermes-final-image-layout.test.ts
  • test/hermes-messaging-runtime-setup.test.ts
  • test/hermes-start.test.ts
  • test/http-proxy-fix-sync.test.ts
  • test/messaging-runtime-preload-packaging.test.ts
  • test/nemoclaw-start-guard-recovery.test.ts
  • test/nemoclaw-start-runtime-env-alias.test.ts
  • test/nemoclaw-start-slack-runtime.test.ts
  • test/nemoclaw-start-telegram-runtime.test.ts
  • test/nemoclaw-start.test.ts
  • test/nemotron-inference-fix.test.ts
  • test/support/entrypoint-script-fixture.ts

Comment thread scripts/lib/sandbox-init.sh Outdated
Comment thread src/lib/messaging/channels/whatsapp/runtime/whatsapp-hermes-session.ts Outdated
Signed-off-by: San Dang <sdang@nvidia.com>

@cjagwani cjagwani left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
cjagwani previously approved these changes Aug 4, 2026

@cjagwani cjagwani left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sandl99 added 2 commits August 5, 2026 00:09
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Clean up the temporary directory in a finally block.

fs.rmSync(tmpDir, ...) runs only after setup and spawnSync complete normally. If any operation throws, the temporary directory remains. Wrap setup and execution in try/finally to 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

📥 Commits

Reviewing files that changed from the base of the PR and between 57f4e94 and 8b2aa86.

📒 Files selected for processing (1)
  • test/messaging-runtime-secret-scan.test.ts

Signed-off-by: San Dang <sdang@nvidia.com>
@sandl99 sandl99 added v0.0.103 Release target and removed v0.0.102 labels Aug 4, 2026
Signed-off-by: San Dang <sdang@nvidia.com>
@sandl99

sandl99 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Security review addendum — 0a3f3fef4

PASS; no new finding. The only change after the full nine-category review at 24c4688a0 replaces an if statement with an equivalent conditional expression in test/hermes-messaging-runtime-setup.test.ts to satisfy the test-conditional guardrail. Production behavior, trust boundaries, dependencies, configuration, and documentation are unchanged. The route test, Biome, conditional scanner, diff check, commit hooks, pre-push checks, signature verification, and documentation receipt all pass at this head. The prior full review remains applicable.

Comment thread test/hermes-messaging-runtime-setup.test.ts Fixed
@sandl99

sandl99 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Closing after re-testing from main and comparing the behavior with the Hermes v2026.7.20 source.

The observed failed to find key ... to decode mutation messages are emitted by Baileys while synchronizing WhatsApp app-state collections. They reproduce on main, occur after authentication succeeds, and are unrelated to the filesystem location of creds.json.

For the normal hermes whatsapp flow, Hermes already provides compatibility between the two session layouts:

  • The CLI pairs into ~/.hermes/whatsapp/session.
  • The gateway prefers ~/.hermes/platforms/whatsapp/session, but get_hermes_dir() selects the legacy directory when it contains session data.

That explains why the unpatched flow works for other users. The preload in this PR instead rewrites the Node bridge --session argument to the canonical directory while the Python CLI still checks the legacy directory after pairing. This can make the CLI report Pairing may not have completed and risks creating the mismatch the PR intended to remove.

The separate dashboard HERMES_HOME behavior described in #8184 may still deserve investigation, but it needs a dedicated current reproduction showing that dashboard pairing writes valid credentials that the gateway cannot discover. It should not be addressed by changing the working CLI compatibility path.

Closing this PR because the proposed fix is not required for the reproduced CLI flow and is not safe to merge as implemented.

@sandl99 sandl99 closed this Aug 5, 2026
@sandl99 sandl99 changed the title fix(hermes): unify WhatsApp session state fix(hermes): share dashboard WhatsApp session state Aug 5, 2026
@sandl99 sandl99 reopened this Aug 5, 2026
@sandl99

sandl99 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Correction: dashboard scope is valid

The earlier closure analysis was correct for the hermes whatsapp CLI, but it did not cover the dashboard pairing process. The CLI already uses Hermes' legacy session compatibility, so this PR no longer changes or routes the CLI.

The dashboard has a different boundary:

  • It runs with HERMES_HOME=/sandbox/.hermes/dashboard-home.
  • Its WhatsApp bridge therefore receives a dashboard-local session path.
  • The gateway runs from the primary Hermes home and reads /sandbox/.hermes/platforms/whatsapp/session.

The final patch at 1ff42e8d5 addresses only that mismatch. Its preload matches the exact dashboard bridge path /sandbox/.hermes/dashboard-home/scripts/whatsapp-bridge/bridge.js and replaces only that process's --session value with /sandbox/.hermes/platforms/whatsapp/session. Tests explicitly verify that the CLI/gateway bridge paths remain unchanged. The manifest is boot-only; there is no command routing, session_path config injection, or .env rewrite handling.

The Baileys failed to find key ... to decode mutation app-state errors remain unrelated and outside this PR.

Security review — 1ff42e8d5

PASS; no findings. The dashboard process match and target session path are constants, malformed or duplicate --session arguments fail closed, and unrelated Node processes are untouched. The preload is packaged root-owned and read-only. Dashboard-created session files remain group-private for the dashboard and gateway users. No dependency, network policy, authentication, authorization, cryptography, or privileged-execution behavior changes.

Validation at this head:

  • Focused dashboard and CLI/gateway boundary suites: 70/70 passed.
  • Wider relevant runtime selection: 349/353 passed. The four failures are unchanged macOS rlimit probes that fail before WhatsApp logic and reproduce when run alone.
  • TypeScript pre-push hooks passed.
  • Documentation writer review passed for all 27 changed files.
  • npm run docs passed with 0 errors and two existing warnings.
  • Every PR commit is GitHub Verified.

@sandl99
sandl99 marked this pull request as ready for review August 5, 2026 14:14
@sandl99
sandl99 marked this pull request as draft August 5, 2026 14:25
@sandl99

sandl99 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Security review — a7a7f3e47

Verdict

PASS. The final net diff is safe to merge from a security perspective. It changes only the pinned Hermes dashboard helper that selects the WhatsApp session directory and adds a regression test. No findings were identified.

Detailed analysis

  1. Secrets and credentials — PASS. No secret values, tokens, credentials, .env files, or key material are added or logged. The path contains session credentials but no credential contents.
  2. Input validation and sanitization — PASS. The dashboard path is a fixed NemoClaw-owned location; no user-controlled path or shell evaluation is introduced.
  3. Authentication and authorization — PASS. No endpoint, authentication, authorization, sender policy, or access-control behavior changes.
  4. Dependencies and third-party libraries — PASS. No dependency, lockfile, registry, or version changes.
  5. Error handling and logging — PASS. No error or logging behavior changes, and the new test does not expose session contents.
  6. Cryptography and data protection — PASS. No cryptography changes. Dashboard pairing now targets the existing persistent WhatsApp session directory already provisioned by NemoClaw.
  7. Configuration and security headers — PASS. No configuration, .env, header, port, container-user, or permission code changes.
  8. Security testing — PASS. The regression test applies the production patch and verifies the canonical path; a negative test verifies that Hermes CLI and gateway sources are not patched.
  9. System security — PASS. No Landlock, network-policy, credential-provider, privilege, restart-seal, or sandbox-boundary control is weakened.

Files reviewed:

  • agents/hermes/whatsapp-proxy.patch
  • test/hermes-whatsapp-dashboard-session-patch.test.ts

Validation: exact patch application against pinned Hermes v2026.7.20, focused tests 13/13, Biome, git diff --check, normal commit hooks, pre-push TypeScript checks, and gitleaks all passed.

@sandl99 sandl99 changed the title fix(hermes): share dashboard WhatsApp session state fix(hermes): align dashboard WhatsApp session path Aug 5, 2026
@sandl99
sandl99 marked this pull request as ready for review August 5, 2026 14:58
@apurvvkumaria

Copy link
Copy Markdown
Collaborator

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.

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

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.

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

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.

@cjagwani cjagwani added v0.0.104 Release target and removed v0.0.103 Release target labels Aug 5, 2026
@sandl99

sandl99 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Security review — final head 9dcef9107

Verdict: PASS. I found no blocking security issue in the four-file net diff against current main.

  1. Secrets and credentials — PASS. No credential value is committed or logged. The dashboard now writes QR-pairing state to the existing gateway-owned durable session directory; it does not add another copy or expose the state outside the established /sandbox/.hermes boundary.
  2. Input validation — PASS. The change consumes no user-controlled path. It replaces the dashboard-relative lookup with one constant, image-owned path, and the existing source patch is exact-shape guarded during the base-image build.
  3. Authentication and authorization — PASS. No API, sender allowlist, pairing authorization, gateway authentication, CLI pairing, or gateway source changes. The regression test explicitly rejects CLI and gateway patch hunks.
  4. Dependencies and supply chain — PASS. No package version or dependency graph changes. Trusted workflow run 31031662054, attempt 1, built both architectures from source commit a7a7f3e470a75c404d316d2054445e16bb63b48c and published immutable OCI index sha256:3d54b928baef9df403227e846f73079d13ca8424a27cd5268ca97bac3f030b27; the final Dockerfile pins that exact index.
  5. Error handling and logging — PASS. No exception, retry, or logging behavior changes, and the path contains no secret material.
  6. Cryptography and data protection — PASS. No cryptographic implementation changes. The image is selected by immutable SHA-256 digest, and existing session-directory ownership and modes remain unchanged.
  7. Security configuration — PASS. The final image retains the established shared Hermes session-directory boundary and its permissions. No network policy, environment allowlist, security header, sandbox capability, or credential-routing change is introduced.
  8. Security testing — PASS. The production patch applies to the pinned source; focused tests passed 24/24; the published base and merged-main final images both contain the patched helper; amd64 and arm64 base builds passed the existing source, dependency, and proxy guards; normal commit and push hooks passed.
  9. System and container security — PASS. The patch is limited to the dashboard helper and does not add symlinks, fallback lookup, migration logic, elevated processes, writable code, or host-path access. The final image built successfully from current main with the new immutable base digest.

Residual scope: this review does not claim to fix Baileys code 515, missing sync keys, reconnect reliability, or message delivery. Live WhatsApp pairing/send/receive remains separate runtime evidence under the existing Hermes dependency review.

@sandl99

sandl99 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Advisor warning disposition

PRA-1 is covered without expanding this narrow source-patch change:

  • Trusted workflow run 31031662054, attempt 1, built the exact PR source commit for amd64 and arm64. The build applies whatsapp-proxy.patch to the pinned Hermes source with the existing exact-source guard and published OCI index sha256:3d54b928baef9df403227e846f73079d13ca8424a27cd5268ca97bac3f030b27.
  • The final Dockerfile pins that exact OCI index.
  • A direct container probe against the published base verified that its installed hermes_cli/web_server.py contains return Path("/sandbox/.hermes/platforms/whatsapp/session").
  • The final image was rebuilt after merging current main; a second direct container probe verified the same installed helper.
  • The focused regression test additionally proves that the production patch leaves Hermes CLI and gateway sources untouched.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: messaging Messaging channels, bridges, manifests, or channel lifecycle integration: hermes Hermes integration behavior integration: whatsapp WhatsApp integration or channel behavior release-target v0.0.104 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants