fix(messaging): diagnose Hermes WhatsApp session split - #8242
Conversation
|
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:
📝 WalkthroughWalkthroughHermes WhatsApp status checks inspect gateway and dashboard session files, report session-location diagnostics, and provide pairing remediation. Channel removal clears both session paths. The manifest, tests, and documentation cover Hermes and OpenClaw behavior. ChangesHermes WhatsApp diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ChannelStatus
participant HermesStatusHealth
participant Sandbox
participant DiagnosticsEvaluator
ChannelStatus->>HermesStatusHealth: request detailed WhatsApp status
HermesStatusHealth->>Sandbox: check gateway and dashboard credential paths
Sandbox-->>HermesStatusHealth: return validated presence markers
HermesStatusHealth->>DiagnosticsEvaluator: provide session locations and reachability
DiagnosticsEvaluator-->>ChannelStatus: return verdict and pairing guidance
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
sandl99
added
area: messaging
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 4 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
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
99f3a57 to
bdb02de
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/manage-sandboxes/set-up-whatsapp.mdx (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a verification step before removing the override.
The instruction gives a condition but does not tell the operator how to prove it.
Point to the status output or another observable result that confirms dashboard pairing and gateway startup use the same session path.
As per coding guidelines, task-oriented documentation must explain what to do, when to do it, and how to verify it.🤖 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 `@docs/manage-sandboxes/set-up-whatsapp.mdx` at line 46, Update the instruction about removing the override in the Hermes WhatsApp session setup to include a verification step first: have the operator inspect the status output or another observable result confirming dashboard pairing and gateway startup use the same shared session path, then remove the override only after that confirmation.Source: Coding guidelines
🤖 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 `@docs/manage-sandboxes/set-up-whatsapp.mdx`:
- Line 44: Update the sentence describing --config-accept-new-path to replace
the user-facing term “dotpath” with “configuration key,” specifically referring
to platforms.whatsapp.extra.session_path.
- Around line 41-48: The warning in the WhatsApp sandbox setup documentation
should distinguish direct-edit hash rejection from supervisor quarantine. Update
the text near the config set command to state that manual edits may cause the
integrity guard to reject restart with hash-mismatch, while supervisor
quarantine occurs after repeated relaunch failures or other integrity failures;
preserve the existing guidance against manually editing the Hermes files.
---
Nitpick comments:
In `@docs/manage-sandboxes/set-up-whatsapp.mdx`:
- Line 46: Update the instruction about removing the override in the Hermes
WhatsApp session setup to include a verification step first: have the operator
inspect the status output or another observable result confirming dashboard
pairing and gateway startup use the same shared session path, then remove the
override only after that confirmation.
🪄 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: c588cb65-babe-42de-89be-6881a0da8907
📒 Files selected for processing (10)
docs/manage-sandboxes/set-up-whatsapp.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/channel-status.test.tssrc/lib/actions/sandbox/channel-status.tssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health-eval.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health-eval.tssrc/lib/messaging/channels/whatsapp/hooks/status-health.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health.tssrc/lib/messaging/channels/whatsapp/manifest.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/lib/actions/sandbox/channel-status.ts
- src/lib/messaging/channels/whatsapp/manifest.ts
- src/lib/messaging/channels/manifests.test.ts
- src/lib/messaging/channels/whatsapp/hooks/status-health-eval.test.ts
- src/lib/actions/sandbox/channel-status.test.ts
- src/lib/messaging/channels/whatsapp/hooks/status-health.test.ts
- docs/reference/commands.mdx
- src/lib/messaging/channels/whatsapp/hooks/status-health.ts
- src/lib/messaging/channels/whatsapp/hooks/status-health-eval.ts
5f900b4 to
65def02
Compare
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 `@src/lib/actions/sandbox/policy-channel-remove-flow.test.ts`:
- Around line 95-100: Update the test around removeSandboxChannel to capture the
original NEMOCLAW_NON_INTERACTIVE value before overwriting it, then restore that
exact value in the finally block; delete the variable only when it was
originally unset.
🪄 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: b942cf5c-43b6-4553-bfd1-0e12b56e97b1
📒 Files selected for processing (12)
docs/manage-sandboxes/set-up-whatsapp.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/channel-status.test.tssrc/lib/actions/sandbox/channel-status.tssrc/lib/actions/sandbox/policy-channel-remove-flow.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health-eval.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health-eval.tssrc/lib/messaging/channels/whatsapp/hooks/status-health.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health.tssrc/lib/messaging/channels/whatsapp/manifest.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/manage-sandboxes/set-up-whatsapp.mdx
- src/lib/actions/sandbox/channel-status.ts
- src/lib/messaging/channels/manifests.test.ts
- src/lib/messaging/channels/whatsapp/manifest.ts
- src/lib/messaging/channels/whatsapp/hooks/status-health-eval.test.ts
- src/lib/actions/sandbox/channel-status.test.ts
- src/lib/messaging/channels/whatsapp/hooks/status-health.ts
- src/lib/messaging/channels/whatsapp/hooks/status-health-eval.ts
- docs/reference/commands.mdx
ce076e0 to
bd75a4a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
bd75a4a to
7262027
Compare
|
The merge gate cannot pass because four commits do not appear as Verified in GitHub. Repository policy requires every contributor commit to appear as Verified. Maintainers cannot amend, sign, or force-push contributor history. Please replace the branch history with contributor-created, verified commits. After that update, the required workflows can run and maintainers can continue review. |
7262027 to
dc4bb22
Compare
|
Replaced the contributor branch history with one DCO-signed SSH-signed commit, dc4bb22. GitHub API reports the commit verification as |
Signed-off-by: danielpolimac <danielpolimac@gmail.com>
dc4bb22 to
122c51a
Compare
|
Follow-up: amended the signed commit to address the advisor docs warning. Current head is 122c51a, and GitHub reports it as |
Signed-off-by: danielpolimac <danielpolimac@gmail.com>
|
Security review completed for revision
Also confirmed: all three commits currently in the PR are DCO-signed and GitHub reports each as verified. The fork workflow authorization gate was approved after this review; required CI is now running. |
|
Gate update:
No additional code change is indicated at this time. |
|
The selected E2E run completed successfully. The current branch now has 43 passing checks, no pending or failed checks, and no unresolved review threads. The remaining merge blocker is independent approval. |
|
Addressed advisor warning PRA-1 in signed commit |
|
Addressed the evaluator coverage warning in revision |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed advisor warning |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Verdict
Security review for revision 1de30e61ac7128ef58a4cdf86feee477d82d5ef1 against base c6713c9dd17a9682654713ccb6829b8714f1b307: PASS with no findings. The Hermes WhatsApp probe exposes only fixed file-presence booleans, and durable-state removal remains allowlisted, shell-quoted, ordered before lifecycle mutations, and fail-closed across both cleanup transports. The two test-only advisor follow-ups strengthen coverage without changing production behavior. All repository gates remain mandatory.
Findings Table
No findings.
Detailed Analysis
-
Secrets and Credentials — PASS. The Hermes probe checks only whether two fixed
creds.jsonpaths exist. It never reads, lists, copies, or prints credential contents. The status report contains fixed booleans and fixed diagnostic text only, and the change adds no secrets, key material, environment files, or credential fixtures. -
Input Validation and Data Sanitization — PASS. Probe commands use fixed paths and fixed output tokens. The parser accepts only the sentinel plus
presentormissingvalues and fails closed on malformed or nonzero results. Cleanup paths come from repository-owned agent definitions, are restricted to/sandbox/., reject.., pass an allowlist expression, and are shell-quoted beforerm -rf --. -
Authentication and Authorization — PASS. No identity, permission, pairing authority, or channel-ownership rule changes. Session-file presence remains diagnostic evidence only and cannot produce a healthy verdict or grant access.
-
Dependencies and Third-Party Libraries — PASS. No dependency, lockfile, image, download, registry, or external repository changes are present.
-
Error Handling and Logging — PASS. A thrown probe, nonzero command, missing sentinel, or malformed output becomes
probe_failedwithout echoing sandbox-owned text. Cleanup requires the success sentinel, falls back once from sandbox exec to SSH, and refuses all later lifecycle mutations if neither transport confirms deletion. -
Cryptography and Data Protection — PASS. No cryptographic operation changes. WhatsApp credentials are neither transformed nor copied; removal expands the existing destructive cleanup to the fixed canonical Dashboard-profile path and fixed legacy migration source.
-
Configuration and Security Headers — PASS. The documented repair uses the existing bounded
platforms.whatsapp.extra.session_pathconfiguration surface with the explicit new-path opt-in. No network policy, privilege, port, container boundary, HTTP endpoint, CORS, or security-header changes are introduced. -
Security Testing — PASS. Focused validation passes 38 tests across the evaluator and remove-flow suites. Coverage includes dashboard-only, gateway-only, both-present, malformed-probe, SSH-fallback, and both-transports-fail cases. The failure case verifies that gateway, registry/plan, policy, and rebuild state remain untouched. Commit hooks, gitleaks, CLI type checking, test-title policy, test-size policy, and diff hygiene pass.
-
Holistic Security Posture — PASS. The change reduces stale-credential risk by clearing all supported Hermes WhatsApp session locations before state mutation and by proving the operation fails closed. Diagnostics deliberately avoid treating durable credential files as live delivery or authentication proof. No sandbox escape, SSRF, policy bypass, privilege escalation, TOCTOU, or unsafe concurrency path was found.
Files Reviewed
docs/manage-sandboxes/set-up-whatsapp.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/channel-status.test.tssrc/lib/actions/sandbox/channel-status.tssrc/lib/actions/sandbox/policy-channel-remove-flow.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health-eval.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health-eval.tssrc/lib/messaging/channels/whatsapp/hooks/status-health.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health.tssrc/lib/messaging/channels/whatsapp/manifest.ts
The reviewed 12-file scope has stable patch ID 22bd5210daa7312c52ca945bc2760afa786fcbd5 against the stated base. Fresh CI, protected E2E, documentation receipt, automated review, and independent approval remain required; this security review waives none of them.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the advisor documentation warning in revision |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Verdict
Security review for revision 987d04d477f110e168f2deb12555c85e9e9bd84c against base c6713c9dd17a9682654713ccb6829b8714f1b307: PASS with no findings. The Hermes WhatsApp probe exposes only fixed file-presence booleans, and durable-state removal remains allowlisted, shell-quoted, ordered before lifecycle mutations, and fail-closed across both cleanup transports. The focused follow-ups strengthen coverage and correct documentation ownership without changing production behavior. All repository gates remain mandatory.
Findings Table
No findings.
Detailed Analysis
-
Secrets and Credentials — PASS. The Hermes probe checks only whether two fixed
creds.jsonpaths exist. It never reads, lists, copies, or prints credential contents. The status report contains fixed booleans and fixed diagnostic text only, and the change adds no secrets, key material, environment files, or credential fixtures. -
Input Validation and Data Sanitization — PASS. Probe commands use fixed paths and fixed output tokens. The parser accepts only the sentinel plus
presentormissingvalues and fails closed on malformed or nonzero results. Cleanup paths come from repository-owned agent definitions, are restricted to/sandbox/., reject.., pass an allowlist expression, and are shell-quoted beforerm -rf --. -
Authentication and Authorization — PASS. No identity, permission, pairing authority, or channel-ownership rule changes. Session-file presence remains diagnostic evidence only and cannot produce a healthy verdict or grant access.
-
Dependencies and Third-Party Libraries — PASS. No dependency, lockfile, image, download, registry, or external repository changes are present.
-
Error Handling and Logging — PASS. A thrown probe, nonzero command, missing sentinel, or malformed output becomes
probe_failedwithout echoing sandbox-owned text. Cleanup requires the success sentinel, falls back once from sandbox exec to SSH, and refuses all later lifecycle mutations if neither transport confirms deletion. -
Cryptography and Data Protection — PASS. No cryptographic operation changes. WhatsApp credentials are neither transformed nor copied; removal expands the existing destructive cleanup to the fixed canonical Dashboard-profile path and fixed legacy migration source.
-
Configuration and Security Headers — PASS. The documented repair uses the existing bounded
platforms.whatsapp.extra.session_pathconfiguration surface with the explicit new-path opt-in. The corrected wording accurately states that Dashboard pairing creates the credentials and the repair redirects the gateway to that existing path. No network policy, privilege, port, container boundary, HTTP endpoint, CORS, or security-header changes are introduced. -
Security Testing — PASS. Focused validation passes 38 tests across the evaluator and remove-flow suites. Coverage includes dashboard-only, gateway-only, both-present, malformed-probe, SSH-fallback, and both-transports-fail cases. The failure case verifies that gateway, registry/plan, policy, and rebuild state remain untouched. Commit hooks, gitleaks, CLI type checking, test-title policy, test-size policy, and diff hygiene pass. Strict documentation validation passes the current revision with 0 errors and 2 existing Fern warnings.
-
Holistic Security Posture — PASS. The change reduces stale-credential risk by clearing all supported Hermes WhatsApp session locations before state mutation and by proving the operation fails closed. Diagnostics deliberately avoid treating durable credential files as live delivery or authentication proof. No sandbox escape, SSRF, policy bypass, privilege escalation, TOCTOU, or unsafe concurrency path was found.
Files Reviewed
docs/manage-sandboxes/set-up-whatsapp.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/channel-status.test.tssrc/lib/actions/sandbox/channel-status.tssrc/lib/actions/sandbox/policy-channel-remove-flow.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health-eval.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health-eval.tssrc/lib/messaging/channels/whatsapp/hooks/status-health.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health.tssrc/lib/messaging/channels/whatsapp/manifest.ts
The reviewed 12-file scope has stable patch ID a5dac3e616eeb3c5166bd6814d7ce504538c8b19 against the stated base. Fresh CI, protected E2E, documentation receipt, automated review, and independent approval remain required; this security review waives none of them.
|
Advisor disposition for the current revision: the Terra lane stopped during scope analysis because its response omitted a required structured section. The preserved artifact contains no findings, so this is classified as a transient advisor-output failure rather than a PR defect. Nemotron is still running; the single permitted Terra retry will be requested after the workflow finishes. No code or documentation change is indicated by this failure. |
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Verdict
Security review — exact head d10e083df0e997d9aee73d67621a3b8b48f55303 against current base 1608281462923ff8282151c7be1a109ee262b4e9: PASS with no findings, subject to every repository merge gate.
Findings Table
No findings.
Detailed Analysis
-
Secrets and Credentials — PASS. The Hermes probes and cleanup operate on three fixed session paths only. They do not read, copy, enumerate, or print credential contents, and status output remains bounded to fixed sentinels and state labels.
-
Input Validation and Data Sanitization — PASS. Probe and removal commands use fixed, shell-quoted paths with bounded output parsing. The documented repair uses the existing configuration command and numeric allowlist guidance.
-
Authentication and Authorization — PASS. No identity, pairing authority, ownership, or permission rule changes. Durable credential-file presence remains an
unknownhealth signal rather than proof of authentication or delivery. -
Dependencies and Third-Party Libraries — PASS. No dependency, lockfile, image, download, or external source changes.
-
Error Handling and Logging — PASS. Removal first tries the OpenShell exec transport, then SSH. If both fail, it exits before registry, policy, or rebuild mutation. Added tests bind both the successful fallback and fail-closed dual-transport path without exposing sensitive data.
-
Cryptography and Data Protection — PASS. The change does not transform credential material. Cleanup remains limited to the fixed default, Dashboard-profile, and legacy Hermes session locations.
-
Configuration and Security Headers — PASS. The bounded
platforms.whatsapp.extra.session_pathrepair redirects the gateway to the Dashboard pairing path without rewriting pairing state. No network, privilege, capability, port, or security-header boundary changes. -
Security Testing — PASS. Exact-head focused messaging validation passes 87/87, CLI build and typecheck pass, and Fern docs validation passes with 0 errors. Raw patch SHA-256 is
b1db28f19390f5533c922363594c56a6464dd5a1eddd1d99c42a2f1de4382a2e; stable patch ID isa5dac3e616eeb3c5166bd6814d7ce504538c8b19. Fresh GitHub Actions remains authoritative. -
System Security — PASS. Removal clears every supported residual session location before mutating channel state, while status diagnostics do not overclaim live inbound health. The current-base refresh is conflict-free and its disjoint GGUF acquisition delta creates no new trust-boundary interaction.
Provenance and Required Gates
This review covers the contributor's test-only fallback additions and documentation clarification, plus the signed refresh merge with parents 987d04d477f110e168f2deb12555c85e9e9bd84c and 1608281462923ff8282151c7be1a109ee262b4e9. It supplements the prior exact-head review at #8242 (review) and applies the current rubric from .agents tree 7320c9e331ddbcd9ff83e0fce9387a004e3bd1aa. Documentation writer review passed at this exact head.
Fresh required CI, protected E2E, the exact documentation receipt, unresolved-review resolution, GitHub commit verification, and an independent exact-head approval remain mandatory. This review waives none of those gates.
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Verdict
Security review — exact head 99fc773840d7ab54749594f9a282559a30c05fdc against current base b148bc13ee53b7a1f612d07e42275ec732e71877: PASS with no findings, subject to every repository merge gate.
Findings Table
No findings.
Detailed Analysis
-
Secrets and Credentials — PASS. The effective PR patch is unchanged and adds no secret, credential source, environment exposure, or sensitive logging. The refreshed base introduces no overlapping credential path.
-
Input Validation and Data Sanitization — PASS. Reviewed validation, command construction, and path handling remain unchanged. The portable-profile base delta has no exact file overlap with this PR.
-
Authentication and Authorization — PASS. No identity, ownership, permission, or authorization boundary is widened by the PR patch or its integration with the refreshed base.
-
Dependencies and Third-Party Libraries — PASS. The PR's reviewed dependency and artifact identities are unchanged. Base PRs #8333 and #8376 add disjoint E2E artifact restoration and portable experimental onboarding behavior.
-
Error Handling and Logging — PASS. Reviewed fail-closed behavior and diagnostics remain unchanged; the signed integrations add no overlapping error or logging path.
-
Cryptography and Data Protection — PASS. Reviewed cryptographic and protected-data behavior remains unchanged, with no new cross-patch data flow.
-
Configuration and Security Headers — PASS. The effective PR patch preserves its reviewed configuration, network, privilege, capability, port, and header boundaries. The hidden portable profile does not bypass this PR's authority checks.
-
Security Testing — PASS. Raw patch SHA-256 remains
b1db28f19390f5533c922363594c56a6464dd5a1eddd1d99c42a2f1de4382a2eand stable patch ID remainsa5dac3e616eeb3c5166bd6814d7ce504538c8b19. The prior exact 87-test messaging evidence, CLI build/typecheck, and 0-error Fern validation remain applicable. Exact-head documentation writer revalidation passed; fresh GitHub Actions is authoritative. -
System Security — PASS. Both signed refreshes are conflict-free, preserve the effective patch byte-for-byte, and introduce no new runtime trust-boundary interaction. For #7853 specifically, the portable profile supplies onboarding defaults but neither stages messaging credentials nor bypasses registry-driven credential-drift validation.
Provenance and Required Gates
The exact head preserves contributor history through signed merge commits and applies the current rubric from .agents tree 7320c9e331ddbcd9ff83e0fce9387a004e3bd1aa. This supplements the full prior nine-category review at #8242 (review).
Fresh required CI, protected E2E, the exact documentation receipt, unresolved-review resolution, GitHub commit verification, and an independent exact-head approval remain mandatory. This review waives none of those gates.
|
Protected E2E is complete for the current branch/base comparison: every selected scenario passed in run 31047447403, including managed-image startup on both architectures, inference routing, live network-policy probes, onboard repair/resume, channel add/remove, and channel stop/start for OpenClaw and Hermes. The trusted controller also completed successfully: https://github.com/NVIDIA/NemoClaw/actions/runs/31047401176 The PR is approved and its current automated gates are green. Since |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Verdict
Security review for the current branch revision 7dfbf402a against main revision 35b15d503: PASS with no findings, subject to every repository merge gate.
Findings Table
No findings.
Detailed Analysis
- Secrets and credentials — PASS. The Hermes probe tests only whether two fixed
creds.jsonpaths exist. It does not read, enumerate, copy, or print credential contents. The host receives only a fixed sentinel and allowlistedpresentormissingvalues. - Input validation and data sanitization — PASS. Probe output requires the fixed sentinel and both bounded state lines. Malformed or incomplete output fails closed. Cleanup paths are constructed from repository-owned agent definitions, checked against the sandbox path allowlist, and shell-quoted before removal.
- Authentication and authorization — PASS. No identity, pairing authority, channel ownership, or permission boundary changes. Credential-file presence remains an
unknownlive-health signal rather than proof of authentication or message delivery. - Dependencies and third-party libraries — PASS. The PR adds no dependency, lockfile, image, download, or external source.
- Error handling and logging — PASS. Probe and cleanup failures do not expose credential content. Removal tries the OpenShell exec transport and then SSH; if both fail, it stops before registry, policy, gateway, or rebuild state changes.
- Cryptography and data protection — PASS. No cryptographic operation changes. Cleanup is limited to the fixed Hermes default, Dashboard-profile, and legacy migration session locations.
- Configuration and security headers — PASS. The documented repair uses the existing supported configuration command. No network policy, privilege, capability, port, HTTP header, or container boundary changes.
- Security testing — PASS. Tests cover dashboard-only, gateway-only, duplicate-session, malformed-probe, bounded-command, OpenShell-to-SSH fallback, and fail-closed dual-transport cases. The failure case verifies that registry, policy, gateway, and rebuild state remain untouched.
- Holistic security posture — PASS. The change reduces stale-session risk and improves diagnostics without treating durable credentials as live delivery evidence. The refresh preserved the reviewed patch identity. The intervening local-serving and portable-profile changes do not create a new messaging or credential data flow; the shared command documentation merged cleanly.
Files Reviewed
docs/manage-sandboxes/set-up-whatsapp.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/channel-status.test.tssrc/lib/actions/sandbox/channel-status.tssrc/lib/actions/sandbox/policy-channel-remove-flow.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health-eval.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health-eval.tssrc/lib/messaging/channels/whatsapp/hooks/status-health.test.tssrc/lib/messaging/channels/whatsapp/hooks/status-health.tssrc/lib/messaging/channels/whatsapp/manifest.ts
All 19 commits currently in the PR appear as Verified. Fresh CI, documentation review, automated advisor results, protected E2E, unresolved-review status, and approval remain authoritative; this security review waives none of those gates.
|
Advisor warning The guidance documents the canonical Hermes WhatsApp identifier form; it does not claim that a leading The NemoClaw fixtures with a leading |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Verdict
Security re-review for current branch revision 4ea2dd059: PASS with no findings, subject to every repository merge gate.
Findings Table
No findings.
Detailed Analysis
- Secrets and credentials — PASS. The follow-up does not read, copy, print, or change any credential value. It only narrows path selection by agent identity.
- Input validation and data sanitization — PASS. The fixed cleanup paths remain repository-owned, sandbox-path allowlisted, and shell-quoted. No user-controlled path is introduced.
- Authentication and authorization — PASS. The guard restores agent ownership: Hermes receives its Dashboard-profile and legacy session paths; OpenClaw retains its own WhatsApp state path.
- Dependencies and third-party libraries — PASS. No dependency, lockfile, image, download, or external source changes.
- Error handling and logging — PASS. Existing fail-closed cleanup behavior is unchanged. Dual-transport failure still stops before registry, policy, gateway, or rebuild mutation.
- Cryptography and data protection — PASS. No cryptographic operation changes. The guard prevents OpenClaw cleanup from targeting a Hermes-only path shape.
- Configuration and security headers — PASS. No network, privilege, capability, port, header, or container-boundary change.
- Security testing — PASS. The regression asserts that OpenClaw cleanup never receives a Hermes Dashboard path. The original failing teardown suite now passes 7/7, and five focused messaging suites pass 99/99. Repository hooks, secret scanning, CLI type checking, and pre-push validation pass.
- Holistic security posture — PASS. The change reduces cleanup ambiguity after the base introduced a
profilesstate directory for another agent. It preserves the previously reviewed Hermes credential-removal behavior and restores documented OpenClaw removal behavior.
Files Reviewed
src/lib/actions/sandbox/policy-channel.tstest/channels-remove-full-teardown.test.ts
The new commit appears as Verified in GitHub and includes the required DCO sign-off. This review supplements the full 12-file security review on the preceding branch revision. Fresh CI, documentation review, automated advisor results, protected E2E, unresolved-review status, and approval remain authoritative; this security re-review waives none of those gates.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Security re-review for revision 678a4700c7aa65196b02ac3609ee3a6fb66ea492: PASS — no findings.
I reviewed the complete change against current main, including the refreshed integration and the follow-up that keeps WhatsApp profile cleanup scoped to Hermes. The review covered:
- Authorization and trust boundaries
- Input validation and command injection
- Credential and sensitive-data handling
- Cryptography and secret generation
- Network and SSRF exposure
- Dependency and supply-chain changes
- Logging and information disclosure
- Resource exhaustion and denial of service
- Unsafe configuration and cleanup behavior
The Hermes probe executes a fixed command over two fixed in-sandbox paths, emits only boolean presence signals, and never reads or logs credential contents. Probe parsing fails closed. Cleanup remains constrained by the existing safe-path validator, and the new agent guard prevents OpenClaw from receiving Hermes Dashboard cleanup paths.
Validation evidence: the teardown suite passed 7/7, the focused messaging suites passed 99/99, the conditional-growth scan passed, repository hooks passed, CLI type checking passed, git diff --check passed, every PR commit is GitHub Verified, and the two follow-up commits include DCO sign-offs.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the advisor's Shields-safety warning in The Hermes repair procedure now lowers Shields with a specific maintenance reason, runs the supported Validation passed: the focused variant suite (18/18), strict documentation build (0 errors; 2 existing Fern warnings), test-title and test-size policies, conditional-growth scan, repository hooks, secret scanning, CLI type checking, and independent documentation review. |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Security supplement for revision c296f46e19d7f82738836d285b92f9b53019653d: PASS — no findings.
The follow-up changes only the Hermes documentation procedure and its rendered-page contract test. It closes the advisor's safety gap by requiring the existing Shields maintenance transition before configuration mutation and restoring the restrictive posture afterward. It introduces no runtime code, dependency, network, credential-handling, authorization, or cryptographic change.
The command sequence is repository-owned and uses the existing supported config surface. The focused test verifies the rendered Hermes order, and strict docs validation, repository hooks, secret scanning, diff hygiene, and CLI type checking pass. The commit is GitHub Verified and includes the required DCO sign-off. All repository gates remain authoritative.
5 participants
Summary
Adds Hermes WhatsApp status diagnostics for the Dashboard-profile versus gateway session-path split. The detailed status command now reports a documented bounded compatibility repair path through
platforms.whatsapp.extra.session_pathwithout treating Hermes session files as live inbound-health evidence.Related Issue
Related to #8184
Changes
creds.jsonpresence booleans for the gateway and canonical Dashboard-profile session paths.unknownlive-health evidence instead of marking the channel healthy.--config-accept-new-path, and digits-only allowlist guidance.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/manage-sandboxes/set-up-whatsapp.mdxdocuments the Hermes gateway and Dashboard session split plus a Shields-down, supported configuration repair, and Shields-up sequence.docs/reference/commands.mdxdocuments Hermes-only cleanup of the current Dashboard profile and legacy migration path while preserving OpenClaw's own cleanup path. Independent review confirmed that the procedure is accurate, safe, concise, and variant-correct. The rendered Hermes-page contract test passes, the strict docs build reports 0 errors and 2 existing Fern warnings, andgit diff --checkpasses.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchangedVerification
Signed-off-by:line and all 23 commits appear asVerifiedin GitHub.main; the change remains limited to WhatsApp diagnostics, cleanup, documentation, and focused tests.npm run docsbuilds without warnings (doc changes only) — 2 existing Fern warnings remainSigned-off-by: danielpolimac danielpolimac@gmail.com