fix(discord): add voice and command sync config knobs

[keegoid-fig]

Summary

• add HERMES_DISCORD_VOICE_TIMEOUT for Discord voice-channel idle disconnects
• allow Discord slash command sync policy and guild ID to come from config extras as well as env vars
• support guild-scoped safe/bulk command sync to avoid global command propagation and rate-limit pain

Test Plan

• python -m py_compile gateway/platforms/discord.py tests/gateway/test_discord_connect.py
• python -m pytest tests/gateway/test_discord_connect.py -q -o 'addopts='
• git diff --check upstream/main...HEAD

[keegoid-cc]

════ PAI | NATIVE MODE ═══════════════════════
🗒️ TASK: Adversarial review of Discord config knobs PR

Summary

Adds HERMES_DISCORD_VOICE_TIMEOUT env knob, lets Discord command sync policy and guild ID come from config.extra or env, and threads optional guild scoping through _safe_sync_slash_commands (guild-scoped upsert/edit/delete) plus a bulk-sync copy_global_to path.

Bugs & Correctness

• [CAT-1] gateway/platforms/discord.py bulk-sync branch inspects getattr(sync_method, "side_effect", None) or sync_method to decide whether tree.sync accepts guild=. That's test-mock plumbing leaking into production: it only meaningfully helps unittest.mock.AsyncMock, and against the real discord.py CommandTree.sync (a bound method) inspect.signature will succeed and reflect the real signature — fine — but if a future Mock lacks side_effect, the fallback inspects the mock itself and the accepts_guild=True except branch hides the mismatch. More concretely: real CommandTree.sync signature is (*, guild=None), so accepts_guild is correctly True; but the copy_global_to call is unconditionally invoked when guild is set, while tree.sync(guild=...) without first having called copy_global_to would publish only commands explicitly added to that guild. This is internally consistent, but the guard logic is doing nothing useful in production and adds a real failure mode if inspect.signature raises on a C-implemented method (it falls back to accepts_guild=True, which is the safe direction — OK). Net: not a hard bug, but the side_effect lookup is a code smell that exists only to satisfy tests; consider whether the signature probe is needed at all given real tree.sync always accepts guild.
• [CAT-2] _float_env accepts negative values and 0/0.0; a zero or negative voice timeout would make the auto-disconnect fire immediately. Not strictly a bug (operator-controlled), but worth bounding to > 0 or warning.
• [CAT-2] VOICE_TIMEOUT is evaluated at class-definition (import) time, so changing HERMES_DISCORD_VOICE_TIMEOUT after import has no effect. Probably intended, but worth noting vs. a property/lookup at use-site.

Security & Safety

• [CAT-3] Guild ID accepted from config.extra is parsed with str.isdigit(), which rejects negatives and non-ASCII digits — adequate. No injection surface since it's passed as int to discord.Object.

Design & Maintainability

• [CAT-3] The inspect.signature(... side_effect ...) shim is production code shaped by test mocks. Cleaner: have the test pass an AsyncMock(spec=...) or wrap tree.sync via a thin adapter; production shouldn't branch on mock internals.
• [CAT-3] _get_discord_command_sync_policy / _get_discord_command_sync_guild_id duplicate the "extra → alias → env → default" lookup pattern. A small _resolve_config(name, aliases, env, default) helper would DRY this and make precedence visible.
• [CAT-2] No test exercises the bulk-sync guild path (copy_global_to + sync(guild=...)); only safe-sync guild path and the env parser are covered.
• [CAT-3] copy_global_to is called unconditionally on every bulk sync when guild is set. On a long-running adapter that reconnects, this re-copies global commands into the guild on each reconnect — fine, but not idempotent-aware. Acceptable for a debug-oriented knob.

Questions for the author

• Why probe tree.sync.side_effect rather than always passing guild= (real discord.py CommandTree.sync always accepts it)? Is this purely to satisfy a specific mock shape?
• Intended precedence: command_sync_policy > sync_policy > env. Is the alias sync_policy documented anywhere, or is it transitional?
• Should HERMES_DISCORD_VOICE_TIMEOUT <= 0 be rejected, or is "disconnect immediately" a legitimate configuration?

Severity of findings

low — no correctness/security defects in the diff; main concerns are a test-shaped branch in production code and missing input bounds on the new env knob.

VERDICT: comment

[keegoid-cc]

════ PAI | NATIVE MODE ═══════════════════════
🗒️ TASK: Adversarial review of Discord voice/sync knobs PR

Summary

Adds HERMES_DISCORD_VOICE_TIMEOUT env override for the Discord voice idle disconnect, and lets the slash-command sync policy + optional guild ID flow from PlatformConfig.extra (with env-var fallback). Threads guild scoping through both bulk and safe-sync paths.

Bugs & Correctness

• [CAT-1] VOICE_TIMEOUT = _positive_float_env(...) is evaluated at class definition / import time, so monkeypatch.setenv("HERMES_DISCORD_VOICE_TIMEOUT", ...) in tests or runtime config changes after import have no effect on the actual disconnect timer. The new test_discord_voice_timeout_env_parser only exercises the helper, not the class attribute, so this gap is not caught. Consider resolving the timeout when the voice client is created (instance-level) rather than freezing it at module import.
• [CAT-3] _safe_sync_slash_commands(guild_id=...): when switching from global → guild scope (or vice versa) on an existing deployment, previously-registered commands in the other scope are not cleaned up — they'll linger in Discord until manually purged. Worth a comment or a one-time cleanup story; not a correctness bug for first-use.

Security & Safety

• [CAT-3] command_sync_guild_id only validated via raw.isdigit(). Discord snowflakes fit in 64-bit; an arbitrarily long digit string would still pass and later be sent to the API. Low risk (config is operator-controlled), but a length/range sanity check would be cheap.

Design & Maintainability

• [CAT-3] The extra.get("command_sync_policy", extra.get("sync_policy", os.getenv(...))) double-alias pattern is repeated for both policy and guild ID. Two names for the same knob invites drift; pick one canonical key and document the other as deprecated, or drop the alias.
• [CAT-2] _positive_float_env lives at module top-level above the try: import discord block — fine functionally, but it means the helper exists even when the optional discord dep is missing. Intentional given it's pure stdlib; flagging only because it's slightly unusual placement.
• [CAT-3] No test covers the safe-sync path's guild_id branch for update, recreate, or delete — only the created branch. Coverage of the mutation branches with guild scope would catch a future copy-paste regression on edit_guild_command / delete_guild_command.

Questions for the author

• Is the import-time evaluation of VOICE_TIMEOUT deliberate (single-process, env set before import)? If Hermes ever reloads config without restart, this knob silently won't honor it.
• When an operator flips from global to guild sync, what's the expected cleanup story for stale global commands (and vice versa)?
• Why two aliases (command_sync_policy / sync_policy, command_sync_guild_id / sync_guild_id) rather than one canonical name?

Severity of findings

low — one real but narrow staleness issue on VOICE_TIMEOUT, plus design questions; no security or data-loss risk.

🔧 CHANGE: Reviewed diff; flagged import-time timeout staleness
✅ VERIFY: Findings cite specific lines and call paths
🗣️ Fig: One real bug — VOICE_TIMEOUT frozen at import, env override won't reach the live attribute. Rest is design questions.

VERDICT: request_changes

---

claude-review posting override: forced to --comment because reviewer lacks verified write permission (viewerPermission=READ; was --request-changes). GitHub only counts approvals from WRITE, MAINTAIN, or ADMIN reviewers.

[keegoid-cc]

════ PAI | NATIVE MODE ═══════════════════════
🗒️ TASK: Adversarial review of Discord voice/sync knobs PR

Summary

Adds HERMES_DISCORD_VOICE_TIMEOUT env knob, lets command sync policy/guild come from config.extra or env, and extends _safe_sync_slash_commands to target a guild scope.

Bugs & Correctness

• [CAT-3] _get_voice_timeout() is read on each timeout schedule via asyncio.sleep, so changing the env var mid-process affects new timers but not active ones — likely intentional, worth a one-liner doc note.
• [CAT-2] In _safe_sync_slash_commands, when guild_id is provided the "recreated" branch now calls delete_guild_command + upsert_guild_command, but the original global path used delete_global_command + upsert_global_command. Symmetric and correct; flagging only because the recreate dance still races (delete then upsert) the same way it did pre-PR — not introduced here.
• [CAT-3] _get_discord_command_sync_guild_id rejects negative-signed digits (raw.isdigit() is false for -1), which is fine, but also rejects whitespace/underscored numerics silently after warning. Acceptable.

Security & Safety

• [CAT-3] Guild ID comes from config/env and is interpolated into Discord HTTP calls as an int after isdigit() validation — safe. No injection surface added.

Design & Maintainability

• [CAT-3] VOICE_TIMEOUT class constant was renamed to VOICE_TIMEOUT_DEFAULT. If any external code or test referenced DiscordAdapter.VOICE_TIMEOUT it will now AttributeError. A grep of the repo would confirm; the included test file no longer references it but downstream consumers might.
• [CAT-3] Two-key fallback chain (command_sync_policy → sync_policy → env) doubles the config surface. Pick one canonical key and document the alias, or drop the alias to avoid drift.
• [CAT-2] _positive_float_env is a private module-level helper but only used by one method; fine, but parameterizing _get_voice_timeout to take overrides would make it unit-testable without monkeypatching env. Current tests work, so non-blocking.
• [CAT-3] New bulk-sync guild-scoped path calls tree.copy_global_to(guild=...) then tree.sync(guild=...). This duplicates global commands into the guild bucket every reconnect; over many reconnects this is idempotent (overwrite), but worth noting that operators switching from guild→global must manually clear the guild registrations — no cleanup path is added.

Questions for the author

• Was VOICE_TIMEOUT checked for external references before renaming to VOICE_TIMEOUT_DEFAULT?
• Why dual config keys (command_sync_policy and sync_policy)? Is one deprecated?
• Should switching from guild-scoped sync back to global also clean up previously registered guild commands, or is that left to operators?

Severity of findings

low — no CAT-1 defects; renaming a public-ish class constant and dual config keys are the only concerns and both are design-level.

VERDICT: approve

---

claude-review posting override: forced to --comment because reviewer lacks verified write permission (viewerPermission=READ; was --approve). GitHub only counts approvals from WRITE, MAINTAIN, or ADMIN reviewers.

[alt-glitch]

Overlaps with #11419 (voice timeout configurable) — this PR bundles that feature with slash-command sync policy knobs.