fix(hermes): normalize Discord pairing access

[chengjiew]

Summary

Normalize Hermes Discord guild access before generating build-time env/config so blank guild keys cannot turn an empty user allowlist into pairing-gated behavior. Also patches the bundled Hermes pairing approval help to show the NemoHermes hermes pairing approve command instead of openclaw pairing approve.

Related Issue

Fixes #4070

Changes

• Filter blank Discord guild keys before generating NEMOCLAW_DISCORD_GUILD_IDS, DISCORD_ALLOWED_USERS, DISCORD_ALLOW_ALL_USERS, and discord.require_mention.
• Preserve the intended empty user allowlist behavior for valid Discord guilds by emitting DISCORD_ALLOW_ALL_USERS=true.
• Add a build-time Hermes source patch that rewrites the upstream pairing approval command text from openclaw pairing approve to hermes pairing approve.
• Add focused regression coverage for Discord guild normalization and the Hermes pairing-message patch helper.

Type of Change

• Code change (feature, bug fix, or refactor)
• Code change with doc updates
• Doc only (prose changes, no code sample modifications)
• Doc only (includes code sample changes)

Verification

Focused checks passed. Full npm test and npx prek run --all-files were attempted but did not pass in this local worktree due unrelated project-wide environment/setup failures: missing compiled/plugin artifacts or dependency (dist/*, nemoclaw/node_modules/json5), local OpenShell gateway/version drift expectations, macOS installer fixture assumptions, and local HTTP fixture 403/timeouts.

• npx prek run --all-files passes
• npm test passes
• Tests added or updated for new or changed behavior
• No secrets, API keys, or credentials committed
• Docs updated for user-facing behavior changes
• npm run docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

Additional focused verification run locally:

• npm test -- --run test/generate-hermes-config.test.ts test/hermes-pairing-message-patch.test.ts passed: 2 files, 22 tests.
• npm run typecheck:cli passed.
• git diff --check passed.

---

Signed-off-by: Chengjie Wang chengjiew@nvidia.com

[wscurran]

✨ Thanks for normalizing Hermes Discord guild access before generating build-time env/config and patching the bundled pairing approval help to show the NemoHermes hermes pairing approve command. This proposes a way to filter blank Discord guild keys to preserve empty user allowlist behavior and rewrite the pairing approval command text at build time with focused regression coverage.

---

Related open issues:

• #4070 [NemoHermes][Brev][Agent&Skills] Discord DM sends pairing code on first message when no user allowlist is configured — subsequent messages processed without approval

[cv]

Manual PR Review Advisor result

This PR Review Advisor analysis was run manually via workflow_dispatch, so the workflow did not post its usual sticky comment. Posting the advisor summary here to populate the PR with advisor feedback.

Run: https://github.com/NVIDIA/NemoClaw/actions/runs/28209656446

Recommendation: merge_after_fixes (medium confidence; findings: 4)

The config-level fix and patch helper are well covered, but the linked Discord acceptance path still needs runtime validation or an explicit justification because the bug occurs in Hermes' live DM/pairing behavior.

---

PR Review Advisor

The config-level fix and patch helper are well covered, but the linked Discord acceptance path still needs runtime validation or an explicit justification because the bug occurs in Hermes' live DM/pairing behavior.

Required before merge

• None.

Resolve or justify before merge

• Source-of-truth review needed: Build-time Hermes pairing approval text patch: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Verification hint: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: test/hermes-pairing-message-patch.test.ts covers rewriting legacy text, accepting already-corrected text, and failing closed when neither command text is found.
  • Evidence: agents/hermes/Dockerfile.base runs python3 /usr/local/bin/nemoclaw-patch-hermes-pairing-message.py /opt/hermes; the script has compatibility behavior but no explicit retirement note.
• Runtime validation is still needed for the linked Discord pairing behavior (agents/hermes/config/messaging-config.ts:67): The linked issue reports live NemoHermes Discord behavior: with a valid guild and an empty Discord user allowlist, the first server-member DM should not enter the pairing/access-not-configured path and subsequent messages should not differ. This PR proves the generated .env now emits DISCORD_ALLOW_ALL_USERS=true, but there is no adapter/runtime evidence that Hermes consumes that env value in the actual DM path that regressed.
  • Impact: A config unit test can pass while the production sandbox still shows a pairing code or inconsistent first/subsequent DM behavior if Hermes ignores, renames, or interprets DISCORD_ALLOW_ALL_USERS differently at runtime.
  • Recommendation: Add or identify a focused runtime/integration validation for the Hermes Discord path with Discord enabled, one valid guild ID, and no user allowlist, asserting the server-member DM path does not produce access not configured or a pairing code. If an external live Discord test is intentionally out of scope, add a narrower Hermes adapter-level/sandbox smoke that proves the built env/config is accepted by the runtime path.
  • Verification hint: Shortest read-only check: compare test/generate-hermes-config.test.ts assertions around DISCORD_ALLOW_ALL_USERS=true with Hermes' runtime Discord adapter contract for that env var; then inspect the added/identified runtime test name and assertion for the no-pairing first-DM path.
  • Missing regression test: Add Hermes Discord valid guild with empty user allowlist treats server-member DM as allowed without pairing or an equivalent sandbox/adapter-level test that exercises the actual runtime decision rather than only generated .env text.
  • Evidence: test/generate-hermes-config.test.ts covers generated env lines, but no changed test exercises Hermes' live Discord authorization/pairing decision. The synthetic validation context also classified agents/hermes/Dockerfile.base, agents/hermes/config/messaging-config.ts, and agents/hermes/patch-pairing-message.py as requiring runtime validation.

In-scope improvements

• Document the removal condition for the Hermes source patch (agents/hermes/patch-pairing-message.py:3): The new build-time patch is a local workaround for upstream Hermes release text that still says openclaw pairing approve. The helper is fail-closed and tested, but the changed code does not record when this monkeypatch should be removed or which upstream Hermes condition makes it obsolete.
  • Impact: Without a removal condition, the patch can become permanent installer drift after Hermes releases corrected pairing help, making future source updates harder to reason about and obscuring the true source of the behavior.
  • Recommendation: Add a short comment near the script docstring or Dockerfile invocation stating that the patch should be removed once the minimum pinned Hermes release contains hermes pairing approve in the Discord pairing approval help. Keep the existing fail-closed behavior and already-corrected upstream compatibility.
  • Verification hint: Read agents/hermes/patch-pairing-message.py and agents/hermes/Dockerfile.base around the COPY/RUN invocation; confirm a maintainer-facing comment names the upstream corrected-command condition for removal.
  • Missing regression test: Existing tests already cover the workaround behavior: rewrites upstream pairing approval help from OpenClaw to Hermes, accepts Hermes versions that already have the corrected command, and fails closed if the upstream pairing approval text is not found.
  • Evidence: patch_pairing_message tolerates already-corrected upstream text and fails closed when neither command is found, but neither the script nor Dockerfile explains when the local source patch can be retired.
• Reuse the normalized Discord guild map for the allow-all check (agents/hermes/config/messaging-config.ts:68): The new code creates normalizedDiscordGuilds and uses it for guild IDs and allowed users, but the DISCORD_ALLOW_ALL_USERS branch still recomputes nonblank guild existence from the raw discordGuilds object.
  • Impact: The current expression is equivalent because it trims keys, but using two normalization paths makes this access-control condition easier to drift in a future edit.
  • Recommendation: Replace Object.keys(discordGuilds).filter((guildId) => guildId.trim()).length > 0 with Object.keys(normalizedDiscordGuilds).length > 0 in the DISCORD_ALLOW_ALL_USERS branch.
  • Verification hint: Read agents/hermes/config/messaging-config.ts lines 29-71 and confirm the allow-all condition uses the same normalizedDiscordGuilds object used for NEMOCLAW_DISCORD_GUILD_IDS and collectDiscordAllowedUsers.
  • Missing regression test: The existing tests does not allow all Discord users for empty guild config keys and filters blank Discord guild keys while preserving valid guild access already cover the intended behavior after this shrink.
  • Evidence: normalizedDiscordGuilds is computed at the top of buildMessagingEnvLines, but the else-if at lines 67-70 re-filters the raw object.

Test follow-ups to resolve or justify

• Runtime validation — Hermes Discord valid guild with empty user allowlist treats a server-member DM as allowed without emitting access not configured or a pairing code.. The added unit tests are focused and cover generated config text plus the Python patch helper. Because the linked regression manifests in a sandboxed Hermes Discord runtime path and the Dockerfile mutates third-party source at build time, targeted runtime or adapter-level validation would materially improve confidence.
• Runtime validation — Built Hermes image/source patch smoke confirms the pairing help path contains hermes pairing approve and no openclaw pairing approve after the Dockerfile patch step.. The added unit tests are focused and cover generated config text plus the Python patch helper. Because the linked regression manifests in a sandboxed Hermes Discord runtime path and the Dockerfile mutates third-party source at build time, targeted runtime or adapter-level validation would materially improve confidence.
• Runtime validation — Hermes image build or patch-step validation fails closed when the pinned upstream source contains neither the legacy nor corrected pairing approval command text.. The added unit tests are focused and cover generated config text plus the Python patch helper. Because the linked regression manifests in a sandboxed Hermes Discord runtime path and the Dockerfile mutates third-party source at build time, targeted runtime or adapter-level validation would materially improve confidence.
• Runtime validation is still needed for the linked Discord pairing behavior — Add or identify a focused runtime/integration validation for the Hermes Discord path with Discord enabled, one valid guild ID, and no user allowlist, asserting the server-member DM path does not produce access not configured or a pairing code. If an external live Discord test is intentionally out of scope, add a narrower Hermes adapter-level/sandbox smoke that proves the built env/config is accepted by the runtime path.
• Acceptance clause: When nemohermes onboard is completed with the Discord User ID allowlist left empty (optional field — empty is documented to mean "allow all server members"), the bot sends a pairing code on the first DM from any server member. — add test evidence or identify existing coverage. The diff makes buildMessagingEnvLines emit DISCORD_ALLOW_ALL_USERS=true when Discord is enabled, at least one nonblank guild exists, and no explicit Discord users are collected. test/generate-hermes-config.test.ts covers the generated env line, but no runtime Hermes DM/pairing path test is shown.
• Acceptance clause: However, subsequent messages from the same user are processed normally by the AI without the pairing ever being approved — inconsistent behavior that also shows a confusing UX with an approval command referencing "openclaw" which does not exist on a NemoHermes-only install. — add test evidence or identify existing coverage. The generated-env fix should avoid the pairing path for the empty-allowlist valid-guild case, and the Docker build now runs patch-pairing-message.py to rewrite openclaw pairing approve to hermes pairing approve. Tests cover config output and patch helper behavior, but not the first/subsequent live DM inconsistency.
• Acceptance clause: 1. Run: nemohermes onboard (with NEMOCLAW_AGENT=hermes) — add test evidence or identify existing coverage. Changed tests exercise the downstream Hermes config generator that onboard/build env feeds, not the interactive onboard flow itself.
• Acceptance clause: 6. Complete onboard (policy presets → confirm) — add test evidence or identify existing coverage. The generator tests cover the resulting build-time env/config output, but the interactive completion/policy preset flow is not exercised in this PR.

What looks good

• The Discord guild normalization is small and directly covered by negative tests for blank-only and mixed blank/valid guild configurations.
• The Hermes source patch helper is safer than an inline shell replacement because it is unit-tested and fails closed when the expected upstream text disappears.
• Credential handling remains placeholder-based; the PR does not move Discord tokens into config.yaml or logs.
• The Vitest additions use simple local filesystem fixtures and argv-array process spawning without introducing new test frameworks or workflow layers.

[sandl99]

I believe we should avoid the build-time Hermes source patch here.

The patch-pairing-message.py approach scans and mutates the unpacked upstream Hermes tree under /opt/hermes, which is a brittle source-boundary workaround and does not fit the manifest-first messaging model. The pinned Hermes sources I checked already emit hermes pairing approve ..., so this patch is either a no-op for the current pin or masking an unclear version/source mismatch.

For the actual Discord empty-allowlist behavior, this belongs in the Discord messaging manifest/template path rather than a Hermes-specific config generator or Docker patch. The current architecture already has the right ownership boundary under src/lib/messaging/channels/ discord: the manifest renders DISCORD_ALLOW_ALL_USERS, DISCORD_ALLOWED_USERS, and NEMOCLAW_DISCORD_GUILD_IDS, and the Discord template resolver should normalize blank guild IDs before deciding whether to emit allow-all.

Recommended change:

• Drop agents/hermes/patch-pairing-message.py and the Dockerfile invocation.
• Move/keep the blank-guild normalization in the Discord manifest resolver/runtime render path under src/lib/messaging/channels/discord.
• Add a focused regression test proving:
  • blank-only Discord guild config emits no guild IDs, no allowed users, and no DISCORD_ALLOW_ALL_USERS;
  • valid guild + empty user allowlist emits NEMOCLAW_DISCORD_GUILD_IDS=<id> and DISCORD_ALLOW_ALL_USERS=true;
  • valid guild + explicit users emits DISCORD_ALLOWED_USERS and does not emit allow-all.

I would not use a Discord runtime hook for the pairing-message text: Hermes gateway is Python, the existing NemoClaw runtime preload path is Node-oriented, and the relevant Hermes plugin hook runs before authorization, not after the unauthorized DM pairing response is generated.

@cv Please don't merge PR until @chengjiew address above comments.

[sandl99]

Close PR as #4070 already closed without any action