fix(e2e): bind Hermes Discord websocket endpoint - #10296
Conversation
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit a21a881 in the TypeScript / code-coverage/cliThe overall line coverage in commit a21a881 in the Show a line coverage summary of the most impacted files.
Updated |
Security review —
|
wscurran
added
area: ci
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
Handoff: #10281 now contains this PR's protocol-aware fake Hermes Discord credential binding and negative fixture coverage, plus the production inactive-preset correction required by #10153. #10281 is the composed candidate; these two PRs should not both merge. No exact hermes-discord selector has passed yet, so #10281 stays draft until that branch E2E executes and passes. I am leaving this PR open for maintainer confirmation rather than closing it unilaterally. |
<!-- markdownlint-disable MD041 --> ## Summary Hermes post-boot policy selection could retain repository-owned messaging presets when `enabledChannels` did not include those channels. The selector now removes those inactive presets, preserves same-name custom presets, and retains required presets for enabled channels without changing OpenClaw selection. ## Related Issue Part of #10153. Supersedes #10296. This PR preserves Charan Jagwani's protocol-aware Hermes Discord fixture correction as commit `3cf9274893309fa837c994ae15680ce9849ae185` and adds the production inactive-preset correction. ## E2E Root-Cause Claim E2E root cause: Hermes policy selection / post-boot preset reconciliation / an inactive repository-owned messaging preset survives the authoritative enabled-channel set and references an unattached provider. Source run: https://github.com/NVIDIA/NemoClaw/actions/runs/32869481833 Affected jobs: - Hermes open egress, job 97873593428 - Hermes Discord rebuild, job 97873593660 Stable signature: `credential_binding references provider '<sandbox>-slack-app', but that provider is not attached to the sandbox`. Scope: one root cause across two jobs. PR #10271 separately owns the earlier equal-specificity Slack route ambiguity. The Hermes Slack API HTTP 500 result is not part of this PR. Focused E2E selectors: `common-egress-agent-hermes-open-reference,hermes-discord`. ## Changes - Derive repository-owned messaging preset names from the channel manifest registry. - Exclude inactive Hermes messaging presets from fresh suggestions, recorded resume state, applied-preset preservation, and custom-list reconciliation. - Preserve same-name custom presets and existing OpenClaw behavior. - Merge required presets for enabled Hermes messaging channels after pruning inactive presets. - Persist the selected preset list only after gateway synchronization and post-sync readiness succeed, so failed removal remains retryable. - Add regression coverage for empty enabled channels, Discord-only and Google Chat selection, stale applied Slack state, custom ownership, failed removal and resume, custom lists, and OpenClaw compatibility. The shared pruner serves the fresh, resume, and custom selection consumers. Separate filters at each call site would duplicate the channel-to-preset authority. The focused suggestion, resume, selection, and handler tests protect these paths. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior. Justification: - [ ] Tests not applicable. Justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded. Reviewer/approval link/justification: Codex Desktop independently reviewed all nine security categories at commit under review `f53c448bdb2bc04a99c7ef7059821cec886a4eda`; result `PASS` with no findings. The latest commit is a signed empty child of `39aaae46641a133818622f441d63410905b4f574` with the identical reviewed tree. The selector narrows repository-owned Hermes messaging presets to enabled channels, preserves same-name custom policies, restores required presets after pruning, and persists the selection only after live synchronization and readiness succeed. The change does not expose credentials or widen hosts, providers, methods, protocols, or policy authority. - [ ] Non-success, skipped, or missing CI check accepted by maintainer. Check name, approval link, and follow-up issue: None accepted. Current exact-head checks are still in progress. ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable - Station profile/scenario: Not applicable - Result: Not applicable - Supporting evidence: Not applicable ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] `npm run validate:pr` passed for the unchanged effective patch at `0273a1deb451f2c106658c27d7d0482eb9f913f9`. Commit under review `f53c448bdb2bc04a99c7ef7059821cec886a4eda` is a signed empty child with the identical tree, based on current main `803a587752aef1896c659f0ed7373d501b3b4b24`. - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above. For the unchanged effective patch, 177 focused deterministic tests and the added rebuild regression passed 1/1. The CLI build and typecheck, repository checks, `npm run validate:pr`, `npm run docs` with 0 errors and 2 existing warnings, and `git diff --check` passed. The current signed empty child has the identical tree and stable patch ID. - [ ] Applicable broad gate passed. Not run because this change affects one policy-selection failure class; the focused suite and `npm run validate:pr` passed. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Reviewed the complete 24-file effective diff at commit under review `f53c448bdb2bc04a99c7ef7059821cec886a4eda` against base SHA `803a587752aef1896c659f0ed7373d501b3b4b24`. The latest PR commit is a signed empty child of `39aaae46641a133818622f441d63410905b4f574` with the same tree, and the stable patch ID matches the previously corrected diff. The four changed documentation pages accurately limit inactive built-in messaging preset removal and exact custom-policy preservation to Hermes. Shared onboarding guidance accurately states that skip mode applies required presets for enabled channels. The diagnostic, comments, and behavior-oriented test titles match the implementation. The supplied CLI build and typecheck, repository checks, `npm run validate:pr`, docs build with 0 errors and 2 existing warnings, focused deterministic tests, rebuild regression, and fresh `git diff --check` passed. - Agent: Codex documentation writer subagent (`/root/docs_review_10270`) <!-- docs-review-head-sha: f53c448 --> <!-- docs-review-agents-blob-sha: 281e79f --> --- Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Hermes onboarding and sandbox rebuilds by removing inactive built-in messaging presets. * Preserves enabled-channel and custom presets, including custom presets sharing names with inactive channels. * Added Google Chat support to channel-based preset selection. * Improved interrupted onboarding recovery, persistence failures, and retry behavior. * Preserves required presets in custom and skip modes for enabled messaging channels. * **Documentation** * Clarified messaging-channel behavior during onboarding and sandbox recovery. * **Tests** * Expanded coverage for onboarding, rebuilds, resume selection, custom ownership, retries, and supported channels. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Co-authored-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Co-authored-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
|
Closing as superseded by merged PR #10281. That PR incorporated this protocol-aware Hermes Discord WebSocket credential-binding correction, together with the production inactive-preset fix, and its exact Hermes E2E selectors passed before merge. Keeping this draft open would leave duplicate ownership of changes already delivered on main. Thank you for isolating and validating the original correction. |
3 participants
Summary
The Hermes Discord E2E now binds its fake Gateway credential provider to the WebSocket endpoint explicitly. Before this change, an HTTP and WebSocket endpoint sharing the same host and port could leave the credential on the wrong endpoint, causing OpenShell to close the native Discord Gateway connection before READY.
Related Issue
Part of #10155.
Changes
websocketfrom the live Hermes Discord scenario.Type of Change
Quality Gates
a21a881b4f23c21d51224545487efeef4882e926against the nine-category security checklist. The change narrows credential binding to the WebSocket endpoint and the follow-up commit only centralizes a test invocation; it does not change credential values, redaction, endpoint access, or sandbox permissions. The exact-head PR Review Advisor Trust specialist and synthesis also completed with no findings. Verdict: PASS.DGX Station Hardware Evidence
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 unavailable — all normal hooks passed fora21a881b4f23c21d51224545487efeef4882e926; the pre-push CLI build/typecheck passed after generating the clean worktree's ignoreddist/artifacts.git diff --check; the exact-head PR Review Advisor completed all specialists and synthesis with high-confidencemerge_as_isand no findings.hermes-discordlive E2E is waiting on the trusted catalogue-profile correction in fix(e2e): build changed sandbox images locally #10263; this PR remains draft until that live target passes.npm run docsbuilds without warnings (doc changes only)Documentation Writer Review
no-docs-neededa21a881b4f23c21d51224545487efeef4882e926changes only three E2E fixture/live/support files and does not change shipped Hermes Discord behavior or user workflow.Exact E2E Validation
hermes-discorda21a881b4f23c21d51224545487efeef4882e92632915283135attempt 1 stopped inAuthenticate manual PR dispatchbecause the workflow token returned HTTP 403; the selector did not execute.32915283135attempt 2 passed manual-PR authentication and matrix generation, but onboarding stopped before the Discord assertions because the reusable catalogue profile did not receive the trusted managed-image revision and GHCR correctly returned HTTP 404 for unpublished PR versionv0.0.114-107-ga21a881b4.Signed-off-by: Charan Jagwani cjagwani@nvidia.com