Skip to content

fix(telegram): restore 2x2 exec-approval button layout - #70615

Closed
HexLab98 wants to merge 2 commits into
NousResearch:mainfrom
HexLab98:fix/telegram-approval-button-2x2-layout
Closed

fix(telegram): restore 2x2 exec-approval button layout#70615
HexLab98 wants to merge 2 commits into
NousResearch:mainfrom
HexLab98:fix/telegram-approval-button-2x2-layout

Conversation

@HexLab98

@HexLab98 HexLab98 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Exec-approval buttons on Telegram were flattened into a single 4-button row after the smart-deny conditional keyboard change, so labels truncated on mobile (Allo… / Ses… / Alw…).
  • Pair the built button list into rows of two so the full set is again a readable 2×2 grid (Allow Once | Session / Always | Deny), including the 3- and 2-button variants.
  • Add regression coverage for the full 2×2 layout and the three-button case.

Test plan

  • scripts/run_tests.sh tests/gateway/test_telegram_approval_buttons.py -q
  • Trigger a dangerous command approval in Telegram and confirm buttons render as two rows of two with full labels
  • With smart deny / allow_permanent=false, confirm the reduced button sets still render without a single cramped row

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/telegram Telegram bot adapter labels Jul 24, 2026
HexLab98 added 2 commits July 24, 2026 14:29
Pair conditional approval buttons into rows of two so the full Allow Once /
Session / Always / Deny set stays readable instead of one truncated 4x1 row.
Assert the full set renders as 2x2 and the three-button case keeps Deny on
its own second row.
@HexLab98
HexLab98 force-pushed the fix/telegram-approval-button-2x2-layout branch from 7058fe1 to b0ce246 Compare July 24, 2026 07:29
kshitijk4poor added a commit to kshitijk4poor/hermes-agent that referenced this pull request Jul 24, 2026
Follow-up for salvaged PR NousResearch#70615 — the 2-button smart_deny case
(Allow Once + Deny only) was exercised by an existing test but only
at the flat-label level, not asserting the row pairing. Adds the
missing row-structure assertion using the same capture pattern as
the 4-button and 3-button tests.
kshitijk4poor added a commit that referenced this pull request Jul 25, 2026
Follow-up for salvaged PR #70615 — the 2-button smart_deny case
(Allow Once + Deny only) was exercised by an existing test but only
at the flat-label level, not asserting the row pairing. Adds the
missing row-structure assertion using the same capture pattern as
the 4-button and 3-button tests.
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #70912 — your commit was cherry-picked with authorship preserved (rebase-merge). A follow-up test for the 2-button smart_deny row-structure case was added on top. Thanks for the clean fix!

kokpepijn added a commit to kokpepijn/hermes-agent that referenced this pull request Jul 25, 2026
* feat(relay): Phase 1 parity — supported_ops discovery, wire identity fields, /handoff aliasing, provision displayName (NousResearch#71300)

- CapabilityDescriptor.supported_ops + supports_op() with legacy-op-set
  fallback (additive, contract doc §2 updated)
- get_chat_info gated on op discovery (skip round trip on legacy connectors)
- _event_from_wire consumes user_display_name/user_handle (§3 fields
  previously dropped); native display-name parity, session-key stable
- /handoff <fronted-platform> works on relay-fronted gateways: CLI pre-check
  reads the GATEWAY_RELAY_PLATFORMS fronted set; watcher resolves through
  resolve_delivery_transport and replies via send_for_platform
- self-provision forwards displayName (env > skin branding, stock brand
  suppressed) — the primary name source for gg#171 attribution

* fix(telegram): restore 2x2 exec-approval button layout

Pair conditional approval buttons into rows of two so the full Allow Once /
Session / Always / Deny set stays readable instead of one truncated 4x1 row.

* test(telegram): cover exec-approval keyboard row pairing

Assert the full set renders as 2x2 and the three-button case keeps Deny on
its own second row.

* test(telegram): cover smart_deny 2-button row structure

Follow-up for salvaged PR NousResearch#70615 — the 2-button smart_deny case
(Allow Once + Deny only) was exercised by an existing test but only
at the flat-label level, not asserting the row pairing. Adds the
missing row-structure assertion using the same capture pattern as
the 4-button and 3-button tests.

* feat(relay): Phase 2 media parity — send_media egress + inbound media localization (NousResearch#71363)

- gateway/relay/media.py: RelayMediaClient for the connector's /relay/media
  plane (upload local files → re-host reference for send_media; download
  re-hosted inbound attachments → local temp paths). Same connector base URL
  the WS dials, same per-gateway signed bearer as the upgrade (auth.py) —
  no new configuration. stdlib urllib in a thread executor (no new deps);
  25MB cap mirroring the connector's MEDIA_MAX_BYTES.
- RelayAdapter: send_image / send_image_file / send_voice / send_video /
  send_document overrides route through ONE send_media op (media by
  reference: local paths upload first, public URLs pass through). Gated on
  supported_ops advertising send_media — legacy connectors keep today's
  text fallbacks; connector declines/failed uploads degrade the same way.
  Scope/user egress discriminators ride metadata exactly like send.
- Inbound: _localize_inbound_media downloads each media_urls entry to a
  local temp path (native-adapter parity — vision/file tools consume
  paths); dead re-host refs are dropped, public URLs survive a missing
  client. Best-effort, never blocks handle_message.
- docs/relay-connector-contract.md §4: send_media op row + media
  ingress/egress semantics (replaces the 'deferred to a later revision'
  note). Additive within contract_version 1.
- tests: tests/gateway/relay/test_relay_media.py (15) — kind mapping,
  upload-first path handling, op gating (explicit + legacy-empty),
  decline/upload-failure fallbacks, scope metadata, inbound localization
  matrix, client URL derivation/credential gating. Stub connector grew a
  canned send_media result.

Cross-repo pair: gateway-gateway 'Phase 2 media parity' PR (re-host plane +
four ingress lanes + four platform send_media senders).

* fix(tests): a run that collects nothing can no longer look green

Three foot-guns in the canonical test runner, each of which cost real
debugging time by making an unverified run look verified.

1. Zero collection across the whole run reported success-shaped output.
   Per-file rc=5 is rewritten to rc=0 so a platform-gated file (every test
   skipped on this OS) doesn't fail the suite — correct, but it also meant a
   run where NOTHING was collected anywhere printed
   "0 tests passed, 0 failed (100% complete)" and, with no failures
   recorded, could exit 0. Now the run-level guard counts every collected
   outcome (passed/failed/skipped/errors/xfailed/xpassed): an all-skipped
   file still passes, but zero-collected-anywhere prints an explicit
   "✗ NO TESTS RAN — this is NOT a pass" block naming the likely causes and
   returns 1.

2. A venv without pytest was selected merely for existing. The probe
   accepted any directory with bin/activate, so in a checkout/worktree
   without a local .venv it picked the RELEASE venv
   (~/.hermes/hermes-agent/venv, no pytest). Every file then died with
   "No module named pytest" and the run reported 0 tests. Candidates are now
   import-checked for pytest — the same guard the HERMES_PYTHON fallback
   already applied — and a skipped candidate is named on stderr.

3. Pytest node ids were silently discarded. This runner is file-granular,
   so `tests/foo.py::TestBar::test_baz` isn't an existing path: discovery
   dropped it and the run ended "No test files to run" while the selector
   looked accepted. Node ids are now translated to the FILE plus an inferred
   `-k` on the leaf name (parametrized ids reduced to the function name),
   with a note explaining the translation. An explicit caller `-k` wins over
   the inferred one.

Tests: 4 behavior contracts in tests/test_run_tests_parallel.py. Verified
by sabotage — reverting the runner fails 3 of the 4 (the fourth pins the
pre-existing all-skipped tolerance so fix 1 can't regress it).

* fix(tests): gate WAL-dependent tests on the linked SQLite's real capability

Two tests fail deterministically on main depending only on which SQLite the
test interpreter links — nothing about the code under test. Both are green in
isolation and red in the suite / on an older library, the worst diagnostic
shape.

Root cause A — WAL is not always WAL. Hermes refuses journal_mode=WAL on
SQLite builds carrying the upstream WAL-reset corruption bug (3.7.0–3.51.2,
excluding backports 3.50.7 / 3.44.6) and falls back to DELETE. On such a
build NO -wal sidecar is ever created, so
test_wal_checkpoint_truncates_wal_file asserts on a file that cannot exist.
Invisible locally when the repo .venv and the Hermes managed runtime link
different versions (observed: .venv 3.50.4 → DELETE, runtime 3.53.1 → WAL),
so the same test passes for one interpreter and fails for the other.

  - tests/conftest.py: add a `requires_wal` marker plus a
    pytest_collection_modifyitems hook that skips such tests when the linked
    library will fall back to DELETE. The skip reason names the actual
    version so it is diagnosable rather than mysterious.
  - pyproject.toml: register the marker.
  - test_kanban_db_repair.py: mark the -wal-sidecar test.

Root cause B — process-global warn-once dedup. The WAL-fallback warning is
emitted at most once per (process, db_label). Any earlier test in
test_kanban_db.py that opens a kanban.db consumes that one-shot, so
test_connect_falls_back_to_delete_on_locking_protocol sees zero warnings and
fails — but only as part of the file, never alone.

  - test_kanban_db.py: clear both dedup sets in the test that asserts on the
    warning, with a comment explaining the isolation trap.

The gate deliberately does NOT import hermes_state. That module computes
DEFAULT_DB_PATH from get_hermes_home() at import time, so importing it during
collection — before the per-test _isolate_hermes_home fixture redirects
HERMES_HOME — permanently caches the developer's REAL ~/.hermes/state.db for
the whole session. The first version of this change did exactly that and made
tests read a live 31,881-session production database (test_console_engine
asserted "Total sessions: 2" and got 31881). The version predicate is
duplicated instead, and tests/test_conftest_wal_gate.py pins the two
implementations in agreement across every documented upstream boundary plus
guards against the import coming back.

Verified: on SQLite 3.50.4 the sidecar test SKIPS naming the version; on
3.53.1 it RUNS and passes, so coverage is not lost where WAL works. Clean
main fails exactly these 2 tests under
`scripts/run_tests.sh tests/hermes_cli/ tests/test_hermes_state.py`
(9926 passed, 2 failed); with this change the same scope is green.

Tests: 726 passed across the two kanban files, test_hermes_state.py, and the
new gate tests.

* fix: stabilize runtime safety and credential routing

* fix: isolate credential rotation retry state

---------

Co-authored-by: Ben Barclay <ben@nousresearch.com>
Co-authored-by: HexLab98 <liruixinch@outlook.com>
Co-authored-by: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com>
Co-authored-by: teknium1 <127238744+teknium1@users.noreply.github.com>
Co-authored-by: Hermes Vibhavadi <hermes_vibhavadi@proton.me>
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
Follow-up for salvaged PR NousResearch#70615 — the 2-button smart_deny case
(Allow Once + Deny only) was exercised by an existing test but only
at the flat-label level, not asserting the row pairing. Adds the
missing row-structure assertion using the same capture pattern as
the 4-button and 3-button tests.
prmartinow pushed a commit to prmartinow/hermes-agent that referenced this pull request Aug 26, 2026
Follow-up for salvaged PR NousResearch#70615 — the 2-button smart_deny case
(Allow Once + Deny only) was exercised by an existing test but only
at the flat-label level, not asserting the row pairing. Adds the
missing row-structure assertion using the same capture pattern as
the 4-button and 3-button tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/telegram Telegram bot adapter type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants