fix: address Codex PR #25 follow-up issues (false feature activations + PyNaCl error path) - #27
fix: address Codex PR #25 follow-up issues (false feature activations + PyNaCl error path)#27dizhaky wants to merge 5 commits into
Conversation
active_features() was using any() over all specs, which caused false activations when a transitive package (aiohttp, cbor2, starlette) shared by multiple features was installed. For example: - aiohttp from Slack -> platform.discord falsely active - cbor2 from Modal -> terminal.vercel falsely active (and vice-versa) - starlette from MCP -> tool.dashboard falsely active Fix: check only specs[0] (the primary/unique package). By convention the first tuple element is always a package exclusive to that feature. This prevents hermes update from installing packages for features the user never enabled. Addresses Codex PR #25 comments.
Returning False from join_voice_channel() when PyNaCl is absent bypassed the PyNaCl-specific error guidance in GatewayRunner._handle_voice_channel_join (gateway/run.py:10937). The caller only shows the install hint when the call raises with "pynacl" in the error message; a False return fell through to the generic "Check bot permissions" message instead. Addresses Codex PR #25.
🔎 Lint report:
|
The adapter now raises RuntimeError (instead of returning False) so that GatewayRunner._handle_voice_channel_join can surface the PyNaCl-specific install guidance. Update the test to match.
Automated Codex review follow-upFixed the blocking nix CI failure by updating the stale npm lockfile hashes (applied the values computed by the
This unblocks the 🤖 Applied by the automated Codex review routine. |
🤖 Automated Codex Review — Ready to mergeAll 27 CI checks pass. This PR addresses all 4 P2 Codex findings from merged PR #25:
Changes touch only Recommendation: merge. This is ready. 🤖 Reviewed by the automated Codex follow-up routine. |
|
Closing this draft and reopening as a non-draft PR so it can be merged. All CI is green. |
1 participant
Summary
Addresses all 4 P2 Codex review comments left on PR #25 (merged 2026-06-22).
Problem 1 — False feature activations in
active_features()(3 issues)PR #25 added transitive packages (
aiohttp,cbor2,starlette) to severalLAZY_DEPSentries to ensurehermes updaterefreshes audited pins. Howeveractive_features()usesany(_is_present(s) for s in specs), so any installed spec marks the whole feature as active. This causes:aiohttpvia Slackplatform.discordappears activecbor2via Modalterminal.vercelappears active (and vice-versa)starlettevia MCPtool.dashboardappears activeFix: Check only
specs[0](the primary/unique package). By convention the first tuple element is always a package that only appears because that feature was explicitly installed. The install path (ensure()) still installs every spec; only the detection heuristic changes.Problem 2 — PyNaCl guard returns
Falseinstead of raisingThe missing-PyNaCl guard in
join_voice_channel()(added in PR #25) returnedFalseonImportError. The caller inGatewayRunner._handle_voice_channel_join(gateway/run.py:10937) only surfaces the PyNaCl-specific install guidance when the call raises with "pynacl" in the error message. AFalsereturn fell through to the generic "Check bot permissions" message — exactly the wrong feedback for a missing-dependency failure.Fix: Raise
RuntimeErrorwith "PyNaCl" in the message so the existing caller error-path shows the correct install hint.Files changed
tools/lazy_deps.py—active_features():any(...)→_is_present(specs[0])plugins/platforms/discord/adapter.py— PyNaCl guard:return False→raise RuntimeError(...)Test plan
hermes updateon a fresh install with only Slack enabled: confirmplatform.discordis NOT refreshedhermes updateon a Modal-only install: confirmterminal.vercelis NOT refreshedhermes updateon an MCP-enabled install without dashboard: confirmtool.dashboardis NOT refreshed/voice joinwithout PyNaCl installed: confirm message shows "PyNaCl is not installed" + pip install hint, NOT "Check bot permissions"🤖 Generated by Claude Code (automated Codex review follow-up routine)