Skip to content

fix(gateway): preserve detached approvals and bind Telegram cards - #117560

Open
cresslank wants to merge 1 commit into
NousResearch:mainfrom
cresslank:fix/detached-approval-ownership
Open

cresslank wants to merge 1 commit into
NousResearch:mainfrom
cresslank:fix/detached-approval-ownership

Conversation

@cresslank

@cresslank cresslank commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes an approval-lifetime bug where a detached child remains alive but its parent's normal return withdraws the child's pending approval and removes delivery for future prompts. A later Telegram button tap then resolves nothing, and the command does not run.

The parent currently unregisters approval state by shared session key. This change makes cleanup execution-owned: detached work retains its own delivery route before submission, and parent completion closes only the parent's owner. Actual child termination and session teardown still withdraw the relevant waits.

Telegram cards also carry the exact core request ID. A stale card cannot fall back to approving another request in the same session. These changes belong together because retained requests need correctly bound cards, including when delivery finishes after settlement.

Related Issue

Related Work and Credit

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tools/approval_ownership.py, approval wait logic and gateway/run_turn_runner.py: add execution-scoped ownership using existing queue, withdrawal and settlement primitives; prevent old cleanup from removing a newer turn's route.
  • Async delegation and child-run paths: capture ownership before submission, propagate it into execution threads, and release it on teardown or failed admission. Soft steering preserves the route; hard stops revoke it. Start the stale monitor before submitting work so startup failure cannot orphan an admitted child.
  • Shared approval prompt and Telegram adapter: pass request IDs into native cards and resolve only the bound request. Missing identity fails closed. Keep authorization checks and derive the displayed result from authoritative settlement.
  • plugins/platforms/telegram/exec_approval.py: retire settled cards through early, in-flight and late publication. Failed callback acknowledgement does not skip retirement. Failed automatic retirement edits get one best-effort interim notice with the original chat/topic metadata; ambiguous prompt delivery is not resent.
  • Add lifecycle and delivery regressions; replace an affected source-order assertion with a behavioral stale-topic fallback test.

How to Test

Use a disposable HOME and HERMES_HOME with plugins.enabled: []:

HERMES_TEST_FILE_RETRIES=0 scripts/run_tests.sh \
  tests/tools/test_delegated_approval_lifecycle.py \
  tests/tools/test_approval_ownership.py \
  tests/gateway/test_detached_approval_lifetime.py \
  tests/tools/test_async_delegation.py \
  tests/plugins/telegram/test_exec_approval_lifecycle.py \
  tests/plugins/telegram/test_exec_approval_delivery.py \
  tests/gateway/test_telegram_approval_buttons.py \
  tests/gateway/test_approval_timeout_notice.py \
  -j 3 --file-timeout 120

Local macOS validation against base a1183a8ef9b26b71d745b68cc48d02faa82c9407:

  • The three detached-lifetime cases fail on the unchanged base and pass with this fix. The soft-steer and monitor-admission regressions also fail before their repairs and pass afterward.
  • Focused command above: 92 passed, 0 failed. These are included in the broader 76-file run: 1,057 passed, 2 failed. Both failures reproduce on the unchanged base: test_nonrecursive_verification_artifact_cleanup_is_not_dangerous and test_child_dedicated_db_follows_parents_db_path.
  • Ruff, git diff --check, Windows-footgun, compatibility-pointer, temporary-path and OS-marker checks pass. Ty is not clean: 170 diagnostics versus 172 on the base, with no new file/message diagnostic shapes.
  • Independent core and Telegram reviews were followed by targeted repair verification. The final two-repair re-review passed 11 cases, including six independent probes, with no scoped blockers.

Validation uses synthetic transport, not live Telegram. The full repository suite and hosted CI have not been run for this candidate; this is not a claim of exhaustive concurrency coverage.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate. The active native-card overlap and prior lifecycle work are explicitly disclosed under Related Issue.
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass. Not claimed: validation used the canonical scripts/run_tests.sh on selected suites; the full repository suite was not run, and the broader selected run has two failures also reproduced on the unchanged base.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS, using isolated test state and synthetic transport. No live Telegram/manual end-to-end validation is claimed.

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — ownership and native-settlement contracts are documented in code docstrings/comments; no new user-facing configuration or commands.
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A: no config keys changed.
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — neither file changed; the execution-ownership change is documented in implementation docstrings and this description instead.
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — Windows-footgun and OS-marker checks pass; runtime testing was on macOS, not Windows/Linux.
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A: no model-facing tool descriptions, parameters, or approval choices changed.

Screenshots / Logs

Local canonical-runner summaries for commit 4bf6ba5448f5b4eab12488db44e313351fa0c43a:

Focused:                 8 files,   92 passed, 0 failed
Broader regression:     76 files, 1057 passed, 2 failed

The two broader failures and their unchanged-base controls are detailed in How to Test above. No live Telegram screenshots are available. Hosted workflows currently require maintainer approval (action_required); no hosted passing result is claimed.

Not in Scope

No auto-approval, timeout increase, permission-policy change, new approval command grammar, restart persistence, or expansion of detached retention to API/TUI paths. Permanent transport refusal or event-loop shutdown can still prevent a terminal notice from reaching Telegram; logical settlement does not depend on notification success.

Retain execution-scoped approval delivery for detached children without extending approval policy or session teardown lifetime. Bind Telegram cards to exact requests and settle their UI across timeout, withdrawal, failed edits, and late delivery.

Add ownership, admission, steering, request-binding, and transport regressions. Mark terminal card edits final so current Telegram pacing cannot silently skip retirement.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets comp/plugins Plugin system and bundled plugins tool/delegate Subagent delegation platform/telegram Telegram bot adapter area/auth Authentication, OAuth, credential pools sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Sep 20, 2026

@andrexibiza andrexibiza 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.

Review — detached approval ownership and Telegram settlement

Reviewed head: 4bf6ba5448f5b4eab12488db44e313351fa0c43a.

Conclusion: I did not establish a new blocking implementation defect in the execution-owner and Telegram paths inspected here. The ownership/settlement separation is supported by the source inspection and the isolated probes below. This is not a merge-ready or all-CI-green verdict: hosted CI, Docker, and Nix are blocked at action_required, and neither the full repository suite nor the current-main integration was executed in this review.

Ownership and authority boundaries checked

The important distinction is between retaining a delivery route and extending approval authority. This patch forks the former without introducing a new permission grant.

  • Admission and lifetime. retain_gateway_approval_owner forks the context-bound owner, not whichever callback now occupies the session registry. A closed old context stays closed. _dispatch retains before submission, while _run_single_child creates the execution-specific owner under the inherited context. I followed the handoffs through propagate_context_to_thread, parallel batch copy_context().run, and _ChildRun.await_child; these carry the owner rather than reconstructing it from the current session route.
  • Cleanup scope. Owner closure removes only entries whose entry.owner is self; it removes the session callback only when that callback is still the same owner. This prevents an old turn's cleanup from unregistering a newer turn. clear_session and legacy unregister_gateway_notify deliberately close all owners for the session. The child startup check distinguishes a hard interrupt from a pending soft steer, and _signal_child_stop explicitly retires the child's route.
  • Admission failure and actual completion. The stale monitor starts before executor submission. Rejected admission closes the retained owner; worker completion has a done-callback release in addition to record finalization. I checked the distinction between the async unit's retained owner and the separately retained child owner: closing the unit alone is not the child-stop mechanism; the interrupt callback and child teardown remain important parts of that path.
  • Queue settlement. _await_gateway_decision confines coalescing to the same owner. Admission/closure and result removal share approval._lock; transport work and settle callbacks run outside it. The late-deadline result read and the second cancellation-cause read preserve the distinction between an actual answer, timeout, and withdrawal. I also checked the file-write guard's switch to the owner-aware callback accessor rather than leaving a direct session-map reader behind.

Telegram binding and publication races checked

handle_callback performs authorization before consuming state, rejects legacy/unbound cards, validates the allowed choice and actual chat/message, and calls the resolver with the exact request ID. The displayed outcome depends on the resolver's count; a stale click does not claim approval. A callback-toast failure does not skip the subsequent card edit.

The settlement hook is installed before publication. Its record survives while the send is in flight, so a timeout/withdrawal/external answer can be applied when on_sent eventually supplies the message ID. The observer is inside the Telegram send coroutine, which is the relevant placement for a cancellation-resistant late acknowledgement. On the runner side, the exec_approval_settlement marker prevents replacement with the generic timeout-only hook, and the ambiguous-send branch returns without resending or overwriting the hook. Terminal edits use finalize=True; a failed automatic edit falls back through the original topic/interim metadata.

Independent execution evidence — deliberately bounded

I executed 40 isolated checks, all passing, against complete, unmodified copies of these two production modules from the reviewed head. Git blob hashes were checked before import:

Module Verified Git blob
tools/approval_ownership.py 1b08cbc90b54f5bb24846abbdbbb56fe9be714bf
plugins/platforms/telegram/exec_approval.py 9052650c647c2890bb6c0f5142b04e23c7685415

The harness used Python 3.13.5 and dependency doubles at the core-facade, Telegram SDK, and adapter seams. These are not 40 repository tests and not a live Telegram or full TurnRunner integration run. In particular, callback probes verify the exact resolver arguments and handling of its return value; the resolver itself was doubled there.

Probe group Checks Observed behavior
Owner lifetime and isolation 11 Parent close preserves child delivery; old cleanup preserves a newer route; closed-context retention cannot borrow it; child/session closure stays scoped; closure proceeds while a notifier is blocked on another thread; owner registry is cleaned up.
Callback identity and terminal rendering 14 Unauthorized, disallowed-choice, wrong-chat/message, legacy, unbound, missing, invalid and malformed controls never call the resolver; B's click carries B's ID without consuming A; replay is inert; failed toast still edits for both zero and nonzero resolution; zero resolution never renders approval.
Settlement × acknowledgement ordering 10 Each of timeout, interrupted, session_closed, notify_failed, and resolved, both before and after acknowledgement: state removed and exactly one final card edit. Settlement was invoked from another thread.
Publication and delivery failures 5 Already-settled/missing-ID requests do not publish; definitive failure removes state; ambiguous delivery remains settlement-managed and its late acknowledgement retires the card without another send; failed terminal edit retains the original interim topic in the fallback.

I also inspected the added regression fixtures for pending/after-parent/queued detached execution, soft/hard/legacy interruption, monitor/submission refusal, and Telegram late delivery. The PR's 92-pass focused run and 1,057-pass/2-failure broader run remain author-reported evidence, not runs I reproduced. I have not independently reproduced the two claimed baseline failures or the reported type-check diagnostic comparison.

Repository interlocks and provenance

#104960 remains complementary and overlapping, not superseded. Its current inspected head is 89311702f35e3f1dd6afbeb688c11f3c3c5be05d; it is open and reported non-mergeable. The changed-file intersection includes gateway/run_turn_runner.py, plugins/platforms/telegram/adapter.py, and tests/gateway/test_telegram_approval_buttons.py.

The integration must preserve both sides: this PR's execution ownership, early settlement hook, late-send observer, and exact ApprovalCard; and #104960's broader native-surface binding and fail-closed treatment of legacy adapters. Replacing either runner/Telegram implementation wholesale with the other would lose part of that contract. Whichever lands second needs a fresh review of the reconciled code and both regression selections. This Telegram result must not be used to close the cross-platform FIFO/stale-control defect class. The shared owner-lifetime change applies beyond Telegram, while those other native bindings remain separate work.

I verified #108700 is closed and unmerged, not a landed dependency. Its bmassenz ownership/routing precedent deserves the stated credit, but its exact-selector grammar and broader identity carrier are not implemented by this narrower change. The existing report, withdrawal-attribution, and earlier request-binding lineage in the PR body should remain intact; this review does not reassign that authorship.

Outstanding merge evidence

The exact-head Actions query returned 13 runs, all action_required, including:

The commit check-runs endpoint returned zero checks; combined status was pending with zero status contexts. These are missing execution evidence, not passing checks and not demonstrated code-test failures. The outstanding acceptance requirement is successful required CI on the real implementation head, including any subsequent integration commit—not a proof-only or CI-trigger commit.

For version clarity: the candidate's merge-base is a1183a8ef9b26b71d745b68cc48d02faa82c9407; the PR metadata reports base SHA 118984d7a02f8a8baec11255002cbbab7c202e06, while an independent refs/heads/main read returned 5dd70d7cb6560c3ff8aff294ec44ec4f8d1558e5. The local probe result above belongs only to the two specified blobs at 4bf6ba5, not to an untested merge with that newer main.

Disposition: no demonstrated new code blocker in the inspected scope; hosted validation and the explicit native-binding integration boundary remain open.

This branch has not been deployed

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

Labels

area/auth Authentication, OAuth, credential pools comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists platform/telegram Telegram bot adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/delegate Subagent delegation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants