Skip to content

feat: confirmation prompt for gateway /update command - #64519

Closed
jcjc81 wants to merge 7 commits into
NousResearch:mainfrom
jcjc81:feat/update-confirm-rebased
Closed

jcjc81 wants to merge 7 commits into
NousResearch:mainfrom
jcjc81:feat/update-confirm-rebased

Conversation

@jcjc81

@jcjc81 jcjc81 commented Jul 14, 2026

Copy link
Copy Markdown

/## /update fires instantly -- no "are you sure?"

/update pulls new code and restarts the running gateway, interrupting every
active session on the host -- but it fired the instant the command was received,
with no confirmation. An accidental or fat-fingered invoke had no undo.

This PR: always prompt, never persist approval

/update now always routes through the existing slash-confirm primitive
before spawning: native Approve Once / Cancel buttons on Telegram,
Discord, and Slack, with a text fallback elsewhere.

There is deliberately no "Always Approve" button and no config opt-out --
unlike /reload-mcp and the destructive session commands (/new, /reset,
/undo), where a permanent one-tap opt-out is reasonable because they run
often and are low-stakes. /update is rare and high-stakes: a permanent
one-tap disable would reintroduce the exact accidental-fire footgun this
confirmation exists to close.

Mechanism

Area Change
send_slash_confirm (base + Telegram/Discord/Slack/WhatsApp) Added allow_always: bool = True; when False, the middle button is suppressed
_request_slash_confirm (run.py) Accepts + forwards allow_always
/update handler Passes allow_always=False; always prompts; any non-cancel choice proceeds once, persists nothing
locales/en.yaml Two-button prompt (Approve Once / Cancel), no always_followup

Design decisions

  • Default allow_always=True means /reload-mcp and the destructive commands
    are byte-for-byte unchanged -- this is a generic widening of the shared
    hook, not a special-case.
  • Cheap pre-flight validation (platform allowlist, managed-install, git-repo,
    hermes-binary) still fast-fails before prompting.
  • Detached-spawn mechanics extracted verbatim into _execute_update; the
    confirm handler calls it on approval. No change to the spawn.

Tests

  • tests/gateway/test_update_command.py: spawn-mechanics tests call
    _execute_update directly (post-approval); validation/platform-gate tests
    still hit _handle_update_command with a stubbed confirm hook.
  • tests/gateway/test_update_slash_confirm.py: always-prompts,
    no-always-button, allow_always=False forwarded to adapter, once/cancel
    resolution, defense-in-depth that a stray "always" proceeds once and never
    persists, and pre-flight-beats-prompt.
  • 64 tests pass across update, slash-confirm, destructive-confirm,
    Telegram, Discord, and Slack suites.

@alt-glitch alt-glitch added type/feature New feature or request comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages P3 Low — cosmetic, nice to have labels Jul 14, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding a confirmation gate to a command that restarts the gateway. The premise is confirmed on current main: gateway/slash_commands.py:4611-4706 writes the update marker and starts the detached process without a prior confirmation.

Problems

  • gateway/run.py:14435 passes allow_always to every adapter invocation, including the default three-button flow. Platform adapters are an extension surface: gateway/platforms/ADDING_A_PLATFORM.md:121 documents the existing optional signature. A deployed adapter with that signature raises TypeError; the broad catch at gateway/run.py:14439-14443 silently falls back to text and loses its native confirmation buttons.

Suggested changes

  • Pass allow_always only when it is False, and add a legacy-adapter regression test for the default path. That preserves existing adapter behavior while allowing /update to use text fallback where a platform has not adopted the two-button extension.

Automated hermes-sweeper review.

Comment thread gateway/run.py Outdated
session_key=session_key,
confirm_id=confirm_id,
metadata=metadata,
allow_always=allow_always,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing this new keyword even when it is True breaks already-deployed external adapters implementing the documented pre-change signature; the catch below turns their TypeError into a silent text fallback. Please only add this kwarg when allow_always is False, and cover a legacy-signature adapter in the default path.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f726277f5: allow_always is now only added to kwargs when it's False (non-default); legacy adapters never see the new keyword. Added test_default_allow_always_not_passed_to_legacy_adapter to cover the default path. All 47 tests in test_update_slash_confirm.py + test_cmd_update.py pass.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 16, 2026
@jcjc81

jcjc81 commented Jul 17, 2026

Copy link
Copy Markdown
Author

CI is green on e422431cd. Two issues found and fixed beyond the allow_always gating:

  1. test_i18n.py::test_catalog_keys_match_english (15 locales) — the original commit added gateway.update.confirm_prompt / gateway.update.cancelled to locales/en.yaml only. Translated both keys into all 15 other locale catalogs (af, de, es, fr, ga, hu, it, ja, ko, pt, ru, tr, uk, zh-hant, zh), matching the tone/structure of the existing gateway.reload_mcp.confirm_prompt translations already in each file.
  2. test_update_streaming.py::test_spawns_with_gateway_flag — stale test calling _handle_update_command directly and asserting on subprocess.Popen, which is no longer reached before confirm-approval. Updated it to call _execute_update directly, matching the pattern test_update_command.py already uses.

Both pre-existed on the original commit (confirmed by reproducing locally before either fix), unrelated to the allow_always change discussed above.

@teknium1 teknium1 added the area/install-update Installer, updater, packaging, wheels, doctor label Jul 19, 2026
/update pulls new code and restarts the running gateway, interrupting
every active session on the host — but it fired the instant the command
was received, with no confirmation. An accidental or fat-fingered invoke
had no undo.

/update now ALWAYS routes through the existing slash-confirm primitive
before spawning: native Approve Once / Cancel buttons on Telegram,
Discord, and Slack, with a text fallback elsewhere. There is
deliberately NO 'Always Approve' button and no config opt-out — unlike
/reload-mcp and the destructive session commands (/new, /reset, /undo),
where a permanent one-tap opt-out is reasonable because they run often
and are low-stakes. /update is rare and high-stakes: a permanent
one-tap disable would reintroduce the exact accidental-fire footgun this
confirmation exists to close.

Mechanism:
- Widen the shared send_slash_confirm hook (base + Telegram/Discord/
  Slack/WhatsApp adapters) and _request_slash_confirm with
  allow_always: bool = True. When False, the middle 'Always Approve'
  button is suppressed. Default True keeps /reload-mcp and the
  destructive commands byte-for-byte unchanged.
- /update passes allow_always=False and its handler treats any
  non-cancel choice as proceed-once, persisting nothing.
- Cheap pre-flight validation (platform allowlist, managed-install,
  git-repo, hermes-binary) still fast-fails BEFORE prompting.
- Detached-spawn mechanics extracted verbatim into _execute_update;
  the confirm handler calls it on approval. No change to the spawn.

Tests:
- tests/gateway/test_update_command.py: spawn-mechanics tests now call
  _execute_update directly (the post-approval unit); validation/
  platform-gate tests still hit _handle_update_command with a stubbed
  confirm hook.
- tests/gateway/test_update_slash_confirm.py: always-prompts, no-always-
  button, allow_always=False forwarded to adapter, once/cancel
  resolution, defense-in-depth that a stray 'always' proceeds once and
  never persists, and pre-flight-beats-prompt.

201 tests pass across the update, slash-confirm, destructive-confirm,
telegram, whatsapp, and discord suites; all touched modules import clean.

(cherry picked from commit d5c54a7136dd480028672b91af4868ce431f05ea)
…True for legacy compat

Bot review flagged that allow_always was passed to every adapter invocation
including legacy ones without the parameter. A legacy adapter would raise
TypeError and silently fall back to text, losing native confirmation buttons.

Gate the kwarg: only include allow_always when it is False (non-default).
Legacy adapters use their own default (True) and stay unaffected.

Test added: test_default_allow_always_not_passed_to_legacy_adapter
…ale catalogs

The /update confirmation prompt (feat: confirmation prompt for gateway
/update command) added gateway.update.confirm_prompt and
gateway.update.cancelled to locales/en.yaml only, missing the other 15
locale catalogs. test_i18n.py::test_catalog_keys_match_english caught
the drift.

Translated both keys into af, de, es, fr, ga, hu, it, ja, ko, pt, ru,
tr, uk, zh-hant, zh — matching the tone and structure of the existing
gateway.reload_mcp.confirm_prompt / cancelled keys already translated
in each catalog.
…flow refactor

/update now always routes through the slash-confirm primitive before
spawning (_handle_update_command -> _request_slash_confirm -> _execute_update
on approval). test_update_command.py's spawn-mechanics tests were
updated to call _execute_update directly, but this sibling test in
test_update_streaming.py still called _handle_update_command and
asserted on subprocess.Popen — which is never reached before approval,
making mock_popen.call_args None.

Update the test to call _execute_update directly, mirroring the
pattern already used in test_update_command.py.
The 2026-08-25 upstream commit a96f7c8 added a 16th locale (ar.yaml)
after our original i18n fix covered 15. This adds the two new keys
(confirm_prompt, cancelled) to ar.yaml with Arabic translations,
matching the tone of the existing ar.yaml gateway.update block.
After the confirm-flow refactor, /update always routes through the
confirm gate before spawning. These two tests in test_update_command.py
were calling _handle_update_command directly (which now returns early
after the confirm prompt) instead of _execute_update (the post-approval
path). Updated both to call _execute_update directly, matching the
pattern used in test_update_streaming.py and test_update_slash_confirm.py.
@jcjc81
jcjc81 force-pushed the feat/update-confirm-rebased branch from e422431 to 8c810c1 Compare September 4, 2026 10:44
@jcjc81

jcjc81 commented Sep 4, 2026

Copy link
Copy Markdown
Author

Rebased onto current main (999703fd43). The previous head was ~4 weeks stale against upstream.

What changed in the rebase:

  1. Windows /update spawn fix (3f6d4c6338) — upstream reworked the Windows spawn path in the same function this PR refactors. Auto-merged cleanly; both intents are now present (confirm-flow + module spawn).

  2. New ar.yaml locale (a96f7c805b) — upstream added a 16th locale after our original i18n fix covered 15. Added the two new keys (gateway.update.confirm_prompt, gateway.update.cancelled) to ar.yaml with Arabic translations.

  3. Test updates — test_update_command.py had evolved on main (new notification tests, different _make_runner). Took upstream main's version and re-applied the 2 confirm-flow test updates (test_writes_pending_marker, test_fallback_when_no_setsid) to call _execute_update directly.

Commit history (6 commits on main):

958648efbc feat: confirmation prompt for gateway /update command
0f18f3dabc fix(gateway): only pass allow_always=False to adapters
4f0968f5ed fix(i18n): add gateway.update keys to all 15 original locales
d4fbece5f6 test(gateway): fix stale test_spawns_with_gateway_flag
ad1f07d358 fix(i18n): add gateway.update keys to ar.yaml (new locale)
8c810c106d test(gateway): update pending-marker and setsid tests for confirm-flow

CI should be green on the new head. The 1 remaining test failure (test_managed_install_blocks_before_prompt) is pre-existing on main.

HERMES_MANAGED=homebrew is in _IGNORED_MANAGED_VALUES (upstream
intentionally ignores homebrew since Hermes self-updates via its own
brew tap). The test's intent — managed installs block before the
confirm prompt — is still valid; it just needs a non-ignored value.
Switched to nix, which IS blocked.
@jcjc81

jcjc81 commented Sep 4, 2026

Copy link
Copy Markdown
Author

Fixed the one remaining CI failure: test_managed_install_blocks_before_prompt.

Root cause: The test used HERMES_MANAGED=homebrew, but upstream intentionally ignores homebrew in _IGNORED_MANAGED_VALUES (hermes_cli/config.py:447) — homebrew installs self-update via the Hermes tap, so the managed-install block doesn't apply. The test was outdated.

Fix: Switched the test to use HERMES_MANAGED=nix (which IS blocked) and updated the assertion from "managed by Homebrew" to "managed by nix". The test's intent — managed installs block before the confirm prompt — is unchanged.

New head: 805a80c9d1. CI should be fully green.

@jcjc81

jcjc81 commented Sep 4, 2026

Copy link
Copy Markdown
Author

Closing — the feature in this PR is already on main.

The /update confirmation prompt (with allow_always=False), the allow_always parameter on send_slash_confirm across all button adapters, the _execute_update extraction, and the i18n keys across all 16 locales (including the new ar.yaml) are all present on current main. The feature was merged through a rebased version while this PR was open.

Verified on origin/main:

  • gateway/slash_commands.py — _handle_update_command confirm gate + _execute_update
  • gateway/run.py — _request_slash_confirm(allow_always=...)
  • gateway/platforms/base.py + telegram/discord/slack/whatsapp adapters — allow_always param + conditional Always button
  • locales/*.yaml (16 files) — gateway.update.confirm_prompt / cancelled
  • tests/gateway/test_update_slash_confirm.py — present with upstream's managed-install fix

Nothing here is missing from main, so this PR is redundant. Thanks for the review time.

@jcjc81 jcjc81 closed this Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants