Skip to content

fix(gateway): handle credential broker callbacks - #38124

Open
stavmatis wants to merge 1 commit into
NousResearch:mainfrom
stavmatis:fix/telegram-credential-callbacks
Open

fix(gateway): handle credential broker callbacks#38124
stavmatis wants to merge 1 commit into
NousResearch:mainfrom
stavmatis:fix/telegram-credential-callbacks

Conversation

@stavmatis

Copy link
Copy Markdown

Summary

  • Handle cred: Telegram inline callbacks from the secure credential broker
  • Gate credential approvals through the existing callback authorization helper
  • Invoke the profile-local scripts/secure_credential_broker.py approve/deny command and clear the inline buttons after resolution

Test Plan

  • python -m pytest tests/gateway/test_telegram_callback_auth_fail_closed.py -q -o 'addopts='\n- python -m pytest tests/gateway/test_telegram_approval_buttons.py tests/gateway/test_telegram_model_picker.py tests/gateway/test_telegram_callback_auth_fail_closed.py -q -o 'addopts='\n- python -m py_compile gateway/platforms/telegram.py tests/gateway/test_telegram_callback_auth_fail_closed.py\n- git diff --check

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have platform/telegram Telegram bot adapter comp/gateway Gateway runner, session dispatch, delivery labels Jun 3, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding authorization before credential approval.

Problems

  • gateway/platforms/telegram.py was renamed to plugins/platforms/telegram/adapter.py by 5600105478ffde29d7566b45421b100eaa29c4ef; current main's live dispatcher is plugins/platforms/telegram/adapter.py:5316, so this patch cannot be directly salvaged onto its original target.
  • The proposed broker command at gateway/platforms/telegram.py:3277 relies on secure_credential_broker.py, but current tracked HEAD defines neither that script nor a cred: callback producer/contract. The added test mocks subprocess.run, so it does not validate a runnable supported integration.

Suggested changes

  • Establish the broker interface and producer first, then port the handler to plugins/platforms/telegram/adapter.py with an end-to-end temporary-HERMES_HOME test.
  • If this is an external broker, re-scope it as a standalone extension instead of hard-coding its private profile script contract into Hermes.

Automated hermes-sweeper review.


from hermes_constants import get_hermes_home

broker = get_hermes_home() / "scripts" / "secure_credential_broker.py"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This hard-codes a secure_credential_broker.py profile-script contract that current tracked HEAD does not define or produce callbacks for. Please establish a supported extension/interface and an integration test before wiring this into the Telegram adapter.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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 14, 2026

@GottZ GottZ left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was generated by AI during triage.

Summary

Two PRs address Telegram callback authorization, but they fix distinct causes: #38124 adds authorization and broker dispatch for cred: callbacks, while #65589 repairs the existing authorization path for multiplexed Telegram profiles.

Related pull requests

  • #38124 related — (+146/-1) — keep open, substantial rework required: the diff adds an authorization-gated cred: callback handler, but targets the renamed gateway/platforms/telegram.py and mocks a profile-local broker script and callback contract that current tracked HEAD does not provide. Consistent with the keep_open review (salvageability=low), the viable path is to establish the broker producer/interface, port the handler to plugins/platforms/telegram/adapter.py, and add an end-to-end temporary-HERMES_HOME test.
  • #65589 related — (+88/-6) — merge candidate: the diff fixes multiplexed-profile callback authorization in the current adapter by preferring the already injected profile-bound authorization check over bound-handler introspection, while preserving legacy and fail-closed fallbacks. This matches the keep_open review (salvageability=high) and adds allow, deny, and callback-error coverage for the identified closure-handler failure.

Suggested consolidation

Merge #65589 as the focused, current-tree fix for multiplexed callback authorization. Keep #38124 open for the explicitly documented broker-contract, current-adapter port, and end-to-end-test rework; it is not a duplicate of #65589, so neither PR should be closed as a duplicate.

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 13 kB of PR diffs, 3 kB of issue/PR text, 2 kB of discussion (2 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have platform/telegram Telegram bot adapter 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-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants